diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-11-18 16:16:36 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-11-18 16:16:36 +0300 |
commit | 311b0269b4eb9839fa63f80c8d7a58f32b8138a0 (patch) | |
tree | 07e7870bca8aed6d61fdcc810731c50d2c40af47 /spec/lib/gitlab/github_import | |
parent | 27909cef6c4170ed9205afa7426b8d3de47cbb0c (diff) |
Add latest changes from gitlab-org/gitlab@14-5-stable-eev14.5.0-rc42
Diffstat (limited to 'spec/lib/gitlab/github_import')
9 files changed, 508 insertions, 347 deletions
diff --git a/spec/lib/gitlab/github_import/bulk_importing_spec.rb b/spec/lib/gitlab/github_import/bulk_importing_spec.rb index 6c94973b5a8..e170496ff7b 100644 --- a/spec/lib/gitlab/github_import/bulk_importing_spec.rb +++ b/spec/lib/gitlab/github_import/bulk_importing_spec.rb @@ -116,13 +116,13 @@ RSpec.describe Gitlab::GithubImport::BulkImporting do value: 5 ) - expect(Gitlab::Database.main) - .to receive(:bulk_insert) + expect(ApplicationRecord) + .to receive(:legacy_bulk_insert) .ordered .with('kittens', rows.first(5)) - expect(Gitlab::Database.main) - .to receive(:bulk_insert) + expect(ApplicationRecord) + .to receive(:legacy_bulk_insert) .ordered .with('kittens', rows.last(5)) diff --git a/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb b/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb index 3dc15c7c059..0448ada6bca 100644 --- a/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/diff_note_importer_spec.rb @@ -2,156 +2,226 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Importer::DiffNoteImporter do - let(:project) { create(:project) } - let(:client) { double(:client) } - let(:user) { create(:user) } - let(:created_at) { Time.new(2017, 1, 1, 12, 00) } - let(:updated_at) { Time.new(2017, 1, 1, 12, 15) } +RSpec.describe Gitlab::GithubImport::Importer::DiffNoteImporter, :aggregate_failures do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } - let(:hunk) do - '@@ -1 +1 @@ + let(:client) { double(:client) } + let(:discussion_id) { 'b0fa404393eeebb4e82becb8104f238812bb1fe6' } + let(:created_at) { Time.new(2017, 1, 1, 12, 00).utc } + let(:updated_at) { Time.new(2017, 1, 1, 12, 15).utc } + let(:note_body) { 'Hello' } + let(:file_path) { 'files/ruby/popen.rb' } + + let(:diff_hunk) do + '@@ -14 +14 @@ -Hello +Hello world' end - let(:note) do + let(:note_representation) do Gitlab::GithubImport::Representation::DiffNote.new( noteable_type: 'MergeRequest', noteable_id: 1, commit_id: '123abc', original_commit_id: 'original123abc', - file_path: 'README.md', - diff_hunk: hunk, - author: Gitlab::GithubImport::Representation::User - .new(id: user.id, login: user.username), - note: 'Hello', + file_path: file_path, + author: Gitlab::GithubImport::Representation::User.new(id: user.id, login: user.username), + note: note_body, created_at: created_at, updated_at: updated_at, - github_id: 1 + start_line: nil, + end_line: 15, + github_id: 1, + diff_hunk: diff_hunk, + side: 'RIGHT' ) end - let(:importer) { described_class.new(note, project, client) } + subject(:importer) { described_class.new(note_representation, project, client) } + + shared_examples 'diff notes without suggestion' do + it 'imports the note as legacy diff note' do + stub_user_finder(user.id, true) + + expect { subject.execute } + .to change(LegacyDiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note).to be_valid + expect(note.author_id).to eq(user.id) + expect(note.commit_id).to eq('original123abc') + expect(note.created_at).to eq(created_at) + expect(note.diff).to be_an_instance_of(Gitlab::Git::Diff) + expect(note.discussion_id).to eq(discussion_id) + expect(note.line_code).to eq(note_representation.line_code) + expect(note.note).to eq('Hello') + expect(note.noteable_id).to eq(merge_request.id) + expect(note.noteable_type).to eq('MergeRequest') + expect(note.project_id).to eq(project.id) + expect(note.st_diff).to eq(note_representation.diff_hash) + expect(note.system).to eq(false) + expect(note.type).to eq('LegacyDiffNote') + expect(note.updated_at).to eq(updated_at) + end + + it 'adds a "created by:" note when the author cannot be found' do + stub_user_finder(project.creator_id, false) + + expect { subject.execute } + .to change(LegacyDiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note).to be_valid + expect(note.author_id).to eq(project.creator_id) + expect(note.note).to eq("*Created by: #{user.username}*\n\nHello") + end + + it 'does not import the note when a foreign key error is raised' do + stub_user_finder(project.creator_id, false) + + expect(ApplicationRecord) + .to receive(:legacy_bulk_insert) + .and_raise(ActiveRecord::InvalidForeignKey, 'invalid foreign key') + + expect { subject.execute } + .not_to change(LegacyDiffNote, :count) + end + end describe '#execute' do context 'when the merge request no longer exists' do it 'does not import anything' do - expect(Gitlab::Database.main).not_to receive(:bulk_insert) + expect(ApplicationRecord).not_to receive(:legacy_bulk_insert) - importer.execute + expect { subject.execute } + .to not_change(DiffNote, :count) + .and not_change(LegacyDiffNote, :count) end end context 'when the merge request exists' do - let!(:merge_request) do + let_it_be(:merge_request) do create(:merge_request, source_project: project, target_project: project) end before do - allow(importer) - .to receive(:find_merge_request_id) - .and_return(merge_request.id) + expect_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |finder| + expect(finder) + .to receive(:database_id) + .and_return(merge_request.id) + end + + expect(Discussion) + .to receive(:discussion_id) + .and_return(discussion_id) end - it 'imports the note' do - allow(importer.user_finder) - .to receive(:author_id_for) - .and_return([user.id, true]) - - expect(Gitlab::Database.main) - .to receive(:bulk_insert) - .with( - LegacyDiffNote.table_name, - [ - { - discussion_id: anything, - noteable_type: 'MergeRequest', - noteable_id: merge_request.id, - project_id: project.id, - author_id: user.id, - note: 'Hello', - system: false, - commit_id: 'original123abc', - line_code: note.line_code, - type: 'LegacyDiffNote', - created_at: created_at, - updated_at: updated_at, - st_diff: note.diff_hash.to_yaml - } - ] - ) - .and_call_original - - importer.execute + context 'when github_importer_use_diff_note_with_suggestions is disabled' do + before do + stub_feature_flags(github_importer_use_diff_note_with_suggestions: false) + end + + it_behaves_like 'diff notes without suggestion' + + context 'when the note has suggestions' do + let(:note_body) do + <<~EOB + Suggestion: + ```suggestion + what do you think to do it like this + ``` + EOB + end + + it 'imports the note' do + stub_user_finder(user.id, true) + + expect { subject.execute } + .to change(LegacyDiffNote, :count) + .and not_change(DiffNote, :count) + + note = project.notes.diff_notes.take + expect(note).to be_valid + expect(note.note) + .to eq <<~NOTE + Suggestion: + ```suggestion:-0+0 + what do you think to do it like this + ``` + NOTE + end + end end - it 'imports the note when the author could not be found' do - allow(importer.user_finder) - .to receive(:author_id_for) - .and_return([project.creator_id, false]) - - expect(Gitlab::Database.main) - .to receive(:bulk_insert) - .with( - LegacyDiffNote.table_name, - [ - { - discussion_id: anything, - noteable_type: 'MergeRequest', - noteable_id: merge_request.id, - project_id: project.id, - author_id: project.creator_id, - note: "*Created by: #{user.username}*\n\nHello", - system: false, - commit_id: 'original123abc', - line_code: note.line_code, - type: 'LegacyDiffNote', - created_at: created_at, - updated_at: updated_at, - st_diff: note.diff_hash.to_yaml - } - ] - ) - .and_call_original - - importer.execute - end - - it 'produces a valid LegacyDiffNote' do - allow(importer.user_finder) - .to receive(:author_id_for) - .and_return([user.id, true]) - - importer.execute - - note = project.notes.diff_notes.take - - expect(note).to be_valid - expect(note.diff).to be_an_instance_of(Gitlab::Git::Diff) - end - - it 'does not import the note when a foreign key error is raised' do - allow(importer.user_finder) - .to receive(:author_id_for) - .and_return([project.creator_id, false]) - - expect(Gitlab::Database.main) - .to receive(:bulk_insert) - .and_raise(ActiveRecord::InvalidForeignKey, 'invalid foreign key') - - expect { importer.execute }.not_to raise_error + context 'when github_importer_use_diff_note_with_suggestions is enabled' do + before do + stub_feature_flags(github_importer_use_diff_note_with_suggestions: true) + end + + it_behaves_like 'diff notes without suggestion' + + context 'when the note has suggestions' do + let(:note_body) do + <<~EOB + Suggestion: + ```suggestion + what do you think to do it like this + ``` + EOB + end + + it 'imports the note as diff note' do + stub_user_finder(user.id, true) + + expect { subject.execute } + .to change(DiffNote, :count) + .by(1) + + note = project.notes.diff_notes.take + expect(note).to be_valid + expect(note.noteable_type).to eq('MergeRequest') + expect(note.noteable_id).to eq(merge_request.id) + expect(note.project_id).to eq(project.id) + expect(note.author_id).to eq(user.id) + expect(note.system).to eq(false) + expect(note.discussion_id).to eq(discussion_id) + expect(note.commit_id).to eq('original123abc') + expect(note.line_code).to eq(note_representation.line_code) + expect(note.type).to eq('DiffNote') + expect(note.created_at).to eq(created_at) + expect(note.updated_at).to eq(updated_at) + expect(note.position.to_h).to eq({ + base_sha: merge_request.diffs.diff_refs.base_sha, + head_sha: merge_request.diffs.diff_refs.head_sha, + start_sha: merge_request.diffs.diff_refs.start_sha, + new_line: 15, + old_line: nil, + new_path: file_path, + old_path: file_path, + position_type: 'text', + line_range: nil + }) + expect(note.note) + .to eq <<~NOTE + Suggestion: + ```suggestion:-0+0 + what do you think to do it like this + ``` + NOTE + end + end end end end - describe '#find_merge_request_id' do - it 'returns a merge request ID' do - expect_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |instance| - expect(instance).to receive(:database_id).and_return(10) - end - - expect(importer.find_merge_request_id).to eq(10) + def stub_user_finder(user, found) + expect_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| + expect(finder) + .to receive(:author_id_for) + .and_return([user, found]) end end end diff --git a/spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb b/spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb index be4fc3cbf16..1c7b35ed928 100644 --- a/spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb @@ -19,7 +19,9 @@ RSpec.describe Gitlab::GithubImport::Importer::DiffNotesImporter do updated_at: Time.zone.now, line: 23, start_line: nil, + in_reply_to_id: nil, id: 1, + side: 'RIGHT', body: <<~BODY Hello World diff --git a/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb b/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb index 0926000428c..4287c32b947 100644 --- a/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/issue_importer_spec.rb @@ -190,8 +190,8 @@ RSpec.describe Gitlab::GithubImport::Importer::IssueImporter, :clean_gitlab_redi .with(issue.assignees[1]) .and_return(5) - expect(Gitlab::Database.main) - .to receive(:bulk_insert) + expect(ApplicationRecord) + .to receive(:legacy_bulk_insert) .with( IssueAssignee.table_name, [{ issue_id: 1, user_id: 4 }, { issue_id: 1, user_id: 5 }] diff --git a/spec/lib/gitlab/github_import/importer/label_links_importer_spec.rb b/spec/lib/gitlab/github_import/importer/label_links_importer_spec.rb index 241a0fef600..e68849755b2 100644 --- a/spec/lib/gitlab/github_import/importer/label_links_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/label_links_importer_spec.rb @@ -39,8 +39,8 @@ RSpec.describe Gitlab::GithubImport::Importer::LabelLinksImporter do .and_return(1) freeze_time do - expect(Gitlab::Database.main) - .to receive(:bulk_insert) + expect(ApplicationRecord) + .to receive(:legacy_bulk_insert) .with( LabelLink.table_name, [ @@ -64,8 +64,8 @@ RSpec.describe Gitlab::GithubImport::Importer::LabelLinksImporter do .with('bug') .and_return(nil) - expect(Gitlab::Database.main) - .to receive(:bulk_insert) + expect(ApplicationRecord) + .to receive(:legacy_bulk_insert) .with(LabelLink.table_name, []) importer.create_labels diff --git a/spec/lib/gitlab/github_import/importer/note_importer_spec.rb b/spec/lib/gitlab/github_import/importer/note_importer_spec.rb index 820f46c7286..96d8acbd3de 100644 --- a/spec/lib/gitlab/github_import/importer/note_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/note_importer_spec.rb @@ -41,8 +41,8 @@ RSpec.describe Gitlab::GithubImport::Importer::NoteImporter do .with(github_note) .and_return([user.id, true]) - expect(Gitlab::Database.main) - .to receive(:bulk_insert) + expect(ApplicationRecord) + .to receive(:legacy_bulk_insert) .with( Note.table_name, [ @@ -71,8 +71,8 @@ RSpec.describe Gitlab::GithubImport::Importer::NoteImporter do .with(github_note) .and_return([project.creator_id, false]) - expect(Gitlab::Database.main) - .to receive(:bulk_insert) + expect(ApplicationRecord) + .to receive(:legacy_bulk_insert) .with( Note.table_name, [ @@ -115,7 +115,7 @@ RSpec.describe Gitlab::GithubImport::Importer::NoteImporter do context 'when the noteable does not exist' do it 'does not import the note' do - expect(Gitlab::Database.main).not_to receive(:bulk_insert) + expect(ApplicationRecord).not_to receive(:legacy_bulk_insert) importer.execute end @@ -134,8 +134,8 @@ RSpec.describe Gitlab::GithubImport::Importer::NoteImporter do .with(github_note) .and_return([user.id, true]) - expect(Gitlab::Database.main) - .to receive(:bulk_insert) + expect(ApplicationRecord) + .to receive(:legacy_bulk_insert) .and_raise(ActiveRecord::InvalidForeignKey, 'invalid foreign key') expect { importer.execute }.not_to raise_error diff --git a/spec/lib/gitlab/github_import/importer/pull_requests_merged_by_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_requests_merged_by_importer_spec.rb index 4a47d103cde..b6c162aafa9 100644 --- a/spec/lib/gitlab/github_import/importer/pull_requests_merged_by_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_requests_merged_by_importer_spec.rb @@ -4,7 +4,8 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Importer::PullRequestsMergedByImporter do let(:client) { double } - let(:project) { create(:project, import_source: 'http://somegithub.com') } + + let_it_be(:project) { create(:project, import_source: 'http://somegithub.com') } subject { described_class.new(project, client) } @@ -27,14 +28,11 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestsMergedByImporter do end describe '#each_object_to_import', :clean_gitlab_redis_cache do - it 'fetchs the merged pull requests data' do - create( - :merged_merge_request, - iid: 999, - source_project: project, - target_project: project - ) + let!(:merge_request) do + create(:merged_merge_request, iid: 999, source_project: project, target_project: project) + end + it 'fetches the merged pull requests data' do pull_request = double allow(client) @@ -48,5 +46,16 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestsMergedByImporter do subject.each_object_to_import {} end + + it 'skips cached merge requests' do + Gitlab::Cache::Import::Caching.set_add( + "github-importer/already-imported/#{project.id}/pull_requests_merged_by", + merge_request.id + ) + + expect(client).not_to receive(:pull_request) + + subject.each_object_to_import {} + end end end diff --git a/spec/lib/gitlab/github_import/representation/diff_note_spec.rb b/spec/lib/gitlab/github_import/representation/diff_note_spec.rb index 81722c0eba7..63834cfdb94 100644 --- a/spec/lib/gitlab/github_import/representation/diff_note_spec.rb +++ b/spec/lib/gitlab/github_import/representation/diff_note_spec.rb @@ -2,23 +2,44 @@ require 'spec_helper' -RSpec.describe Gitlab::GithubImport::Representation::DiffNote do +RSpec.describe Gitlab::GithubImport::Representation::DiffNote, :clean_gitlab_redis_shared_state do let(:hunk) do '@@ -1 +1 @@ -Hello +Hello world' end + let(:merge_request) do + double( + :merge_request, + id: 54, + diff_refs: double( + :refs, + base_sha: 'base', + start_sha: 'start', + head_sha: 'head' + ) + ) + end + + let(:project) { double(:project, id: 836) } + let(:note_id) { 1 } + let(:in_reply_to_id) { nil } + let(:start_line) { nil } + let(:end_line) { 23 } + let(:note_body) { 'Hello world' } + let(:user_data) { { 'id' => 4, 'login' => 'alice' } } + let(:side) { 'RIGHT' } let(:created_at) { Time.new(2017, 1, 1, 12, 00) } let(:updated_at) { Time.new(2017, 1, 1, 12, 15) } - shared_examples 'a DiffNote' do + shared_examples 'a DiffNote representation' do it 'returns an instance of DiffNote' do expect(note).to be_an_instance_of(described_class) end context 'the returned DiffNote' do - it 'includes the number of the note' do + it 'includes the number of the merge request' do expect(note.noteable_id).to eq(42) end @@ -30,18 +51,6 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do expect(note.commit_id).to eq('123abc') end - it 'includes the user details' do - expect(note.author) - .to be_an_instance_of(Gitlab::GithubImport::Representation::User) - - expect(note.author.id).to eq(4) - expect(note.author.login).to eq('alice') - end - - it 'includes the note body' do - expect(note.note).to eq('Hello world') - end - it 'includes the created timestamp' do expect(note.created_at).to eq(created_at) end @@ -51,209 +60,250 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNote do end it 'includes the GitHub ID' do - expect(note.note_id).to eq(1) + expect(note.note_id).to eq(note_id) end it 'returns the noteable type' do expect(note.noteable_type).to eq('MergeRequest') end - end - end - - describe '.from_api_response' do - let(:response) do - double( - :response, - html_url: 'https://github.com/foo/bar/pull/42', - path: 'README.md', - commit_id: '123abc', - original_commit_id: 'original123abc', - diff_hunk: hunk, - user: double(:user, id: 4, login: 'alice'), - body: 'Hello world', - created_at: created_at, - updated_at: updated_at, - line: 23, - start_line: nil, - id: 1 - ) - end - - it_behaves_like 'a DiffNote' do - let(:note) { described_class.from_api_response(response) } - end - - it 'does not set the user if the response did not include a user' do - allow(response) - .to receive(:user) - .and_return(nil) - - note = described_class.from_api_response(response) - - expect(note.author).to be_nil - end - - it 'formats a suggestion in the note body' do - allow(response) - .to receive(:body) - .and_return <<~BODY - ```suggestion - Hello World - ``` - BODY - - note = described_class.from_api_response(response) - - expect(note.note).to eq <<~BODY - ```suggestion:-0+0 - Hello World - ``` - BODY - end - end - - describe '.from_json_hash' do - let(:hash) do - { - 'noteable_type' => 'MergeRequest', - 'noteable_id' => 42, - 'file_path' => 'README.md', - 'commit_id' => '123abc', - 'original_commit_id' => 'original123abc', - 'diff_hunk' => hunk, - 'author' => { 'id' => 4, 'login' => 'alice' }, - 'note' => 'Hello world', - 'created_at' => created_at.to_s, - 'updated_at' => updated_at.to_s, - 'note_id' => 1 - } - end - it_behaves_like 'a DiffNote' do - let(:note) { described_class.from_json_hash(hash) } - end - - it 'does not convert the author if it was not specified' do - hash.delete('author') - - note = described_class.from_json_hash(hash) + describe '#diff_hash' do + it 'returns a Hash containing the diff details' do + expect(note.diff_hash).to eq( + diff: hunk, + new_path: 'README.md', + old_path: 'README.md', + a_mode: '100644', + b_mode: '100644', + new_file: false + ) + end + end - expect(note.author).to be_nil - end + describe '#diff_position' do + before do + note.merge_request = double( + :merge_request, + diff_refs: double( + :refs, + base_sha: 'base', + start_sha: 'start', + head_sha: 'head' + ) + ) + end + + context 'when the diff is an addition' do + it 'returns a Gitlab::Diff::Position' do + expect(note.diff_position.to_h).to eq( + base_sha: 'base', + head_sha: 'head', + line_range: nil, + new_line: 23, + new_path: 'README.md', + old_line: nil, + old_path: 'README.md', + position_type: 'text', + start_sha: 'start' + ) + end + end + + context 'when the diff is an deletion' do + let(:side) { 'LEFT' } + + it 'returns a Gitlab::Diff::Position' do + expect(note.diff_position.to_h).to eq( + base_sha: 'base', + head_sha: 'head', + line_range: nil, + old_line: 23, + new_path: 'README.md', + new_line: nil, + old_path: 'README.md', + position_type: 'text', + start_sha: 'start' + ) + end + end + end - it 'formats a suggestion in the note body' do - hash['note'] = <<~BODY - ```suggestion - Hello World - ``` - BODY + describe '#discussion_id' do + before do + note.project = project + note.merge_request = merge_request + end + + context 'when the note is a reply to a discussion' do + it 'uses the cached value as the discussion_id only when responding an existing discussion' do + expect(Discussion) + .to receive(:discussion_id) + .and_return('FIRST_DISCUSSION_ID', 'SECOND_DISCUSSION_ID') + + # Creates the first discussion id and caches its value + expect(note.discussion_id) + .to eq('FIRST_DISCUSSION_ID') + + reply_note = described_class.from_json_hash( + 'note_id' => note.note_id + 1, + 'in_reply_to_id' => note.note_id + ) + reply_note.project = project + reply_note.merge_request = merge_request + + # Reading from the cached value + expect(reply_note.discussion_id) + .to eq('FIRST_DISCUSSION_ID') + + new_discussion_note = described_class.from_json_hash( + 'note_id' => note.note_id + 2, + 'in_reply_to_id' => nil + ) + new_discussion_note.project = project + new_discussion_note.merge_request = merge_request + + # Because it's a new discussion, it must not use the cached value + expect(new_discussion_note.discussion_id) + .to eq('SECOND_DISCUSSION_ID') + end + end + end - note = described_class.from_json_hash(hash) + describe '#github_identifiers' do + it 'returns a hash with needed identifiers' do + expect(note.github_identifiers).to eq( + noteable_id: 42, + noteable_type: 'MergeRequest', + note_id: 1 + ) + end + end - expect(note.note).to eq <<~BODY - ```suggestion:-0+0 - Hello World - ``` - BODY - end - end + describe '#line_code' do + it 'generates the proper line code' do + note = described_class.new(diff_hunk: hunk, file_path: 'README.md') - describe '#line_code' do - it 'returns a String' do - note = described_class.new(diff_hunk: hunk, file_path: 'README.md') + expect(note.line_code).to eq('8ec9a00bfd09b3190ac6b22251dbb1aa95a0579d_2_2') + end + end - expect(note.line_code).to be_an_instance_of(String) + describe '#note and #contains_suggestion?' do + it 'includes the note body' do + expect(note.note).to eq('Hello world') + expect(note.contains_suggestion?).to eq(false) + end + + context 'when the note have a suggestion' do + let(:note_body) do + <<~BODY + ```suggestion + Hello World + ``` + BODY + end + + it 'returns the suggestion formatted in the note' do + expect(note.note).to eq <<~BODY + ```suggestion:-0+0 + Hello World + ``` + BODY + expect(note.contains_suggestion?).to eq(true) + end + end + + context 'when the note have a multiline suggestion' do + let(:start_line) { 20 } + let(:end_line) { 23 } + let(:note_body) do + <<~BODY + ```suggestion + Hello World + ``` + BODY + end + + it 'returns the multi-line suggestion formatted in the note' do + expect(note.note).to eq <<~BODY + ```suggestion:-3+0 + Hello World + ``` + BODY + expect(note.contains_suggestion?).to eq(true) + end + end + + describe '#author' do + it 'includes the user details' do + expect(note.author).to be_an_instance_of( + Gitlab::GithubImport::Representation::User + ) + + expect(note.author.id).to eq(4) + expect(note.author.login).to eq('alice') + end + + context 'when the author is empty' do + let(:user_data) { nil } + + it 'does not set the user if the response did not include a user' do + expect(note.author).to be_nil + end + end + end + end end end - describe '#diff_hash' do - it 'returns a Hash containing the diff details' do - note = described_class.from_json_hash( - 'noteable_type' => 'MergeRequest', - 'noteable_id' => 42, - 'file_path' => 'README.md', - 'commit_id' => '123abc', - 'original_commit_id' => 'original123abc', - 'diff_hunk' => hunk, - 'author' => { 'id' => 4, 'login' => 'alice' }, - 'note' => 'Hello world', - 'created_at' => created_at.to_s, - 'updated_at' => updated_at.to_s, - 'note_id' => 1 - ) - - expect(note.diff_hash).to eq( - diff: hunk, - new_path: 'README.md', - old_path: 'README.md', - a_mode: '100644', - b_mode: '100644', - new_file: false - ) - end - end + describe '.from_api_response' do + it_behaves_like 'a DiffNote representation' do + let(:response) do + double( + :response, + id: note_id, + html_url: 'https://github.com/foo/bar/pull/42', + path: 'README.md', + commit_id: '123abc', + original_commit_id: 'original123abc', + side: side, + user: user_data && double(:user, user_data), + diff_hunk: hunk, + body: note_body, + created_at: created_at, + updated_at: updated_at, + line: end_line, + start_line: start_line, + in_reply_to_id: in_reply_to_id + ) + end - describe '#github_identifiers' do - it 'returns a hash with needed identifiers' do - github_identifiers = { - noteable_id: 42, - noteable_type: 'MergeRequest', - note_id: 1 - } - other_attributes = { something_else: '_something_else_' } - note = described_class.new(github_identifiers.merge(other_attributes)) - - expect(note.github_identifiers).to eq(github_identifiers) + subject(:note) { described_class.from_api_response(response) } end end - describe '#note' do - it 'returns the given note' do - hash = { - 'note': 'simple text' - } - - note = described_class.new(hash) - - expect(note.note).to eq 'simple text' - end - - it 'returns the suggestion formatted in the note' do - hash = { - 'note': <<~BODY - ```suggestion - Hello World - ``` - BODY - } - - note = described_class.new(hash) - - expect(note.note).to eq <<~BODY - ```suggestion:-0+0 - Hello World - ``` - BODY - end + describe '.from_json_hash' do + it_behaves_like 'a DiffNote representation' do + let(:hash) do + { + 'note_id' => note_id, + 'noteable_type' => 'MergeRequest', + 'noteable_id' => 42, + 'file_path' => 'README.md', + 'commit_id' => '123abc', + 'original_commit_id' => 'original123abc', + 'side' => side, + 'author' => user_data, + 'diff_hunk' => hunk, + 'note' => note_body, + 'created_at' => created_at.to_s, + 'updated_at' => updated_at.to_s, + 'end_line' => end_line, + 'start_line' => start_line, + 'in_reply_to_id' => in_reply_to_id + } + end - it 'returns the multi-line suggestion formatted in the note' do - hash = { - 'start_line': 20, - 'end_line': 23, - 'note': <<~BODY - ```suggestion - Hello World - ``` - BODY - } - - note = described_class.new(hash) - - expect(note.note).to eq <<~BODY - ```suggestion:-3+0 - Hello World - ``` - BODY + subject(:note) { described_class.from_json_hash(hash) } end end end diff --git a/spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb b/spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb index 2ffd5f50d3b..bcb8575bdbf 100644 --- a/spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb @@ -9,13 +9,19 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(note) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(note) + expect(note_formatter.contains_suggestion?).to eq(false) end it 'handles nil value for note' do note = nil - expect(described_class.formatted_note_for(note: note)).to eq(note) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(note) + expect(note_formatter.contains_suggestion?).to eq(false) end it 'does not allow over 3 leading spaces for valid suggestion' do @@ -26,7 +32,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(note) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(note) + expect(note_formatter.contains_suggestion?).to eq(false) end it 'allows up to 3 leading spaces' do @@ -44,7 +53,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(expected) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(expected) + expect(note_formatter.contains_suggestion?).to eq(true) end it 'does nothing when there is any text without space after the suggestion tag' do @@ -53,7 +65,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(note) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(note) + expect(note_formatter.contains_suggestion?).to eq(false) end it 'formats single-line suggestions' do @@ -71,7 +86,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(expected) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(expected) + expect(note_formatter.contains_suggestion?).to eq(true) end it 'ignores text after suggestion tag on the same line' do @@ -89,7 +107,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(expected) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(expected) + expect(note_formatter.contains_suggestion?).to eq(true) end it 'formats multiple single-line suggestions' do @@ -115,7 +136,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat ``` BODY - expect(described_class.formatted_note_for(note: note)).to eq(expected) + note_formatter = described_class.new(note: note) + + expect(note_formatter.formatted_note).to eq(expected) + expect(note_formatter.contains_suggestion?).to eq(true) end it 'formats multi-line suggestions' do @@ -133,7 +157,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat ``` BODY - expect(described_class.formatted_note_for(note: note, start_line: 6, end_line: 8)).to eq(expected) + note_formatter = described_class.new(note: note, start_line: 6, end_line: 8) + + expect(note_formatter.formatted_note).to eq(expected) + expect(note_formatter.contains_suggestion?).to eq(true) end it 'formats multiple multi-line suggestions' do @@ -159,6 +186,9 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat ``` BODY - expect(described_class.formatted_note_for(note: note, start_line: 6, end_line: 8)).to eq(expected) + note_formatter = described_class.new(note: note, start_line: 6, end_line: 8) + + expect(note_formatter.formatted_note).to eq(expected) + expect(note_formatter.contains_suggestion?).to eq(true) end end |