diff options
Diffstat (limited to 'spec/lib/gitlab/github_import')
10 files changed, 527 insertions, 156 deletions
diff --git a/spec/lib/gitlab/github_import/attachments_downloader_spec.rb b/spec/lib/gitlab/github_import/attachments_downloader_spec.rb index 57391e06192..dc9f939a19b 100644 --- a/spec/lib/gitlab/github_import/attachments_downloader_spec.rb +++ b/spec/lib/gitlab/github_import/attachments_downloader_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Gitlab::GithubImport::AttachmentsDownloader do let_it_be(:content_type) { 'application/octet-stream' } let(:content_length) { 1000 } - let(:chunk_double) { instance_double(HTTParty::FragmentWithResponse, code: 200) } + let(:chunk_double) { instance_double(HTTParty::ResponseFragment, code: 200) } let(:headers_double) do instance_double( HTTParty::Response, diff --git a/spec/lib/gitlab/github_import/client_spec.rb b/spec/lib/gitlab/github_import/client_spec.rb index 3361b039a27..95f7933fbc5 100644 --- a/spec/lib/gitlab/github_import/client_spec.rb +++ b/spec/lib/gitlab/github_import/client_spec.rb @@ -3,24 +3,24 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Client do - describe '#parallel?' do - it 'returns true when the client is running in parallel mode' do - client = described_class.new('foo', parallel: true) + subject(:client) { described_class.new('foo', parallel: parallel) } + + let(:parallel) { true } - expect(client).to be_parallel + describe '#parallel?' do + context 'when the client is running in parallel mode' do + it { expect(client).to be_parallel } end - it 'returns false when the client is running in sequential mode' do - client = described_class.new('foo', parallel: false) + context 'when the client is running in sequential mode' do + let(:parallel) { false } - expect(client).not_to be_parallel + it { expect(client).not_to be_parallel } end end describe '#user' do it 'returns the details for the given username' do - client = described_class.new('foo') - expect(client.octokit).to receive(:user).with('foo') expect(client).to receive(:with_rate_limit).and_yield @@ -30,8 +30,6 @@ RSpec.describe Gitlab::GithubImport::Client do describe '#pull_request_reviews' do it 'returns the pull request reviews' do - client = described_class.new('foo') - expect(client) .to receive(:each_object) .with(:pull_request_reviews, 'foo/bar', 999) @@ -40,10 +38,17 @@ RSpec.describe Gitlab::GithubImport::Client do end end + describe '#pull_request_review_requests' do + it 'returns the pull request review requests' do + expect(client.octokit).to receive(:pull_request_review_requests).with('foo/bar', 999) + expect(client).to receive(:with_rate_limit).and_yield + + client.pull_request_review_requests('foo/bar', 999) + end + end + describe '#repos' do it 'returns the user\'s repositories as a hash' do - client = described_class.new('foo') - stub_request(:get, 'https://api.github.com/rate_limit') .to_return(status: 200, headers: { 'X-RateLimit-Limit' => 5000, 'X-RateLimit-Remaining' => 5000 }) @@ -58,8 +63,6 @@ RSpec.describe Gitlab::GithubImport::Client do describe '#repository' do it 'returns the details of a repository' do - client = described_class.new('foo') - expect(client.octokit).to receive(:repo).with('foo/bar') expect(client).to receive(:with_rate_limit).and_yield @@ -67,8 +70,6 @@ RSpec.describe Gitlab::GithubImport::Client do end it 'returns repository data as a hash' do - client = described_class.new('foo') - stub_request(:get, 'https://api.github.com/rate_limit') .to_return(status: 200, headers: { 'X-RateLimit-Limit' => 5000, 'X-RateLimit-Remaining' => 5000 }) @@ -83,8 +84,6 @@ RSpec.describe Gitlab::GithubImport::Client do describe '#pull_request' do it 'returns the details of a pull_request' do - client = described_class.new('foo') - expect(client.octokit).to receive(:pull_request).with('foo/bar', 999) expect(client).to receive(:with_rate_limit).and_yield @@ -94,8 +93,6 @@ RSpec.describe Gitlab::GithubImport::Client do describe '#labels' do it 'returns the labels' do - client = described_class.new('foo') - expect(client) .to receive(:each_object) .with(:labels, 'foo/bar') @@ -106,8 +103,6 @@ RSpec.describe Gitlab::GithubImport::Client do describe '#milestones' do it 'returns the milestones' do - client = described_class.new('foo') - expect(client) .to receive(:each_object) .with(:milestones, 'foo/bar') @@ -118,8 +113,6 @@ RSpec.describe Gitlab::GithubImport::Client do describe '#releases' do it 'returns the releases' do - client = described_class.new('foo') - expect(client) .to receive(:each_object) .with(:releases, 'foo/bar') @@ -130,8 +123,6 @@ RSpec.describe Gitlab::GithubImport::Client do describe '#branches' do it 'returns the branches' do - client = described_class.new('foo') - expect(client) .to receive(:each_object) .with(:branches, 'foo/bar') @@ -142,8 +133,6 @@ RSpec.describe Gitlab::GithubImport::Client do describe '#branch_protection' do it 'returns the protection details for the given branch' do - client = described_class.new('foo') - expect(client.octokit) .to receive(:branch_protection).with('org/repo', 'bar') expect(client).to receive(:with_rate_limit).and_yield @@ -156,8 +145,6 @@ RSpec.describe Gitlab::GithubImport::Client do describe '#each_object' do it 'converts each object into a hash' do - client = described_class.new('foo') - stub_request(:get, 'https://api.github.com/rate_limit') .to_return(status: 200, headers: { 'X-RateLimit-Limit' => 5000, 'X-RateLimit-Remaining' => 5000 }) @@ -171,7 +158,6 @@ RSpec.describe Gitlab::GithubImport::Client do end describe '#each_page' do - let(:client) { described_class.new('foo') } let(:object1) { double(:object1) } let(:object2) { double(:object2) } @@ -242,8 +228,6 @@ RSpec.describe Gitlab::GithubImport::Client do end describe '#with_rate_limit' do - let(:client) { described_class.new('foo') } - it 'yields the supplied block when enough requests remain' do expect(client).to receive(:requests_remaining?).and_return(true) @@ -340,8 +324,6 @@ RSpec.describe Gitlab::GithubImport::Client do end describe '#requests_remaining?' do - let(:client) { described_class.new('foo') } - context 'when default requests limit is set' do before do allow(client).to receive(:requests_limit).and_return(5000) @@ -380,44 +362,43 @@ RSpec.describe Gitlab::GithubImport::Client do end describe '#raise_or_wait_for_rate_limit' do - it 'raises RateLimitError when running in parallel mode' do - client = described_class.new('foo', parallel: true) - - expect { client.raise_or_wait_for_rate_limit } - .to raise_error(Gitlab::GithubImport::RateLimitError) + context 'when running in parallel mode' do + it 'raises RateLimitError' do + expect { client.raise_or_wait_for_rate_limit } + .to raise_error(Gitlab::GithubImport::RateLimitError) + end end - it 'sleeps when running in sequential mode' do - client = described_class.new('foo', parallel: false) - - expect(client).to receive(:rate_limit_resets_in).and_return(1) - expect(client).to receive(:sleep).with(1) + context 'when running in sequential mode' do + let(:parallel) { false } - client.raise_or_wait_for_rate_limit - end + it 'sleeps' do + expect(client).to receive(:rate_limit_resets_in).and_return(1) + expect(client).to receive(:sleep).with(1) - it 'increments the rate limit counter' do - client = described_class.new('foo', parallel: false) + client.raise_or_wait_for_rate_limit + end - expect(client) - .to receive(:rate_limit_resets_in) - .and_return(1) + it 'increments the rate limit counter' do + expect(client) + .to receive(:rate_limit_resets_in) + .and_return(1) - expect(client) - .to receive(:sleep) - .with(1) + expect(client) + .to receive(:sleep) + .with(1) - expect(client.rate_limit_counter) - .to receive(:increment) - .and_call_original + expect(client.rate_limit_counter) + .to receive(:increment) + .and_call_original - client.raise_or_wait_for_rate_limit + client.raise_or_wait_for_rate_limit + end end end describe '#remaining_requests' do it 'returns the number of remaining requests' do - client = described_class.new('foo') rate_limit = double(remaining: 1) expect(client.octokit).to receive(:rate_limit).and_return(rate_limit) @@ -427,7 +408,6 @@ RSpec.describe Gitlab::GithubImport::Client do describe '#requests_limit' do it 'returns requests limit' do - client = described_class.new('foo') rate_limit = double(limit: 1) expect(client.octokit).to receive(:rate_limit).and_return(rate_limit) @@ -437,7 +417,6 @@ RSpec.describe Gitlab::GithubImport::Client do describe '#rate_limit_resets_in' do it 'returns the number of seconds after which the rate limit is reset' do - client = described_class.new('foo') rate_limit = double(resets_in: 1) expect(client.octokit).to receive(:rate_limit).and_return(rate_limit) @@ -447,8 +426,6 @@ RSpec.describe Gitlab::GithubImport::Client do end describe '#api_endpoint' do - let(:client) { described_class.new('foo') } - context 'without a custom endpoint configured in Omniauth' do it 'returns the default API endpoint' do expect(client) @@ -473,8 +450,6 @@ RSpec.describe Gitlab::GithubImport::Client do end describe '#web_endpoint' do - let(:client) { described_class.new('foo') } - context 'without a custom endpoint configured in Omniauth' do it 'returns the default web endpoint' do expect(client) @@ -499,8 +474,6 @@ RSpec.describe Gitlab::GithubImport::Client do end describe '#custom_api_endpoint' do - let(:client) { described_class.new('foo') } - context 'without a custom endpoint' do it 'returns nil' do expect(client) @@ -533,8 +506,6 @@ RSpec.describe Gitlab::GithubImport::Client do end describe '#verify_ssl' do - let(:client) { described_class.new('foo') } - context 'without a custom configuration' do it 'returns true' do expect(client) @@ -553,8 +524,6 @@ RSpec.describe Gitlab::GithubImport::Client do end describe '#github_omniauth_provider' do - let(:client) { described_class.new('foo') } - context 'without a configured provider' do it 'returns an empty Hash' do expect(Gitlab.config.omniauth) @@ -576,8 +545,6 @@ RSpec.describe Gitlab::GithubImport::Client do end describe '#rate_limiting_enabled?' do - let(:client) { described_class.new('foo') } - it 'returns true when using GitHub.com' do expect(client.rate_limiting_enabled?).to eq(true) end @@ -592,7 +559,6 @@ RSpec.describe Gitlab::GithubImport::Client do end describe 'search' do - let(:client) { described_class.new('foo') } let(:user) { { login: 'user' } } let(:org1) { { login: 'org1' } } let(:org2) { { login: 'org2' } } diff --git a/spec/lib/gitlab/github_import/importer/events/changed_label_spec.rb b/spec/lib/gitlab/github_import/importer/events/changed_label_spec.rb index 4476b4123ee..6a409762599 100644 --- a/spec/lib/gitlab/github_import/importer/events/changed_label_spec.rb +++ b/spec/lib/gitlab/github_import/importer/events/changed_label_spec.rb @@ -10,7 +10,9 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedLabel do let(:client) { instance_double('Gitlab::GithubImport::Client') } let(:issuable) { create(:issue, project: project) } - let!(:label) { create(:label, project: project) } + let(:label) { create(:label, project: project) } + let(:label_title) { label.title } + let(:label_id) { label.id } let(:issue_event) do Gitlab::GithubImport::Representation::IssueEvent.from_json_hash( @@ -18,7 +20,7 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedLabel do 'actor' => { 'id' => user.id, 'login' => user.username }, 'event' => event_type, 'commit_id' => nil, - 'label_title' => label.title, + 'label_title' => label_title, 'created_at' => '2022-04-26 18:30:53 UTC', 'issue' => { 'number' => issuable.iid, pull_request: issuable.is_a?(MergeRequest) } ) @@ -27,7 +29,7 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedLabel do let(:event_attrs) do { user_id: user.id, - label_id: label.id, + label_id: label_id, created_at: issue_event.created_at }.stringify_keys end @@ -42,7 +44,6 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedLabel do end before do - allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(label.id) allow_next_instance_of(Gitlab::GithubImport::IssuableFinder) do |finder| allow(finder).to receive(:database_id).and_return(issuable.id) end @@ -52,16 +53,35 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedLabel do end context 'with Issue' do - context 'when importing a labeled event' do - let(:event_type) { 'labeled' } - let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'add') } + context 'when importing event with associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(label.id) + end - it_behaves_like 'new event' + context 'when importing a labeled event' do + let(:event_type) { 'labeled' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + end + + context 'when importing an unlabeled event' do + let(:event_type) { 'unlabeled' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'remove') } + + it_behaves_like 'new event' + end end - context 'when importing an unlabeled event' do - let(:event_type) { 'unlabeled' } - let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'remove') } + context 'when importing event without associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(nil) + end + + let(:label_title) { 'deleted_label' } + let(:label_id) { nil } + let(:event_type) { 'labeled' } + let(:expected_event_attrs) { event_attrs.merge(issue_id: issuable.id, action: 'add') } it_behaves_like 'new event' end @@ -70,16 +90,35 @@ RSpec.describe Gitlab::GithubImport::Importer::Events::ChangedLabel do context 'with MergeRequest' do let(:issuable) { create(:merge_request, source_project: project, target_project: project) } - context 'when importing a labeled event' do - let(:event_type) { 'labeled' } - let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'add') } + context 'when importing event with associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(label.id) + end - it_behaves_like 'new event' + context 'when importing a labeled event' do + let(:event_type) { 'labeled' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'add') } + + it_behaves_like 'new event' + end + + context 'when importing an unlabeled event' do + let(:event_type) { 'unlabeled' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'remove') } + + it_behaves_like 'new event' + end end - context 'when importing an unlabeled event' do - let(:event_type) { 'unlabeled' } - let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'remove') } + context 'when importing event without associated label' do + before do + allow(Gitlab::Cache::Import::Caching).to receive(:read_integer).and_return(nil) + end + + let(:label_title) { 'deleted_label' } + let(:label_id) { nil } + let(:event_type) { 'labeled' } + let(:expected_event_attrs) { event_attrs.merge(merge_request_id: issuable.id, action: 'add') } it_behaves_like 'new event' end diff --git a/spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb b/spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb index 027b2ac422e..d6b7411e640 100644 --- a/spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/protected_branch_importer_spec.rb @@ -6,20 +6,23 @@ RSpec.describe Gitlab::GithubImport::Importer::ProtectedBranchImporter do subject(:importer) { described_class.new(github_protected_branch, project, client) } let(:branch_name) { 'protection' } - let(:allow_force_pushes_on_github) { true } + let(:allow_force_pushes_on_github) { false } + let(:require_code_owner_reviews_on_github) { false } let(:required_conversation_resolution) { false } let(:required_signatures) { false } let(:required_pull_request_reviews) { false } let(:expected_push_access_level) { Gitlab::Access::MAINTAINER } let(:expected_merge_access_level) { Gitlab::Access::MAINTAINER } - let(:expected_allow_force_push) { true } + let(:expected_allow_force_push) { false } + let(:expected_code_owner_approval_required) { false } let(:github_protected_branch) do Gitlab::GithubImport::Representation::ProtectedBranch.new( id: branch_name, allow_force_pushes: allow_force_pushes_on_github, required_conversation_resolution: required_conversation_resolution, required_signatures: required_signatures, - required_pull_request_reviews: required_pull_request_reviews + required_pull_request_reviews: required_pull_request_reviews, + require_code_owner_reviews: require_code_owner_reviews_on_github ) end @@ -35,7 +38,8 @@ RSpec.describe Gitlab::GithubImport::Importer::ProtectedBranchImporter do name: 'protection', push_access_levels_attributes: [{ access_level: expected_push_access_level }], merge_access_levels_attributes: [{ access_level: expected_merge_access_level }], - allow_force_push: expected_allow_force_push + allow_force_push: expected_allow_force_push, + code_owner_approval_required: expected_code_owner_approval_required } end @@ -70,41 +74,35 @@ RSpec.describe Gitlab::GithubImport::Importer::ProtectedBranchImporter do end context 'when branch is protected on GitLab' do - before do - create( - :protected_branch, - project: project, - name: 'protect*', - allow_force_push: allow_force_pushes_on_gitlab - ) + using RSpec::Parameterized::TableSyntax + + where( + :allow_force_pushes_on_github, + :allow_force_pushes_on_gitlab, + :expected_allow_force_push + ) do + true | true | true + true | false | false + false | true | false + false | false | false end - context 'when branch protection rule on Gitlab is stricter than on Github' do - let(:allow_force_pushes_on_github) { true } - let(:allow_force_pushes_on_gitlab) { false } - let(:expected_allow_force_push) { false } - - it_behaves_like 'create branch protection by the strictest ruleset' - end - - context 'when branch protection rule on Github is stricter than on Gitlab' do - let(:allow_force_pushes_on_github) { false } - let(:allow_force_pushes_on_gitlab) { true } - let(:expected_allow_force_push) { false } - - it_behaves_like 'create branch protection by the strictest ruleset' - end - - context 'when branch protection rules on Github and Gitlab are the same' do - let(:allow_force_pushes_on_github) { true } - let(:allow_force_pushes_on_gitlab) { true } - let(:expected_allow_force_push) { true } + with_them do + before do + create( + :protected_branch, + project: project, + name: 'protect*', + allow_force_push: allow_force_pushes_on_gitlab + ) + end it_behaves_like 'create branch protection by the strictest ruleset' end end context 'when branch is not protected on GitLab' do + let(:allow_force_pushes_on_github) { true } let(:expected_allow_force_push) { true } it_behaves_like 'create branch protection by the strictest ruleset' @@ -115,6 +113,30 @@ RSpec.describe Gitlab::GithubImport::Importer::ProtectedBranchImporter do allow(project).to receive(:default_branch).and_return(branch_name) end + context 'when "allow force pushes - everyone" rule is enabled' do + let(:allow_force_pushes_on_github) { true } + + context 'when there is any default branch protection' do + before do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_FULL) + end + + let(:expected_allow_force_push) { false } + + it_behaves_like 'create branch protection by the strictest ruleset' + end + + context 'when there is no default branch protection' do + before do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE) + end + + let(:expected_allow_force_push) { allow_force_pushes_on_github } + + it_behaves_like 'create branch protection by the strictest ruleset' + end + end + context 'when required_conversation_resolution rule is enabled' do let(:required_conversation_resolution) { true } @@ -241,7 +263,8 @@ RSpec.describe Gitlab::GithubImport::Importer::ProtectedBranchImporter do :protected_branch, project: project, name: 'protect*', - allow_force_push: true + allow_force_push: true, + code_owner_approval_required: false ) end @@ -297,5 +320,67 @@ RSpec.describe Gitlab::GithubImport::Importer::ProtectedBranchImporter do it_behaves_like 'create branch protection by the strictest ruleset' end end + + context 'when the code_owner_approval_required feature is available', if: Gitlab.ee? do + before do + stub_licensed_features(code_owner_approval_required: true) + end + + context 'when branch is protected on GitLab' do + using RSpec::Parameterized::TableSyntax + + where( + :require_code_owner_reviews_on_github, + :require_code_owner_reviews_on_gitlab, + :expected_code_owner_approval_required + ) do + true | true | true + true | false | true + false | true | true + false | false | false + end + + with_them do + before do + create( + :protected_branch, + project: project, + name: 'protect*', + allow_force_push: true, + code_owner_approval_required: require_code_owner_reviews_on_gitlab + ) + end + + it_behaves_like 'create branch protection by the strictest ruleset' + end + end + + context 'when branch is not protected on GitLab' do + context 'when require_code_owner_reviews rule is enabled on GitHub' do + let(:require_code_owner_reviews_on_github) { true } + let(:expected_code_owner_approval_required) { true } + + it_behaves_like 'create branch protection by the strictest ruleset' + end + + context 'when require_code_owner_reviews rule is disabled on GitHub' do + let(:require_code_owner_reviews_on_github) { false } + let(:expected_code_owner_approval_required) { false } + + it_behaves_like 'create branch protection by the strictest ruleset' + end + end + end + + context 'when the code_owner_approval_required feature is not available' do + before do + stub_licensed_features(code_owner_approval_required: false) + end + + let(:require_code_owner_reviews_on_github) { true } + let(:expected_code_owner_approval_required) { false } + + it_behaves_like 'create branch protection by the strictest ruleset' + end end end diff --git a/spec/lib/gitlab/github_import/importer/protected_branches_importer_spec.rb b/spec/lib/gitlab/github_import/importer/protected_branches_importer_spec.rb index a0ced456391..8809d58a252 100644 --- a/spec/lib/gitlab/github_import/importer/protected_branches_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/protected_branches_importer_spec.rb @@ -29,7 +29,10 @@ RSpec.describe Gitlab::GithubImport::Importer::ProtectedBranchesImporter do required_signatures = Struct.new(:url, :enabled, keyword_init: true) enforce_admins = Struct.new(:url, :enabled, keyword_init: true) allow_option = Struct.new(:enabled, keyword_init: true) - required_pull_request_reviews = Struct.new(:url, :dismissal_restrictions, keyword_init: true) + required_pull_request_reviews = Struct.new( + :url, :dismissal_restrictions, :require_code_owner_reviews, + keyword_init: true + ) response.new( name: 'main', url: 'https://example.com/branches/main/protection', @@ -58,7 +61,8 @@ RSpec.describe Gitlab::GithubImport::Importer::ProtectedBranchesImporter do ), required_pull_request_reviews: required_pull_request_reviews.new( url: 'https://example.com/branches/main/protection/required_pull_request_reviews', - dismissal_restrictions: {} + dismissal_restrictions: {}, + require_code_owner_reviews: true ) ) end @@ -160,6 +164,7 @@ RSpec.describe Gitlab::GithubImport::Importer::ProtectedBranchesImporter do let(:branch_struct) { Struct.new(:protection, :name, :url, keyword_init: true) } let(:protection_struct) { Struct.new(:enabled, keyword_init: true) } let(:protected_branch) { branch_struct.new(name: 'main', protection: protection_struct.new(enabled: true)) } + let(:second_protected_branch) { branch_struct.new(name: 'fix', protection: protection_struct.new(enabled: true)) } let(:unprotected_branch) { branch_struct.new(name: 'staging', protection: protection_struct.new(enabled: false)) } # when user has no admin rights on repo let(:unknown_protection_branch) { branch_struct.new(name: 'development', protection: nil) } @@ -168,9 +173,9 @@ RSpec.describe Gitlab::GithubImport::Importer::ProtectedBranchesImporter do before do allow(client).to receive(:branches).with(project.import_source) - .and_return([protected_branch, unprotected_branch, unknown_protection_branch]) + .and_return([protected_branch, second_protected_branch, unprotected_branch, unknown_protection_branch]) allow(client).to receive(:branch_protection) - .with(project.import_source, protected_branch.name).once + .with(project.import_source, anything) .and_return(github_protection_rule) allow(Gitlab::GithubImport::ObjectCounter).to receive(:increment) .with(project, :protected_branch, :fetched) @@ -180,12 +185,13 @@ RSpec.describe Gitlab::GithubImport::Importer::ProtectedBranchesImporter do subject.each_object_to_import do |object| expect(object).to eq github_protection_rule end - expect(Gitlab::GithubImport::ObjectCounter).to have_received(:increment).once + expect(Gitlab::GithubImport::ObjectCounter).to have_received(:increment).twice end context 'when protected branch is already processed' do it "doesn't process this branch" do subject.mark_as_imported(protected_branch) + subject.mark_as_imported(second_protected_branch) subject.each_object_to_import {} expect(Gitlab::GithubImport::ObjectCounter).not_to have_received(:increment) diff --git a/spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb index fb6024d0952..49794eceb5a 100644 --- a/spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb @@ -8,11 +8,48 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean let_it_be(:merge_request) { create(:merge_request) } let(:project) { merge_request.project } - let(:client_double) { double(user: { id: 999, login: 'author', email: 'author@email.com' }) } let(:submitted_at) { Time.new(2017, 1, 1, 12, 00).utc } + let(:client_double) do + instance_double( + 'Gitlab::GithubImport::Client', + user: { id: 999, login: 'author', email: 'author@email.com' } + ) + end subject { described_class.new(review, project, client_double) } + shared_examples 'imports a reviewer for the Merge Request' do + it 'creates reviewer for the Merge Request' do + expect { subject.execute }.to change(MergeRequestReviewer, :count).by(1) + + expect(merge_request.reviewers).to contain_exactly(author) + end + + context 'when reviewer already exists' do + before do + create( + :merge_request_reviewer, + reviewer: author, merge_request: merge_request, state: 'unreviewed' + ) + end + + it 'does not change Merge Request reviewers' do + expect { subject.execute }.not_to change(MergeRequestReviewer, :count) + + expect(merge_request.reviewers).to contain_exactly(author) + end + end + end + + shared_examples 'imports an approval for the Merge Request' do + it 'creates an approval for the Merge Request' do + expect { subject.execute }.to change(Approval, :count).by(1) + + expect(merge_request.approved_by_users.reload).to include(author) + expect(merge_request.approvals.last.created_at).to eq(submitted_at) + end + end + context 'when the review author can be mapped to a gitlab user' do let_it_be(:author) { create(:user, email: 'author@email.com') } @@ -20,34 +57,38 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean context 'when the review is "APPROVED"' do let(:review) { create_review(type: 'APPROVED', note: '') } - it 'creates a note for the review and approves the Merge Request' do - expect { subject.execute } - .to change(Note, :count).by(1) - .and change(Approval, :count).by(1) + it_behaves_like 'imports an approval for the Merge Request' + it_behaves_like 'imports a reviewer for the Merge Request' + + it 'creates a note for the review' do + expect { subject.execute }.to change(Note, :count).by(1) last_note = merge_request.notes.last expect(last_note.note).to eq('approved this merge request') expect(last_note.author).to eq(author) expect(last_note.created_at).to eq(submitted_at) expect(last_note.system_note_metadata.action).to eq('approved') - - expect(merge_request.approved_by_users.reload).to include(author) - expect(merge_request.approvals.last.created_at).to eq(submitted_at) end - it 'does nothing if the user already approved the merge request' do - create(:approval, merge_request: merge_request, user: author) + context 'when the user already approved the merge request' do + before do + create(:approval, merge_request: merge_request, user: author) + end - expect { subject.execute } - .to change(Note, :count).by(0) - .and change(Approval, :count).by(0) + it 'does not import second approve and note' do + expect { subject.execute } + .to change(Note, :count).by(0) + .and change(Approval, :count).by(0) + end end end context 'when the review is "COMMENTED"' do let(:review) { create_review(type: 'COMMENTED', note: '') } - it 'creates a note for the review' do + it_behaves_like 'imports a reviewer for the Merge Request' + + it 'does not create note for the review' do expect { subject.execute }.not_to change(Note, :count) end end @@ -55,7 +96,9 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean context 'when the review is "CHANGES_REQUESTED"' do let(:review) { create_review(type: 'CHANGES_REQUESTED', note: '') } - it 'creates a note for the review' do + it_behaves_like 'imports a reviewer for the Merge Request' + + it 'does not create a note for the review' do expect { subject.execute }.not_to change(Note, :count) end end @@ -65,10 +108,11 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean context 'when the review is "APPROVED"' do let(:review) { create_review(type: 'APPROVED') } + it_behaves_like 'imports an approval for the Merge Request' + it_behaves_like 'imports a reviewer for the Merge Request' + it 'creates a note for the review' do - expect { subject.execute } - .to change(Note, :count).by(2) - .and change(Approval, :count).by(1) + expect { subject.execute }.to change(Note, :count).by(2) note = merge_request.notes.where(system: false).last expect(note.note).to eq("**Review:** Approved\n\nnote") @@ -80,9 +124,6 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean expect(system_note.author).to eq(author) expect(system_note.created_at).to eq(submitted_at) expect(system_note.system_note_metadata.action).to eq('approved') - - expect(merge_request.approved_by_users.reload).to include(author) - expect(merge_request.approvals.last.created_at).to eq(submitted_at) end end diff --git a/spec/lib/gitlab/github_import/importer/pull_requests/review_request_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_requests/review_request_importer_spec.rb new file mode 100644 index 00000000000..6dcbc4e32e6 --- /dev/null +++ b/spec/lib/gitlab/github_import/importer/pull_requests/review_request_importer_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::GithubImport::Importer::PullRequests::ReviewRequestImporter, :clean_gitlab_redis_cache do + subject(:importer) { described_class.new(review_request, project, client) } + + let(:project) { instance_double('Project') } + let(:client) { instance_double('Gitlab::GithubImport::Client') } + let(:merge_request) { create(:merge_request) } + let(:reviewer) { create(:user, username: 'alice') } + let(:review_request) do + Gitlab::GithubImport::Representation::PullRequests::ReviewRequests.from_json_hash( + merge_request_id: merge_request.id, + users: [ + { 'id' => 1, 'login' => reviewer.username }, + { 'id' => 2, 'login' => 'foo' } + ] + ) + end + + before do + allow_next_instance_of(Gitlab::GithubImport::UserFinder) do |finder| + allow(finder).to receive(:find).with(1, reviewer.username).and_return(reviewer.id) + allow(finder).to receive(:find).with(2, 'foo').and_return(nil) + end + end + + it 'imports merge request reviewers that were found' do + importer.execute + + expect(merge_request.reviewers.size).to eq 1 + expect(merge_request.reviewers[0].id).to eq reviewer.id + end +end diff --git a/spec/lib/gitlab/github_import/importer/pull_requests/review_requests_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_requests/review_requests_importer_spec.rb new file mode 100644 index 00000000000..6c7fc4d5b15 --- /dev/null +++ b/spec/lib/gitlab/github_import/importer/pull_requests/review_requests_importer_spec.rb @@ -0,0 +1,141 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::GithubImport::Importer::PullRequests::ReviewRequestsImporter, :clean_gitlab_redis_cache do + subject(:importer) { described_class.new(project, client) } + + let_it_be(:project) { create(:project, import_source: 'foo') } + + let(:client) { instance_double(Gitlab::GithubImport::Client) } + let(:review_request_struct) { Struct.new(:merge_request_id, :users, keyword_init: true) } + let(:user_struct) { Struct.new(:id, :login, keyword_init: true) } + + shared_context 'when project with merge requests' do + let_it_be(:merge_request_1) { create(:merge_request, source_project: project, target_branch: 'feature1') } + let_it_be(:merge_request_2) { create(:merge_request, source_project: project, target_branch: 'feature2') } + + let(:importer_stub) { instance_double('Gitlab::GithubImport::Importer::NoteAttachmentsImporter') } + let(:importer_attrs) do + [instance_of(Gitlab::GithubImport::Representation::PullRequests::ReviewRequests), project, client] + end + + let(:review_requests_1) do + { + users: [ + { id: 4, login: 'alice' }, + { id: 5, login: 'bob' } + ] + } + end + + let(:review_requests_2) do + { + users: [{ id: 4, login: 'alice' }] + } + end + + before do + allow(client).to receive(:pull_request_review_requests) + .with(project.import_source, merge_request_1.iid).and_return(review_requests_1) + allow(client).to receive(:pull_request_review_requests) + .with(project.import_source, merge_request_2.iid).and_return(review_requests_2) + end + end + + describe '#sequential_import' do + include_context 'when project with merge requests' + + it 'imports each project merge request reviewers' do + expect_next_instances_of( + Gitlab::GithubImport::Importer::PullRequests::ReviewRequestImporter, 2, false, *importer_attrs + ) do |note_attachments_importer| + expect(note_attachments_importer).to receive(:execute) + end + + importer.sequential_import + end + + context 'when merge request is already processed' do + before do + Gitlab::Cache::Import::Caching.set_add( + "github-importer/pull_requests/pull_request_review_requests/already-imported/#{project.id}", + merge_request_1.iid + ) + end + + it "doesn't import this merge request reviewers" do + expect_next_instance_of( + Gitlab::GithubImport::Importer::PullRequests::ReviewRequestImporter, *importer_attrs + ) do |note_attachments_importer| + expect(note_attachments_importer).to receive(:execute) + end + + importer.sequential_import + end + end + end + + describe '#parallel_import' do + include_context 'when project with merge requests' + + let(:expected_worker_payload) do + [ + [ + project.id, + { + merge_request_id: merge_request_1.id, + users: [ + { id: 4, login: 'alice' }, + { id: 5, login: 'bob' } + ] + }, + instance_of(String) + ], + [ + project.id, + { + merge_request_id: merge_request_2.id, + users: [ + { id: 4, login: 'alice' } + ] + }, + instance_of(String) + ] + ] + end + + it 'schedule import for each merge request reviewers' do + expect(Gitlab::GithubImport::PullRequests::ImportReviewRequestWorker) + .to receive(:bulk_perform_in).with( + 1.second, + expected_worker_payload, + batch_size: 1000, + batch_delay: 1.minute + ) + + importer.parallel_import + end + + context 'when merge request is already processed' do + before do + Gitlab::Cache::Import::Caching.set_add( + "github-importer/pull_requests/pull_request_review_requests/already-imported/#{project.id}", + merge_request_1.iid + ) + end + + it "doesn't schedule import this merge request reviewers" do + expect(Gitlab::GithubImport::PullRequests::ImportReviewRequestWorker) + .to receive(:bulk_perform_in).with( + 1.second, + expected_worker_payload.slice(1, 1), + batch_size: 1000, + batch_delay: 1.minute + ) + + importer.parallel_import + end + end + end +end diff --git a/spec/lib/gitlab/github_import/representation/protected_branch_spec.rb b/spec/lib/gitlab/github_import/representation/protected_branch_spec.rb index 30b29659eee..60cae79459e 100644 --- a/spec/lib/gitlab/github_import/representation/protected_branch_spec.rb +++ b/spec/lib/gitlab/github_import/representation/protected_branch_spec.rb @@ -24,6 +24,10 @@ RSpec.describe Gitlab::GithubImport::Representation::ProtectedBranch do it 'includes the protected branch required_pull_request_reviews' do expect(protected_branch.required_pull_request_reviews).to eq true end + + it 'includes the protected branch require_code_owner_reviews' do + expect(protected_branch.require_code_owner_reviews).to eq true + end end end @@ -35,7 +39,10 @@ RSpec.describe Gitlab::GithubImport::Representation::ProtectedBranch do keyword_init: true ) enabled_setting = Struct.new(:enabled, keyword_init: true) - required_pull_request_reviews = Struct.new(:url, :dismissal_restrictions, keyword_init: true) + required_pull_request_reviews = Struct.new( + :url, :dismissal_restrictions, :require_code_owner_reviews, + keyword_init: true + ) response.new( url: 'https://example.com/branches/main/protection', allow_force_pushes: enabled_setting.new( @@ -49,7 +56,8 @@ RSpec.describe Gitlab::GithubImport::Representation::ProtectedBranch do ), required_pull_request_reviews: required_pull_request_reviews.new( url: 'https://example.com/branches/main/protection/required_pull_request_reviews', - dismissal_restrictions: {} + dismissal_restrictions: {}, + require_code_owner_reviews: true ) ) end @@ -67,7 +75,8 @@ RSpec.describe Gitlab::GithubImport::Representation::ProtectedBranch do 'allow_force_pushes' => true, 'required_conversation_resolution' => true, 'required_signatures' => true, - 'required_pull_request_reviews' => true + 'required_pull_request_reviews' => true, + 'require_code_owner_reviews' => true } end diff --git a/spec/lib/gitlab/github_import/representation/pull_requests/review_requests_spec.rb b/spec/lib/gitlab/github_import/representation/pull_requests/review_requests_spec.rb new file mode 100644 index 00000000000..0393f692a69 --- /dev/null +++ b/spec/lib/gitlab/github_import/representation/pull_requests/review_requests_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::GithubImport::Representation::PullRequests::ReviewRequests do + shared_examples 'Review requests' do + it 'returns an instance of Review Request' do + expect(review_requests).to be_an_instance_of(described_class) + end + + context 'for returned Review Requests' do + it 'includes merge request id' do + expect(review_requests.merge_request_id).to eq(merge_request_id) + end + + it 'includes reviewers' do + expect(review_requests.users.size).to eq 2 + + user = review_requests.users[0] + expect(user).to be_an_instance_of(Gitlab::GithubImport::Representation::User) + expect(user.id).to eq(4) + expect(user.login).to eq('alice') + end + end + end + + let(:merge_request_id) { 6501124486 } + let(:response) do + { + 'merge_request_id' => merge_request_id, + 'users' => [ + { 'id' => 4, 'login' => 'alice' }, + { 'id' => 5, 'login' => 'bob' } + ] + } + end + + describe '.from_api_response' do + it_behaves_like 'Review requests' do + let(:review_requests) { described_class.from_api_response(response) } + end + end + + describe '.from_json_hash' do + it_behaves_like 'Review requests' do + let(:review_requests) { described_class.from_json_hash(response) } + end + end +end |