diff options
Diffstat (limited to 'spec/services')
107 files changed, 2461 insertions, 902 deletions
diff --git a/spec/services/admin/propagate_service_template_spec.rb b/spec/services/admin/propagate_service_template_spec.rb deleted file mode 100644 index c8ca3173f99..00000000000 --- a/spec/services/admin/propagate_service_template_spec.rb +++ /dev/null @@ -1,60 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Admin::PropagateServiceTemplate do - describe '.propagate' do - let_it_be(:project) { create(:project) } - - let!(:service_template) do - Integrations::Pushover.create!( - template: true, - active: true, - push_events: false, - properties: { - device: 'MyDevice', - sound: 'mic', - priority: 4, - user_key: 'asdf', - api_key: '123456789' - } - ) - end - - it 'calls to PropagateIntegrationProjectWorker' do - expect(PropagateIntegrationProjectWorker).to receive(:perform_async) - .with(service_template.id, project.id, project.id) - - described_class.propagate(service_template) - end - - context 'with a project that has another service' do - before do - Integrations::Bamboo.create!( - active: true, - project: project, - properties: { - bamboo_url: 'http://gitlab.com', - username: 'mic', - password: 'password', - build_key: 'build' - } - ) - end - - it 'calls to PropagateIntegrationProjectWorker' do - expect(PropagateIntegrationProjectWorker).to receive(:perform_async) - .with(service_template.id, project.id, project.id) - - described_class.propagate(service_template) - end - end - - it 'does not create the service if it exists already' do - Integration.build_from_integration(service_template, project_id: project.id).save! - - expect { described_class.propagate(service_template) } - .not_to change { Integration.count } - end - end -end diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index 4124696ac08..b456f7a2745 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -17,11 +17,6 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do project.add_developer(current_user) end - shared_examples 'an unmodified token' do - it_behaves_like 'a valid token' - it { expect(payload['access']).not_to include(have_key('migration_eligible')) } - end - shared_examples 'a modified token with migration eligibility' do |eligible| it_behaves_like 'a valid token' it { expect(payload['access']).to include(include('migration_eligible' => eligible)) } @@ -71,7 +66,7 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do { scopes: ["repository:#{project.full_path}:pull"] } end - it_behaves_like 'an unmodified token' + it_behaves_like 'a modified token' end context 'with push action' do @@ -82,20 +77,12 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'a modified token' end - context 'with multiple actions including push' do + context 'with multiple actions' do let(:current_params) do { scopes: ["repository:#{project.full_path}:pull,push,delete"] } end it_behaves_like 'a modified token' end - - context 'with multiple actions excluding push' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull,delete"] } - end - - it_behaves_like 'an unmodified token' - end end end diff --git a/spec/services/auth/dependency_proxy_authentication_service_spec.rb b/spec/services/auth/dependency_proxy_authentication_service_spec.rb index 35e6d59b456..667f361dc34 100644 --- a/spec/services/auth/dependency_proxy_authentication_service_spec.rb +++ b/spec/services/auth/dependency_proxy_authentication_service_spec.rb @@ -21,6 +21,12 @@ RSpec.describe Auth::DependencyProxyAuthenticationService do end end + shared_examples 'returning a token' do + it 'returns a token' do + expect(subject[:token]).not_to be_nil + end + end + context 'dependency proxy is not enabled' do before do stub_config(dependency_proxy: { enabled: false }) @@ -36,15 +42,13 @@ RSpec.describe Auth::DependencyProxyAuthenticationService do end context 'with a deploy token as user' do - let_it_be(:user) { create(:deploy_token) } + let_it_be(:user) { create(:deploy_token, :group, :dependency_proxy_scopes) } - it_behaves_like 'returning', status: 403, message: 'access forbidden' + it_behaves_like 'returning a token' end context 'with a user' do - it 'returns a token' do - expect(subject[:token]).not_to be_nil - end + it_behaves_like 'returning a token' end end end diff --git a/spec/services/authorized_project_update/project_recalculate_per_user_service_spec.rb b/spec/services/authorized_project_update/project_recalculate_per_user_service_spec.rb new file mode 100644 index 00000000000..62862d0e558 --- /dev/null +++ b/spec/services/authorized_project_update/project_recalculate_per_user_service_spec.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AuthorizedProjectUpdate::ProjectRecalculatePerUserService, '#execute' do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:another_user) { create(:user) } + + subject(:execute) { described_class.new(project, user).execute } + + it 'returns success' do + expect(execute.success?).to eq(true) + end + + context 'when there are no changes to be made' do + it 'does not change authorizations' do + expect { execute }.not_to(change { ProjectAuthorization.count }) + end + end + + context 'when there are changes to be made' do + context 'when addition is required' do + before do + project.add_developer(user) + project.add_developer(another_user) + project.project_authorizations.where(user: [user, another_user]).delete_all + end + + it 'adds a new authorization record for the specific user' do + expect { execute }.to( + change { project.project_authorizations.where(user: user).count } + .from(0).to(1) + ) + end + + it 'does not add a new authorization record for the other user' do + expect { execute }.not_to( + change { project.project_authorizations.where(user: another_user).count } + ) + end + + it 'adds a new authorization record with the correct access level for the specific user' do + execute + + project_authorization = project.project_authorizations.where( + user: user, + access_level: Gitlab::Access::DEVELOPER + ) + + expect(project_authorization).to exist + end + end + + context 'when removal is required' do + before do + create(:project_authorization, user: user, project: project) + create(:project_authorization, user: another_user, project: project) + end + + it 'removes the authorization record for the specific user' do + expect { execute }.to( + change { project.project_authorizations.where(user: user).count } + .from(1).to(0) + ) + end + + it 'does not remove the authorization record for the other user' do + expect { execute }.not_to( + change { project.project_authorizations.where(user: another_user).count } + ) + end + end + + context 'when an update in access level is required' do + before do + project.add_developer(user) + project.add_developer(another_user) + project.project_authorizations.where(user: [user, another_user]).delete_all + create(:project_authorization, project: project, user: user, access_level: Gitlab::Access::GUEST) + create(:project_authorization, project: project, user: another_user, access_level: Gitlab::Access::GUEST) + end + + it 'updates the authorization of the specific user to the correct access level' do + expect { execute }.to( + change { project.project_authorizations.find_by(user: user).access_level } + .from(Gitlab::Access::GUEST).to(Gitlab::Access::DEVELOPER) + ) + end + + it 'does not update the authorization of the other user to the correct access level' do + expect { execute }.not_to( + change { project.project_authorizations.find_by(user: another_user).access_level } + .from(Gitlab::Access::GUEST) + ) + end + end + end +end diff --git a/spec/services/boards/issues/list_service_spec.rb b/spec/services/boards/issues/list_service_spec.rb index 2fd544ab949..bbdc178b234 100644 --- a/spec/services/boards/issues/list_service_spec.rb +++ b/spec/services/boards/issues/list_service_spec.rb @@ -55,6 +55,15 @@ RSpec.describe Boards::Issues::ListService do it_behaves_like 'issues list service' end + + context 'when filtering by type' do + it 'only returns the specified type' do + issue = create(:labeled_issue, project: project, milestone: m1, labels: [development, p1], issue_type: 'incident') + params = { board_id: board.id, id: list1.id, issue_types: 'incident' } + + expect(described_class.new(parent, user, params).execute).to eq [issue] + end + end end # rubocop: disable RSpec/MultipleMemoizedHelpers diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/bulk_create_integration_service_spec.rb index 4b0029e27cb..517222c0e69 100644 --- a/spec/services/bulk_create_integration_service_spec.rb +++ b/spec/services/bulk_create_integration_service_spec.rb @@ -21,7 +21,7 @@ RSpec.describe BulkCreateIntegrationService do described_class.new(integration, batch, association).execute expect(created_integration.attributes.except(*excluded_attributes)) - .to eq(integration.attributes.except(*excluded_attributes)) + .to eq(integration.reload.attributes.except(*excluded_attributes)) end context 'integration with data fields' do @@ -96,18 +96,4 @@ RSpec.describe BulkCreateIntegrationService do it_behaves_like 'updates inherit_from_id' end end - - context 'passing a template integration' do - let(:integration) { template_integration } - - context 'with a project association' do - let!(:project) { create(:project) } - let(:created_integration) { project.jira_integration } - let(:batch) { Project.where(id: project.id) } - let(:association) { 'project' } - let(:inherit_from_id) { integration.id } - - it_behaves_like 'creates integration from batch ids' - end - end end diff --git a/spec/services/bulk_update_integration_service_spec.rb b/spec/services/bulk_update_integration_service_spec.rb index b6b7d1936a2..c10a9b75648 100644 --- a/spec/services/bulk_update_integration_service_spec.rb +++ b/spec/services/bulk_update_integration_service_spec.rb @@ -51,11 +51,11 @@ RSpec.describe BulkUpdateIntegrationService do context 'with inherited integration' do it 'updates the integration', :aggregate_failures do - described_class.new(subgroup_integration, batch).execute + described_class.new(subgroup_integration.reload, batch).execute expect(integration.reload.inherit_from_id).to eq(group_integration.id) expect(integration.reload.attributes.except(*excluded_attributes)) - .to eq(subgroup_integration.attributes.except(*excluded_attributes)) + .to eq(subgroup_integration.reload.attributes.except(*excluded_attributes)) expect(excluded_integration.reload.inherit_from_id).not_to eq(group_integration.id) expect(excluded_integration.reload.attributes.except(*excluded_attributes)) @@ -76,4 +76,16 @@ RSpec.describe BulkUpdateIntegrationService do end end end + + it 'works with batch as an ActiveRecord::Relation' do + expect do + described_class.new(group_integration, Integration.where(id: integration.id)).execute + end.to change { integration.reload.url }.to(group_integration.url) + end + + it 'works with batch as an array of ActiveRecord objects' do + expect do + described_class.new(group_integration, [integration]).execute + end.to change { integration.reload.url }.to(group_integration.url) + end end diff --git a/spec/services/ci/after_requeue_job_service_spec.rb b/spec/services/ci/after_requeue_job_service_spec.rb index f8c49060ce0..df5ddcafb37 100644 --- a/spec/services/ci/after_requeue_job_service_spec.rb +++ b/spec/services/ci/after_requeue_job_service_spec.rb @@ -8,37 +8,41 @@ RSpec.describe Ci::AfterRequeueJobService do let(:pipeline) { create(:ci_pipeline, project: project) } + let!(:build) { create(:ci_build, pipeline: pipeline, stage_idx: 0, name: 'build') } let!(:test1) { create(:ci_build, :success, pipeline: pipeline, stage_idx: 1) } let!(:test2) { create(:ci_build, :skipped, pipeline: pipeline, stage_idx: 1) } - let!(:build) { create(:ci_build, pipeline: pipeline, stage_idx: 0, name: 'build') } + let!(:test3) { create(:ci_build, :skipped, :dependent, pipeline: pipeline, stage_idx: 1, needed: build) } + let!(:deploy) { create(:ci_build, :skipped, :dependent, pipeline: pipeline, stage_idx: 2, needed: test3) } subject(:execute_service) { described_class.new(project, user).execute(build) } it 'marks subsequent skipped jobs as processable' do expect(test1.reload).to be_success expect(test2.reload).to be_skipped + expect(test3.reload).to be_skipped + expect(deploy.reload).to be_skipped execute_service expect(test1.reload).to be_success expect(test2.reload).to be_created + expect(test3.reload).to be_created + expect(deploy.reload).to be_created end context 'when there is a job need from the same stage' do - let!(:test3) do + let!(:test4) do create(:ci_build, :skipped, + :dependent, pipeline: pipeline, stage_idx: 0, - scheduling_type: :dag) - end - - before do - create(:ci_build_need, build: test3, name: 'build') + scheduling_type: :dag, + needed: build) end it 'marks subsequent skipped jobs as processable' do - expect { execute_service }.to change { test3.reload.status }.from('skipped').to('created') + expect { execute_service }.to change { test4.reload.status }.from('skipped').to('created') end context 'with ci_same_stage_job_needs FF disabled' do @@ -47,7 +51,7 @@ RSpec.describe Ci::AfterRequeueJobService do end it 'does nothing with the build' do - expect { execute_service }.not_to change { test3.reload.status } + expect { execute_service }.not_to change { test4.reload.status } end end end diff --git a/spec/services/ci/append_build_trace_service_spec.rb b/spec/services/ci/append_build_trace_service_spec.rb index b251f00158f..487dbacbe90 100644 --- a/spec/services/ci/append_build_trace_service_spec.rb +++ b/spec/services/ci/append_build_trace_service_spec.rb @@ -75,25 +75,5 @@ RSpec.describe Ci::AppendBuildTraceService do expect(build.reload).to be_failed expect(build.failure_reason).to eq 'trace_size_exceeded' end - - context 'when the feature flag is disabled' do - before do - stub_feature_flags(ci_jobs_trace_size_limit: false) - end - - it 'appends trace chunks' do - stream_size = 1.25.megabytes - body_data = 'x' * stream_size - content_range = "0-#{stream_size}" - - result = described_class - .new(build, content_range: content_range) - .execute(body_data) - - expect(result.status).to eq 202 - expect(result.stream_size).to eq stream_size - expect(build.trace_chunks.count).to eq 10 - end - end end end diff --git a/spec/services/ci/build_cancel_service_spec.rb b/spec/services/ci/build_cancel_service_spec.rb new file mode 100644 index 00000000000..fe036dc1368 --- /dev/null +++ b/spec/services/ci/build_cancel_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::BuildCancelService do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:pipeline) { create(:ci_pipeline, project: project) } + + describe '#execute' do + subject(:execute) { described_class.new(build, user).execute } + + context 'when user is authorized to cancel the build' do + before do + project.add_maintainer(user) + end + + context 'when build is cancelable' do + let!(:build) { create(:ci_build, :cancelable, pipeline: pipeline) } + + it 'transits build to canceled', :aggregate_failures do + response = execute + + expect(response).to be_success + expect(response.payload.reload).to be_canceled + end + end + + context 'when build is not cancelable' do + let!(:build) { create(:ci_build, :canceled, pipeline: pipeline) } + + it 'responds with unprocessable entity', :aggregate_failures do + response = execute + + expect(response).to be_error + expect(response.http_status).to eq(:unprocessable_entity) + end + end + end + + context 'when user is not authorized to cancel the build' do + let!(:build) { create(:ci_build, :cancelable, pipeline: pipeline) } + + it 'responds with forbidden', :aggregate_failures do + response = execute + + expect(response).to be_error + expect(response.http_status).to eq(:forbidden) + end + end + end +end diff --git a/spec/services/ci/build_unschedule_service_spec.rb b/spec/services/ci/build_unschedule_service_spec.rb new file mode 100644 index 00000000000..d784d9a2754 --- /dev/null +++ b/spec/services/ci/build_unschedule_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::BuildUnscheduleService do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:pipeline) { create(:ci_pipeline, project: project) } + + describe '#execute' do + subject(:execute) { described_class.new(build, user).execute } + + context 'when user is authorized to unschedule the build' do + before do + project.add_maintainer(user) + end + + context 'when build is scheduled' do + let!(:build) { create(:ci_build, :scheduled, pipeline: pipeline) } + + it 'transits build to manual' do + response = execute + + expect(response).to be_success + expect(response.payload.reload).to be_manual + end + end + + context 'when build is not scheduled' do + let!(:build) { create(:ci_build, pipeline: pipeline) } + + it 'responds with unprocessable entity', :aggregate_failures do + response = execute + + expect(response).to be_error + expect(response.http_status).to eq(:unprocessable_entity) + end + end + end + + context 'when user is not authorized to unschedule the build' do + let!(:build) { create(:ci_build, :scheduled, pipeline: pipeline) } + + it 'responds with forbidden', :aggregate_failures do + response = execute + + expect(response).to be_error + expect(response.http_status).to eq(:forbidden) + end + end + end +end diff --git a/spec/services/ci/create_downstream_pipeline_service_spec.rb b/spec/services/ci/create_downstream_pipeline_service_spec.rb index 18bd59a17f0..2237fd76d07 100644 --- a/spec/services/ci/create_downstream_pipeline_service_spec.rb +++ b/spec/services/ci/create_downstream_pipeline_service_spec.rb @@ -624,6 +624,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do let(:primary_pipeline) do Ci::CreatePipelineService.new(upstream_project, upstream_project.owner, { ref: 'master' }) .execute(:push, save_on_errors: false) + .payload end let(:bridge) { primary_pipeline.processables.find_by(name: 'bridge-job') } diff --git a/spec/services/ci/create_pipeline_service/cache_spec.rb b/spec/services/ci/create_pipeline_service/cache_spec.rb index f9767a794db..f5f162e4578 100644 --- a/spec/services/ci/create_pipeline_service/cache_spec.rb +++ b/spec/services/ci/create_pipeline_service/cache_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Ci::CreatePipelineService do let(:ref) { 'refs/heads/master' } let(:source) { :push } let(:service) { described_class.new(project, user, { ref: ref }) } - let(:pipeline) { service.execute(source) } + let(:pipeline) { service.execute(source).payload } let(:job) { pipeline.builds.find_by(name: 'job') } before do diff --git a/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb b/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb index a42770aae20..c69c91593ae 100644 --- a/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb +++ b/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Ci::CreatePipelineService do let(:ref) { 'refs/heads/master' } let(:source) { :push } let(:service) { described_class.new(project, user, { ref: ref }) } - let(:pipeline) { service.execute(source) } + let(:pipeline) { service.execute(source).payload } before do stub_ci_pipeline_yaml_file(config) diff --git a/spec/services/ci/create_pipeline_service/cross_project_pipeline_spec.rb b/spec/services/ci/create_pipeline_service/cross_project_pipeline_spec.rb index b3b8e34dd8e..e1d60ed57ef 100644 --- a/spec/services/ci/create_pipeline_service/cross_project_pipeline_spec.rb +++ b/spec/services/ci/create_pipeline_service/cross_project_pipeline_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do let_it_be(:group) { create(:group, name: 'my-organization') } let(:upstream_project) { create(:project, :repository, name: 'upstream', group: group) } - let(:downstram_project) { create(:project, :repository, name: 'downstream', group: group) } + let(:downstream_project) { create(:project, :repository, name: 'downstream', group: group) } let(:user) { create(:user) } let(:service) do @@ -15,9 +15,9 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do before do upstream_project.add_developer(user) - downstram_project.add_developer(user) + downstream_project.add_developer(user) create_gitlab_ci_yml(upstream_project, upstream_config) - create_gitlab_ci_yml(downstram_project, downstream_config) + create_gitlab_ci_yml(downstream_project, downstream_config) end context 'with resource group', :aggregate_failures do @@ -79,7 +79,7 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do end def create_pipeline! - service.execute(:push) + service.execute(:push).payload end def create_gitlab_ci_yml(project, content) diff --git a/spec/services/ci/create_pipeline_service/custom_config_content_spec.rb b/spec/services/ci/create_pipeline_service/custom_config_content_spec.rb index 42c3f52541b..f150a4f8b51 100644 --- a/spec/services/ci/create_pipeline_service/custom_config_content_spec.rb +++ b/spec/services/ci/create_pipeline_service/custom_config_content_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Ci::CreatePipelineService do let(:upstream_pipeline) { create(:ci_pipeline, project: project) } let(:bridge) { create(:ci_bridge, pipeline: upstream_pipeline) } - subject { service.execute(:push, bridge: bridge) } + subject { service.execute(:push, bridge: bridge).payload } context 'custom config content' do let(:bridge) do diff --git a/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb b/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb index 5dceb9f57f0..026111d59f1 100644 --- a/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb +++ b/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Ci::CreatePipelineService do let(:ref) { 'refs/heads/master' } let(:source) { :push } let(:service) { described_class.new(project, user, { ref: ref }) } - let(:pipeline) { service.execute(source) } + let(:pipeline) { service.execute(source).payload } before do stub_ci_pipeline_yaml_file(config) diff --git a/spec/services/ci/create_pipeline_service/dry_run_spec.rb b/spec/services/ci/create_pipeline_service/dry_run_spec.rb index 01df7772eef..ae43c63b516 100644 --- a/spec/services/ci/create_pipeline_service/dry_run_spec.rb +++ b/spec/services/ci/create_pipeline_service/dry_run_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Ci::CreatePipelineService do let(:ref) { 'refs/heads/master' } let(:service) { described_class.new(project, user, { ref: ref }) } - subject { service.execute(:push, dry_run: true) } + subject { service.execute(:push, dry_run: true).payload } before do stub_ci_pipeline_yaml_file(config) diff --git a/spec/services/ci/create_pipeline_service/environment_spec.rb b/spec/services/ci/create_pipeline_service/environment_spec.rb index e77591298ad..43b5220334c 100644 --- a/spec/services/ci/create_pipeline_service/environment_spec.rb +++ b/spec/services/ci/create_pipeline_service/environment_spec.rb @@ -14,7 +14,7 @@ RSpec.describe Ci::CreatePipelineService do end describe '#execute' do - subject { service.execute(:push) } + subject { service.execute(:push).payload } context 'with deployment tier' do before do diff --git a/spec/services/ci/create_pipeline_service/evaluate_runner_tags_spec.rb b/spec/services/ci/create_pipeline_service/evaluate_runner_tags_spec.rb index df881c1ac8f..9add096d782 100644 --- a/spec/services/ci/create_pipeline_service/evaluate_runner_tags_spec.rb +++ b/spec/services/ci/create_pipeline_service/evaluate_runner_tags_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Ci::CreatePipelineService do let_it_be(:user) { create(:user) } let(:service) { described_class.new(project, user, ref: 'master') } - let(:pipeline) { service.execute(:push) } + let(:pipeline) { service.execute(:push).payload } let(:job) { pipeline.builds.find_by(name: 'job') } before do diff --git a/spec/services/ci/create_pipeline_service/include_spec.rb b/spec/services/ci/create_pipeline_service/include_spec.rb new file mode 100644 index 00000000000..46271ee36c0 --- /dev/null +++ b/spec/services/ci/create_pipeline_service/include_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::CreatePipelineService do + context 'include:' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.owner } + + let(:ref) { 'refs/heads/master' } + let(:source) { :push } + let(:service) { described_class.new(project, user, { ref: ref }) } + let(:pipeline) { service.execute(source).payload } + + let(:file_location) { 'spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml' } + + before do + allow(project.repository) + .to receive(:blob_data_at).with(project.commit.id, '.gitlab-ci.yml') + .and_return(config) + + allow(project.repository) + .to receive(:blob_data_at).with(project.commit.id, file_location) + .and_return(File.read(Rails.root.join(file_location))) + end + + context 'with a local file' do + let(:config) do + <<~EOY + include: #{file_location} + job: + script: exit 0 + EOY + end + + it 'includes the job in the file' do + expect(pipeline).to be_created_successfully + expect(pipeline.processables.pluck(:name)).to contain_exactly('job', 'rspec') + end + end + + context 'with a local file with rules' do + let(:config) do + <<~EOY + include: + - local: #{file_location} + rules: + - if: $CI_PROJECT_ID == "#{project_id}" + job: + script: exit 0 + EOY + end + + context 'when the rules matches' do + let(:project_id) { project.id } + + it 'includes the job in the file' do + expect(pipeline).to be_created_successfully + expect(pipeline.processables.pluck(:name)).to contain_exactly('job', 'rspec') + end + + context 'when the FF ci_include_rules is disabled' do + before do + stub_feature_flags(ci_include_rules: false) + end + + it 'includes the job in the file' do + expect(pipeline).to be_created_successfully + expect(pipeline.processables.pluck(:name)).to contain_exactly('job', 'rspec') + end + end + end + + context 'when the rules does not match' do + let(:project_id) { non_existing_record_id } + + it 'does not include the job in the file' do + expect(pipeline).to be_created_successfully + expect(pipeline.processables.pluck(:name)).to contain_exactly('job') + end + + context 'when the FF ci_include_rules is disabled' do + before do + stub_feature_flags(ci_include_rules: false) + end + + it 'includes the job in the file' do + expect(pipeline).to be_created_successfully + expect(pipeline.processables.pluck(:name)).to contain_exactly('job', 'rspec') + end + end + end + end + end +end diff --git a/spec/services/ci/create_pipeline_service/merge_requests_spec.rb b/spec/services/ci/create_pipeline_service/merge_requests_spec.rb index e5347faed6a..a1f85faa69f 100644 --- a/spec/services/ci/create_pipeline_service/merge_requests_spec.rb +++ b/spec/services/ci/create_pipeline_service/merge_requests_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Ci::CreatePipelineService do let(:ref) { 'refs/heads/feature' } let(:source) { :push } let(:service) { described_class.new(project, user, { ref: ref }) } - let(:pipeline) { service.execute(source) } + let(:pipeline) { service.execute(source).payload } before do stub_ci_pipeline_yaml_file <<-EOS diff --git a/spec/services/ci/create_pipeline_service/needs_spec.rb b/spec/services/ci/create_pipeline_service/needs_spec.rb index d096db10d0b..9070d86f7f6 100644 --- a/spec/services/ci/create_pipeline_service/needs_spec.rb +++ b/spec/services/ci/create_pipeline_service/needs_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Ci::CreatePipelineService do let(:ref) { 'refs/heads/master' } let(:source) { :push } let(:service) { described_class.new(project, user, { ref: ref }) } - let(:pipeline) { service.execute(source) } + let(:pipeline) { service.execute(source).payload } before do stub_ci_pipeline_yaml_file(config) diff --git a/spec/services/ci/create_pipeline_service/parallel_spec.rb b/spec/services/ci/create_pipeline_service/parallel_spec.rb index 5e34a67d376..6b455bf4874 100644 --- a/spec/services/ci/create_pipeline_service/parallel_spec.rb +++ b/spec/services/ci/create_pipeline_service/parallel_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Ci::CreatePipelineService do let_it_be(:user) { project.owner } let(:service) { described_class.new(project, user, { ref: 'master' }) } - let(:pipeline) { service.execute(:push) } + let(:pipeline) { service.execute(:push).payload } before do stub_ci_pipeline_yaml_file(config) diff --git a/spec/services/ci/create_pipeline_service/parameter_content_spec.rb b/spec/services/ci/create_pipeline_service/parameter_content_spec.rb index 94500a550c6..761504ffb58 100644 --- a/spec/services/ci/create_pipeline_service/parameter_content_spec.rb +++ b/spec/services/ci/create_pipeline_service/parameter_content_spec.rb @@ -30,7 +30,7 @@ RSpec.describe Ci::CreatePipelineService do describe '#execute' do context 'when source is a dangling build' do - subject { service.execute(:ondemand_dast_scan, content: content) } + subject { service.execute(:ondemand_dast_scan, content: content).payload } context 'parameter config content' do it 'creates a pipeline' do diff --git a/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb b/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb index 7a6535ed3fa..6eb1315fff4 100644 --- a/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb +++ b/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb @@ -369,6 +369,6 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do end def create_pipeline! - service.execute(:push) + service.execute(:push).payload end end diff --git a/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb b/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb index c84d9a53973..5e34eeb99db 100644 --- a/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb +++ b/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb @@ -8,7 +8,7 @@ RSpec.describe Ci::CreatePipelineService do let(:source) { :push } let(:service) { described_class.new(project, user, { ref: ref }) } - let(:pipeline) { service.execute(source) } + let(:pipeline) { service.execute(source).payload } let(:config) do <<~YAML diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index acdf38bbc13..d0915f099de 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Ci::CreatePipelineService do let(:ref) { 'refs/heads/master' } let(:source) { :push } let(:service) { described_class.new(project, user, { ref: ref }) } - let(:pipeline) { service.execute(source) } + let(:pipeline) { service.execute(source).payload } let(:build_names) { pipeline.builds.pluck(:name) } context 'job:rules' do diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 56bfeda3bff..2fdb0ed3c0d 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -19,7 +19,6 @@ RSpec.describe Ci::CreatePipelineService do def execute_service( source: :push, after: project.commit.id, - message: 'Message', ref: ref_name, trigger_request: nil, variables_attributes: nil, @@ -32,7 +31,6 @@ RSpec.describe Ci::CreatePipelineService do params = { ref: ref, before: '00000000', after: after, - commits: [{ message: message }], variables_attributes: variables_attributes, push_options: push_options, source_sha: source_sha, @@ -49,12 +47,16 @@ RSpec.describe Ci::CreatePipelineService do # rubocop:enable Metrics/ParameterLists context 'valid params' do - let(:pipeline) { execute_service } + let(:pipeline) { execute_service.payload } let(:pipeline_on_previous_commit) do execute_service( after: previous_commit_sha_from_ref('master') - ) + ).payload + end + + it 'responds with success' do + expect(execute_service).to be_success end it 'creates a pipeline' do @@ -128,7 +130,7 @@ RSpec.describe Ci::CreatePipelineService do merge_request_1 merge_request_2 - head_pipeline = execute_service(ref: 'feature', after: nil) + head_pipeline = execute_service(ref: 'feature', after: nil).payload expect(merge_request_1.reload.head_pipeline).to eq(head_pipeline) expect(merge_request_2.reload.head_pipeline).to eq(head_pipeline) @@ -157,7 +159,7 @@ RSpec.describe Ci::CreatePipelineService do target_branch: "branch_1", source_project: project) - head_pipeline = execute_service + head_pipeline = execute_service.payload expect(merge_request.reload.head_pipeline).not_to eq(head_pipeline) end @@ -178,7 +180,7 @@ RSpec.describe Ci::CreatePipelineService do source_project: project, target_project: target_project) - head_pipeline = execute_service(ref: 'feature', after: nil) + head_pipeline = execute_service(ref: 'feature', after: nil).payload expect(merge_request.reload.head_pipeline).to eq(head_pipeline) end @@ -209,7 +211,7 @@ RSpec.describe Ci::CreatePipelineService do target_branch: 'feature', source_project: project) - head_pipeline = execute_service + head_pipeline = execute_service.payload expect(head_pipeline).to be_persisted expect(head_pipeline.yaml_errors).to be_present @@ -230,7 +232,7 @@ RSpec.describe Ci::CreatePipelineService do target_branch: 'feature', source_project: project) - head_pipeline = execute_service + head_pipeline = execute_service.payload expect(head_pipeline).to be_skipped expect(head_pipeline).to be_persisted @@ -260,7 +262,7 @@ RSpec.describe Ci::CreatePipelineService do it 'cancels running outdated pipelines', :sidekiq_inline do pipeline_on_previous_commit.reload.run - head_pipeline = execute_service + head_pipeline = execute_service.payload expect(pipeline_on_previous_commit.reload).to have_attributes(status: 'canceled', auto_canceled_by_id: head_pipeline.id) end @@ -276,7 +278,8 @@ RSpec.describe Ci::CreatePipelineService do new_pipeline = execute_service( ref: 'refs/heads/feature', after: previous_commit_sha_from_ref('feature') - ) + ).payload + pipeline expect(new_pipeline.reload).to have_attributes(status: 'created', auto_canceled_by_id: nil) @@ -290,7 +293,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'is cancelable' do - pipeline = execute_service + pipeline = execute_service.payload expect(pipeline.builds.find_by(name: 'rspec').interruptible).to be_nil end @@ -303,7 +306,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'is cancelable' do - pipeline = execute_service + pipeline = execute_service.payload expect(pipeline.builds.find_by(name: 'rspec').interruptible).to be_truthy end @@ -316,7 +319,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'is not cancelable' do - pipeline = execute_service + pipeline = execute_service.payload expect(pipeline.builds.find_by(name: 'rspec').interruptible).to be_falsy end @@ -476,21 +479,25 @@ RSpec.describe Ci::CreatePipelineService do context "skip tag if there is no build for it" do it "creates commit if there is appropriate job" do - expect(execute_service).to be_persisted + expect(execute_service.payload).to be_persisted end it "creates commit if there is no appropriate job but deploy job has right ref setting" do config = YAML.dump({ deploy: { script: "ls", only: ["master"] } }) stub_ci_pipeline_yaml_file(config) - expect(execute_service).to be_persisted + expect(execute_service.payload).to be_persisted end end - it 'skips creating pipeline for refs without .gitlab-ci.yml' do + it 'skips creating pipeline for refs without .gitlab-ci.yml', :aggregate_failures do stub_ci_pipeline_yaml_file(nil) - expect(execute_service).not_to be_persisted + response = execute_service + + expect(response).to be_error + expect(response.message).to eq('Missing CI config file') + expect(response.payload).not_to be_persisted expect(Ci::Pipeline.count).to eq(0) expect(Namespaces::OnboardingPipelineCreatedWorker).not_to receive(:perform_async) end @@ -499,7 +506,7 @@ RSpec.describe Ci::CreatePipelineService do it 'creates failed pipeline' do stub_ci_pipeline_yaml_file(ci_yaml) - pipeline = execute_service(message: message) + pipeline = execute_service.payload expect(pipeline).to be_persisted expect(pipeline.builds.any?).to be false @@ -516,7 +523,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'pull it from the repository' do - pipeline = execute_service + pipeline = execute_service.payload expect(pipeline).to be_repository_source expect(pipeline.builds.map(&:name)).to eq ['rspec'] end @@ -530,7 +537,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'pull it from Auto-DevOps' do - pipeline = execute_service + pipeline = execute_service.payload expect(pipeline).to be_auto_devops_source expect(pipeline.builds.map(&:name)).to match_array(%w[brakeman-sast build code_quality eslint-sast secret_detection semgrep-sast test]) end @@ -541,11 +548,12 @@ RSpec.describe Ci::CreatePipelineService do stub_ci_pipeline_yaml_file(nil) end - it 'attaches errors to the pipeline' do - pipeline = execute_service + it 'responds with error message', :aggregate_failures do + response = execute_service - expect(pipeline.errors.full_messages).to eq ['Missing CI config file'] - expect(pipeline).not_to be_persisted + expect(response).to be_error + expect(response.message).to eq('Missing CI config file') + expect(response.payload).not_to be_persisted end end @@ -556,7 +564,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'saves error in pipeline' do - pipeline = execute_service + pipeline = execute_service.payload expect(pipeline.yaml_errors).to include('Undefined error') end @@ -648,7 +656,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'saves error in pipeline' do - pipeline = execute_service + pipeline = execute_service.payload expect(pipeline.yaml_errors).to include('Undefined error') end @@ -661,26 +669,18 @@ RSpec.describe Ci::CreatePipelineService do end context 'when commit contains a [ci skip] directive' do - let(:message) { "some message[ci skip]" } - - ci_messages = [ - "some message[ci skip]", - "some message[skip ci]", - "some message[CI SKIP]", - "some message[SKIP CI]", - "some message[ci_skip]", - "some message[skip_ci]", - "some message[ci-skip]", - "some message[skip-ci]" - ] + shared_examples 'creating a pipeline' do + it 'does not skip pipeline creation' do + pipeline = execute_service.payload - before do - allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { message } + expect(pipeline).to be_persisted + expect(pipeline.builds.first.name).to eq("rspec") + end end - ci_messages.each do |ci_message| - it "skips builds creation if the commit message is #{ci_message}" do - pipeline = execute_service(message: ci_message) + shared_examples 'skipping a pipeline' do + it 'skips pipeline creation' do + pipeline = execute_service.payload expect(pipeline).to be_persisted expect(pipeline.builds.any?).to be false @@ -688,14 +688,26 @@ RSpec.describe Ci::CreatePipelineService do end end - shared_examples 'creating a pipeline' do - it 'does not skip pipeline creation' do - allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { commit_message } + before do + allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { commit_message } + end + + skip_commit_messages = [ + "some message[ci skip]", + "some message[skip ci]", + "some message[CI SKIP]", + "some message[SKIP CI]", + "some message[ci_skip]", + "some message[skip_ci]", + "some message[ci-skip]", + "some message[skip-ci]" + ] - pipeline = execute_service(message: commit_message) + skip_commit_messages.each do |skip_commit_message| + context "when the commit message is #{skip_commit_message}" do + let(:commit_message) { skip_commit_message } - expect(pipeline).to be_persisted - expect(pipeline.builds.first.name).to eq("rspec") + it_behaves_like 'skipping a pipeline' end end @@ -712,9 +724,14 @@ RSpec.describe Ci::CreatePipelineService do end context 'when there is [ci skip] tag in commit message and yaml is invalid' do + let(:commit_message) { 'some message [ci skip]' } let(:ci_yaml) { 'invalid: file: fiile' } - it_behaves_like 'a failed pipeline' + before do + stub_ci_pipeline_yaml_file(ci_yaml) + end + + it_behaves_like 'skipping a pipeline' end end @@ -724,7 +741,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'creates a pipline in the skipped state' do - pipeline = execute_service(push_options: push_options) + pipeline = execute_service(push_options: push_options).payload # TODO: DRY these up with "skips builds creation if the commit message" expect(pipeline).to be_persisted @@ -739,10 +756,12 @@ RSpec.describe Ci::CreatePipelineService do stub_ci_pipeline_yaml_file(config) end - it 'does not create a new pipeline' do + it 'does not create a new pipeline', :aggregate_failures do result = execute_service - expect(result).not_to be_persisted + expect(result).to be_error + expect(result.message).to eq('No stages / jobs for this pipeline.') + expect(result.payload).not_to be_persisted expect(Ci::Build.all).to be_empty expect(Ci::Pipeline.count).to eq(0) end @@ -757,10 +776,11 @@ RSpec.describe Ci::CreatePipelineService do .and_call_original end - it 'rewinds iid' do + it 'rewinds iid', :aggregate_failures do result = execute_service - expect(result).not_to be_persisted + expect(result).to be_error + expect(result.payload).not_to be_persisted expect(internal_id.last_value).to eq(0) end end @@ -773,7 +793,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'does not create a new pipeline', :sidekiq_inline do - result = execute_service + result = execute_service.payload expect(result).to be_persisted expect(result.manual_actions).not_to be_empty @@ -793,7 +813,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'creates the environment with tags' do - result = execute_service + result = execute_service.payload expect(result).to be_persisted expect(Environment.find_by(name: "review/master")).to be_present @@ -815,7 +835,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'creates the environment with auto stop in' do - result = execute_service + result = execute_service.payload expect(result).to be_persisted expect(result.builds.first.options[:environment][:auto_stop_in]).to eq('1 day') @@ -835,7 +855,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'skipps persisted variables in environment name' do - result = execute_service + result = execute_service.payload expect(result).to be_persisted expect(Environment.find_by(name: "review/id1/id2")).to be_present @@ -860,7 +880,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'stores the requested namespace' do - result = execute_service + result = execute_service.payload build = result.builds.first expect(result).to be_persisted @@ -876,7 +896,7 @@ RSpec.describe Ci::CreatePipelineService do it 'does not create an environment' do expect do - result = execute_service + result = execute_service.payload expect(result).to be_persisted end.not_to change { Environment.count } @@ -896,7 +916,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'creates a pipeline with the environment' do - result = execute_service + result = execute_service.payload expect(result).to be_persisted expect(Environment.find_by(name: 'production')).to be_present @@ -906,7 +926,7 @@ RSpec.describe Ci::CreatePipelineService do end context 'when builds with auto-retries are configured' do - let(:pipeline) { execute_service } + let(:pipeline) { execute_service.payload } let(:rspec_job) { pipeline.builds.find_by(name: 'rspec') } before do @@ -946,7 +966,7 @@ RSpec.describe Ci::CreatePipelineService do let(:resource_group_key) { 'iOS' } it 'persists the association correctly' do - result = execute_service + result = execute_service.payload deploy_job = result.builds.find_by_name!(:test) resource_group = project.resource_groups.find_by_key!(resource_group_key) @@ -962,7 +982,7 @@ RSpec.describe Ci::CreatePipelineService do let(:resource_group_key) { '$CI_COMMIT_REF_NAME-$CI_JOB_NAME' } it 'interpolates the variables into the key correctly' do - result = execute_service + result = execute_service.payload expect(result).to be_persisted expect(project.resource_groups.exists?(key: 'master-test')).to eq(true) @@ -979,7 +999,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'correctly creates builds with custom timeout value configured' do - pipeline = execute_service + pipeline = execute_service.payload expect(pipeline).to be_persisted expect(pipeline.builds.find_by(name: 'rspec').options[:job_timeout]).to eq 123 @@ -994,7 +1014,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'is valid config' do - pipeline = execute_service + pipeline = execute_service.payload build = pipeline.builds.first expect(pipeline).to be_kind_of(Ci::Pipeline) expect(pipeline).to be_valid @@ -1059,14 +1079,14 @@ RSpec.describe Ci::CreatePipelineService do project.add_developer(user) end - it 'does not create a pipeline' do - expect(execute_service).not_to be_persisted + it 'does not create a pipeline', :aggregate_failures do + expect(execute_service.payload).not_to be_persisted expect(Ci::Pipeline.count).to eq(0) end end context 'when user is maintainer' do - let(:pipeline) { execute_service } + let(:pipeline) { execute_service.payload } before do project.add_maintainer(user) @@ -1083,9 +1103,11 @@ RSpec.describe Ci::CreatePipelineService do let(:user) {} let(:trigger_request) { create(:ci_trigger_request) } - it 'does not create a pipeline' do - expect(execute_service(trigger_request: trigger_request)) - .not_to be_persisted + it 'does not create a pipeline', :aggregate_failures do + response = execute_service(trigger_request: trigger_request) + + expect(response).to be_error + expect(response.payload).not_to be_persisted expect(Ci::Pipeline.count).to eq(0) end end @@ -1099,9 +1121,11 @@ RSpec.describe Ci::CreatePipelineService do project.add_developer(user) end - it 'does not create a pipeline' do - expect(execute_service(trigger_request: trigger_request)) - .not_to be_persisted + it 'does not create a pipeline', :aggregate_failures do + response = execute_service(trigger_request: trigger_request) + + expect(response).to be_error + expect(response.payload).not_to be_persisted expect(Ci::Pipeline.count).to eq(0) end end @@ -1116,7 +1140,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'creates a pipeline' do - expect(execute_service(trigger_request: trigger_request)) + expect(execute_service(trigger_request: trigger_request).payload) .to be_persisted expect(Ci::Pipeline.count).to eq(1) end @@ -1150,7 +1174,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'creates a tagged pipeline' do - pipeline = execute_service(ref: 'v1.0.0') + pipeline = execute_service(ref: 'v1.0.0').payload expect(pipeline.tag?).to be true end @@ -1161,7 +1185,7 @@ RSpec.describe Ci::CreatePipelineService do let(:ref_name) { 'refs/heads/nonexistant-branch' } - let(:pipeline) { execute_service } + let(:pipeline) { execute_service.payload } it 'does not create the pipeline' do expect(pipeline).not_to be_created_successfully @@ -1185,7 +1209,7 @@ RSpec.describe Ci::CreatePipelineService do # v1.1.0 is on the test repo as branch and tag let(:ref_name) { 'refs/heads/v1.1.0' } - let(:pipeline) { execute_service } + let(:pipeline) { execute_service.payload } it 'creates the pipeline for the branch' do expect(pipeline).to be_created_successfully @@ -1200,7 +1224,7 @@ RSpec.describe Ci::CreatePipelineService do # v1.1.0 is on the test repo as branch and tag let(:ref_name) { 'refs/tags/v1.1.0' } - let(:pipeline) { execute_service } + let(:pipeline) { execute_service.payload } it 'creates the pipeline for the tag' do expect(pipeline).to be_created_successfully @@ -1215,7 +1239,7 @@ RSpec.describe Ci::CreatePipelineService do # v1.1.0 is on the test repo as branch and tag let(:ref_name) { 'v1.1.0' } - let(:pipeline) { execute_service } + let(:pipeline) { execute_service.payload } it 'does not create the pipeline' do expect(pipeline).not_to be_created_successfully @@ -1229,16 +1253,16 @@ RSpec.describe Ci::CreatePipelineService do { key: 'second', secret_value: 'second_world' }] end - subject { execute_service(variables_attributes: variables_attributes) } + subject(:pipeline) { execute_service(variables_attributes: variables_attributes).payload } it 'creates a pipeline with specified variables' do - expect(subject.variables.map { |var| var.slice(:key, :secret_value) }) + expect(pipeline.variables.map { |var| var.slice(:key, :secret_value) }) .to eq variables_attributes.map(&:with_indifferent_access) end end context 'when pipeline has a job with environment' do - let(:pipeline) { execute_service } + let(:pipeline) { execute_service.payload } before do stub_ci_pipeline_yaml_file(YAML.dump(config)) @@ -1286,7 +1310,7 @@ RSpec.describe Ci::CreatePipelineService do end describe 'Pipeline for external pull requests' do - let(:pipeline) do + let(:response) do execute_service(source: source, external_pull_request: pull_request, ref: ref_name, @@ -1294,6 +1318,8 @@ RSpec.describe Ci::CreatePipelineService do target_sha: target_sha) end + let(:pipeline) { response.payload } + before do stub_ci_pipeline_yaml_file(YAML.dump(config)) end @@ -1342,9 +1368,11 @@ RSpec.describe Ci::CreatePipelineService do context 'when ref is tag' do let(:ref_name) { 'refs/tags/v1.1.0' } - it 'does not create an extrnal pull request pipeline' do + it 'does not create an extrnal pull request pipeline', :aggregate_failures do + expect(response).to be_error + expect(response.message).to eq('Tag is not included in the list and Failed to build the pipeline!') expect(pipeline).not_to be_persisted - expect(pipeline.errors[:tag]).to eq(["is not included in the list"]) + expect(pipeline.errors[:tag]).to eq(['is not included in the list']) end end @@ -1363,9 +1391,11 @@ RSpec.describe Ci::CreatePipelineService do } end - it 'does not create a detached merge request pipeline' do + it 'does not create a detached merge request pipeline', :aggregate_failures do + expect(response).to be_error + expect(response.message).to eq('No stages / jobs for this pipeline.') expect(pipeline).not_to be_persisted - expect(pipeline.errors[:base]).to eq(["No stages / jobs for this pipeline."]) + expect(pipeline.errors[:base]).to eq(['No stages / jobs for this pipeline.']) end end end @@ -1373,7 +1403,9 @@ RSpec.describe Ci::CreatePipelineService do context 'when external pull request is not specified' do let(:pull_request) { nil } - it 'does not create an external pull request pipeline' do + it 'does not create an external pull request pipeline', :aggregate_failures do + expect(response).to be_error + expect(response.message).to eq("External pull request can't be blank and Failed to build the pipeline!") expect(pipeline).not_to be_persisted expect(pipeline.errors[:external_pull_request]).to eq(["can't be blank"]) end @@ -1420,11 +1452,11 @@ RSpec.describe Ci::CreatePipelineService do context 'when external pull request is not specified' do let(:pull_request) { nil } - it 'does not create an external pull request pipeline' do + it 'does not create an external pull request pipeline', :aggregate_failures do + expect(response).to be_error + expect(response.message).to eq("External pull request can't be blank and Failed to build the pipeline!") expect(pipeline).not_to be_persisted - - expect(pipeline.errors[:base]) - .to eq(['Failed to build the pipeline!']) + expect(pipeline.errors[:base]).to eq(['Failed to build the pipeline!']) end end end @@ -1432,7 +1464,7 @@ RSpec.describe Ci::CreatePipelineService do end describe 'Pipelines for merge requests' do - let(:pipeline) do + let(:response) do execute_service(source: source, merge_request: merge_request, ref: ref_name, @@ -1440,6 +1472,8 @@ RSpec.describe Ci::CreatePipelineService do target_sha: target_sha) end + let(:pipeline) { response.payload } + before do stub_ci_pipeline_yaml_file(YAML.dump(config)) end @@ -1522,9 +1556,11 @@ RSpec.describe Ci::CreatePipelineService do context 'when ref is tag' do let(:ref_name) { 'refs/tags/v1.1.0' } - it 'does not create a merge request pipeline' do + it 'does not create a merge request pipeline', :aggregate_failures do + expect(response).to be_error + expect(response.message).to eq('Tag is not included in the list and Failed to build the pipeline!') expect(pipeline).not_to be_persisted - expect(pipeline.errors[:tag]).to eq(["is not included in the list"]) + expect(pipeline.errors[:tag]).to eq(['is not included in the list']) end end @@ -1564,9 +1600,10 @@ RSpec.describe Ci::CreatePipelineService do } end - it 'does not create a detached merge request pipeline' do + it 'does not create a detached merge request pipeline', :aggregate_failures do + expect(response).to be_error + expect(response.message).to eq('No stages / jobs for this pipeline.') expect(pipeline).not_to be_persisted - expect(pipeline.errors[:base]).to eq(["No stages / jobs for this pipeline."]) end end end @@ -1599,11 +1636,10 @@ RSpec.describe Ci::CreatePipelineService do target_branch: 'master') end - it 'does not create a detached merge request pipeline' do + it 'does not create a detached merge request pipeline', :aggregate_failures do + expect(response).to be_error + expect(response.message).to eq('No stages / jobs for this pipeline.') expect(pipeline).not_to be_persisted - - expect(pipeline.errors[:base]) - .to eq(['No stages / jobs for this pipeline.']) end end end @@ -1628,11 +1664,10 @@ RSpec.describe Ci::CreatePipelineService do target_branch: 'master') end - it 'does not create a detached merge request pipeline' do + it 'does not create a detached merge request pipeline', :aggregate_failures do + expect(response).to be_error + expect(response.message).to eq('No stages / jobs for this pipeline.') expect(pipeline).not_to be_persisted - - expect(pipeline.errors[:base]) - .to eq(['No stages / jobs for this pipeline.']) end end end @@ -1659,11 +1694,10 @@ RSpec.describe Ci::CreatePipelineService do target_branch: 'master') end - it 'does not create a detached merge request pipeline' do + it 'does not create a detached merge request pipeline', :aggregate_failures do + expect(response).to be_error + expect(response.message).to eq('No stages / jobs for this pipeline.') expect(pipeline).not_to be_persisted - - expect(pipeline.errors[:base]) - .to eq(['No stages / jobs for this pipeline.']) end end end @@ -1688,11 +1722,10 @@ RSpec.describe Ci::CreatePipelineService do target_branch: 'master') end - it 'does not create a detached merge request pipeline' do + it 'does not create a detached merge request pipeline', :aggregate_failures do + expect(response).to be_error + expect(response.message).to eq('No stages / jobs for this pipeline.') expect(pipeline).not_to be_persisted - - expect(pipeline.errors[:base]) - .to eq(['No stages / jobs for this pipeline.']) end end end @@ -1733,7 +1766,8 @@ RSpec.describe Ci::CreatePipelineService do end context 'when needs is used' do - let(:pipeline) { execute_service } + let(:response) { execute_service } + let(:pipeline) { response.payload } let(:config) do { @@ -1779,7 +1813,7 @@ RSpec.describe Ci::CreatePipelineService do let(:ref_name) { 'refs/heads/feature' } shared_examples 'has errors' do - it 'contains the expected errors' do + it 'contains the expected errors', :aggregate_failures do expect(pipeline.builds).to be_empty error_message = "'test_a' job needs 'build_a' job, but 'build_a' is not in any previous stage" @@ -1790,9 +1824,12 @@ RSpec.describe Ci::CreatePipelineService do end context 'when save_on_errors is enabled' do - let(:pipeline) { execute_service(save_on_errors: true) } + let(:response) { execute_service(save_on_errors: true) } + let(:pipeline) { response.payload } - it 'does create a pipeline as test_a depends on build_a' do + it 'does create a pipeline as test_a depends on build_a', :aggregate_failures do + expect(response).to be_error + expect(response.message).to eq("'test_a' job needs 'build_a' job, but 'build_a' is not in any previous stage") expect(pipeline).to be_persisted end @@ -1800,9 +1837,11 @@ RSpec.describe Ci::CreatePipelineService do end context 'when save_on_errors is disabled' do - let(:pipeline) { execute_service(save_on_errors: false) } + let(:response) { execute_service(save_on_errors: false) } + let(:pipeline) { response.payload } - it 'does not create a pipeline as test_a depends on build_a' do + it 'does not create a pipeline as test_a depends on build_a', :aggregate_failures do + expect(response).to be_error expect(pipeline).not_to be_persisted end @@ -1822,7 +1861,8 @@ RSpec.describe Ci::CreatePipelineService do context 'when rules are used' do let(:ref_name) { 'refs/heads/master' } - let(:pipeline) { execute_service } + let(:response) { execute_service } + let(:pipeline) { response.payload } let(:build_names) { pipeline.builds.pluck(:name) } let(:regular_job) { find_job('regular-job') } let(:rules_job) { find_job('rules-job') } @@ -2379,8 +2419,9 @@ RSpec.describe Ci::CreatePipelineService do end context 'when inside freeze period' do - it 'does not create the pipeline' do + it 'does not create the pipeline', :aggregate_failures do Timecop.freeze(2020, 4, 10, 23, 1) do + expect(response).to be_error expect(pipeline).not_to be_persisted end end @@ -2410,7 +2451,8 @@ RSpec.describe Ci::CreatePipelineService do context 'with no matches' do let(:ref_name) { 'refs/heads/feature' } - it 'does not create a pipeline' do + it 'does not create a pipeline', :aggregate_failures do + expect(response).to be_error expect(pipeline).not_to be_persisted end end @@ -2418,7 +2460,7 @@ RSpec.describe Ci::CreatePipelineService do context 'with workflow rules with pipeline variables' do let(:pipeline) do - execute_service(variables_attributes: variables_attributes) + execute_service(variables_attributes: variables_attributes).payload end let(:config) do @@ -2446,7 +2488,8 @@ RSpec.describe Ci::CreatePipelineService do context 'with no matches' do let(:variables_attributes) { {} } - it 'does not create a pipeline' do + it 'does not create a pipeline', :aggregate_failures do + expect(response).to be_error expect(pipeline).not_to be_persisted end end @@ -2456,7 +2499,7 @@ RSpec.describe Ci::CreatePipelineService do let(:pipeline) do execute_service do |pipeline| pipeline.variables.build(variables) - end + end.payload end let(:config) do @@ -2514,7 +2557,8 @@ RSpec.describe Ci::CreatePipelineService do context 'with no matches' do let(:variables) { {} } - it 'does not create a pipeline' do + it 'does not create a pipeline', :aggregate_failures do + expect(response).to be_error expect(pipeline).not_to be_persisted end @@ -2542,7 +2586,8 @@ RSpec.describe Ci::CreatePipelineService do EOY end - it 'does not create a pipeline' do + it 'does not create a pipeline', :aggregate_failures do + expect(response).to be_error expect(pipeline).not_to be_persisted end end diff --git a/spec/services/ci/daily_build_group_report_result_service_spec.rb b/spec/services/ci/daily_build_group_report_result_service_spec.rb index e58a5de26a1..32651247adb 100644 --- a/spec/services/ci/daily_build_group_report_result_service_spec.rb +++ b/spec/services/ci/daily_build_group_report_result_service_spec.rb @@ -41,6 +41,17 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do expect(Ci::DailyBuildGroupReportResult.find_by(group_name: 'extra')).to be_nil end + it 'creates a project_ci_feature_usage record for the pipeline project' do + described_class.new.execute(pipeline) + + expect(Projects::CiFeatureUsage.count).to eq(1) + expect(Projects::CiFeatureUsage.first).to have_attributes( + project_id: pipeline.project.id, + feature: 'code_coverage', + default_branch: false + ) + end + context 'when there are multiple builds with the same group name that report coverage' do let!(:test_job_1) { create(:ci_build, pipeline: pipeline, name: 'test 1/2', coverage: 70) } let!(:test_job_2) { create(:ci_build, pipeline: pipeline, name: 'test 2/2', coverage: 80) } @@ -99,6 +110,16 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do data: { 'coverage' => new_karma_job.coverage } ) end + + it 'does not create a new project_ci_feature_usage record for the pipeline project' do + expect { described_class.new.execute(pipeline) }.not_to change { Projects::CiFeatureUsage.count } + + expect(Projects::CiFeatureUsage.first).to have_attributes( + project_id: pipeline.project.id, + feature: 'code_coverage', + default_branch: false + ) + end end context 'when the ID of the pipeline is older than the last_pipeline_id' do @@ -161,6 +182,8 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do it 'does nothing' do expect { described_class.new.execute(new_pipeline) }.not_to raise_error + expect(Ci::DailyBuildGroupReportResult.count).to eq(0) + expect(Projects::CiFeatureUsage.count).to eq(0) end end @@ -178,6 +201,17 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do expect(coverage.default_branch).to be_truthy end end + + it 'creates a project_ci_feature_usage record for the pipeline project for default branch' do + described_class.new.execute(pipeline) + + expect(Projects::CiFeatureUsage.count).to eq(1) + expect(Projects::CiFeatureUsage.first).to have_attributes( + project_id: pipeline.project.id, + feature: 'code_coverage', + default_branch: true + ) + end end context 'when pipeline ref_path is not the project default branch' do diff --git a/spec/services/ci/destroy_pipeline_service_spec.rb b/spec/services/ci/destroy_pipeline_service_spec.rb index 588ff0b1762..6c1c02b2875 100644 --- a/spec/services/ci/destroy_pipeline_service_spec.rb +++ b/spec/services/ci/destroy_pipeline_service_spec.rb @@ -78,18 +78,6 @@ RSpec.describe ::Ci::DestroyPipelineService do subject end - - context 'when cancel_pipelines_prior_to_destroy is disabled' do - before do - stub_feature_flags(cancel_pipelines_prior_to_destroy: false) - end - - it "doesn't cancel the pipeline" do - expect(pipeline).not_to receive(:cancel_running) - - subject - end - end end end diff --git a/spec/services/ci/drop_pipeline_service_spec.rb b/spec/services/ci/drop_pipeline_service_spec.rb index 4adbb99b9e2..c6a118c6083 100644 --- a/spec/services/ci/drop_pipeline_service_spec.rb +++ b/spec/services/ci/drop_pipeline_service_spec.rb @@ -9,8 +9,10 @@ RSpec.describe Ci::DropPipelineService do let!(:cancelable_pipeline) { create(:ci_pipeline, :running, user: user) } let!(:running_build) { create(:ci_build, :running, pipeline: cancelable_pipeline) } + let!(:commit_status_running) { create(:commit_status, :running, pipeline: cancelable_pipeline) } let!(:success_pipeline) { create(:ci_pipeline, :success, user: user) } let!(:success_build) { create(:ci_build, :success, pipeline: success_pipeline) } + let!(:commit_status_success) { create(:commit_status, :success, pipeline: cancelable_pipeline) } describe '#execute_async_for_all' do subject { described_class.new.execute_async_for_all(user.pipelines, failure_reason, user) } diff --git a/spec/services/ci/external_pull_requests/create_pipeline_service_spec.rb b/spec/services/ci/external_pull_requests/create_pipeline_service_spec.rb index e25dd351bb3..2b310443b37 100644 --- a/spec/services/ci/external_pull_requests/create_pipeline_service_spec.rb +++ b/spec/services/ci/external_pull_requests/create_pipeline_service_spec.rb @@ -6,14 +6,13 @@ RSpec.describe Ci::ExternalPullRequests::CreatePipelineService do describe '#execute' do let_it_be(:project) { create(:project, :auto_devops, :repository) } let_it_be(:user) { create(:user) } - - let(:pull_request) { create(:external_pull_request, project: project) } + let_it_be_with_reload(:pull_request) { create(:external_pull_request, project: project) } before do project.add_maintainer(user) end - subject { described_class.new(project, user).execute(pull_request) } + subject(:response) { described_class.new(project, user).execute(pull_request) } context 'when pull request is open' do before do @@ -28,17 +27,20 @@ RSpec.describe Ci::ExternalPullRequests::CreatePipelineService do pull_request.update!(source_branch: source_branch.name, source_sha: source_branch.target) end - it 'creates a pipeline for external pull request' do - expect(subject).to be_valid - expect(subject).to be_persisted - expect(subject).to be_external_pull_request_event - expect(subject).to eq(project.ci_pipelines.last) - expect(subject.external_pull_request).to eq(pull_request) - expect(subject.user).to eq(user) - expect(subject.status).to eq('created') - expect(subject.ref).to eq(pull_request.source_branch) - expect(subject.sha).to eq(pull_request.source_sha) - expect(subject.source_sha).to eq(pull_request.source_sha) + it 'creates a pipeline for external pull request', :aggregate_failures do + pipeline = response.payload + + expect(response).to be_success + expect(pipeline).to be_valid + expect(pipeline).to be_persisted + expect(pipeline).to be_external_pull_request_event + expect(pipeline).to eq(project.ci_pipelines.last) + expect(pipeline.external_pull_request).to eq(pull_request) + expect(pipeline.user).to eq(user) + expect(pipeline.status).to eq('created') + expect(pipeline.ref).to eq(pull_request.source_branch) + expect(pipeline.sha).to eq(pull_request.source_sha) + expect(pipeline.source_sha).to eq(pull_request.source_sha) end end @@ -50,10 +52,12 @@ RSpec.describe Ci::ExternalPullRequests::CreatePipelineService do pull_request.update!(source_branch: source_branch.name, source_sha: commit.sha) end - it 'does nothing' do + it 'does nothing', :aggregate_failures do expect(Ci::CreatePipelineService).not_to receive(:new) - expect(subject).to be_nil + expect(response).to be_error + expect(response.message).to eq('The source sha is not the head of the source branch') + expect(response.payload).to be_nil end end end @@ -63,10 +67,12 @@ RSpec.describe Ci::ExternalPullRequests::CreatePipelineService do pull_request.update!(status: :closed) end - it 'does nothing' do + it 'does nothing', :aggregate_failures do expect(Ci::CreatePipelineService).not_to receive(:new) - expect(subject).to be_nil + expect(response).to be_error + expect(response.message).to eq('The pull request is not opened') + expect(response.payload).to be_nil end end end diff --git a/spec/services/ci/extract_sections_from_build_trace_service_spec.rb b/spec/services/ci/extract_sections_from_build_trace_service_spec.rb deleted file mode 100644 index c6ffcdcc6a8..00000000000 --- a/spec/services/ci/extract_sections_from_build_trace_service_spec.rb +++ /dev/null @@ -1,57 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Ci::ExtractSectionsFromBuildTraceService, '#execute' do - let(:user) { create(:user) } - let(:project) { create(:project) } - let(:build) { create(:ci_build, project: project) } - - subject { described_class.new(project, user) } - - shared_examples 'build trace has sections markers' do - before do - build.trace.set(File.read(expand_fixture_path('trace/trace_with_sections'))) - end - - it 'saves the correct extracted sections' do - expect(build.trace_sections).to be_empty - expect(subject.execute(build)).to be(true) - expect(build.trace_sections).not_to be_empty - end - - it "fails if trace_sections isn't empty" do - expect(subject.execute(build)).to be(true) - expect(build.trace_sections).not_to be_empty - - expect(subject.execute(build)).to be(false) - expect(build.trace_sections).not_to be_empty - end - end - - shared_examples 'build trace has no sections markers' do - before do - build.trace.set('no markerts') - end - - it 'extracts no sections' do - expect(build.trace_sections).to be_empty - expect(subject.execute(build)).to be(true) - expect(build.trace_sections).to be_empty - end - end - - context 'when the build has no user' do - it_behaves_like 'build trace has sections markers' - it_behaves_like 'build trace has no sections markers' - end - - context 'when the build has a valid user' do - before do - build.user = user - end - - it_behaves_like 'build trace has sections markers' - it_behaves_like 'build trace has no sections markers' - end -end diff --git a/spec/services/ci/pipeline_processing/shared_processing_service.rb b/spec/services/ci/pipeline_processing/shared_processing_service.rb index 5089f8d5dba..a4bc8e68b2d 100644 --- a/spec/services/ci/pipeline_processing/shared_processing_service.rb +++ b/spec/services/ci/pipeline_processing/shared_processing_service.rb @@ -871,7 +871,7 @@ RSpec.shared_examples 'Pipeline Processing Service' do end let(:pipeline) do - Ci::CreatePipelineService.new(project, user, { ref: 'master' }).execute(:push) + Ci::CreatePipelineService.new(project, user, { ref: 'master' }).execute(:push).payload end before do diff --git a/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb b/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb index 572808cd2db..b4ad2512593 100644 --- a/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb +++ b/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb @@ -10,7 +10,7 @@ RSpec.shared_context 'Pipeline Processing Service Tests With Yaml' do with_them do let(:test_file) { YAML.load_file(test_file_path) } - let(:pipeline) { Ci::CreatePipelineService.new(project, user, ref: 'master').execute(:pipeline) } + let(:pipeline) { Ci::CreatePipelineService.new(project, user, ref: 'master').execute(:pipeline).payload } before do stub_ci_pipeline_yaml_file(YAML.dump(test_file['config'])) diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_same_and_different_stage_needs.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_same_and_different_stage_needs.yml new file mode 100644 index 00000000000..115258c656e --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_same_and_different_stage_needs.yml @@ -0,0 +1,54 @@ +config: + stages: [first, second, third] + + job_a: + when: manual + stage: first + script: + - echo + + job_b: + when: manual + stage: second + script: + - echo + + job_c: + needs: ["job_b"] + stage: third + script: + - echo + + job_d: + needs: ["job_a"] + stage: third + script: + - echo + +init: + expect: + pipeline: skipped + stages: + first: skipped + second: skipped + third: skipped + jobs: + job_a: manual + job_b: manual + job_c: skipped + job_d: skipped + +transitions: + - event: play + jobs: [job_b] + expect: + pipeline: pending + stages: + first: skipped + second: pending + third: pending + jobs: + job_a: manual + job_b: pending + job_c: created + job_d: skipped diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_same_stage_needs.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_same_stage_needs.yml new file mode 100644 index 00000000000..fd15f7d1b57 --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_same_stage_needs.yml @@ -0,0 +1,70 @@ +config: + stages: [first, second, third, fourth] + + first_job: + stage: first + script: + - echo + + second_job: + stage: second + script: + - echo + when: manual + + third_job: + stage: third + needs: ["second_job"] + script: + - echo + + fourth_job: + stage: fourth + needs: ["third_job"] + script: + - echo + +init: + expect: + pipeline: pending + stages: + first: pending + second: created + third: created + fourth: created + jobs: + first_job: pending + second_job: created + third_job: created + fourth_job: created + +transitions: + - event: success + jobs: [first_job] + expect: + pipeline: success + stages: + first: success + second: skipped + third: skipped + fourth: skipped + jobs: + first_job: success + second_job: manual + third_job: skipped + fourth_job: skipped + + - event: play + jobs: [second_job] + expect: + pipeline: running + stages: + first: success + second: pending + third: skipped + fourth: skipped + jobs: + first_job: success + second_job: pending + third_job: created + fourth_job: created diff --git a/spec/services/ci/pipeline_trigger_service_spec.rb b/spec/services/ci/pipeline_trigger_service_spec.rb index 080ca1cf0cd..2f93b1ecd3c 100644 --- a/spec/services/ci/pipeline_trigger_service_spec.rb +++ b/spec/services/ci/pipeline_trigger_service_spec.rb @@ -24,9 +24,11 @@ RSpec.describe Ci::PipelineTriggerService do context 'when the pipeline was not created successfully' do let(:fail_pipeline) do receive(:execute).and_wrap_original do |original, *args| - pipeline = original.call(*args) + response = original.call(*args) + pipeline = response.payload pipeline.update!(failure_reason: 'unknown_failure') - pipeline + + response end end diff --git a/spec/services/ci/pipelines/add_job_service_spec.rb b/spec/services/ci/pipelines/add_job_service_spec.rb index a72ffbfdc87..bdf7e577fa8 100644 --- a/spec/services/ci/pipelines/add_job_service_spec.rb +++ b/spec/services/ci/pipelines/add_job_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Ci::Pipelines::AddJobService do + include ExclusiveLeaseHelpers + let_it_be(:pipeline) { create(:ci_pipeline) } let(:job) { build(:ci_build) } @@ -68,5 +70,38 @@ RSpec.describe Ci::Pipelines::AddJobService do execute end end + + context 'exclusive lock' do + let(:lock_uuid) { 'test' } + let(:lock_key) { "ci:pipelines:#{pipeline.id}:add-job" } + let(:lock_timeout) { 1.minute } + + before do + # "Please stub a default value first if message might be received with other args as well." + allow(Gitlab::ExclusiveLease).to receive(:new).and_call_original + end + + it 'uses exclusive lock' do + lease = stub_exclusive_lease(lock_key, lock_uuid, timeout: lock_timeout) + expect(lease).to receive(:try_obtain) + expect(lease).to receive(:cancel) + + expect(execute).to be_success + expect(execute.payload[:job]).to eq(job) + end + + context 'when the FF ci_pipeline_add_job_with_lock is disabled' do + before do + stub_feature_flags(ci_pipeline_add_job_with_lock: false) + end + + it 'does not use exclusive lock' do + expect(Gitlab::ExclusiveLease).not_to receive(:new).with(lock_key, timeout: lock_timeout) + + expect(execute).to be_success + expect(execute.payload[:job]).to eq(job) + end + end + end end end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 6e5d7725a7a..2f37d0ea42d 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -5,8 +5,8 @@ require 'spec_helper' module Ci RSpec.describe RegisterJobService do let_it_be(:group) { create(:group) } - let_it_be(:project, reload: true) { create(:project, group: group, shared_runners_enabled: false, group_runners_enabled: false) } - let_it_be(:pipeline) { create(:ci_pipeline, project: project) } + let_it_be_with_reload(:project) { create(:project, group: group, shared_runners_enabled: false, group_runners_enabled: false) } + let_it_be_with_reload(:pipeline) { create(:ci_pipeline, project: project) } let!(:shared_runner) { create(:ci_runner, :instance) } let!(:specific_runner) { create(:ci_runner, :project, projects: [project]) } @@ -14,7 +14,7 @@ module Ci let!(:pending_job) { create(:ci_build, :pending, :queued, pipeline: pipeline) } describe '#execute' do - context 'checks database loadbalancing stickiness' do + context 'checks database loadbalancing stickiness', :db_load_balancing do subject { described_class.new(shared_runner).execute } before do @@ -22,9 +22,6 @@ module Ci end it 'result is valid if replica did caught-up' do - allow(Gitlab::Database::LoadBalancing).to receive(:enable?) - .and_return(true) - expect(Gitlab::Database::LoadBalancing::Sticking).to receive(:all_caught_up?) .with(:runner, shared_runner.id) { true } @@ -32,9 +29,6 @@ module Ci end it 'result is invalid if replica did not caught-up' do - allow(Gitlab::Database::LoadBalancing).to receive(:enable?) - .and_return(true) - expect(Gitlab::Database::LoadBalancing::Sticking).to receive(:all_caught_up?) .with(:runner, shared_runner.id) { false } @@ -96,6 +90,9 @@ module Ci context 'allow shared runners' do before do project.update!(shared_runners_enabled: true) + pipeline.reload + pending_job.reload + pending_job.create_queuing_entry! end context 'for multiple builds' do @@ -470,13 +467,27 @@ module Ci context 'when depended job has not been completed yet' do let!(:pre_stage_job) { create(:ci_build, :pending, :queued, :manual, pipeline: pipeline, name: 'test', stage_idx: 0) } - it { expect(subject).to eq(pending_job) } + it { is_expected.to eq(pending_job) } end context 'when artifacts of depended job has been expired' do let!(:pre_stage_job) { create(:ci_build, :success, :expired, pipeline: pipeline, name: 'test', stage_idx: 0) } - it_behaves_like 'not pick' + context 'when the pipeline is locked' do + before do + pipeline.artifacts_locked! + end + + it { is_expected.to eq(pending_job) } + end + + context 'when the pipeline is unlocked' do + before do + pipeline.unlocked! + end + + it_behaves_like 'not pick' + end end context 'when artifacts of depended job has been erased' do @@ -493,8 +504,12 @@ module Ci let!(:pre_stage_job) { create(:ci_build, :success, :expired, pipeline: pipeline, name: 'test', stage_idx: 0) } before do - allow_any_instance_of(Ci::Build).to receive(:drop!) - .and_raise(ActiveRecord::StaleObjectError.new(pending_job, :drop!)) + pipeline.unlocked! + + allow_next_instance_of(Ci::Build) do |build| + expect(build).to receive(:drop!) + .and_raise(ActiveRecord::StaleObjectError.new(pending_job, :drop!)) + end end it 'does not drop nor pick' do @@ -709,7 +724,21 @@ module Ci stub_feature_flags(ci_pending_builds_queue_source: true) end - include_examples 'handles runner assignment' + context 'with ci_queueing_denormalize_shared_runners_information enabled' do + before do + stub_feature_flags(ci_queueing_denormalize_shared_runners_information: true) + end + + include_examples 'handles runner assignment' + end + + context 'with ci_queueing_denormalize_shared_runners_information disabled' do + before do + stub_feature_flags(ci_queueing_denormalize_shared_runners_information: false) + end + + include_examples 'handles runner assignment' + end end context 'when not using pending builds table' do @@ -783,6 +812,11 @@ module Ci end context 'when shared runner is used' do + before do + pending_job.reload + pending_job.create_queuing_entry! + end + let(:runner) { create(:ci_runner, :instance, tag_list: %w(tag1 tag2)) } let(:expected_shared_runner) { true } let(:expected_shard) { ::Gitlab::Ci::Queue::Metrics::DEFAULT_METRICS_SHARD } diff --git a/spec/services/ci/resource_groups/assign_resource_from_resource_group_service_spec.rb b/spec/services/ci/resource_groups/assign_resource_from_resource_group_service_spec.rb index a741e3b49e7..53aa842bc28 100644 --- a/spec/services/ci/resource_groups/assign_resource_from_resource_group_service_spec.rb +++ b/spec/services/ci/resource_groups/assign_resource_from_resource_group_service_spec.rb @@ -51,14 +51,32 @@ RSpec.describe Ci::ResourceGroups::AssignResourceFromResourceGroupService do end context 'when there are no available resources' do + let!(:other_build) { create(:ci_build) } + before do - resource_group.assign_resource_to(create(:ci_build)) + resource_group.assign_resource_to(other_build) end it 'does not request resource' do expect_any_instance_of(Ci::Build).not_to receive(:enqueue_waiting_for_resource) subject + + expect(build.reload).to be_waiting_for_resource + end + + context 'when there is a stale build assigned to a resource' do + before do + other_build.doom! + other_build.update_column(:updated_at, 10.minutes.ago) + end + + it 'releases the resource from the stale build and assignes to the waiting build' do + subject + + expect(build.reload).to be_pending + expect(build.resource).to be_present + end end end end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 42d6e66b38b..ce2e6ba5e15 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -60,7 +60,7 @@ RSpec.describe Ci::RetryBuildService do artifacts_file artifacts_metadata artifacts_size commands resource resource_group_id processed security_scans author pipeline_id report_results pending_state pages_deployments - queuing_entry runtime_metadata].freeze + queuing_entry runtime_metadata trace_metadata].freeze shared_examples 'build duplication' do let_it_be(:another_pipeline) { create(:ci_empty_pipeline, project: project) } diff --git a/spec/services/dependency_proxy/auth_token_service_spec.rb b/spec/services/dependency_proxy/auth_token_service_spec.rb index 4b96f9d75a9..6214d75dfa0 100644 --- a/spec/services/dependency_proxy/auth_token_service_spec.rb +++ b/spec/services/dependency_proxy/auth_token_service_spec.rb @@ -14,6 +14,19 @@ RSpec.describe DependencyProxy::AuthTokenService do result = subject expect(result['user_id']).to eq(user.id) + expect(result['deploy_token']).to be_nil + end + + context 'with a deploy token' do + let_it_be(:deploy_token) { create(:deploy_token) } + let_it_be(:token) { build_jwt(deploy_token) } + + it 'returns the deploy token' do + result = subject + + expect(result['deploy_token']).to eq(deploy_token.token) + expect(result['user_id']).to be_nil + end end it 'raises an error if the token is expired' do diff --git a/spec/services/dependency_proxy/download_blob_service_spec.rb b/spec/services/dependency_proxy/download_blob_service_spec.rb index 4b5c6b5bd6a..2f293b8a46b 100644 --- a/spec/services/dependency_proxy/download_blob_service_spec.rb +++ b/spec/services/dependency_proxy/download_blob_service_spec.rb @@ -8,7 +8,7 @@ RSpec.describe DependencyProxy::DownloadBlobService do let(:token) { Digest::SHA256.hexdigest('123') } let(:blob_sha) { Digest::SHA256.hexdigest('ruby:2.7.0') } - subject { described_class.new(image, blob_sha, token).execute } + subject(:download_blob) { described_class.new(image, blob_sha, token).execute } context 'remote request is successful' do before do @@ -18,6 +18,21 @@ RSpec.describe DependencyProxy::DownloadBlobService do it { expect(subject[:status]).to eq(:success) } it { expect(subject[:file]).to be_a(Tempfile) } it { expect(subject[:file].size).to eq(6) } + + it 'streams the download' do + expected_options = { headers: anything, stream_body: true } + + expect(Gitlab::HTTP).to receive(:perform_request).with(Net::HTTP::Get, anything, expected_options) + + download_blob + end + + it 'skips read_total_timeout', :aggregate_failures do + stub_const('GitLab::HTTP::DEFAULT_READ_TOTAL_TIMEOUT', 0) + + expect(Gitlab::Metrics::System).not_to receive(:monotonic_time) + expect(download_blob).to include(status: :success) + end end context 'remote request is not found' do diff --git a/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb b/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb index 4ba53d49d38..3fac749be29 100644 --- a/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb +++ b/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb @@ -26,6 +26,7 @@ RSpec.describe DependencyProxy::FindOrCreateBlobService do expect(subject[:status]).to eq(:success) expect(subject[:blob]).to be_a(DependencyProxy::Blob) expect(subject[:blob]).to be_persisted + expect(subject[:from_cache]).to eq false end end @@ -36,6 +37,7 @@ RSpec.describe DependencyProxy::FindOrCreateBlobService do expect(subject[:status]).to eq(:success) expect(subject[:blob]).to be_a(DependencyProxy::Blob) expect(subject[:blob]).to eq(blob) + expect(subject[:from_cache]).to eq true end end diff --git a/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb b/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb index 1c8ae860d10..5896aa255f0 100644 --- a/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb +++ b/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb @@ -30,6 +30,7 @@ RSpec.describe DependencyProxy::FindOrCreateManifestService do expect(subject[:status]).to eq(:success) expect(subject[:manifest]).to be_a(DependencyProxy::Manifest) expect(subject[:manifest]).to be_persisted + expect(subject[:from_cache]).to eq false end end @@ -62,6 +63,7 @@ RSpec.describe DependencyProxy::FindOrCreateManifestService do expect(subject[:status]).to eq(:success) expect(subject[:manifest]).to be_a(DependencyProxy::Manifest) expect(subject[:manifest]).to eq(dependency_proxy_manifest) + expect(subject[:from_cache]).to eq true end end @@ -81,6 +83,7 @@ RSpec.describe DependencyProxy::FindOrCreateManifestService do expect(subject[:manifest]).to eq(dependency_proxy_manifest) expect(subject[:manifest].content_type).to eq(content_type) expect(subject[:manifest].digest).to eq(digest) + expect(subject[:from_cache]).to eq false end end diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb index 2e1de367da3..4f761454516 100644 --- a/spec/services/draft_notes/publish_service_spec.rb +++ b/spec/services/draft_notes/publish_service_spec.rb @@ -66,8 +66,8 @@ RSpec.describe DraftNotes::PublishService do let(:commit_id) { nil } before do - create(:draft_note, merge_request: merge_request, author: user, note: 'first note', commit_id: commit_id, position: position) - create(:draft_note, merge_request: merge_request, author: user, note: 'second note', commit_id: commit_id, position: position) + create(:draft_note_on_text_diff, merge_request: merge_request, author: user, note: 'first note', commit_id: commit_id, position: position) + create(:draft_note_on_text_diff, merge_request: merge_request, author: user, note: 'second note', commit_id: commit_id, position: position) end context 'when review fails to create' do @@ -127,6 +127,30 @@ RSpec.describe DraftNotes::PublishService do publish end + context 'capturing diff notes positions' do + before do + # Need to execute this to ensure that we'll be able to test creation of + # DiffNotePosition records as that only happens when the `MergeRequest#merge_ref_head` + # is present. This service creates that for the specified merge request. + MergeRequests::MergeToRefService.new(project: project, current_user: user).execute(merge_request) + end + + it 'creates diff_note_positions for diff notes' do + publish + + notes = merge_request.notes.order(id: :asc) + expect(notes.first.diff_note_positions).to be_any + expect(notes.last.diff_note_positions).to be_any + end + + it 'does not requests a lot from Gitaly', :request_store do + # NOTE: This should be reduced as we work on reducing Gitaly calls. + # Gitaly requests shouldn't go above this threshold as much as possible + # as it may add more to the Gitaly N+1 issue we are experiencing. + expect { publish }.to change { Gitlab::GitalyClient.get_request_count }.by(11) + end + end + context 'commit_id is set' do let(:commit_id) { commit.id } diff --git a/spec/services/ci/stop_environments_service_spec.rb b/spec/services/environments/stop_service_spec.rb index d5ef67c871c..52be512612d 100644 --- a/spec/services/ci/stop_environments_service_spec.rb +++ b/spec/services/environments/stop_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::StopEnvironmentsService do +RSpec.describe Environments::StopService do include CreateEnvironmentsHelpers let(:project) { create(:project, :private, :repository) } @@ -11,6 +11,59 @@ RSpec.describe Ci::StopEnvironmentsService do let(:service) { described_class.new(project, user) } describe '#execute' do + subject { service.execute(environment) } + + let_it_be(:project) { create(:project, :private, :repository) } + let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } } + + let(:user) { developer } + + context 'with a deployment' do + let!(:environment) { review_job.persisted_environment } + let!(:pipeline) { create(:ci_pipeline, project: project) } + let!(:review_job) { create(:ci_build, :with_deployment, :start_review_app, pipeline: pipeline, project: project) } + let!(:stop_review_job) { create(:ci_build, :with_deployment, :stop_review_app, :manual, pipeline: pipeline, project: project) } + + before do + review_job.success! + end + + it 'stops the environment' do + expect { subject }.to change { environment.reload.state }.from('available').to('stopped') + end + + it 'plays the stop action' do + expect { subject }.to change { stop_review_job.reload.status }.from('manual').to('pending') + end + + context 'when an environment has already been stopped' do + let!(:environment) { create(:environment, :stopped, project: project) } + + it 'does not play the stop action' do + expect { subject }.not_to change { stop_review_job.reload.status } + end + end + end + + context 'without a deployment' do + let!(:environment) { create(:environment, project: project) } + + it 'stops the environment' do + expect { subject }.to change { environment.reload.state }.from('available').to('stopped') + end + + context 'when the actor is a reporter' do + let(:user) { reporter } + + it 'does not stop the environment' do + expect { subject }.not_to change { environment.reload.state } + end + end + end + end + + describe '#execute_for_branch' do context 'when environment with review app exists' do before do create(:environment, :with_review_app, project: project, @@ -48,8 +101,9 @@ RSpec.describe Ci::StopEnvironmentsService do context 'when environment is not stopped' do before do - allow_any_instance_of(Environment) - .to receive(:state).and_return(:stopped) + allow_next_found_instance_of(Environment) do |environment| + allow(environment).to receive(:state).and_return(:stopped) + end end it 'does not stop environment' do @@ -101,7 +155,7 @@ RSpec.describe Ci::StopEnvironmentsService do context 'when environment does not exist' do it 'does not raise error' do - expect { service.execute('master') } + expect { service.execute_for_branch('master') } .not_to raise_error end end @@ -238,16 +292,12 @@ RSpec.describe Ci::StopEnvironmentsService do end def expect_environment_stopped_on(branch) - expect_any_instance_of(Environment) - .to receive(:stop!) - - service.execute(branch) + expect { service.execute_for_branch(branch) } + .to change { Environment.last.state }.from('available').to('stopped') end def expect_environment_not_stopped_on(branch) - expect_any_instance_of(Environment) - .not_to receive(:stop!) - - service.execute(branch) + expect { service.execute_for_branch(branch) } + .not_to change { Environment.last.state } end end diff --git a/spec/services/error_tracking/issue_details_service_spec.rb b/spec/services/error_tracking/issue_details_service_spec.rb index 1954640a512..8cc2688d198 100644 --- a/spec/services/error_tracking/issue_details_service_spec.rb +++ b/spec/services/error_tracking/issue_details_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe ErrorTracking::IssueDetailsService do describe '#execute' do context 'with authorized user' do context 'when issue_details returns a detailed error' do - let(:detailed_error) { build(:detailed_error_tracking_error) } + let(:detailed_error) { build(:error_tracking_sentry_detailed_error) } let(:params) { { issue_id: detailed_error.id } } before do @@ -39,6 +39,21 @@ RSpec.describe ErrorTracking::IssueDetailsService do include_examples 'error tracking service data not ready', :issue_details include_examples 'error tracking service sentry error handling', :issue_details include_examples 'error tracking service http status handling', :issue_details + + context 'integrated error tracking' do + let_it_be(:error) { create(:error_tracking_error, project: project) } + + let(:params) { { issue_id: error.id } } + + before do + error_tracking_setting.update!(integrated: true) + end + + it 'returns the error in detailed format' do + expect(result[:status]).to eq(:success) + expect(result[:issue].to_json).to eq(error.to_sentry_detailed_error.to_json) + end + end end include_examples 'error tracking service unauthorized user' diff --git a/spec/services/error_tracking/issue_latest_event_service_spec.rb b/spec/services/error_tracking/issue_latest_event_service_spec.rb index b7560762ae4..e914cb1241e 100644 --- a/spec/services/error_tracking/issue_latest_event_service_spec.rb +++ b/spec/services/error_tracking/issue_latest_event_service_spec.rb @@ -5,12 +5,14 @@ require 'spec_helper' RSpec.describe ErrorTracking::IssueLatestEventService do include_context 'sentry error tracking context' - subject { described_class.new(project, user) } + let(:params) { {} } + + subject { described_class.new(project, user, params) } describe '#execute' do context 'with authorized user' do context 'when issue_latest_event returns an error event' do - let(:error_event) { build(:error_tracking_error_event) } + let(:error_event) { build(:error_tracking_sentry_error_event) } before do expect(error_tracking_setting) @@ -25,6 +27,22 @@ RSpec.describe ErrorTracking::IssueLatestEventService do include_examples 'error tracking service data not ready', :issue_latest_event include_examples 'error tracking service sentry error handling', :issue_latest_event include_examples 'error tracking service http status handling', :issue_latest_event + + context 'integrated error tracking' do + let_it_be(:error) { create(:error_tracking_error, project: project) } + let_it_be(:event) { create(:error_tracking_error_event, error: error) } + + let(:params) { { issue_id: error.id } } + + before do + error_tracking_setting.update!(integrated: true) + end + + it 'returns the latest event in expected format' do + expect(result[:status]).to eq(:success) + expect(result[:latest_event].to_json).to eq(event.to_sentry_error_event.to_json) + end + end end include_examples 'error tracking service unauthorized user' diff --git a/spec/services/error_tracking/issue_update_service_spec.rb b/spec/services/error_tracking/issue_update_service_spec.rb index 9ed24038ed8..31a66654100 100644 --- a/spec/services/error_tracking/issue_update_service_spec.rb +++ b/spec/services/error_tracking/issue_update_service_spec.rb @@ -114,6 +114,21 @@ RSpec.describe ErrorTracking::IssueUpdateService do end include_examples 'error tracking service sentry error handling', :update_issue + + context 'integrated error tracking' do + let(:error) { create(:error_tracking_error, project: project) } + let(:arguments) { { issue_id: error.id, status: 'resolved' } } + let(:update_issue_response) { { updated: true, status: :success, closed_issue_iid: nil } } + + before do + error_tracking_setting.update!(integrated: true) + end + + it 'resolves the error and responds with expected format' do + expect(update_service.execute).to eq(update_issue_response) + expect(error.reload.status).to eq('resolved') + end + end end include_examples 'error tracking service unauthorized user' diff --git a/spec/services/error_tracking/list_issues_service_spec.rb b/spec/services/error_tracking/list_issues_service_spec.rb index 518f2a80826..b49095ab8b9 100644 --- a/spec/services/error_tracking/list_issues_service_spec.rb +++ b/spec/services/error_tracking/list_issues_service_spec.rb @@ -52,6 +52,20 @@ RSpec.describe ErrorTracking::ListIssuesService do include_examples 'error tracking service unauthorized user' include_examples 'error tracking service disabled' + + context 'integrated error tracking' do + let_it_be(:error) { create(:error_tracking_error, project: project) } + + before do + error_tracking_setting.update!(integrated: true) + end + + it 'returns the error in expected format' do + expect(result[:status]).to eq(:success) + expect(result[:issues].size).to eq(1) + expect(result[:issues].first.to_json).to eq(error.to_sentry_error.to_json) + end + end end describe '#external_url' do diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index 19694a0a354..a93f594b360 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -92,7 +92,7 @@ RSpec.describe Git::BranchHooksService, :clean_gitlab_redis_shared_state do end describe 'Push Event' do - let(:event) { Event.pushed_action.first } + let(:event) { Event.pushed_action.take } subject(:execute_service) { service.execute } @@ -134,7 +134,7 @@ RSpec.describe Git::BranchHooksService, :clean_gitlab_redis_shared_state do context 'when usage ping is disabled' do before do - stub_application_setting(usage_ping_enabled: false) + allow(::ServicePing::ServicePingSettings).to receive(:enabled?).and_return(false) end it 'does not track the event' do @@ -171,7 +171,7 @@ RSpec.describe Git::BranchHooksService, :clean_gitlab_redis_shared_state do end end - context "with a new branch" do + context "with a new default branch" do let(:oldrev) { Gitlab::Git::BLANK_SHA } it 'generates a push event with more than one commit' do @@ -183,12 +183,32 @@ RSpec.describe Git::BranchHooksService, :clean_gitlab_redis_shared_state do expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) expect(event.push_event_payload.commit_from).to be_nil expect(event.push_event_payload.commit_to).to eq(newrev) - expect(event.push_event_payload.commit_title).to eq('Initial commit') + expect(event.push_event_payload.commit_title).to eq('Change some files') expect(event.push_event_payload.ref).to eq('master') expect(event.push_event_payload.commit_count).to be > 1 end end + context "with a new non-default branch" do + let(:oldrev) { Gitlab::Git::BLANK_SHA } + let(:branch) { 'fix' } + let(:commit_id) { project.commit(branch).id } + + it 'generates a push event with more than one commit' do + execute_service + + expect(event).to be_an_instance_of(PushEvent) + expect(event.project).to eq(project) + expect(event).to be_pushed_action + expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) + expect(event.push_event_payload.commit_from).to be_nil + expect(event.push_event_payload.commit_to).to eq(newrev) + expect(event.push_event_payload.commit_title).to eq('Test file for directories with a leading dot') + expect(event.push_event_payload.ref).to eq('fix') + expect(event.push_event_payload.commit_count).to be > 1 + end + end + context 'removing a branch' do let(:newrev) { Gitlab::Git::BLANK_SHA } diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index fc629fe583d..d70e458ba5e 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -151,9 +151,9 @@ RSpec.describe Git::BranchPushService, services: true do it "when pushing a new branch for the first time" do expect(UpdateMergeRequestsWorker) .to receive(:perform_async) - .with(project.id, user.id, blankrev, 'newrev', ref) + .with(project.id, user.id, blankrev, newrev, ref) - execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) + execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) end end @@ -162,13 +162,13 @@ RSpec.describe Git::BranchPushService, services: true do it "calls the copy attributes method for the first push to the default branch" do expect(project.repository).to receive(:copy_gitattributes).with('master') - execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) + execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) end it "calls the copy attributes method for changes to the default branch" do expect(project.repository).to receive(:copy_gitattributes).with(ref) - execute_service(project, user, oldrev: 'oldrev', newrev: 'newrev', ref: ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end end @@ -195,7 +195,7 @@ RSpec.describe Git::BranchPushService, services: true do it "when pushing a branch for the first time" do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) + execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) expect(project.protected_branches).not_to be_empty expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER]) expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER]) @@ -206,7 +206,7 @@ RSpec.describe Git::BranchPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) + execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) expect(project.protected_branches).to be_empty end @@ -216,7 +216,7 @@ RSpec.describe Git::BranchPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) + execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) expect(project.protected_branches).not_to be_empty expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER]) @@ -231,7 +231,7 @@ RSpec.describe Git::BranchPushService, services: true do expect(project.default_branch).to eq("master") expect(ProtectedBranches::CreateService).not_to receive(:new) - execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) + execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) expect(project.protected_branches).not_to be_empty expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::NO_ACCESS]) @@ -243,7 +243,7 @@ RSpec.describe Git::BranchPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - execute_service(project, user, oldrev: blankrev, newrev: 'newrev', ref: ref) + execute_service(project, user, oldrev: blankrev, newrev: newrev, ref: ref) expect(project.protected_branches).not_to be_empty expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MAINTAINER]) expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER]) @@ -251,7 +251,7 @@ RSpec.describe Git::BranchPushService, services: true do it "when pushing new commits to existing branch" do expect(project).to receive(:execute_hooks) - execute_service(project, user, oldrev: 'oldrev', newrev: 'newrev', ref: ref) + execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end end end @@ -597,7 +597,7 @@ RSpec.describe Git::BranchPushService, services: true do let(:oldrev) { blankrev } it 'does nothing' do - expect(::Ci::StopEnvironmentsService).not_to receive(:new) + expect(::Environments::StopService).not_to receive(:new) execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end @@ -605,7 +605,7 @@ RSpec.describe Git::BranchPushService, services: true do context 'update branch' do it 'does nothing' do - expect(::Ci::StopEnvironmentsService).not_to receive(:new) + expect(::Environments::StopService).not_to receive(:new) execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) end @@ -615,10 +615,10 @@ RSpec.describe Git::BranchPushService, services: true do let(:newrev) { blankrev } it 'stops environments' do - expect_next_instance_of(::Ci::StopEnvironmentsService) do |stop_service| + expect_next_instance_of(::Environments::StopService) do |stop_service| expect(stop_service.project).to eq(project) expect(stop_service.current_user).to eq(user) - expect(stop_service).to receive(:execute).with(branch) + expect(stop_service).to receive(:execute_for_branch).with(branch) end execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref) diff --git a/spec/services/git/process_ref_changes_service_spec.rb b/spec/services/git/process_ref_changes_service_spec.rb index ac9bac4e6ad..2a223091d0c 100644 --- a/spec/services/git/process_ref_changes_service_spec.rb +++ b/spec/services/git/process_ref_changes_service_spec.rb @@ -116,6 +116,8 @@ RSpec.describe Git::ProcessRefChangesService do if changes_method == :tag_changes allow_any_instance_of(Repository).to receive(:tag_exists?) { true } end + + allow(Gitlab::Git::Commit).to receive(:between) { [] } end context 'when git_push_create_all_pipelines is disabled' do @@ -150,6 +152,8 @@ RSpec.describe Git::ProcessRefChangesService do context 'with invalid .gitlab-ci.yml' do before do stub_ci_pipeline_yaml_file(nil) + + allow(Gitlab::Git::Commit).to receive(:between) { [] } end it 'does not create a pipeline' do @@ -190,6 +194,8 @@ RSpec.describe Git::ProcessRefChangesService do allow(MergeRequests::PushedBranchesService).to receive(:new).and_return( double(execute: %w(create1 create2)), double(execute: %w(changed1)), double(execute: %w(removed2)) ) + + allow(Gitlab::Git::Commit).to receive(:between).and_return([]) end it 'schedules job for existing merge requests' do diff --git a/spec/services/ide/terminal_config_service_spec.rb b/spec/services/ide/terminal_config_service_spec.rb index 483b6413be3..73614f28b06 100644 --- a/spec/services/ide/terminal_config_service_spec.rb +++ b/spec/services/ide/terminal_config_service_spec.rb @@ -47,7 +47,6 @@ RSpec.describe Ide::TerminalConfigService do status: :success, terminal: { tag_list: [], - yaml_variables: [], job_variables: [], options: { script: ["sleep 60"] } }) @@ -62,7 +61,6 @@ RSpec.describe Ide::TerminalConfigService do status: :success, terminal: { tag_list: [], - yaml_variables: [], job_variables: [], options: { before_script: ["ls"], script: ["sleep 60"] } }) diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 9a70de80123..b1d4877e138 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -222,7 +222,7 @@ RSpec.describe Issues::CloseService do it 'verifies the number of queries' do recorded = ActiveRecord::QueryRecorder.new { close_issue } - expected_queries = 24 + expected_queries = 25 expect(recorded.count).to be <= expected_queries expect(recorded.cached_count).to eq(0) diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 1e922401028..29ac7df88eb 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -517,7 +517,7 @@ RSpec.describe Issues::UpdateService, :mailer do update_issue(description: "- [x] Task 1 #{user.to_reference}\n- [ ] Task 2 #{user.to_reference}") end - expect(recorded.count).to eq(baseline.count - 1) + expect(recorded.count).to eq(baseline.count) expect(recorded.cached_count).to eq(0) end end diff --git a/spec/services/jira/requests/projects/list_service_spec.rb b/spec/services/jira/requests/projects/list_service_spec.rb index ab15254d948..78ee9cb9698 100644 --- a/spec/services/jira/requests/projects/list_service_spec.rb +++ b/spec/services/jira/requests/projects/list_service_spec.rb @@ -43,20 +43,7 @@ RSpec.describe Jira::Requests::Projects::ListService do stub_request(:get, expected_url_pattern).to_return(status: 200, body: response_body, headers: response_headers) end - context 'when the request to Jira returns an error' do - before do - expect_next(JIRA::Client).to receive(:get).and_raise(Timeout::Error) - end - - it 'returns an error response' do - expect(Gitlab::ProjectServiceLogger).to receive(:error).with( - hash_including( - error: hash_including(:exception_class, :exception_message, :exception_backtrace))) - .and_call_original - expect(subject.error?).to be_truthy - expect(subject.message).to eq('Jira request error: Timeout::Error') - end - end + it_behaves_like 'a service that handles Jira API errors' context 'when jira runs on a subpath' do let(:jira_integration) { create(:jira_integration, url: 'http://jira.example.com/jira') } diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index ee5250b5b3d..15ed5c5a33f 100644 --- a/spec/services/members/create_service_spec.rb +++ b/spec/services/members/create_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_cache, :clean_gitlab_redis_shared_state, :sidekiq_inline do - let_it_be(:source) { create(:project) } + let_it_be(:source, reload: true) { create(:project) } let_it_be(:user) { create(:user) } let_it_be(:member) { create(:user) } let_it_be(:user_ids) { member.id.to_s } @@ -89,7 +89,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ context 'when invite_source is not passed' do let(:additional_params) { {} } - it 'tracks the invite source as unknown' do + it 'raises an error' do expect { execute_service }.to raise_error(ArgumentError, 'No invite source provided.') expect_no_snowplow_event @@ -126,4 +126,74 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ end end end + + context 'when tracking the areas of focus', :snowplow do + context 'when areas_of_focus is not passed' do + it 'does not track' do + execute_service + + expect_no_snowplow_event(category: described_class.name, action: 'area_of_focus') + end + end + + context 'when 1 areas_of_focus is passed' do + let(:additional_params) { { invite_source: '_invite_source_', areas_of_focus: ['no_selection'] } } + + it 'tracks the areas_of_focus from params' do + execute_service + + expect_snowplow_event( + category: described_class.name, + action: 'area_of_focus', + label: 'no_selection', + property: source.members.last.id.to_s + ) + end + + context 'when passing many user ids' do + let(:another_user) { create(:user) } + let(:user_ids) { [member.id, another_user.id].join(',') } + + it 'tracks the areas_of_focus from params' do + execute_service + + members = source.members.last(2) + + expect_snowplow_event( + category: described_class.name, + action: 'area_of_focus', + label: 'no_selection', + property: members.first.id.to_s + ) + expect_snowplow_event( + category: described_class.name, + action: 'area_of_focus', + label: 'no_selection', + property: members.last.id.to_s + ) + end + end + end + + context 'when multiple areas_of_focus are passed' do + let(:additional_params) { { invite_source: '_invite_source_', areas_of_focus: %w[no_selection Other] } } + + it 'tracks the areas_of_focus from params' do + execute_service + + expect_snowplow_event( + category: described_class.name, + action: 'area_of_focus', + label: 'no_selection', + property: source.members.last.id.to_s + ) + expect_snowplow_event( + category: described_class.name, + action: 'area_of_focus', + label: 'Other', + property: source.members.last.id.to_s + ) + end + end + end end diff --git a/spec/services/members/import_project_team_service_spec.rb b/spec/services/members/import_project_team_service_spec.rb new file mode 100644 index 00000000000..96e8db1ba73 --- /dev/null +++ b/spec/services/members/import_project_team_service_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::ImportProjectTeamService do + describe '#execute' do + let_it_be(:source_project) { create(:project) } + let_it_be(:target_project) { create(:project) } + let_it_be(:user) { create(:user) } + + subject { described_class.new(user, { id: target_project_id, project_id: source_project_id }) } + + before_all do + source_project.add_guest(user) + target_project.add_maintainer(user) + end + + context 'when project team members are imported successfully' do + let(:source_project_id) { source_project.id } + let(:target_project_id) { target_project.id } + + it 'returns true' do + expect(subject.execute).to be(true) + end + end + + context 'when the project team import fails' do + context 'when the target project cannot be found' do + let(:source_project_id) { source_project.id } + let(:target_project_id) { non_existing_record_id } + + it 'returns false' do + expect(subject.execute).to be(false) + end + end + + context 'when the source project cannot be found' do + let(:source_project_id) { non_existing_record_id } + let(:target_project_id) { target_project.id } + + it 'returns false' do + expect(subject.execute).to be(false) + end + end + + context 'when the user doing the import does not exist' do + let(:user) { nil } + let(:source_project_id) { source_project.id } + let(:target_project_id) { target_project.id } + + it 'returns false' do + expect(subject.execute).to be(false) + end + end + + context 'when the user does not have permission to read the source project members' do + let(:user) { create(:user) } + let(:source_project_id) { create(:project, :private).id } + let(:target_project_id) { target_project.id } + + it 'returns false' do + expect(subject.execute).to be(false) + end + end + + context 'when the user does not have permission to admin the target project' do + let(:source_project_id) { source_project.id } + let(:target_project_id) { create(:project).id } + + it 'returns false' do + expect(subject.execute).to be(false) + end + end + + context 'when the source and target project are valid but the ProjectTeam#import command fails' do + let(:source_project_id) { source_project.id } + let(:target_project_id) { target_project.id } + + before do + allow_next_instance_of(ProjectTeam) do |project_team| + allow(project_team).to receive(:import).and_return(false) + end + end + + it 'returns false' do + expect(subject.execute).to be(false) + end + end + end + end +end diff --git a/spec/services/merge_requests/add_spent_time_service_spec.rb b/spec/services/merge_requests/add_spent_time_service_spec.rb index db3380e9582..1e0b3e07f26 100644 --- a/spec/services/merge_requests/add_spent_time_service_spec.rb +++ b/spec/services/merge_requests/add_spent_time_service_spec.rb @@ -8,7 +8,7 @@ RSpec.describe MergeRequests::AddSpentTimeService do let_it_be_with_reload(:merge_request) { create(:merge_request, :simple, :unique_branches, source_project: project) } let(:duration) { 1500 } - let(:params) { { spend_time: { duration: duration, user_id: user.id } } } + let(:params) { { spend_time: { duration: duration, summary: 'summary', user_id: user.id } } } let(:service) { described_class.new(project: project, current_user: user, params: params) } describe '#execute' do @@ -16,13 +16,14 @@ RSpec.describe MergeRequests::AddSpentTimeService do project.add_developer(user) end - it 'creates a new timelog with the specified duration' do + it 'creates a new timelog with the specified duration and summary' do expect { service.execute(merge_request) }.to change { Timelog.count }.from(0).to(1) timelog = merge_request.timelogs.last expect(timelog).not_to be_nil expect(timelog.time_spent).to eq(1500) + expect(timelog.summary).to eq('summary') end it 'creates a system note with the time added' do diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index f6336a85a25..86d972bc516 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -92,7 +92,7 @@ RSpec.describe MergeRequests::CloseService do end it 'clean up environments for the merge request' do - expect_next_instance_of(Ci::StopEnvironmentsService) do |service| + expect_next_instance_of(::Environments::StopService) do |service| expect(service).to receive(:execute_for_merge_request).with(merge_request) end diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb index a0ac168f3d7..d84ce8d15b4 100644 --- a/spec/services/merge_requests/create_pipeline_service_spec.rb +++ b/spec/services/merge_requests/create_pipeline_service_spec.rb @@ -18,7 +18,7 @@ RSpec.describe MergeRequests::CreatePipelineService do end describe '#execute' do - subject { service.execute(merge_request) } + subject(:response) { service.execute(merge_request) } before do stub_ci_pipeline_yaml_file(YAML.dump(config)) @@ -39,14 +39,15 @@ RSpec.describe MergeRequests::CreatePipelineService do let(:source_project) { project } it 'creates a detached merge request pipeline' do - expect { subject }.to change { Ci::Pipeline.count }.by(1) + expect { response }.to change { Ci::Pipeline.count }.by(1) - expect(subject).to be_persisted - expect(subject).to be_detached_merge_request_pipeline + expect(response).to be_success + expect(response.payload).to be_persisted + expect(response.payload).to be_detached_merge_request_pipeline end it 'defaults to merge_request_event' do - expect(subject.source).to eq('merge_request_event') + expect(response.payload.source).to eq('merge_request_event') end context 'with fork merge request' do @@ -58,7 +59,7 @@ RSpec.describe MergeRequests::CreatePipelineService do let(:actor) { user } it 'creates a pipeline in the target project' do - expect(subject.project).to eq(project) + expect(response.payload.project).to eq(project) end context 'when source branch is protected' do @@ -66,7 +67,7 @@ RSpec.describe MergeRequests::CreatePipelineService do let!(:protected_branch) { create(:protected_branch, name: '*', project: project) } it 'creates a pipeline in the source project' do - expect(subject.project).to eq(source_project) + expect(response.payload.project).to eq(source_project) end end @@ -74,7 +75,7 @@ RSpec.describe MergeRequests::CreatePipelineService do let!(:protected_branch) { create(:protected_branch, :developers_can_merge, name: '*', project: project) } it 'creates a pipeline in the target project' do - expect(subject.project).to eq(project) + expect(response.payload.project).to eq(project) end end end @@ -85,7 +86,7 @@ RSpec.describe MergeRequests::CreatePipelineService do end it 'creates a pipeline in the source project' do - expect(subject.project).to eq(source_project) + expect(response.payload.project).to eq(source_project) end end end @@ -99,15 +100,16 @@ RSpec.describe MergeRequests::CreatePipelineService do end it 'creates a pipeline in the source project' do - expect(subject.project).to eq(source_project) + expect(response.payload.project).to eq(source_project) end end context 'when actor does not have permission to create pipelines' do let(:actor) { create(:user) } - it 'returns nothing' do - expect(subject.full_error_messages).to include('Insufficient permissions to create a new pipeline') + it 'responds with error' do + expect(response).to be_error + expect(response.message).to include('Insufficient permissions to create a new pipeline') end end end @@ -139,7 +141,7 @@ RSpec.describe MergeRequests::CreatePipelineService do end it 'does not create a pipeline' do - expect { subject }.not_to change { Ci::Pipeline.count } + expect { response }.not_to change { Ci::Pipeline.count } end end @@ -154,7 +156,7 @@ RSpec.describe MergeRequests::CreatePipelineService do end it 'does not create a pipeline' do - expect { subject }.not_to change { Ci::Pipeline.count } + expect { response }.not_to change { Ci::Pipeline.count } end end end @@ -170,11 +172,12 @@ RSpec.describe MergeRequests::CreatePipelineService do } end - it 'creates a detached merge request pipeline' do - expect { subject }.to change { Ci::Pipeline.count }.by(1) + it 'creates a detached merge request pipeline', :aggregate_failures do + expect { response }.to change { Ci::Pipeline.count }.by(1) - expect(subject).to be_persisted - expect(subject).to be_detached_merge_request_pipeline + expect(response).to be_success + expect(response.payload).to be_persisted + expect(response.payload).to be_detached_merge_request_pipeline end end @@ -188,10 +191,25 @@ RSpec.describe MergeRequests::CreatePipelineService do } end - it 'does not create a pipeline' do - expect { subject }.not_to change { Ci::Pipeline.count } + it 'does not create a pipeline', :aggregate_failures do + expect { response }.not_to change { Ci::Pipeline.count } + expect(response).to be_error end end end + + context 'when merge request has no commits' do + before do + allow(merge_request).to receive(:has_no_commits?).and_return(true) + end + + it 'does not create a pipeline', :aggregate_failures do + expect { response }.not_to change { Ci::Pipeline.count } + + expect(response).to be_error + expect(response.message).to eq('Cannot create a pipeline for this merge request.') + expect(response.payload).to be_nil + end + end end end diff --git a/spec/services/merge_requests/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb index bb764ff5672..8fc12c6c2b1 100644 --- a/spec/services/merge_requests/merge_to_ref_service_spec.rb +++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb @@ -36,6 +36,37 @@ RSpec.describe MergeRequests::MergeToRefService do expect(repository.ref_exists?(target_ref)).to be(true) expect(ref_head.id).to eq(result[:commit_id]) end + + context 'cache_merge_to_ref_calls flag enabled', :use_clean_rails_memory_store_caching do + before do + stub_feature_flags(cache_merge_to_ref_calls: true) + + # warm the cache + # + service.execute(merge_request) + end + + it 'caches the response', :request_store do + expect { 3.times { service.execute(merge_request) } } + .not_to change(Gitlab::GitalyClient, :get_request_count) + end + end + + context 'cache_merge_to_ref_calls flag disabled', :use_clean_rails_memory_store_caching do + before do + stub_feature_flags(cache_merge_to_ref_calls: false) + + # warm the cache + # + service.execute(merge_request) + end + + it 'does not cache the response', :request_store do + expect(Gitlab::GitalyClient).to receive(:call).at_least(3).times.and_call_original + + 3.times { service.execute(merge_request) } + end + end end shared_examples_for 'successfully evaluates pre-condition checks' do diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 14804aa33d4..8d9a32c3e9e 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -75,7 +75,7 @@ RSpec.describe MergeRequests::PostMergeService do end it 'clean up environments for the merge request' do - expect_next_instance_of(Ci::StopEnvironmentsService) do |stop_environment_service| + expect_next_instance_of(::Environments::StopService) do |stop_environment_service| expect(stop_environment_service).to receive(:execute_for_merge_request).with(merge_request) end diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index 5f76f6f5c44..f00a8928109 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -743,7 +743,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it 'records an error' do service.execute - expect(service.errors).to eq(['Branch my-branch does not exist']) + expect(service.errors).to eq(["Target branch #{project.full_path}:my-branch does not exist"]) end end diff --git a/spec/services/namespaces/in_product_marketing_emails_service_spec.rb b/spec/services/namespaces/in_product_marketing_emails_service_spec.rb index 9d4fcf9ca64..58ba577b7e7 100644 --- a/spec/services/namespaces/in_product_marketing_emails_service_spec.rb +++ b/spec/services/namespaces/in_product_marketing_emails_service_spec.rb @@ -39,19 +39,22 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do using RSpec::Parameterized::TableSyntax where(:track, :interval, :actions_completed) do - :create | 1 | { created_at: frozen_time - 2.days } - :create | 5 | { created_at: frozen_time - 6.days } - :create | 10 | { created_at: frozen_time - 11.days } - :verify | 1 | { created_at: frozen_time - 2.days, git_write_at: frozen_time - 2.days } - :verify | 5 | { created_at: frozen_time - 6.days, git_write_at: frozen_time - 6.days } - :verify | 10 | { created_at: frozen_time - 11.days, git_write_at: frozen_time - 11.days } - :trial | 1 | { created_at: frozen_time - 2.days, git_write_at: frozen_time - 2.days, pipeline_created_at: frozen_time - 2.days } - :trial | 5 | { created_at: frozen_time - 6.days, git_write_at: frozen_time - 6.days, pipeline_created_at: frozen_time - 6.days } - :trial | 10 | { created_at: frozen_time - 11.days, git_write_at: frozen_time - 11.days, pipeline_created_at: frozen_time - 11.days } - :team | 1 | { created_at: frozen_time - 2.days, git_write_at: frozen_time - 2.days, pipeline_created_at: frozen_time - 2.days, trial_started_at: frozen_time - 2.days } - :team | 5 | { created_at: frozen_time - 6.days, git_write_at: frozen_time - 6.days, pipeline_created_at: frozen_time - 6.days, trial_started_at: frozen_time - 6.days } - :team | 10 | { created_at: frozen_time - 11.days, git_write_at: frozen_time - 11.days, pipeline_created_at: frozen_time - 11.days, trial_started_at: frozen_time - 11.days } - :experience | 30 | { created_at: frozen_time - 31.days, git_write_at: frozen_time - 31.days } + :create | 1 | { created_at: frozen_time - 2.days } + :create | 5 | { created_at: frozen_time - 6.days } + :create | 10 | { created_at: frozen_time - 11.days } + :team_short | 1 | { created_at: frozen_time - 2.days, git_write_at: frozen_time - 2.days } + :trial_short | 2 | { created_at: frozen_time - 3.days, git_write_at: frozen_time - 3.days } + :admin_verify | 3 | { created_at: frozen_time - 4.days, git_write_at: frozen_time - 4.days } + :verify | 4 | { created_at: frozen_time - 5.days, git_write_at: frozen_time - 5.days } + :verify | 8 | { created_at: frozen_time - 9.days, git_write_at: frozen_time - 9.days } + :verify | 13 | { created_at: frozen_time - 14.days, git_write_at: frozen_time - 14.days } + :trial | 1 | { created_at: frozen_time - 2.days, git_write_at: frozen_time - 2.days, pipeline_created_at: frozen_time - 2.days } + :trial | 5 | { created_at: frozen_time - 6.days, git_write_at: frozen_time - 6.days, pipeline_created_at: frozen_time - 6.days } + :trial | 10 | { created_at: frozen_time - 11.days, git_write_at: frozen_time - 11.days, pipeline_created_at: frozen_time - 11.days } + :team | 1 | { created_at: frozen_time - 2.days, git_write_at: frozen_time - 2.days, pipeline_created_at: frozen_time - 2.days, trial_started_at: frozen_time - 2.days } + :team | 5 | { created_at: frozen_time - 6.days, git_write_at: frozen_time - 6.days, pipeline_created_at: frozen_time - 6.days, trial_started_at: frozen_time - 6.days } + :team | 10 | { created_at: frozen_time - 11.days, git_write_at: frozen_time - 11.days, pipeline_created_at: frozen_time - 11.days, trial_started_at: frozen_time - 11.days } + :experience | 30 | { created_at: frozen_time - 31.days, git_write_at: frozen_time - 31.days } end with_them do @@ -60,14 +63,14 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do end context 'when initialized with a different track' do - let(:track) { :verify } + let(:track) { :team_short } it { is_expected.not_to send_in_product_marketing_email } context 'when the previous track actions have been completed' do let(:current_action_completed_at) { frozen_time - 2.days } - it { is_expected.to send_in_product_marketing_email(user.id, group.id, :verify, 0) } + it { is_expected.to send_in_product_marketing_email(user.id, group.id, track, 0) } end end @@ -168,7 +171,7 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do subject expect(Notify).to have_received(:in_product_marketing_email).with(user.id, group.id, :create, 0) - expect(Notify).to have_received(:in_product_marketing_email).with(user.id, other_group.id, :verify, 0) + expect(Notify).to have_received(:in_product_marketing_email).with(user.id, other_group.id, :team_short, 0) end end end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 6621ad1f294..793e9ed9848 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -185,6 +185,14 @@ RSpec.describe Notes::CreateService do expect(note.note_diff_file).to be_present expect(note.diff_note_positions).to be_present end + + context 'when skip_capture_diff_note_position execute option is set to true' do + it 'does not execute Discussions::CaptureDiffNotePositionService' do + expect(Discussions::CaptureDiffNotePositionService).not_to receive(:new) + + described_class.new(project_with_repo, user, new_opts).execute(skip_capture_diff_note_position: true) + end + end end context 'when DiffNote is a reply' do diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index ac82e4c025f..3c4d7d50002 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe NotificationService, :mailer do include ExternalAuthorizationServiceHelpers include NotificationHelpers - let_it_be(:project, reload: true) { create(:project, :public) } + let_it_be_with_refind(:project) { create(:project, :public) } let_it_be_with_refind(:assignee) { create(:user) } let(:notification) { described_class.new } @@ -3368,10 +3368,6 @@ RSpec.describe NotificationService, :mailer do project.add_maintainer(u_maintainer2) project.add_developer(u_developer) - # Mock remote update - allow(project.repository).to receive(:async_remove_remote) - allow(project.repository).to receive(:add_remote) - reset_delivered_emails! end diff --git a/spec/services/packages/composer/create_package_service_spec.rb b/spec/services/packages/composer/create_package_service_spec.rb index 553d58fdd86..2ffd0a269f2 100644 --- a/spec/services/packages/composer/create_package_service_spec.rb +++ b/spec/services/packages/composer/create_package_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Packages::Composer::CreatePackageService do let_it_be(:package_name) { 'composer-package-name' } let_it_be(:json) { { name: package_name }.to_json } - let_it_be(:project) { create(:project, :custom_repo, files: { 'composer.json' => json } ) } + let_it_be(:project) { create(:project, :custom_repo, files: { 'composer.json' => json }) } let_it_be(:user) { create(:user) } let(:params) do @@ -24,13 +24,30 @@ RSpec.describe Packages::Composer::CreatePackageService do let(:created_package) { Packages::Package.composer.last } + shared_examples 'using the cache update worker' do + context 'with remove_composer_v1_cache_code enabled' do + it 'does not enqueue a cache update job' do + expect(::Packages::Composer::CacheUpdateWorker).not_to receive(:perform_async) + + subject + end + end + + context 'with remove_composer_v1_cache_code disabled' do + it 'enqueues a cache update job' do + stub_feature_flags(remove_composer_v1_cache_code: true) + expect(::Packages::Composer::CacheUpdateWorker).not_to receive(:perform_async) + + subject + end + end + end + context 'without an existing package' do context 'with a branch' do let(:branch) { project.repository.find_branch('master') } it 'creates the package' do - expect(::Packages::Composer::CacheUpdateWorker).to receive(:perform_async).with(project.id, package_name, nil) - expect { subject } .to change { Packages::Package.composer.count }.by(1) .and change { Packages::Composer::Metadatum.count }.by(1) @@ -47,6 +64,7 @@ RSpec.describe Packages::Composer::CreatePackageService do it_behaves_like 'assigns build to package' it_behaves_like 'assigns status to package' + it_behaves_like 'using the cache update worker' end context 'with a tag' do @@ -57,8 +75,6 @@ RSpec.describe Packages::Composer::CreatePackageService do end it 'creates the package' do - expect(::Packages::Composer::CacheUpdateWorker).to receive(:perform_async).with(project.id, package_name, nil) - expect { subject } .to change { Packages::Package.composer.count }.by(1) .and change { Packages::Composer::Metadatum.count }.by(1) @@ -73,6 +89,7 @@ RSpec.describe Packages::Composer::CreatePackageService do it_behaves_like 'assigns build to package' it_behaves_like 'assigns status to package' + it_behaves_like 'using the cache update worker' end end @@ -85,12 +102,12 @@ RSpec.describe Packages::Composer::CreatePackageService do end it 'does not create a new package' do - expect(::Packages::Composer::CacheUpdateWorker).to receive(:perform_async).with(project.id, package_name, nil) - expect { subject } .to change { Packages::Package.composer.count }.by(0) .and change { Packages::Composer::Metadatum.count }.by(0) end + + it_behaves_like 'using the cache update worker' end context 'belonging to another project' do @@ -108,12 +125,12 @@ RSpec.describe Packages::Composer::CreatePackageService do let!(:other_package) { create(:package, name: package_name, version: 'dev-master', project: other_project) } it 'creates the package' do - expect(::Packages::Composer::CacheUpdateWorker).to receive(:perform_async).with(project.id, package_name, nil) - expect { subject } .to change { Packages::Package.composer.count }.by(1) .and change { Packages::Composer::Metadatum.count }.by(1) end + + it_behaves_like 'using the cache update worker' end end end diff --git a/spec/services/packages/create_dependency_service_spec.rb b/spec/services/packages/create_dependency_service_spec.rb index 3eae9f099f7..261c6b395d5 100644 --- a/spec/services/packages/create_dependency_service_spec.rb +++ b/spec/services/packages/create_dependency_service_spec.rb @@ -58,8 +58,8 @@ RSpec.describe Packages::CreateDependencyService do let_it_be(:rows) { [{ name: 'express', version_pattern: '^4.16.4' }] } it 'creates dependences and links' do - original_bulk_insert = ::Gitlab::Database.method(:bulk_insert) - expect(::Gitlab::Database) + original_bulk_insert = ::Gitlab::Database.main.method(:bulk_insert) + expect(::Gitlab::Database.main) .to receive(:bulk_insert) do |table, rows, return_ids: false, disable_quote: [], on_conflict: nil| call_count = table == Packages::Dependency.table_name ? 2 : 1 call_count.times { original_bulk_insert.call(table, rows, return_ids: return_ids, disable_quote: disable_quote, on_conflict: on_conflict) } diff --git a/spec/services/packages/debian/generate_distribution_key_service_spec.rb b/spec/services/packages/debian/generate_distribution_key_service_spec.rb index b31830c2d3b..f82d577f071 100644 --- a/spec/services/packages/debian/generate_distribution_key_service_spec.rb +++ b/spec/services/packages/debian/generate_distribution_key_service_spec.rb @@ -3,33 +3,21 @@ require 'spec_helper' RSpec.describe Packages::Debian::GenerateDistributionKeyService do - let_it_be(:user) { create(:user) } - let(:params) { {} } - subject { described_class.new(current_user: user, params: params) } + subject { described_class.new(params: params) } let(:response) { subject.execute } - context 'with a user' do - it 'returns an Hash', :aggregate_failures do - expect(GPGME::Ctx).to receive(:new).with(armor: true, offline: true).and_call_original - expect(User).to receive(:random_password).with(no_args).and_call_original - - expect(response).to be_a Hash - expect(response.keys).to contain_exactly(:private_key, :public_key, :fingerprint, :passphrase) - expect(response[:private_key]).to start_with('-----BEGIN PGP PRIVATE KEY BLOCK-----') - expect(response[:public_key]).to start_with('-----BEGIN PGP PUBLIC KEY BLOCK-----') - expect(response[:fingerprint].length).to eq(40) - expect(response[:passphrase].length).to be > 10 - end - end - - context 'without a user' do - let(:user) { nil } + it 'returns an Hash', :aggregate_failures do + expect(GPGME::Ctx).to receive(:new).with(armor: true, offline: true).and_call_original + expect(User).to receive(:random_password).with(no_args).and_call_original - it 'raises an ArgumentError' do - expect { response }.to raise_error(ArgumentError, 'Please provide a user') - end + expect(response).to be_a Hash + expect(response.keys).to contain_exactly(:private_key, :public_key, :fingerprint, :passphrase) + expect(response[:private_key]).to start_with('-----BEGIN PGP PRIVATE KEY BLOCK-----') + expect(response[:public_key]).to start_with('-----BEGIN PGP PUBLIC KEY BLOCK-----') + expect(response[:fingerprint].length).to eq(40) + expect(response[:passphrase].length).to be > 10 end end diff --git a/spec/services/packages/debian/generate_distribution_service_spec.rb b/spec/services/packages/debian/generate_distribution_service_spec.rb index a162e492e7e..53805d03655 100644 --- a/spec/services/packages/debian/generate_distribution_service_spec.rb +++ b/spec/services/packages/debian/generate_distribution_service_spec.rb @@ -6,19 +6,16 @@ RSpec.describe Packages::Debian::GenerateDistributionService do describe '#execute' do subject { described_class.new(distribution).execute } + let(:subject2) { described_class.new(distribution).execute } + let(:subject3) { described_class.new(distribution).execute } + include_context 'with published Debian package' [:project, :group].each do |container_type| context "for #{container_type}" do include_context 'with Debian distribution', container_type - context 'with Debian components and architectures' do - it_behaves_like 'Generate Debian Distribution and component files' - end - - context 'without components and architectures' do - it_behaves_like 'Generate minimal Debian Distribution' - end + it_behaves_like 'Generate Debian Distribution and component files' end end end diff --git a/spec/services/packages/debian/sign_distribution_service_spec.rb b/spec/services/packages/debian/sign_distribution_service_spec.rb new file mode 100644 index 00000000000..2aec0e50636 --- /dev/null +++ b/spec/services/packages/debian/sign_distribution_service_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Debian::SignDistributionService do + let_it_be(:group) { create(:group, :public) } + + let(:content) { FFaker::Lorem.paragraph } + let(:service) { described_class.new(distribution, content, detach: detach) } + + shared_examples 'Sign Distribution' do |container_type, detach: false| + context "for #{container_type} detach=#{detach}" do + let(:detach) { detach } + + if container_type == :group + let_it_be(:distribution) { create('debian_group_distribution', container: group) } + else + let_it_be(:project) { create(:project, group: group) } + let_it_be(:distribution) { create('debian_project_distribution', container: project) } + end + + describe '#execute' do + subject { service.execute } + + context 'without an existing key' do + it 'raises ArgumentError', :aggregate_failures do + expect { subject } + .to raise_error(ArgumentError, 'distribution key is missing') + end + end + + context 'with an existing key' do + let!(:key) { create("debian_#{container_type}_distribution_key", distribution: distribution)} + + it 'returns the content signed', :aggregate_failures do + expect(Packages::Debian::GenerateDistributionKeyService).not_to receive(:new) + + key_class = "Packages::Debian::#{container_type.capitalize}DistributionKey".constantize + + expect { subject } + .to not_change { key_class.count } + + if detach + expect(subject).to start_with("-----BEGIN PGP SIGNATURE-----\n") + else + expect(subject).to start_with("-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA256\n\n#{content}\n-----BEGIN PGP SIGNATURE-----\n") + end + + expect(subject).to end_with("\n-----END PGP SIGNATURE-----\n") + end + end + end + end + end + + it_behaves_like 'Sign Distribution', :project + it_behaves_like 'Sign Distribution', :project, detach: true + it_behaves_like 'Sign Distribution', :group + it_behaves_like 'Sign Distribution', :group, detach: true +end diff --git a/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb b/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb index 328484c3e5a..66ff6a8d03f 100644 --- a/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb +++ b/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb @@ -49,7 +49,7 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ allow(service).to receive(:metadata).and_return(metadata) end - it 'does not update the package' do + it 'does not update the package', :aggregate_failures do expect(service).to receive(:try_obtain_lease).and_call_original expect { subject } @@ -63,213 +63,234 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ end end - context 'with no existing package' do - let(:package_id) { package.id } - - it 'updates package and package file' do - expect { subject } - .to change { ::Packages::Package.count }.by(1) - .and change { Packages::Dependency.count }.by(1) - .and change { Packages::DependencyLink.count }.by(1) - .and change { ::Packages::Nuget::Metadatum.count }.by(0) - - expect(package.reload.name).to eq(package_name) - expect(package.version).to eq(package_version) - expect(package).to be_default - expect(package_file.reload.file_name).to eq(package_file_name) - # hard reset needed to properly reload package_file.file - expect(Packages::PackageFile.find(package_file.id).file.size).not_to eq 0 - end - - it_behaves_like 'taking the lease' + shared_examples 'handling all conditions' do + context 'with no existing package' do + let(:package_id) { package.id } - it_behaves_like 'not updating the package if the lease is taken' - end + it 'updates package and package file', :aggregate_failures do + expect { subject } + .to not_change { ::Packages::Package.count } + .and change { Packages::Dependency.count }.by(1) + .and change { Packages::DependencyLink.count }.by(1) + .and change { ::Packages::Nuget::Metadatum.count }.by(0) - context 'with existing package' do - let!(:existing_package) { create(:nuget_package, project: package.project, name: package_name, version: package_version) } - let(:package_id) { existing_package.id } + expect(package.reload.name).to eq(package_name) + expect(package.version).to eq(package_version) + expect(package).to be_default + expect(package_file.reload.file_name).to eq(package_file_name) + # hard reset needed to properly reload package_file.file + expect(Packages::PackageFile.find(package_file.id).file.size).not_to eq 0 + end - it 'link existing package and updates package file' do - expect(service).to receive(:try_obtain_lease).and_call_original + it_behaves_like 'taking the lease' - expect { subject } - .to change { ::Packages::Package.count }.by(-1) - .and change { Packages::Dependency.count }.by(0) - .and change { Packages::DependencyLink.count }.by(0) - .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(0) - .and change { ::Packages::Nuget::Metadatum.count }.by(0) - expect(package_file.reload.file_name).to eq(package_file_name) - expect(package_file.package).to eq(existing_package) + it_behaves_like 'not updating the package if the lease is taken' end - it_behaves_like 'taking the lease' - - it_behaves_like 'not updating the package if the lease is taken' - end + context 'with existing package' do + let!(:existing_package) { create(:nuget_package, project: package.project, name: package_name, version: package_version) } + let(:package_id) { existing_package.id } - context 'with a nuspec file with metadata' do - let(:nuspec_filepath) { 'packages/nuget/with_metadata.nuspec' } - let(:expected_tags) { %w(foo bar test tag1 tag2 tag3 tag4 tag5) } + it 'link existing package and updates package file', :aggregate_failures do + expect(service).to receive(:try_obtain_lease).and_call_original - before do - allow_next_instance_of(Packages::Nuget::MetadataExtractionService) do |service| - allow(service) - .to receive(:nuspec_file_content).and_return(fixture_file(nuspec_filepath)) + expect { subject } + .to change { ::Packages::Package.count }.by(-1) + .and change { Packages::Dependency.count }.by(0) + .and change { Packages::DependencyLink.count }.by(0) + .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(0) + .and change { ::Packages::Nuget::Metadatum.count }.by(0) + expect(package_file.reload.file_name).to eq(package_file_name) + expect(package_file.package).to eq(existing_package) end - end - it 'creates tags' do - expect(service).to receive(:try_obtain_lease).and_call_original - expect { subject }.to change { ::Packages::Tag.count }.by(8) - expect(package.reload.tags.map(&:name)).to contain_exactly(*expected_tags) + it_behaves_like 'taking the lease' + + it_behaves_like 'not updating the package if the lease is taken' end - context 'with existing package and tags' do - let!(:existing_package) { create(:nuget_package, project: package.project, name: 'DummyProject.WithMetadata', version: '1.2.3') } - let!(:tag1) { create(:packages_tag, package: existing_package, name: 'tag1') } - let!(:tag2) { create(:packages_tag, package: existing_package, name: 'tag2') } - let!(:tag3) { create(:packages_tag, package: existing_package, name: 'tag_not_in_metadata') } + context 'with a nuspec file with metadata' do + let(:nuspec_filepath) { 'packages/nuget/with_metadata.nuspec' } + let(:expected_tags) { %w(foo bar test tag1 tag2 tag3 tag4 tag5) } + + before do + allow_next_instance_of(Packages::Nuget::MetadataExtractionService) do |service| + allow(service) + .to receive(:nuspec_file_content).and_return(fixture_file(nuspec_filepath)) + end + end - it 'creates tags and deletes those not in metadata' do + it 'creates tags' do expect(service).to receive(:try_obtain_lease).and_call_original - expect { subject }.to change { ::Packages::Tag.count }.by(5) - expect(existing_package.tags.map(&:name)).to contain_exactly(*expected_tags) + expect { subject }.to change { ::Packages::Tag.count }.by(8) + expect(package.reload.tags.map(&:name)).to contain_exactly(*expected_tags) end - end - it 'creates nuget metadatum' do - expect { subject } - .to change { ::Packages::Package.count }.by(1) - .and change { ::Packages::Nuget::Metadatum.count }.by(1) + context 'with existing package and tags' do + let!(:existing_package) { create(:nuget_package, project: package.project, name: 'DummyProject.WithMetadata', version: '1.2.3') } + let!(:tag1) { create(:packages_tag, package: existing_package, name: 'tag1') } + let!(:tag2) { create(:packages_tag, package: existing_package, name: 'tag2') } + let!(:tag3) { create(:packages_tag, package: existing_package, name: 'tag_not_in_metadata') } + + it 'creates tags and deletes those not in metadata' do + expect(service).to receive(:try_obtain_lease).and_call_original + expect { subject }.to change { ::Packages::Tag.count }.by(5) + expect(existing_package.tags.map(&:name)).to contain_exactly(*expected_tags) + end + end - metadatum = package_file.reload.package.nuget_metadatum - expect(metadatum.license_url).to eq('https://opensource.org/licenses/MIT') - expect(metadatum.project_url).to eq('https://gitlab.com/gitlab-org/gitlab') - expect(metadatum.icon_url).to eq('https://opensource.org/files/osi_keyhole_300X300_90ppi_0.png') - end + it 'creates nuget metadatum', :aggregate_failures do + expect { subject } + .to not_change { ::Packages::Package.count } + .and change { ::Packages::Nuget::Metadatum.count }.by(1) - context 'with too long url' do - let_it_be(:too_long_url) { "http://localhost/#{'bananas' * 50}" } + metadatum = package_file.reload.package.nuget_metadatum + expect(metadatum.license_url).to eq('https://opensource.org/licenses/MIT') + expect(metadatum.project_url).to eq('https://gitlab.com/gitlab-org/gitlab') + expect(metadatum.icon_url).to eq('https://opensource.org/files/osi_keyhole_300X300_90ppi_0.png') + end - let(:metadata) { { package_name: package_name, package_version: package_version, license_url: too_long_url } } + context 'with too long url' do + let_it_be(:too_long_url) { "http://localhost/#{'bananas' * 50}" } - before do - allow(service).to receive(:metadata).and_return(metadata) - end + let(:metadata) { { package_name: package_name, package_version: package_version, license_url: too_long_url } } + + before do + allow(service).to receive(:metadata).and_return(metadata) + end - it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError + it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError + end end - end - context 'with nuspec file with dependencies' do - let(:nuspec_filepath) { 'packages/nuget/with_dependencies.nuspec' } - let(:package_name) { 'Test.Package' } - let(:package_version) { '3.5.2' } - let(:package_file_name) { 'test.package.3.5.2.nupkg' } + context 'with nuspec file with dependencies' do + let(:nuspec_filepath) { 'packages/nuget/with_dependencies.nuspec' } + let(:package_name) { 'Test.Package' } + let(:package_version) { '3.5.2' } + let(:package_file_name) { 'test.package.3.5.2.nupkg' } - before do - allow_next_instance_of(Packages::Nuget::MetadataExtractionService) do |service| - allow(service) - .to receive(:nuspec_file_content).and_return(fixture_file(nuspec_filepath)) + before do + allow_next_instance_of(Packages::Nuget::MetadataExtractionService) do |service| + allow(service) + .to receive(:nuspec_file_content).and_return(fixture_file(nuspec_filepath)) + end end - end - it 'updates package and package file' do - expect { subject } - .to change { ::Packages::Package.count }.by(1) - .and change { Packages::Dependency.count }.by(4) - .and change { Packages::DependencyLink.count }.by(4) - .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(2) - - expect(package.reload.name).to eq(package_name) - expect(package.version).to eq(package_version) - expect(package).to be_default - expect(package_file.reload.file_name).to eq(package_file_name) - # hard reset needed to properly reload package_file.file - expect(Packages::PackageFile.find(package_file.id).file.size).not_to eq 0 + it 'updates package and package file', :aggregate_failures do + expect { subject } + .to not_change { ::Packages::Package.count } + .and change { Packages::Dependency.count }.by(4) + .and change { Packages::DependencyLink.count }.by(4) + .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(2) + + expect(package.reload.name).to eq(package_name) + expect(package.version).to eq(package_version) + expect(package).to be_default + expect(package_file.reload.file_name).to eq(package_file_name) + # hard reset needed to properly reload package_file.file + expect(Packages::PackageFile.find(package_file.id).file.size).not_to eq 0 + end end - end - context 'with package file not containing a nuspec file' do - before do - allow_next_instance_of(Zip::File) do |file| - allow(file).to receive(:glob).and_return([]) + context 'with package file not containing a nuspec file' do + before do + allow_next_instance_of(Zip::File) do |file| + allow(file).to receive(:glob).and_return([]) + end end + + it_behaves_like 'raising an', ::Packages::Nuget::MetadataExtractionService::ExtractionError end - it_behaves_like 'raising an', ::Packages::Nuget::MetadataExtractionService::ExtractionError - end + context 'with a symbol package' do + let(:package_file) { package.package_files.last } + let(:package_file_name) { 'dummyproject.dummypackage.1.0.0.snupkg' } - context 'with a symbol package' do - let(:package_file) { package.package_files.last } - let(:package_file_name) { 'dummyproject.dummypackage.1.0.0.snupkg' } + context 'with no existing package' do + let(:package_id) { package.id } - context 'with no existing package' do - let(:package_id) { package.id } + it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError + end - it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError - end + context 'with existing package' do + let!(:existing_package) { create(:nuget_package, project: package.project, name: package_name, version: package_version) } + let(:package_id) { existing_package.id } - context 'with existing package' do - let!(:existing_package) { create(:nuget_package, project: package.project, name: package_name, version: package_version) } - let(:package_id) { existing_package.id } + it 'link existing package and updates package file', :aggregate_failures do + expect(service).to receive(:try_obtain_lease).and_call_original + expect(::Packages::Nuget::SyncMetadatumService).not_to receive(:new) + expect(::Packages::UpdateTagsService).not_to receive(:new) - it 'link existing package and updates package file', :aggregate_failures do - expect(service).to receive(:try_obtain_lease).and_call_original - expect(::Packages::Nuget::SyncMetadatumService).not_to receive(:new) - expect(::Packages::UpdateTagsService).not_to receive(:new) - - expect { subject } - .to change { ::Packages::Package.count }.by(-1) - .and change { Packages::Dependency.count }.by(0) - .and change { Packages::DependencyLink.count }.by(0) - .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(0) - .and change { ::Packages::Nuget::Metadatum.count }.by(0) - expect(package_file.reload.file_name).to eq(package_file_name) - expect(package_file.package).to eq(existing_package) - end + expect { subject } + .to change { ::Packages::Package.count }.by(-1) + .and change { Packages::Dependency.count }.by(0) + .and change { Packages::DependencyLink.count }.by(0) + .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(0) + .and change { ::Packages::Nuget::Metadatum.count }.by(0) + expect(package_file.reload.file_name).to eq(package_file_name) + expect(package_file.package).to eq(existing_package) + end - it_behaves_like 'taking the lease' + it_behaves_like 'taking the lease' - it_behaves_like 'not updating the package if the lease is taken' + it_behaves_like 'not updating the package if the lease is taken' + end end - end - context 'with an invalid package name' do - invalid_names = [ - '', - 'My/package', - '../../../my_package', - '%2e%2e%2fmy_package' - ] + context 'with an invalid package name' do + invalid_names = [ + '', + 'My/package', + '../../../my_package', + '%2e%2e%2fmy_package' + ] - invalid_names.each do |invalid_name| - before do - allow(service).to receive(:package_name).and_return(invalid_name) + invalid_names.each do |invalid_name| + before do + allow(service).to receive(:package_name).and_return(invalid_name) + end + + it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError end + end - it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError + context 'with an invalid package version' do + invalid_versions = [ + '', + '555', + '1.2', + '1./2.3', + '../../../../../1.2.3', + '%2e%2e%2f1.2.3' + ] + + invalid_versions.each do |invalid_version| + before do + allow(service).to receive(:package_version).and_return(invalid_version) + end + + it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError + end end end - context 'with an invalid package version' do - invalid_versions = [ - '', - '555', - '1.2', - '1./2.3', - '../../../../../1.2.3', - '%2e%2e%2f1.2.3' - ] - - invalid_versions.each do |invalid_version| - before do - allow(service).to receive(:package_version).and_return(invalid_version) - end + context 'with packages_nuget_new_package_file_updater enabled' do + before do + expect(service).not_to receive(:legacy_execute) + end + + it_behaves_like 'handling all conditions' + end - it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError + context 'with packages_nuget_new_package_file_updater disabled' do + before do + stub_feature_flags(packages_nuget_new_package_file_updater: false) + expect(::Packages::UpdatePackageFileService) + .not_to receive(:new).with(package_file, instance_of(Hash)).and_call_original + expect(service).not_to receive(:new_execute) end + + it_behaves_like 'handling all conditions' end end end diff --git a/spec/services/packages/update_package_file_service_spec.rb b/spec/services/packages/update_package_file_service_spec.rb new file mode 100644 index 00000000000..d988049c43a --- /dev/null +++ b/spec/services/packages/update_package_file_service_spec.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::UpdatePackageFileService do + let_it_be(:another_package) { create(:package) } + let_it_be(:old_file_name) { 'old_file_name.txt' } + let_it_be(:new_file_name) { 'new_file_name.txt' } + + let(:package) { package_file.package } + let(:params) { { package_id: another_package.id, file_name: new_file_name } } + let(:service) { described_class.new(package_file, params) } + + describe '#execute' do + subject { service.execute } + + shared_examples 'updating package file with valid parameters' do + context 'with both parameters set' do + it 'updates the package file accordingly' do + expect { subject } + .to change { package.package_files.count }.from(1).to(0) + .and change { another_package.package_files.count }.from(0).to(1) + .and change { package_file.package_id }.from(package.id).to(another_package.id) + .and change { package_file.file_name }.from(old_file_name).to(new_file_name) + end + end + + context 'with only file_name set' do + let(:params) { { file_name: new_file_name } } + + it 'updates the package file accordingly' do + expect { subject } + .to not_change { package.package_files.count } + .and not_change { another_package.package_files.count } + .and not_change { package_file.package_id } + .and change { package_file.file_name }.from(old_file_name).to(new_file_name) + end + end + + context 'with only package_id set' do + let(:params) { { package_id: another_package.id } } + + it 'updates the package file accordingly' do + expect { subject } + .to change { package.package_files.count }.from(1).to(0) + .and change { another_package.package_files.count }.from(0).to(1) + .and change { package_file.package_id }.from(package.id).to(another_package.id) + .and not_change { package_file.file_name } + end + end + end + + shared_examples 'not updating package with invalid parameters' do + context 'with blank parameters' do + let(:params) { {} } + + it 'raise an argument error' do + expect { subject }.to raise_error(ArgumentError, 'package_id and file_name are blank') + end + end + + context 'with non persisted package file' do + let(:package_file) { build(:package_file) } + + it 'raise an argument error' do + expect { subject }.to raise_error(ArgumentError, 'package_file not persisted') + end + end + end + + context 'with object storage disabled' do + let(:package_file) { create(:package_file, file_name: old_file_name) } + + before do + stub_package_file_object_storage(enabled: false) + end + + it_behaves_like 'updating package file with valid parameters' do + before do + expect(package_file).to receive(:remove_previously_stored_file).and_call_original + expect(package_file).not_to receive(:move_in_object_storage) + end + end + + it_behaves_like 'not updating package with invalid parameters' + end + + context 'with object storage enabled' do + let(:package_file) do + create( + :package_file, + file_name: old_file_name, + file: CarrierWaveStringFile.new_file( + file_content: 'content', + filename: old_file_name, + content_type: 'text/plain' + ), + file_store: ::Packages::PackageFileUploader::Store::REMOTE + ) + end + + before do + stub_package_file_object_storage(enabled: true) + end + + it_behaves_like 'updating package file with valid parameters' do + before do + expect(package_file).not_to receive(:remove_previously_stored_file) + expect(package_file).to receive(:move_in_object_storage).and_call_original + end + end + + it_behaves_like 'not updating package with invalid parameters' do + before do + expect(package_file.file.file).not_to receive(:copy_to) + end + end + end + end +end diff --git a/spec/services/packages/update_tags_service_spec.rb b/spec/services/packages/update_tags_service_spec.rb index 4a122d1c718..6e67489fec9 100644 --- a/spec/services/packages/update_tags_service_spec.rb +++ b/spec/services/packages/update_tags_service_spec.rb @@ -50,7 +50,7 @@ RSpec.describe Packages::UpdateTagsService do it 'is a no op' do expect(package).not_to receive(:tags) - expect(::Gitlab::Database).not_to receive(:bulk_insert) + expect(::Gitlab::Database.main).not_to receive(:bulk_insert) subject end diff --git a/spec/services/post_receive_service_spec.rb b/spec/services/post_receive_service_spec.rb index 871ed95bf28..3f4d37e5ddc 100644 --- a/spec/services/post_receive_service_spec.rb +++ b/spec/services/post_receive_service_spec.rb @@ -131,6 +131,12 @@ RSpec.describe PostReceiveService do project.add_developer(user) end + it 'invalidates the branch name cache' do + expect(service.repository).to receive(:expire_branches_cache).and_call_original + + subject + end + it 'invokes MergeRequests::PushOptionsHandlerService' do expect(MergeRequests::PushOptionsHandlerService).to receive(:new).and_call_original diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index defeadb479a..c3928563125 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -135,7 +135,7 @@ RSpec.describe Projects::CreateService, '#execute' do end it_behaves_like 'storing arguments in the application context' do - let(:expected_params) { { project: subject.full_path, related_class: described_class.to_s } } + let(:expected_params) { { project: subject.full_path } } subject { create_project(user, opts) } end @@ -335,7 +335,7 @@ RSpec.describe Projects::CreateService, '#execute' do it 'does not write repository config' do expect_next_instance_of(Project) do |project| - expect(project).not_to receive(:write_repository_config) + expect(project).not_to receive(:set_full_path) end imported_project @@ -607,65 +607,55 @@ RSpec.describe Projects::CreateService, '#execute' do describe 'create integration for the project' do subject(:project) { create_project(user, opts) } - context 'with an active integration template' do - let!(:template_integration) { create(:prometheus_integration, :template, api_url: 'https://prometheus.template.com/') } + context 'with an active instance-level integration' do + let!(:instance_integration) { create(:prometheus_integration, :instance, api_url: 'https://prometheus.instance.com/') } - it 'creates an integration from the template' do + it 'creates an integration from the instance-level integration' do expect(project.integrations.count).to eq(1) - expect(project.integrations.first.api_url).to eq(template_integration.api_url) - expect(project.integrations.first.inherit_from_id).to be_nil + expect(project.integrations.first.api_url).to eq(instance_integration.api_url) + expect(project.integrations.first.inherit_from_id).to eq(instance_integration.id) end - context 'with an active instance-level integration' do - let!(:instance_integration) { create(:prometheus_integration, :instance, api_url: 'https://prometheus.instance.com/') } + context 'with an active group-level integration' do + let!(:group_integration) { create(:prometheus_integration, group: group, project: nil, api_url: 'https://prometheus.group.com/') } + let!(:group) do + create(:group).tap do |group| + group.add_owner(user) + end + end - it 'creates an integration from the instance-level integration' do + let(:opts) do + { + name: 'GitLab', + namespace_id: group.id + } + end + + it 'creates an integration from the group-level integration' do expect(project.integrations.count).to eq(1) - expect(project.integrations.first.api_url).to eq(instance_integration.api_url) - expect(project.integrations.first.inherit_from_id).to eq(instance_integration.id) + expect(project.integrations.first.api_url).to eq(group_integration.api_url) + expect(project.integrations.first.inherit_from_id).to eq(group_integration.id) end - context 'with an active group-level integration' do - let!(:group_integration) { create(:prometheus_integration, group: group, project: nil, api_url: 'https://prometheus.group.com/') } - let!(:group) do - create(:group).tap do |group| - group.add_owner(user) + context 'with an active subgroup' do + let!(:subgroup_integration) { create(:prometheus_integration, group: subgroup, project: nil, api_url: 'https://prometheus.subgroup.com/') } + let!(:subgroup) do + create(:group, parent: group).tap do |subgroup| + subgroup.add_owner(user) end end let(:opts) do { name: 'GitLab', - namespace_id: group.id + namespace_id: subgroup.id } end - it 'creates an integration from the group-level integration' do + it 'creates an integration from the subgroup-level integration' do expect(project.integrations.count).to eq(1) - expect(project.integrations.first.api_url).to eq(group_integration.api_url) - expect(project.integrations.first.inherit_from_id).to eq(group_integration.id) - end - - context 'with an active subgroup' do - let!(:subgroup_integration) { create(:prometheus_integration, group: subgroup, project: nil, api_url: 'https://prometheus.subgroup.com/') } - let!(:subgroup) do - create(:group, parent: group).tap do |subgroup| - subgroup.add_owner(user) - end - end - - let(:opts) do - { - name: 'GitLab', - namespace_id: subgroup.id - } - end - - it 'creates an integration from the subgroup-level integration' do - expect(project.integrations.count).to eq(1) - expect(project.integrations.first.api_url).to eq(subgroup_integration.api_url) - expect(project.integrations.first.inherit_from_id).to eq(subgroup_integration.id) - end + expect(project.integrations.first.api_url).to eq(subgroup_integration.api_url) + expect(project.integrations.first.inherit_from_id).to eq(subgroup_integration.id) end end end diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb index 47252bcf7a7..d0064873972 100644 --- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -96,13 +96,13 @@ RSpec.describe Projects::HashedStorage::MigrateRepositoryService do end it 'handles Gitlab::Git::CommandError' do - expect(project).to receive(:write_repository_config).and_raise(Gitlab::Git::CommandError) + expect(project).to receive(:set_full_path).and_raise(Gitlab::Git::CommandError) expect { service.execute }.not_to raise_exception end it 'ensures rollback when Gitlab::Git::CommandError' do - expect(project).to receive(:write_repository_config).and_raise(Gitlab::Git::CommandError) + expect(project).to receive(:set_full_path).and_raise(Gitlab::Git::CommandError) expect(service).to receive(:rollback_folder_move).and_call_original service.execute diff --git a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb index af128a532b9..23e776b72bc 100644 --- a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb @@ -96,13 +96,13 @@ RSpec.describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab end it 'handles Gitlab::Git::CommandError' do - expect(project).to receive(:write_repository_config).and_raise(Gitlab::Git::CommandError) + expect(project).to receive(:set_full_path).and_raise(Gitlab::Git::CommandError) expect { service.execute }.not_to raise_exception end it 'ensures rollback when Gitlab::Git::CommandError' do - expect(project).to receive(:write_repository_config).and_raise(Gitlab::Git::CommandError) + expect(project).to receive(:set_full_path).and_raise(Gitlab::Git::CommandError) expect(service).to receive(:rollback_folder_move).and_call_original service.execute diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index f27ebb2e19e..f9ff959fa05 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -90,6 +90,21 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content end + + it 'streams the download' do + expected_options = { headers: anything, stream_body: true } + + expect(Gitlab::HTTP).to receive(:perform_request).with(Net::HTTP::Get, anything, expected_options) + + subject.execute + end + + it 'skips read_total_timeout', :aggregate_failures do + stub_const('GitLab::HTTP::DEFAULT_READ_TOTAL_TIMEOUT', 0) + + expect(Gitlab::Metrics::System).not_to receive(:monotonic_time) + expect(subject.execute).to include(status: :success) + end end context 'when file download fails' do diff --git a/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb index 0799a33f856..981d7027a17 100644 --- a/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb @@ -34,10 +34,24 @@ RSpec.describe Projects::LfsPointers::LfsObjectDownloadListService do subject.execute end - it 'retrieves the download links of non existent objects' do - expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(all_oids) + context 'when no LFS objects exist' do + before do + project.lfs_objects.delete_all + end - subject.execute + it 'retrieves all LFS objects' do + expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(all_oids) + + subject.execute + end + end + + context 'when some LFS objects already exist' do + it 'retrieves the download links of non-existent objects' do + expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(oids) + + subject.execute + end end end diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index f91f879b772..1d9d5f6e938 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -262,6 +262,31 @@ RSpec.describe Projects::Operations::UpdateService do expect(project.error_tracking_setting.previous_changes.keys) .to contain_exactly('enabled') end + + context 'with integrated attribute' do + let(:params) do + { + error_tracking_setting_attributes: { + enabled: true, + integrated: true + } + } + end + + it 'updates integrated attribute' do + expect { result } + .to change { project.reload.error_tracking_setting.integrated } + .from(false) + .to(true) + end + + it 'only updates enabled and integrated attributes' do + result + + expect(project.error_tracking_setting.previous_changes.keys) + .to contain_exactly('enabled', 'integrated') + end + end end context 'without setting' do diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 5898504b463..0f21736eda0 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -4,6 +4,8 @@ require "spec_helper" RSpec.describe Projects::UpdatePagesService do let_it_be(:project, refind: true) { create(:project, :repository) } + + let_it_be(:old_pipeline) { create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) } let_it_be(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) } let(:build) { create(:ci_build, pipeline: pipeline, ref: 'HEAD') } @@ -94,6 +96,7 @@ RSpec.describe Projects::UpdatePagesService do expect(deployment.file_count).to eq(3) expect(deployment.file_sha256).to eq(artifacts_archive.file_sha256) expect(project.pages_metadatum.reload.pages_deployment_id).to eq(deployment.id) + expect(deployment.ci_build_id).to eq(build.id) end it 'fails if another deployment is in progress' do @@ -106,19 +109,6 @@ RSpec.describe Projects::UpdatePagesService do end end - it 'fails if sha on branch was updated before deployment was uploaded' do - expect(subject).to receive(:create_pages_deployment).and_wrap_original do |m, *args| - build.update!(ref: 'feature') - m.call(*args) - end - - expect(execute).not_to eq(:success) - expect(project.pages_metadatum).not_to be_deployed - - expect(deploy_status).to be_failed - expect(deploy_status.description).to eq('build SHA is outdated for this ref') - end - it 'does not fail if pages_metadata is absent' do project.pages_metadatum.destroy! project.reload @@ -158,6 +148,14 @@ RSpec.describe Projects::UpdatePagesService do expect(execute).not_to eq(:success) end + it 'limits pages file count' do + create(:plan_limits, :default_plan, pages_file_entries: 2) + + expect(execute).not_to eq(:success) + + expect(GenericCommitStatus.last.description).to eq("pages site contains 3 file entries, while limit is set to 2") + end + it 'removes pages after destroy' do expect(PagesWorker).to receive(:perform_in) expect(project.pages_deployed?).to be_falsey @@ -175,16 +173,6 @@ RSpec.describe Projects::UpdatePagesService do expect(ProjectPagesMetadatum.find_by_project_id(project)).to be_nil end - it 'fails if sha on branch is not latest' do - build.update!(ref: 'feature') - - expect(execute).not_to eq(:success) - expect(project.pages_metadatum).not_to be_deployed - - expect(deploy_status).to be_failed - expect(deploy_status.description).to eq('build SHA is outdated for this ref') - end - context 'when using empty file' do let(:file) { empty_file } @@ -259,6 +247,75 @@ RSpec.describe Projects::UpdatePagesService do expect(execute).to eq(:success) end end + + context "when sha on branch was updated before deployment was uploaded" do + before do + expect(subject).to receive(:create_pages_deployment).and_wrap_original do |m, *args| + build.update!(ref: 'feature') + m.call(*args) + end + end + + shared_examples 'fails with outdated reference message' do + it 'fails' do + expect(execute).not_to eq(:success) + expect(project.reload.pages_metadatum).not_to be_deployed + + expect(deploy_status).to be_failed + expect(deploy_status.description).to eq('build SHA is outdated for this ref') + end + end + + shared_examples 'successfully deploys' do + it 'succeeds' do + expect do + expect(execute).to eq(:success) + end.to change { project.pages_deployments.count }.by(1) + + deployment = project.pages_deployments.last + expect(deployment.ci_build_id).to eq(build.id) + end + end + + include_examples 'successfully deploys' + + context 'when pages_smart_check_outdated_sha feature flag is disabled' do + before do + stub_feature_flags(pages_smart_check_outdated_sha: false) + end + + include_examples 'fails with outdated reference message' + end + + context 'when old deployment present' do + before do + old_build = create(:ci_build, pipeline: old_pipeline, ref: 'HEAD') + old_deployment = create(:pages_deployment, ci_build: old_build, project: project) + project.update_pages_deployment!(old_deployment) + end + + include_examples 'successfully deploys' + + context 'when pages_smart_check_outdated_sha feature flag is disabled' do + before do + stub_feature_flags(pages_smart_check_outdated_sha: false) + end + + include_examples 'fails with outdated reference message' + end + end + + context 'when newer deployment present' do + before do + new_pipeline = create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) + new_build = create(:ci_build, pipeline: new_pipeline, ref: 'HEAD') + new_deployment = create(:pages_deployment, ci_build: new_build, project: project) + project.update_pages_deployment!(new_deployment) + end + + include_examples 'fails with outdated reference message' + end + end end end @@ -339,9 +396,15 @@ RSpec.describe Projects::UpdatePagesService do create(:ci_job_artifact, :archive, file: file, job: build) create(:ci_job_artifact, :metadata, file: metafile, job: build) - allow(build).to receive(:artifacts_metadata_entry) + allow(build).to receive(:artifacts_metadata_entry).with('public/', recursive: true) .and_return(metadata) allow(metadata).to receive(:total_size).and_return(100) + + # to pass entries count check + root_metadata = double('root metadata') + allow(build).to receive(:artifacts_metadata_entry).with('', recursive: true) + .and_return(root_metadata) + allow(root_metadata).to receive_message_chain(:entries, :count).and_return(10) end it 'raises an error' do diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index feb70ddaa46..f4a6d1b19e7 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -7,44 +7,21 @@ RSpec.describe Projects::UpdateRemoteMirrorService do let_it_be(:remote_project) { create(:forked_project_with_submodules) } let_it_be(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) } - let(:remote_name) { remote_mirror.remote_name } - subject(:service) { described_class.new(project, project.creator) } describe '#execute' do let(:retries) { 0 } - let(:inmemory) { true } subject(:execute!) { service.execute(remote_mirror, retries) } before do - stub_feature_flags(update_remote_mirror_inmemory: inmemory) project.repository.add_branch(project.owner, 'existing-branch', 'master') allow(remote_mirror) .to receive(:update_repository) - .with(inmemory_remote: inmemory) .and_return(double(divergent_refs: [])) end - context 'with in-memory remote disabled' do - let(:inmemory) { false } - - it 'ensures the remote exists' do - expect(remote_mirror).to receive(:ensure_remote!) - - execute! - end - end - - context 'with in-memory remote enabled' do - it 'does not ensure the remote exists' do - expect(remote_mirror).not_to receive(:ensure_remote!) - - execute! - end - end - it 'does not fetch the remote repository' do # See https://gitlab.com/gitlab-org/gitaly/-/issues/2670 expect(project.repository).not_to receive(:fetch_remote) diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index d7f5c39e457..a1b726071d6 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -2080,6 +2080,61 @@ RSpec.describe QuickActions::InterpretService do end end end + + context 'severity command' do + let_it_be_with_reload(:issuable) { create(:incident, project: project) } + + subject(:set_severity) { service.execute(content, issuable) } + + it_behaves_like 'failed command', 'No severity matches the provided parameter' do + let(:content) { '/severity something' } + end + + shared_examples 'updates the severity' do |new_severity| + it do + expect { set_severity }.to change { issuable.severity }.from('unknown').to(new_severity) + end + end + + context 'when quick action is used on creation' do + let(:content) { '/severity s3' } + let(:issuable) { build(:incident, project: project) } + + it_behaves_like 'updates the severity', 'medium' + + context 'issuable does not support severity' do + let(:issuable) { build(:issue, project: project) } + + it_behaves_like 'failed command', '' + end + end + + context 'severity given with S format' do + let(:content) { '/severity s3' } + + it_behaves_like 'updates the severity', 'medium' + end + + context 'severity given with number format' do + let(:content) { '/severity 3' } + + it_behaves_like 'updates the severity', 'medium' + end + + context 'severity given with text format' do + let(:content) { '/severity medium' } + + it_behaves_like 'updates the severity', 'medium' + end + + context 'an issuable that does not support severity' do + let_it_be_with_reload(:issuable) { create(:issue, project: project) } + + it_behaves_like 'failed command', 'Could not apply severity command.' do + let(:content) { '/severity s3' } + end + end + end end describe '#explain' do diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb index bf28fde3d90..7287825a0be 100644 --- a/spec/services/releases/create_service_spec.rb +++ b/spec/services/releases/create_service_spec.rb @@ -44,21 +44,6 @@ RSpec.describe Releases::CreateService do it_behaves_like 'a successful release creation' - context 'when tag is protected and user does not have access to it' do - let!(:protected_tag) { create(:protected_tag, :no_one_can_create, name: '*', project: project) } - - it 'track the error event' do - stub_feature_flags(evalute_protected_tag_for_release_permissions: false) - - expect(Gitlab::ErrorTracking).to receive(:log_exception).with( - kind_of(described_class::ReleaseProtectedTagAccessError), - project_id: project.id, - user_id: user.id) - - service.execute - end - end - context 'when the tag does not exist' do let(:tag_name) { 'non-exist-tag' } diff --git a/spec/services/releases/destroy_service_spec.rb b/spec/services/releases/destroy_service_spec.rb index 38cdcef3825..bc5bff0b31d 100644 --- a/spec/services/releases/destroy_service_spec.rb +++ b/spec/services/releases/destroy_service_spec.rb @@ -28,21 +28,6 @@ RSpec.describe Releases::DestroyService do it 'returns the destroyed object' do is_expected.to include(status: :success, release: release) end - - context 'when tag is protected and user does not have access to it' do - let!(:protected_tag) { create(:protected_tag, :no_one_can_create, name: '*', project: project) } - - it 'track the error event' do - stub_feature_flags(evalute_protected_tag_for_release_permissions: false) - - expect(Gitlab::ErrorTracking).to receive(:log_exception).with( - kind_of(described_class::ReleaseProtectedTagAccessError), - project_id: project.id, - user_id: user.id) - - service.execute - end - end end context 'when tag does not exist in the repository' do diff --git a/spec/services/releases/update_service_spec.rb b/spec/services/releases/update_service_spec.rb index 96b562a8071..932a7fab5ec 100644 --- a/spec/services/releases/update_service_spec.rb +++ b/spec/services/releases/update_service_spec.rb @@ -38,21 +38,6 @@ RSpec.describe Releases::UpdateService do service.execute end - context 'when tag is protected and user does not have access to it' do - let!(:protected_tag) { create(:protected_tag, :no_one_can_create, name: '*', project: project) } - - it 'track the error event' do - stub_feature_flags(evalute_protected_tag_for_release_permissions: false) - - expect(Gitlab::ErrorTracking).to receive(:log_exception).with( - kind_of(described_class::ReleaseProtectedTagAccessError), - project_id: project.id, - user_id: user.id) - - service.execute - end - end - context 'when the tag does not exists' do let(:tag_name) { 'foobar' } diff --git a/spec/services/resource_events/change_labels_service_spec.rb b/spec/services/resource_events/change_labels_service_spec.rb index 012168ef719..b987e3204ad 100644 --- a/spec/services/resource_events/change_labels_service_spec.rb +++ b/spec/services/resource_events/change_labels_service_spec.rb @@ -54,7 +54,7 @@ RSpec.describe ResourceEvents::ChangeLabelsService do let(:removed) { [labels[1]] } it 'creates all label events in a single query' do - expect(Gitlab::Database).to receive(:bulk_insert).once.and_call_original + expect(Gitlab::Database.main).to receive(:bulk_insert).once.and_call_original expect { subject }.to change { resource.resource_label_events.count }.from(0).to(2) end end diff --git a/spec/services/security/merge_reports_service_spec.rb b/spec/services/security/merge_reports_service_spec.rb new file mode 100644 index 00000000000..120ce12aa58 --- /dev/null +++ b/spec/services/security/merge_reports_service_spec.rb @@ -0,0 +1,260 @@ +# frozen_string_literal: true + +require 'spec_helper' + +# rubocop: disable RSpec/MultipleMemoizedHelpers +RSpec.describe Security::MergeReportsService, '#execute' do + let(:scanner_1) { build(:ci_reports_security_scanner, external_id: 'scanner-1', name: 'Scanner 1') } + let(:scanner_2) { build(:ci_reports_security_scanner, external_id: 'scanner-2', name: 'Scanner 2') } + let(:scanner_3) { build(:ci_reports_security_scanner, external_id: 'scanner-3', name: 'Scanner 3') } + + let(:identifier_1_primary) { build(:ci_reports_security_identifier, external_id: 'VULN-1', external_type: 'scanner-1') } + let(:identifier_1_cve) { build(:ci_reports_security_identifier, external_id: 'CVE-2019-123', external_type: 'cve') } + let(:identifier_2_primary) { build(:ci_reports_security_identifier, external_id: 'VULN-2', external_type: 'scanner-2') } + let(:identifier_2_cve) { build(:ci_reports_security_identifier, external_id: 'CVE-2019-456', external_type: 'cve') } + let(:identifier_cwe) { build(:ci_reports_security_identifier, external_id: '789', external_type: 'cwe') } + let(:identifier_wasc) { build(:ci_reports_security_identifier, external_id: '13', external_type: 'wasc') } + + let(:finding_id_1) do + build(:ci_reports_security_finding, + identifiers: [identifier_1_primary, identifier_1_cve], + scanner: scanner_1, + severity: :low + ) + end + + let(:finding_id_1_extra) do + build(:ci_reports_security_finding, + identifiers: [identifier_1_primary, identifier_1_cve], + scanner: scanner_1, + severity: :low + ) + end + + let(:finding_id_2_loc_1) do + build(:ci_reports_security_finding, + identifiers: [identifier_2_primary, identifier_2_cve], + location: build(:ci_reports_security_locations_sast, start_line: 32, end_line: 34), + scanner: scanner_2, + severity: :medium + ) + end + + let(:finding_id_2_loc_1_extra) do + build(:ci_reports_security_finding, + identifiers: [identifier_2_primary, identifier_2_cve], + location: build(:ci_reports_security_locations_sast, start_line: 32, end_line: 34), + scanner: scanner_2, + severity: :medium + ) + end + + let(:finding_id_2_loc_2) do + build(:ci_reports_security_finding, + identifiers: [identifier_2_primary, identifier_2_cve], + location: build(:ci_reports_security_locations_sast, start_line: 42, end_line: 44), + scanner: scanner_2, + severity: :medium + ) + end + + let(:finding_cwe_1) do + build(:ci_reports_security_finding, + identifiers: [identifier_cwe], + scanner: scanner_3, + severity: :high + ) + end + + let(:finding_cwe_2) do + build(:ci_reports_security_finding, + identifiers: [identifier_cwe], + scanner: scanner_1, + severity: :critical + ) + end + + let(:finding_wasc_1) do + build(:ci_reports_security_finding, + identifiers: [identifier_wasc], + scanner: scanner_1, + severity: :medium + ) + end + + let(:finding_wasc_2) do + build(:ci_reports_security_finding, + identifiers: [identifier_wasc], + scanner: scanner_2, + severity: :critical + ) + end + + let(:report_1_findings) { [finding_id_1, finding_id_2_loc_1, finding_id_2_loc_1_extra, finding_cwe_2, finding_wasc_1] } + + let(:scanned_resource) do + ::Gitlab::Ci::Reports::Security::ScannedResource.new(URI.parse('example.com'), 'GET') + end + + let(:scanned_resource_1) do + ::Gitlab::Ci::Reports::Security::ScannedResource.new(URI.parse('example.com'), 'POST') + end + + let(:scanned_resource_2) do + ::Gitlab::Ci::Reports::Security::ScannedResource.new(URI.parse('example.com/2'), 'GET') + end + + let(:scanned_resource_3) do + ::Gitlab::Ci::Reports::Security::ScannedResource.new(URI.parse('example.com/3'), 'GET') + end + + let(:report_1) do + build( + :ci_reports_security_report, + scanners: [scanner_1, scanner_2], + findings: report_1_findings, + identifiers: report_1_findings.flat_map(&:identifiers), + scanned_resources: [scanned_resource, scanned_resource_1, scanned_resource_2] + ) + end + + let(:report_2_findings) { [finding_id_2_loc_2, finding_wasc_2] } + + let(:report_2) do + build( + :ci_reports_security_report, + scanners: [scanner_2], + findings: report_2_findings, + identifiers: finding_id_2_loc_2.identifiers, + scanned_resources: [scanned_resource, scanned_resource_1, scanned_resource_3] + ) + end + + let(:report_3_findings) { [finding_id_1_extra, finding_cwe_1] } + + let(:report_3) do + build( + :ci_reports_security_report, + scanners: [scanner_1, scanner_3], + findings: report_3_findings, + identifiers: report_3_findings.flat_map(&:identifiers) + ) + end + + let(:merge_service) { described_class.new(report_1, report_2, report_3) } + + subject(:merged_report) { merge_service.execute } + + describe 'errors on target report' do + subject { merged_report.errors } + + before do + report_1.add_error('foo', 'bar') + report_2.add_error('zoo', 'baz') + end + + it { is_expected.to eq([{ type: 'foo', message: 'bar' }, { type: 'zoo', message: 'baz' }]) } + end + + it 'copies scanners into target report and eliminates duplicates' do + expect(merged_report.scanners.values).to contain_exactly(scanner_1, scanner_2, scanner_3) + end + + it 'copies identifiers into target report and eliminates duplicates' do + expect(merged_report.identifiers.values).to( + contain_exactly( + identifier_1_primary, + identifier_1_cve, + identifier_2_primary, + identifier_2_cve, + identifier_cwe, + identifier_wasc + ) + ) + end + + it 'deduplicates (except cwe and wasc) and sorts the vulnerabilities by severity (desc) then by compare key' do + expect(merged_report.findings).to( + eq([ + finding_cwe_2, + finding_wasc_2, + finding_cwe_1, + finding_id_2_loc_2, + finding_id_2_loc_1, + finding_wasc_1, + finding_id_1 + ]) + ) + end + + it 'deduplicates scanned resources' do + expect(merged_report.scanned_resources).to( + eq([ + scanned_resource, + scanned_resource_1, + scanned_resource_2, + scanned_resource_3 + ]) + ) + end + + context 'ordering reports for sast analyzers' do + let(:bandit_scanner) { build(:ci_reports_security_scanner, external_id: 'bandit', name: 'Bandit') } + let(:semgrep_scanner) { build(:ci_reports_security_scanner, external_id: 'semgrep', name: 'Semgrep') } + + let(:identifier_bandit) { build(:ci_reports_security_identifier, external_id: 'B403', external_type: 'bandit_test_id') } + let(:identifier_cve) { build(:ci_reports_security_identifier, external_id: 'CVE-2019-123', external_type: 'cve') } + let(:identifier_semgrep) { build(:ci_reports_security_identifier, external_id: 'rules.bandit.B105', external_type: 'semgrep_id') } + + let(:finding_id_1) { build(:ci_reports_security_finding, identifiers: [identifier_bandit, identifier_cve], scanner: bandit_scanner, report_type: :sast) } + let(:finding_id_2) { build(:ci_reports_security_finding, identifiers: [identifier_cve], scanner: semgrep_scanner, report_type: :sast) } + let(:finding_id_3) { build(:ci_reports_security_finding, identifiers: [identifier_semgrep], scanner: semgrep_scanner, report_type: :sast ) } + + let(:bandit_report) do + build( :ci_reports_security_report, + type: :sast, + scanners: [bandit_scanner], + findings: [finding_id_1], + identifiers: finding_id_1.identifiers + ) + end + + let(:semgrep_report) do + build( + :ci_reports_security_report, + type: :sast, + scanners: [semgrep_scanner], + findings: [finding_id_2, finding_id_3], + identifiers: finding_id_2.identifiers + finding_id_3.identifiers + ) + end + + let(:custom_analyzer_report) do + build( + :ci_reports_security_report, + type: :sast, + scanners: [scanner_2], + findings: [finding_id_2_loc_1], + identifiers: finding_id_2_loc_1.identifiers + ) + end + + context 'when reports are gathered in an unprioritized order' do + subject(:sast_merged_report) { described_class.new(semgrep_report, bandit_report).execute } + + specify { expect(sast_merged_report.scanners.values).to eql([bandit_scanner, semgrep_scanner]) } + specify { expect(sast_merged_report.findings.count).to eq(2) } + specify { expect(sast_merged_report.findings.first.identifiers).to eql([identifier_bandit, identifier_cve]) } + specify { expect(sast_merged_report.findings.last.identifiers).to contain_exactly(identifier_semgrep) } + end + + context 'when a custom analyzer is completed before the known analyzers' do + subject(:sast_merged_report) { described_class.new(custom_analyzer_report, semgrep_report, bandit_report).execute } + + specify { expect(sast_merged_report.scanners.values).to eql([bandit_scanner, semgrep_scanner, scanner_2]) } + specify { expect(sast_merged_report.findings.count).to eq(3) } + specify { expect(sast_merged_report.findings.last.identifiers).to match_array(finding_id_2_loc_1.identifiers) } + end + end +end +# rubocop: enable RSpec/MultipleMemoizedHelpers diff --git a/spec/services/service_ping/permit_data_categories_service_spec.rb b/spec/services/service_ping/permit_data_categories_service_spec.rb index 4fd5c6f9ccb..550c0ea5e13 100644 --- a/spec/services/service_ping/permit_data_categories_service_spec.rb +++ b/spec/services/service_ping/permit_data_categories_service_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' RSpec.describe ServicePing::PermitDataCategoriesService do - using RSpec::Parameterized::TableSyntax - describe '#execute', :without_license do subject(:permitted_categories) { described_class.new.execute } @@ -15,7 +13,7 @@ RSpec.describe ServicePing::PermitDataCategoriesService do end it 'returns all categories' do - expect(permitted_categories).to match_array(%w[Standard Subscription Operational Optional]) + expect(permitted_categories).to match_array(%w[standard subscription operational optional]) end end @@ -41,27 +39,4 @@ RSpec.describe ServicePing::PermitDataCategoriesService do end end end - - describe '#product_intelligence_enabled?' do - where(:usage_ping_enabled, :requires_usage_stats_consent, :expected_product_intelligence_enabled) do - # Usage ping enabled - true | false | true - true | true | false - - # Usage ping disabled - false | false | false - false | true | false - end - - with_them do - before do - allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: requires_usage_stats_consent)) - stub_config_setting(usage_ping_enabled: usage_ping_enabled) - end - - it 'has the correct product_intelligence_enabled?' do - expect(described_class.new.product_intelligence_enabled?).to eq(expected_product_intelligence_enabled) - end - end - end end diff --git a/spec/services/service_ping/service_ping_settings_spec.rb b/spec/services/service_ping/service_ping_settings_spec.rb new file mode 100644 index 00000000000..90a5c6b30eb --- /dev/null +++ b/spec/services/service_ping/service_ping_settings_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ServicePing::ServicePingSettings do + using RSpec::Parameterized::TableSyntax + + describe '#product_intelligence_enabled?' do + where(:usage_ping_enabled, :requires_usage_stats_consent, :expected_product_intelligence_enabled) do + # Usage ping enabled + true | false | true + true | true | false + + # Usage ping disabled + false | false | false + false | true | false + end + + with_them do + before do + allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: requires_usage_stats_consent)) + stub_config_setting(usage_ping_enabled: usage_ping_enabled) + end + + it 'has the correct product_intelligence_enabled?' do + expect(described_class.product_intelligence_enabled?).to eq(expected_product_intelligence_enabled) + end + end + end + + describe '#enabled?' do + describe 'has the correct enabled' do + it 'when false' do + stub_config_setting(usage_ping_enabled: false) + + expect(described_class.enabled?).to eq(false) + end + + it 'when true' do + stub_config_setting(usage_ping_enabled: true) + + expect(described_class.enabled?).to eq(true) + end + end + end +end diff --git a/spec/services/service_ping/submit_service_ping_service_spec.rb b/spec/services/service_ping/submit_service_ping_service_spec.rb index 8a3065e6bc6..05df4e49014 100644 --- a/spec/services/service_ping/submit_service_ping_service_spec.rb +++ b/spec/services/service_ping/submit_service_ping_service_spec.rb @@ -100,9 +100,7 @@ RSpec.describe ServicePing::SubmitService do context 'when product_intelligence_enabled is false' do before do - allow_next_instance_of(ServicePing::PermitDataCategoriesService) do |service| - allow(service).to receive(:product_intelligence_enabled?).and_return(false) - end + allow(ServicePing::ServicePingSettings).to receive(:product_intelligence_enabled?).and_return(false) end it_behaves_like 'does not run' @@ -112,9 +110,7 @@ RSpec.describe ServicePing::SubmitService do before do stub_usage_data_connections - allow_next_instance_of(ServicePing::PermitDataCategoriesService) do |service| - allow(service).to receive(:product_intelligence_enabled?).and_return(true) - end + allow(ServicePing::ServicePingSettings).to receive(:product_intelligence_enabled?).and_return(true) end it 'generates service ping' do diff --git a/spec/services/service_response_spec.rb b/spec/services/service_response_spec.rb index 986b26e67d7..082ee4ddc67 100644 --- a/spec/services/service_response_spec.rb +++ b/spec/services/service_response_spec.rb @@ -2,7 +2,7 @@ require 'fast_spec_helper' -ActiveSupport::Dependencies.autoload_paths << 'app/services' +require_relative '../../app/services/service_response' RSpec.describe ServiceResponse do describe '.success' do diff --git a/spec/services/spam/mark_as_spam_service_spec.rb b/spec/services/spam/akismet_mark_as_spam_service_spec.rb index 308a66c3a48..12666e23e47 100644 --- a/spec/services/spam/mark_as_spam_service_spec.rb +++ b/spec/services/spam/akismet_mark_as_spam_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Spam::MarkAsSpamService do +RSpec.describe Spam::AkismetMarkAsSpamService do let(:user_agent_detail) { build(:user_agent_detail) } let(:spammable) { build(:issue, user_agent_detail: user_agent_detail) } let(:fake_akismet_service) { double(:akismet_service, submit_spam: true) } diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb index 3a92e5acb5a..8ddfa7ed3a0 100644 --- a/spec/services/spam/spam_action_service_spec.rb +++ b/spec/services/spam/spam_action_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Spam::SpamActionService do include_context 'includes Spam constants' - let(:issue) { create(:issue, project: project, author: user) } + let(:issue) { create(:issue, project: project, author: author) } let(:fake_ip) { '1.2.3.4' } let(:fake_user_agent) { 'fake-user-agent' } let(:fake_referer) { 'fake-http-referer' } @@ -23,6 +23,7 @@ RSpec.describe Spam::SpamActionService do let_it_be(:project) { create(:project, :public) } let_it_be(:user) { create(:user) } + let_it_be(:author) { create(:user) } before do issue.spam = false diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index e9bd40b058b..5aff5149dcf 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -745,7 +745,7 @@ RSpec.describe SystemNoteService do end describe '.new_alert_issue' do - let(:alert) { build(:alert_management_alert, :with_issue) } + let(:alert) { build(:alert_management_alert, :with_incident) } it 'calls AlertManagementService' do expect_next_instance_of(SystemNotes::AlertManagementService) do |service| diff --git a/spec/services/system_notes/alert_management_service_spec.rb b/spec/services/system_notes/alert_management_service_spec.rb index 1c36a4036cc..6e6bfeaa205 100644 --- a/spec/services/system_notes/alert_management_service_spec.rb +++ b/spec/services/system_notes/alert_management_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe ::SystemNotes::AlertManagementService do let_it_be(:author) { create(:user) } let_it_be(:project) { create(:project, :repository) } - let_it_be(:noteable) { create(:alert_management_alert, :with_issue, :acknowledged, project: project) } + let_it_be(:noteable) { create(:alert_management_alert, :with_incident, :acknowledged, project: project) } describe '#create_new_alert' do subject { described_class.new(noteable: noteable, project: project).create_new_alert('Some Service') } diff --git a/spec/services/users/activity_service_spec.rb b/spec/services/users/activity_service_spec.rb index cfafa9eff45..6c1df5c745f 100644 --- a/spec/services/users/activity_service_spec.rb +++ b/spec/services/users/activity_service_spec.rb @@ -85,19 +85,14 @@ RSpec.describe Users::ActivityService do end end - context 'with DB Load Balancing', :request_store, :redis, :clean_gitlab_redis_shared_state do - include_context 'clear DB Load Balancing configuration' - + context 'with DB Load Balancing' do let(:user) { create(:user, last_activity_on: last_activity_on) } context 'when last activity is in the past' do let(:user) { create(:user, last_activity_on: Date.today - 1.week) } - context 'database load balancing is configured' do + context 'database load balancing is configured', :db_load_balancing do before do - # Do not pollute AR for other tests, but rather simulate effect of configure_proxy. - allow(ActiveRecord::Base.singleton_class).to receive(:prepend) - ::Gitlab::Database::LoadBalancing.configure_proxy allow(ActiveRecord::Base).to receive(:connection).and_return(::Gitlab::Database::LoadBalancing.proxy) end diff --git a/spec/services/users/ban_service_spec.rb b/spec/services/users/ban_service_spec.rb index 0e6ac615da5..6f49ee08782 100644 --- a/spec/services/users/ban_service_spec.rb +++ b/spec/services/users/ban_service_spec.rb @@ -3,47 +3,68 @@ require 'spec_helper' RSpec.describe Users::BanService do - let(:current_user) { create(:admin) } + let(:user) { create(:user) } - subject(:service) { described_class.new(current_user) } + let_it_be(:current_user) { create(:admin) } - describe '#execute' do - subject(:operation) { service.execute(user) } + shared_examples 'does not modify the BannedUser record or user state' do + it 'does not modify the BannedUser record or user state' do + expect { ban_user }.not_to change { Users::BannedUser.count } + expect { ban_user }.not_to change { user.state } + end + end - context 'when successful' do - let(:user) { create(:user) } + context 'ban', :aggregate_failures do + subject(:ban_user) { described_class.new(current_user).execute(user) } - it { is_expected.to eq(status: :success) } + context 'when successful', :enable_admin_mode do + it 'returns success status' do + response = ban_user - it "bans the user" do - expect { operation }.to change { user.state }.to('banned') + expect(response[:status]).to eq(:success) end - it "blocks the user" do - expect { operation }.to change { user.blocked? }.from(false).to(true) + it 'bans the user' do + expect { ban_user }.to change { user.state }.from('active').to('banned') end - it 'logs ban in application logs' do - allow(Gitlab::AppLogger).to receive(:info) + it 'creates a BannedUser' do + expect { ban_user }.to change { Users::BannedUser.count }.by(1) + expect(Users::BannedUser.last.user_id).to eq(user.id) + end - operation + it 'logs ban in application logs' do + expect(Gitlab::AppLogger).to receive(:info).with(message: "User ban", user: "#{user.username}", email: "#{user.email}", ban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") - expect(Gitlab::AppLogger).to have_received(:info).with(message: "User banned", user: "#{user.username}", email: "#{user.email}", banned_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + ban_user end end context 'when failed' do - let(:user) { create(:user, :blocked) } + context 'when user is blocked', :enable_admin_mode do + before do + user.block! + end - it 'returns error result' do - aggregate_failures 'error result' do - expect(operation[:status]).to eq(:error) - expect(operation[:message]).to match(/State cannot transition/) + it 'returns state error message' do + response = ban_user + + expect(response[:status]).to eq(:error) + expect(response[:message]).to match(/State cannot transition/) end + + it_behaves_like 'does not modify the BannedUser record or user state' end - it "does not change the user's state" do - expect { operation }.not_to change { user.state } + context 'when user is not an admin' do + it 'returns permissions error message' do + response = ban_user + + expect(response[:status]).to eq(:error) + expect(response[:message]).to match(/You are not allowed to ban a user/) + end + + it_behaves_like 'does not modify the BannedUser record or user state' end end end diff --git a/spec/services/users/banned_user_base_service_spec.rb b/spec/services/users/banned_user_base_service_spec.rb new file mode 100644 index 00000000000..29a549f0f49 --- /dev/null +++ b/spec/services/users/banned_user_base_service_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::BannedUserBaseService do + let(:admin) { create(:admin) } + let(:base_service) { described_class.new(admin) } + + describe '#initialize' do + it 'sets the current_user instance value' do + expect(base_service.instance_values["current_user"]).to eq(admin) + end + end +end diff --git a/spec/services/users/set_status_service_spec.rb b/spec/services/users/set_status_service_spec.rb index 2c776a0eeb4..76e86506d94 100644 --- a/spec/services/users/set_status_service_spec.rb +++ b/spec/services/users/set_status_service_spec.rb @@ -8,6 +8,18 @@ RSpec.describe Users::SetStatusService do subject(:service) { described_class.new(current_user, params) } describe '#execute' do + shared_examples_for 'bumps user' do + it 'bumps User#updated_at' do + expect { service.execute }.to change { current_user.updated_at } + end + end + + shared_examples_for 'does not bump user' do + it 'does not bump User#updated_at' do + expect { service.execute }.not_to change { current_user.updated_at } + end + end + context 'when params are set' do let(:params) { { emoji: 'taurus', message: 'a random status', availability: 'busy' } } @@ -31,6 +43,8 @@ RSpec.describe Users::SetStatusService do expect(service.execute).to be(true) end + it_behaves_like 'bumps user' + context 'when setting availability to not_set' do before do params[:availability] = 'not_set' @@ -72,6 +86,8 @@ RSpec.describe Users::SetStatusService do it 'does not update the status if the current user is not allowed' do expect { service.execute }.not_to change { target_user.status } end + + it_behaves_like 'does not bump user' end end @@ -79,20 +95,28 @@ RSpec.describe Users::SetStatusService do let(:params) { {} } shared_examples 'removes user status record' do - it 'deletes the status' do - status = create(:user_status, user: current_user) - + it 'deletes the user status record' do expect { service.execute } - .to change { current_user.reload.status }.from(status).to(nil) + .to change { current_user.reload.status }.from(user_status).to(nil) end - end - it_behaves_like 'removes user status record' + it_behaves_like 'bumps user' + end - context 'when not_set is given for availability' do - let(:params) { { availability: 'not_set' } } + context 'when user has existing user status record' do + let!(:user_status) { create(:user_status, user: current_user) } it_behaves_like 'removes user status record' + + context 'when not_set is given for availability' do + let(:params) { { availability: 'not_set' } } + + it_behaves_like 'removes user status record' + end + end + + context 'when user has no existing user status record' do + it_behaves_like 'does not bump user' end end end diff --git a/spec/services/users/unban_service_spec.rb b/spec/services/users/unban_service_spec.rb new file mode 100644 index 00000000000..b2b3140ccb3 --- /dev/null +++ b/spec/services/users/unban_service_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::UnbanService do + let(:user) { create(:user) } + + let_it_be(:current_user) { create(:admin) } + + shared_examples 'does not modify the BannedUser record or user state' do + it 'does not modify the BannedUser record or user state' do + expect { unban_user }.not_to change { Users::BannedUser.count } + expect { unban_user }.not_to change { user.state } + end + end + + context 'unban', :aggregate_failures do + subject(:unban_user) { described_class.new(current_user).execute(user) } + + context 'when successful', :enable_admin_mode do + before do + user.ban! + end + + it 'returns success status' do + response = unban_user + + expect(response[:status]).to eq(:success) + end + + it 'unbans the user' do + expect { unban_user }.to change { user.state }.from('banned').to('active') + end + + it 'removes the BannedUser' do + expect { unban_user }.to change { Users::BannedUser.count }.by(-1) + expect(user.reload.banned_user).to be_nil + end + + it 'logs unban in application logs' do + expect(Gitlab::AppLogger).to receive(:info).with(message: "User unban", user: "#{user.username}", email: "#{user.email}", unban_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + + unban_user + end + end + + context 'when failed' do + context 'when user is already active', :enable_admin_mode do + it 'returns state error message' do + response = unban_user + + expect(response[:status]).to eq(:error) + expect(response[:message]).to match(/State cannot transition/) + end + + it_behaves_like 'does not modify the BannedUser record or user state' + end + + context 'when user is not an admin' do + before do + user.ban! + end + + it 'returns permissions error message' do + response = unban_user + + expect(response[:status]).to eq(:error) + expect(response[:message]).to match(/You are not allowed to unban a user/) + end + + it_behaves_like 'does not modify the BannedUser record or user state' + end + end + end +end |