diff options
Diffstat (limited to 'spec/lib/gitlab/background_migration/fix_merge_request_diff_commit_users_spec.rb')
| -rw-r--r-- | spec/lib/gitlab/background_migration/fix_merge_request_diff_commit_users_spec.rb | 316 |
1 files changed, 316 insertions, 0 deletions
diff --git a/spec/lib/gitlab/background_migration/fix_merge_request_diff_commit_users_spec.rb b/spec/lib/gitlab/background_migration/fix_merge_request_diff_commit_users_spec.rb new file mode 100644 index 00000000000..c343ee438b8 --- /dev/null +++ b/spec/lib/gitlab/background_migration/fix_merge_request_diff_commit_users_spec.rb @@ -0,0 +1,316 @@ +# frozen_string_literal: true + +require 'spec_helper' + +# The underlying migration relies on the global models (e.g. Project). This +# means we also need to use FactoryBot factories to ensure everything is +# operating using the same types. If we use `table()` and similar methods we +# would have to duplicate a lot of logic just for these tests. +# +# rubocop: disable RSpec/FactoriesInMigrationSpecs +RSpec.describe Gitlab::BackgroundMigration::FixMergeRequestDiffCommitUsers do + let(:migration) { described_class.new } + + describe '#perform' do + context 'when the project exists' do + it 'processes the project' do + project = create(:project) + + expect(migration).to receive(:process).with(project) + expect(migration).to receive(:schedule_next_job) + + migration.perform(project.id) + end + + it 'marks the background job as finished' do + project = create(:project) + + Gitlab::Database::BackgroundMigrationJob.create!( + class_name: 'FixMergeRequestDiffCommitUsers', + arguments: [project.id] + ) + + migration.perform(project.id) + + job = Gitlab::Database::BackgroundMigrationJob + .find_by(class_name: 'FixMergeRequestDiffCommitUsers') + + expect(job.status).to eq('succeeded') + end + end + + context 'when the project does not exist' do + it 'does nothing' do + expect(migration).not_to receive(:process) + expect(migration).to receive(:schedule_next_job) + + migration.perform(-1) + end + end + end + + describe '#process' do + it 'processes the merge requests of the project' do + project = create(:project, :repository) + commit = project.commit + mr = create( + :merge_request_with_diffs, + source_project: project, + target_project: project + ) + + diff = mr.merge_request_diffs.first + + create( + :merge_request_diff_commit, + merge_request_diff: diff, + sha: commit.sha, + relative_order: 9000 + ) + + migration.process(project) + + updated = diff + .merge_request_diff_commits + .find_by(sha: commit.sha, relative_order: 9000) + + expect(updated.commit_author_id).not_to be_nil + expect(updated.committer_id).not_to be_nil + end + end + + describe '#update_commit' do + let(:project) { create(:project, :repository) } + let(:mr) do + create( + :merge_request_with_diffs, + source_project: project, + target_project: project + ) + end + + let(:diff) { mr.merge_request_diffs.first } + let(:commit) { project.commit } + + def update_row(migration, project, diff, row) + migration.update_commit(project, row) + + diff + .merge_request_diff_commits + .find_by(sha: row.sha, relative_order: row.relative_order) + end + + it 'populates missing commit authors' do + commit_row = create( + :merge_request_diff_commit, + merge_request_diff: diff, + sha: commit.sha, + relative_order: 9000 + ) + + updated = update_row(migration, project, diff, commit_row) + + expect(updated.commit_author.name).to eq(commit.to_hash[:author_name]) + expect(updated.commit_author.email).to eq(commit.to_hash[:author_email]) + end + + it 'populates missing committers' do + commit_row = create( + :merge_request_diff_commit, + merge_request_diff: diff, + sha: commit.sha, + relative_order: 9000 + ) + + updated = update_row(migration, project, diff, commit_row) + + expect(updated.committer.name).to eq(commit.to_hash[:committer_name]) + expect(updated.committer.email).to eq(commit.to_hash[:committer_email]) + end + + it 'leaves existing commit authors as-is' do + user = create(:merge_request_diff_commit_user) + commit_row = create( + :merge_request_diff_commit, + merge_request_diff: diff, + sha: commit.sha, + relative_order: 9000, + commit_author: user + ) + + updated = update_row(migration, project, diff, commit_row) + + expect(updated.commit_author).to eq(user) + end + + it 'leaves existing committers as-is' do + user = create(:merge_request_diff_commit_user) + commit_row = create( + :merge_request_diff_commit, + merge_request_diff: diff, + sha: commit.sha, + relative_order: 9000, + committer: user + ) + + updated = update_row(migration, project, diff, commit_row) + + expect(updated.committer).to eq(user) + end + + it 'does nothing when both the author and committer are present' do + user = create(:merge_request_diff_commit_user) + commit_row = create( + :merge_request_diff_commit, + merge_request_diff: diff, + sha: commit.sha, + relative_order: 9000, + committer: user, + commit_author: user + ) + + recorder = ActiveRecord::QueryRecorder.new do + migration.update_commit(project, commit_row) + end + + expect(recorder.count).to be_zero + end + + it 'does nothing if the commit does not exist in Git' do + user = create(:merge_request_diff_commit_user) + commit_row = create( + :merge_request_diff_commit, + merge_request_diff: diff, + sha: 'kittens', + relative_order: 9000, + committer: user, + commit_author: user + ) + + recorder = ActiveRecord::QueryRecorder.new do + migration.update_commit(project, commit_row) + end + + expect(recorder.count).to be_zero + end + + it 'does nothing when the committer/author are missing in the Git commit' do + user = create(:merge_request_diff_commit_user) + commit_row = create( + :merge_request_diff_commit, + merge_request_diff: diff, + sha: commit.sha, + relative_order: 9000, + committer: user, + commit_author: user + ) + + allow(migration).to receive(:find_or_create_user).and_return(nil) + + recorder = ActiveRecord::QueryRecorder.new do + migration.update_commit(project, commit_row) + end + + expect(recorder.count).to be_zero + end + end + + describe '#schedule_next_job' do + it 'schedules the next background migration' do + Gitlab::Database::BackgroundMigrationJob + .create!(class_name: 'FixMergeRequestDiffCommitUsers', arguments: [42]) + + expect(BackgroundMigrationWorker) + .to receive(:perform_in) + .with(2.minutes, 'FixMergeRequestDiffCommitUsers', [42]) + + migration.schedule_next_job + end + + it 'does nothing when there are no jobs' do + expect(BackgroundMigrationWorker) + .not_to receive(:perform_in) + + migration.schedule_next_job + end + end + + describe '#find_commit' do + let(:project) { create(:project, :repository) } + + it 'finds a commit using Git' do + commit = project.commit + found = migration.find_commit(project, commit.sha) + + expect(found).to eq(commit.to_hash) + end + + it 'caches the results' do + commit = project.commit + + migration.find_commit(project, commit.sha) + + expect { migration.find_commit(project, commit.sha) } + .not_to change { Gitlab::GitalyClient.get_request_count } + end + + it 'returns an empty hash if the commit does not exist' do + expect(migration.find_commit(project, 'kittens')).to eq({}) + end + end + + describe '#find_or_create_user' do + let(:project) { create(:project, :repository) } + + it 'creates missing users' do + commit = project.commit.to_hash + id = migration.find_or_create_user(commit, :author_name, :author_email) + + expect(MergeRequest::DiffCommitUser.count).to eq(1) + + created = MergeRequest::DiffCommitUser.first + + expect(created.name).to eq(commit[:author_name]) + expect(created.email).to eq(commit[:author_email]) + expect(created.id).to eq(id) + end + + it 'returns users that already exist' do + commit = project.commit.to_hash + user1 = migration.find_or_create_user(commit, :author_name, :author_email) + user2 = migration.find_or_create_user(commit, :author_name, :author_email) + + expect(user1).to eq(user2) + end + + it 'caches the results' do + commit = project.commit.to_hash + + migration.find_or_create_user(commit, :author_name, :author_email) + + recorder = ActiveRecord::QueryRecorder.new do + migration.find_or_create_user(commit, :author_name, :author_email) + end + + expect(recorder.count).to be_zero + end + + it 'returns nil if the commit details are missing' do + id = migration.find_or_create_user({}, :author_name, :author_email) + + expect(id).to be_nil + end + end + + describe '#matches_row' do + it 'returns the query matches for the composite primary key' do + row = double(:commit, merge_request_diff_id: 4, relative_order: 5) + arel = migration.matches_row(row) + + expect(arel.to_sql).to eq( + '("merge_request_diff_commits"."merge_request_diff_id", "merge_request_diff_commits"."relative_order") = (4, 5)' + ) + end + end +end +# rubocop: enable RSpec/FactoriesInMigrationSpecs |