diff options
Diffstat (limited to 'spec/lib/gitlab/github_import/importer')
6 files changed, 216 insertions, 135 deletions
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 |