diff options
| author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-11-17 14:33:21 +0300 |
|---|---|---|
| committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-11-17 14:33:21 +0300 |
| commit | 7021455bd1ed7b125c55eb1b33c5a01f2bc55ee0 (patch) | |
| tree | 5bdc2229f5198d516781f8d24eace62fc7e589e9 /spec/services | |
| parent | 185b095e93520f96e9cfc31d9c3e69b498cdab7c (diff) | |
Add latest changes from gitlab-org/gitlab@15-6-stable-eev15.6.0-rc42
Diffstat (limited to 'spec/services')
128 files changed, 3625 insertions, 3035 deletions
diff --git a/spec/services/admin/set_feature_flag_service_spec.rb b/spec/services/admin/set_feature_flag_service_spec.rb index 6fa806644c9..9a9c5545e23 100644 --- a/spec/services/admin/set_feature_flag_service_spec.rb +++ b/spec/services/admin/set_feature_flag_service_spec.rb @@ -130,6 +130,15 @@ RSpec.describe Admin::SetFeatureFlagService do end end + context 'when enabling for a repository' do + let(:params) { { value: 'true', repository: project.repository.full_path } } + + it 'enables the feature flag' do + expect(Feature).to receive(:enable).with(feature_name, project.repository) + expect(subject).to be_success + end + end + context 'when enabling for a user actor and a feature group' do let(:params) { { value: 'true', user: user.username, feature_group: 'perf_team' } } let(:feature_group) { Feature.group('perf_team') } diff --git a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb index 73d185283b6..676f55be28a 100644 --- a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb @@ -225,7 +225,7 @@ RSpec.describe AutoMerge::MergeWhenPipelineSucceedsService do let!(:build) do create(:ci_build, :created, pipeline: pipeline, ref: ref, - name: 'build', ci_stage: build_stage ) + name: 'build', ci_stage: build_stage) end let!(:test) do diff --git a/spec/services/boards/issues/list_service_spec.rb b/spec/services/boards/issues/list_service_spec.rb index 72027911e51..1959710bb0c 100644 --- a/spec/services/boards/issues/list_service_spec.rb +++ b/spec/services/boards/issues/list_service_spec.rb @@ -27,7 +27,7 @@ RSpec.describe Boards::Issues::ListService do let_it_be(:opened_issue1) { create(:labeled_issue, project: project, milestone: m1, title: 'Issue 1', labels: [bug]) } let_it_be(:opened_issue2) { create(:labeled_issue, project: project, milestone: m2, title: 'Issue 2', labels: [p2]) } - let_it_be(:reopened_issue1) { create(:issue, :opened, project: project, title: 'Reopened Issue 1' ) } + let_it_be(:reopened_issue1) { create(:issue, :opened, project: project, title: 'Reopened Issue 1') } let_it_be(:list1_issue1) { create(:labeled_issue, project: project, milestone: m1, labels: [p2, development]) } let_it_be(:list1_issue2) { create(:labeled_issue, project: project, milestone: m2, labels: [development]) } @@ -110,7 +110,7 @@ RSpec.describe Boards::Issues::ListService do let!(:opened_issue1) { create(:labeled_issue, project: project, milestone: m1, title: 'Issue 1', labels: [bug]) } let!(:opened_issue2) { create(:labeled_issue, project: project, milestone: m2, title: 'Issue 2', labels: [p2, p2_project]) } let!(:opened_issue3) { create(:labeled_issue, project: project_archived, milestone: m1, title: 'Issue 3', labels: [bug]) } - let!(:reopened_issue1) { create(:issue, state: 'opened', project: project, title: 'Reopened Issue 1', closed_at: Time.current ) } + let!(:reopened_issue1) { create(:issue, state: 'opened', project: project, title: 'Reopened Issue 1', closed_at: Time.current) } let!(:list1_issue1) { create(:labeled_issue, project: project, milestone: m1, labels: [p2, p2_project, development]) } let!(:list1_issue2) { create(:labeled_issue, project: project, milestone: m2, labels: [development]) } diff --git a/spec/services/branches/create_service_spec.rb b/spec/services/branches/create_service_spec.rb index 26cc1a0665e..19a32aafa38 100644 --- a/spec/services/branches/create_service_spec.rb +++ b/spec/services/branches/create_service_spec.rb @@ -56,17 +56,6 @@ RSpec.describe Branches::CreateService, :use_clean_rails_redis_caching do end end - context 'when an ambiguous branch name is provided' do - let(:branches) { { 'ambiguous/test' => 'master', 'ambiguous' => 'master' } } - - it 'returns an error that branch could not be created' do - err_msg = 'Failed to create branch \'ambiguous\': 13:reference is ambiguous.' - - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to match_array([err_msg]) - end - end - context 'when PreReceiveError exception' do let(:branches) { { 'error' => 'master' } } @@ -184,18 +173,6 @@ RSpec.describe Branches::CreateService, :use_clean_rails_redis_caching do end end - context 'when an ambiguous branch name is provided' do - it 'returns an error that branch could not be created' do - err_msg = 'Failed to create branch \'feature\': 13:reference is ambiguous.' - - service.execute('feature/widget', 'master') - result = service.execute('feature', 'master') - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq(err_msg) - end - end - it 'logs and returns an error if there is a PreReceiveError exception' do error_message = 'pre receive error' raw_message = "GitLab: #{error_message}" diff --git a/spec/services/bulk_imports/create_pipeline_trackers_service_spec.rb b/spec/services/bulk_imports/create_pipeline_trackers_service_spec.rb index 0de962328c5..5a7852fc32f 100644 --- a/spec/services/bulk_imports/create_pipeline_trackers_service_spec.rb +++ b/spec/services/bulk_imports/create_pipeline_trackers_service_spec.rb @@ -77,6 +77,8 @@ RSpec.describe BulkImports::CreatePipelineTrackersService do message: 'Pipeline skipped as source instance version not compatible with pipeline', bulk_import_entity_id: entity.id, bulk_import_id: entity.bulk_import_id, + bulk_import_entity_type: entity.source_type, + source_full_path: entity.source_full_path, importer: 'gitlab_migration', pipeline_name: 'PipelineClass4', minimum_source_version: '15.1.0', @@ -88,6 +90,8 @@ RSpec.describe BulkImports::CreatePipelineTrackersService do message: 'Pipeline skipped as source instance version not compatible with pipeline', bulk_import_entity_id: entity.id, bulk_import_id: entity.bulk_import_id, + bulk_import_entity_type: entity.source_type, + source_full_path: entity.source_full_path, importer: 'gitlab_migration', pipeline_name: 'PipelineClass5', minimum_source_version: '16.0.0', diff --git a/spec/services/ci/after_requeue_job_service_spec.rb b/spec/services/ci/after_requeue_job_service_spec.rb index 1f692bdb71a..e6f46fb9ebe 100644 --- a/spec/services/ci/after_requeue_job_service_spec.rb +++ b/spec/services/ci/after_requeue_job_service_spec.rb @@ -120,26 +120,6 @@ RSpec.describe Ci::AfterRequeueJobService, :sidekiq_inline do ) end - context 'when the FF ci_requeue_with_dag_object_hierarchy is disabled' do - before do - stub_feature_flags(ci_requeue_with_dag_object_hierarchy: false) - end - - it 'marks subsequent skipped jobs as processable but leaves a3 created' do - execute_after_requeue_service(a1) - - check_jobs_statuses( - a1: 'pending', - a2: 'created', - a3: 'skipped', - b1: 'success', - b2: 'created', - c1: 'created', - c2: 'created' - ) - end - end - context 'when executed by a different user than the original owner' do let(:retryer) { create(:user).tap { |u| project.add_maintainer(u) } } let(:service) { described_class.new(project, retryer) } @@ -312,22 +292,6 @@ RSpec.describe Ci::AfterRequeueJobService, :sidekiq_inline do c: 'created' ) end - - context 'when the FF ci_requeue_with_dag_object_hierarchy is disabled' do - before do - stub_feature_flags(ci_requeue_with_dag_object_hierarchy: false) - end - - it 'marks the next subsequent skipped job as processable but leaves c skipped' do - execute_after_requeue_service(a) - - check_jobs_statuses( - a: 'pending', - b: 'created', - c: 'skipped' - ) - end - end end private diff --git a/spec/services/ci/compare_test_reports_service_spec.rb b/spec/services/ci/compare_test_reports_service_spec.rb index 6d3df0f5383..f259072fe87 100644 --- a/spec/services/ci/compare_test_reports_service_spec.rb +++ b/spec/services/ci/compare_test_reports_service_spec.rb @@ -41,7 +41,7 @@ RSpec.describe Ci::CompareTestReportsService do it 'returns a parsed TestReports success status and failure on the individual suite' do expect(comparison[:status]).to eq(:parsed) expect(comparison.dig(:data, 'status')).to eq('success') - expect(comparison.dig(:data, 'suites', 0, 'status') ).to eq('error') + expect(comparison.dig(:data, 'suites', 0, 'status')).to eq('error') end end diff --git a/spec/services/ci/create_pipeline_service/partitioning_spec.rb b/spec/services/ci/create_pipeline_service/partitioning_spec.rb index 43fbb74ede4..f34d103d965 100644 --- a/spec/services/ci/create_pipeline_service/partitioning_spec.rb +++ b/spec/services/ci/create_pipeline_service/partitioning_spec.rb @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectness, :aggregate_failures do +RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectness, :aggregate_failures, +:ci_partitionable do let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { project.first_owner } @@ -31,7 +32,7 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes end let(:pipeline) { service.execute(:push).payload } - let(:current_partition_id) { 123 } + let(:current_partition_id) { ci_testing_partition_id } before do stub_ci_pipeline_yaml_file(config) diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index c737b8cc329..5fdefb2b306 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -1425,5 +1425,43 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes it_behaves_like 'comparing file changes with workflow rules' end end + + context 'workflow name with rules' do + let(:ref) { 'refs/heads/feature' } + + let(:variables) do + [{ key: 'SOME_VARIABLE', secret_value: 'SOME_VAL' }] + end + + let(:pipeline) do + execute_service do |pipeline| + pipeline.variables.build(variables) + end.payload + end + + let(:config) do + <<-EOY + workflow: + name: '$PIPELINE_NAME $SOME_VARIABLE' + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + variables: + PIPELINE_NAME: 'Name 1' + - if: $CI_COMMIT_REF_NAME =~ /feature/ + variables: + PIPELINE_NAME: 'Name 2' + + job: + stage: test + script: echo 'hello' + EOY + end + + it 'substitutes variables in pipeline name' do + expect(response).not_to be_error + expect(pipeline).to be_persisted + expect(pipeline.name).to eq('Name 2 SOME_VAL') + end + end end end diff --git a/spec/services/ci/create_pipeline_service/variables_spec.rb b/spec/services/ci/create_pipeline_service/variables_spec.rb new file mode 100644 index 00000000000..e9e0cf2c6e0 --- /dev/null +++ b/spec/services/ci/create_pipeline_service/variables_spec.rb @@ -0,0 +1,136 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectness do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.first_owner } + + let(:service) { described_class.new(project, user, { ref: 'master' }) } + let(:pipeline) { service.execute(:push).payload } + + before do + stub_ci_pipeline_yaml_file(config) + end + + context 'when using variables' do + context 'when variables have expand: true/false' do + let(:config) do + <<-YAML + variables: + VAR7: + value: "value 7 $CI_PIPELINE_ID" + expand: false + VAR8: + value: "value 8 $CI_PIPELINE_ID" + expand: false + + rspec: + script: rspec + variables: + VAR1: "JOBID-$CI_JOB_ID" + VAR2: "PIPELINEID-$CI_PIPELINE_ID and $VAR1" + VAR3: + value: "PIPELINEID-$CI_PIPELINE_ID and $VAR1" + expand: false + VAR4: + value: "JOBID-$CI_JOB_ID" + expand: false + VAR5: "PIPELINEID-$CI_PIPELINE_ID and $VAR4" + VAR6: + value: "PIPELINEID-$CI_PIPELINE_ID and $VAR4" + expand: false + VAR7: "overridden value 7 $CI_PIPELINE_ID" + YAML + end + + let(:rspec) { find_job('rspec') } + + it 'creates the pipeline with a job that has variable expanded according to "expand"' do + expect(pipeline).to be_created_successfully + + expect(Ci::BuildRunnerPresenter.new(rspec).runner_variables).to include( + { key: 'VAR1', value: "JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR2', value: "PIPELINEID-#{pipeline.id} and JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR3', value: "PIPELINEID-$CI_PIPELINE_ID and $VAR1", public: true, masked: false, raw: true }, + { key: 'VAR4', value: "JOBID-$CI_JOB_ID", public: true, masked: false, raw: true }, + { key: 'VAR5', value: "PIPELINEID-#{pipeline.id} and $VAR4", public: true, masked: false }, + { key: 'VAR6', value: "PIPELINEID-$CI_PIPELINE_ID and $VAR4", public: true, masked: false, raw: true }, + { key: 'VAR7', value: "overridden value 7 #{pipeline.id}", public: true, masked: false }, + { key: 'VAR8', value: "value 8 $CI_PIPELINE_ID", public: true, masked: false, raw: true } + ) + end + + context 'when the FF ci_raw_variables_in_yaml_config is disabled' do + before do + stub_feature_flags(ci_raw_variables_in_yaml_config: false) + end + + it 'creates the pipeline with a job that has all variables expanded' do + expect(pipeline).to be_created_successfully + + expect(Ci::BuildRunnerPresenter.new(rspec).runner_variables).to include( + { key: 'VAR1', value: "JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR2', value: "PIPELINEID-#{pipeline.id} and JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR3', value: "PIPELINEID-#{pipeline.id} and JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR4', value: "JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR5', value: "PIPELINEID-#{pipeline.id} and JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR6', value: "PIPELINEID-#{pipeline.id} and JOBID-#{rspec.id}", public: true, masked: false }, + { key: 'VAR7', value: "overridden value 7 #{pipeline.id}", public: true, masked: false }, + { key: 'VAR8', value: "value 8 #{pipeline.id}", public: true, masked: false } + ) + end + end + end + + context 'when trigger variables have expand: true/false' do + let(:config) do + <<-YAML + child: + variables: + VAR1: "PROJECTID-$CI_PROJECT_ID" + VAR2: "PIPELINEID-$CI_PIPELINE_ID and $VAR1" + VAR3: + value: "PIPELINEID-$CI_PIPELINE_ID and $VAR1" + expand: false + trigger: + include: child.yml + YAML + end + + let(:child) { find_job('child') } + + it 'creates the pipeline with a trigger job that has downstream_variables expanded according to "expand"' do + expect(pipeline).to be_created_successfully + + expect(child.downstream_variables).to include( + { key: 'VAR1', value: "PROJECTID-#{project.id}" }, + { key: 'VAR2', value: "PIPELINEID-#{pipeline.id} and PROJECTID-$CI_PROJECT_ID" }, + { key: 'VAR3', value: "PIPELINEID-$CI_PIPELINE_ID and $VAR1", raw: true } + ) + end + + context 'when the FF ci_raw_variables_in_yaml_config is disabled' do + before do + stub_feature_flags(ci_raw_variables_in_yaml_config: false) + end + + it 'creates the pipeline with a job that has all variables expanded' do + expect(pipeline).to be_created_successfully + + expect(child.downstream_variables).to include( + { key: 'VAR1', value: "PROJECTID-#{project.id}" }, + { key: 'VAR2', value: "PIPELINEID-#{pipeline.id} and PROJECTID-$CI_PROJECT_ID" }, + { key: 'VAR3', value: "PIPELINEID-#{pipeline.id} and PROJECTID-$CI_PROJECT_ID" } + ) + end + end + end + end + + private + + def find_job(name) + pipeline.processables.find { |job| job.name == name } + end +end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 458692ba1c0..67c13649c6f 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -135,7 +135,7 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes execute_service expect(histogram).to have_received(:observe) - .with({ source: 'push' }, 5) + .with({ source: 'push', plan: project.actual_plan_name }, 5) end it 'tracks included template usage' do @@ -1867,49 +1867,4 @@ RSpec.describe Ci::CreatePipelineService, :yaml_processor_feature_flag_corectnes end end end - - describe '#execute!' do - subject { service.execute!(*args) } - - let(:service) { described_class.new(project, user, ref: ref_name) } - let(:args) { [:push] } - - context 'when user has a permission to create a pipeline' do - let(:user) { create(:user) } - - before do - project.add_developer(user) - end - - it 'does not raise an error' do - expect { subject }.not_to raise_error - end - - it 'creates a pipeline' do - expect { subject }.to change { Ci::Pipeline.count }.by(1) - end - end - - context 'when user does not have a permission to create a pipeline' do - let(:user) { create(:user) } - - it 'raises an error' do - expect { subject } - .to raise_error(described_class::CreateError) - .with_message('Insufficient permissions to create a new pipeline') - end - end - - context 'when a user with permissions has been blocked' do - before do - user.block! - end - - it 'raises an error' do - expect { subject } - .to raise_error(described_class::CreateError) - .with_message('Insufficient permissions to create a new pipeline') - end - end - end end diff --git a/spec/services/ci/job_artifacts/create_service_spec.rb b/spec/services/ci/job_artifacts/create_service_spec.rb index 030ba84951e..5df590a1b78 100644 --- a/spec/services/ci/job_artifacts/create_service_spec.rb +++ b/spec/services/ci/job_artifacts/create_service_spec.rb @@ -181,8 +181,8 @@ RSpec.describe Ci::JobArtifacts::CreateService do end end - context 'with job partitioning' do - let(:pipeline) { create(:ci_pipeline, project: project, partition_id: 123) } + context 'with job partitioning', :ci_partitionable do + let(:pipeline) { create(:ci_pipeline, project: project, partition_id: ci_testing_partition_id) } let(:job) { create(:ci_build, pipeline: pipeline) } it 'sets partition_id on artifacts' do @@ -190,7 +190,7 @@ RSpec.describe Ci::JobArtifacts::CreateService do artifacts_partitions = job.job_artifacts.map(&:partition_id).uniq - expect(artifacts_partitions).to eq([123]) + expect(artifacts_partitions).to eq([ci_testing_partition_id]) end end diff --git a/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb b/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb index 54d1cacc068..79920dcb2c7 100644 --- a/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb +++ b/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb @@ -60,10 +60,9 @@ RSpec.describe Ci::JobArtifacts::DestroyBatchService do execute end - it 'preserves trace artifacts and removes any timestamp' do + it 'preserves trace artifacts' do expect { subject } - .to change { trace_artifact.reload.expire_at }.from(trace_artifact.expire_at).to(nil) - .and not_change { Ci::JobArtifact.exists?(trace_artifact.id) } + .to not_change { Ci::JobArtifact.exists?(trace_artifact.id) } end context 'when artifact belongs to a project that is undergoing stats refresh' do @@ -277,81 +276,5 @@ RSpec.describe Ci::JobArtifacts::DestroyBatchService do is_expected.to eq(destroyed_artifacts_count: 0, statistics_updates: {}, status: :success) end end - - context 'with artifacts that has backfilled expire_at' do - let!(:created_on_00_30_45_minutes_on_21_22_23) do - [ - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-21 00:00:00.000')), - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-21 01:30:00.000')), - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-22 12:00:00.000')), - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-22 12:30:00.000')), - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-23 23:00:00.000')), - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-23 23:30:00.000')), - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-23 06:45:00.000')) - ] - end - - let!(:created_close_to_00_or_30_minutes) do - [ - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-21 00:00:00.001')), - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-21 00:30:00.999')) - ] - end - - let!(:created_on_00_or_30_minutes_on_other_dates) do - [ - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-01 00:00:00.000')), - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-19 12:00:00.000')), - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-24 23:30:00.000')) - ] - end - - let!(:created_at_other_times) do - [ - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-19 00:00:00.000')), - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-19 00:30:00.000')), - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-24 00:00:00.000')), - create(:ci_job_artifact, expire_at: Time.zone.parse('2022-01-24 00:30:00.000')) - ] - end - - let(:artifacts_to_keep) { created_on_00_30_45_minutes_on_21_22_23 } - let(:artifacts_to_delete) { created_close_to_00_or_30_minutes + created_on_00_or_30_minutes_on_other_dates + created_at_other_times } - let(:all_artifacts) { artifacts_to_keep + artifacts_to_delete } - - let(:artifacts) { Ci::JobArtifact.where(id: all_artifacts.map(&:id)) } - - it 'deletes job artifacts that do not have expire_at on 00, 30 or 45 minute of 21, 22, 23 of the month' do - expect { subject }.to change { Ci::JobArtifact.count }.by(artifacts_to_delete.size * -1) - end - - it 'keeps job artifacts that have expire_at on 00, 30 or 45 minute of 21, 22, 23 of the month' do - expect { subject }.not_to change { Ci::JobArtifact.where(id: artifacts_to_keep.map(&:id)).count } - end - - it 'removes expire_at on job artifacts that have expire_at on 00, 30 or 45 minute of 21, 22, 23 of the month' do - subject - - expect(artifacts_to_keep.all? { |artifact| artifact.reload.expire_at.nil? }).to be(true) - end - - context 'when feature flag is disabled' do - before do - stub_feature_flags(ci_detect_wrongly_expired_artifacts: false) - end - - it 'deletes all job artifacts' do - expect { subject }.to change { Ci::JobArtifact.count }.by(all_artifacts.size * -1) - end - end - - context 'when fix_expire_at is false' do - let(:service) { described_class.new(artifacts, pick_up_at: Time.current, fix_expire_at: false) } - - it 'deletes all job artifacts' do - expect { subject }.to change { Ci::JobArtifact.count }.by(all_artifacts.size * -1) - end - end - end end end diff --git a/spec/services/ci/job_artifacts/track_artifact_report_service_spec.rb b/spec/services/ci/job_artifacts/track_artifact_report_service_spec.rb index 6d9fc4c8e34..d4d56825e1f 100644 --- a/spec/services/ci/job_artifacts/track_artifact_report_service_spec.rb +++ b/spec/services/ci/job_artifacts/track_artifact_report_service_spec.rb @@ -9,7 +9,9 @@ RSpec.describe Ci::JobArtifacts::TrackArtifactReportService do let_it_be(:user1) { create(:user) } let_it_be(:user2) { create(:user) } - let(:test_event_name) { 'i_testing_test_report_uploaded' } + let(:test_event_name_1) { 'i_testing_test_report_uploaded' } + let(:test_event_name_2) { 'i_testing_coverage_report_uploaded' } + let(:counter) { Gitlab::UsageDataCounters::HLLRedisCounter } let(:start_time) { 1.week.ago } let(:end_time) { 1.week.from_now } @@ -25,15 +27,15 @@ RSpec.describe Ci::JobArtifacts::TrackArtifactReportService do end end - it 'tracks the event using HLLRedisCounter' do - allow(Gitlab::UsageDataCounters::HLLRedisCounter) + it 'tracks the test event using HLLRedisCounter' do + expect(Gitlab::UsageDataCounters::HLLRedisCounter) .to receive(:track_event) - .with(test_event_name, values: user1.id) + .with(test_event_name_1, values: user1.id) .and_call_original expect { track_artifact_report } .to change { - counter.unique_events(event_names: test_event_name, + counter.unique_events(event_names: test_event_name_1, start_date: start_time, end_date: end_time) } @@ -44,12 +46,20 @@ RSpec.describe Ci::JobArtifacts::TrackArtifactReportService do context 'when pipeline does not have test reports' do let_it_be(:pipeline) { create(:ci_empty_pipeline) } - it 'does not track the event' do + it 'does not track the test event' do track_artifact_report expect(Gitlab::UsageDataCounters::HLLRedisCounter) .not_to receive(:track_event) - .with(anything, test_event_name) + .with(anything, test_event_name_1) + end + + it 'does not track the coverage test event' do + track_artifact_report + + expect(Gitlab::UsageDataCounters::HLLRedisCounter) + .not_to receive(:track_event) + .with(anything, test_event_name_2) end end @@ -57,15 +67,15 @@ RSpec.describe Ci::JobArtifacts::TrackArtifactReportService do let_it_be(:pipeline1) { create(:ci_pipeline, :with_test_reports, project: project, user: user1) } let_it_be(:pipeline2) { create(:ci_pipeline, :with_test_reports, project: project, user: user1) } - it 'tracks all pipelines using HLLRedisCounter by one user_id' do - allow(Gitlab::UsageDataCounters::HLLRedisCounter) + it 'tracks all pipelines using HLLRedisCounter by one user_id for the test event' do + expect(Gitlab::UsageDataCounters::HLLRedisCounter) .to receive(:track_event) - .with(test_event_name, values: user1.id) + .with(test_event_name_1, values: user1.id) .and_call_original - allow(Gitlab::UsageDataCounters::HLLRedisCounter) + expect(Gitlab::UsageDataCounters::HLLRedisCounter) .to receive(:track_event) - .with(test_event_name, values: user1.id) + .with(test_event_name_1, values: user1.id) .and_call_original expect do @@ -73,7 +83,7 @@ RSpec.describe Ci::JobArtifacts::TrackArtifactReportService do described_class.new.execute(pipeline2) end .to change { - counter.unique_events(event_names: test_event_name, + counter.unique_events(event_names: test_event_name_1, start_date: start_time, end_date: end_time) } @@ -85,25 +95,92 @@ RSpec.describe Ci::JobArtifacts::TrackArtifactReportService do let_it_be(:pipeline1) { create(:ci_pipeline, :with_test_reports, project: project, user: user1) } let_it_be(:pipeline2) { create(:ci_pipeline, :with_test_reports, project: project, user: user2) } - it 'tracks all pipelines using HLLRedisCounter by multiple users' do - allow(Gitlab::UsageDataCounters::HLLRedisCounter) + it 'tracks all pipelines using HLLRedisCounter by multiple users for test reports' do + expect(Gitlab::UsageDataCounters::HLLRedisCounter) + .to receive(:track_event) + .with(test_event_name_1, values: user1.id) + .and_call_original + + expect(Gitlab::UsageDataCounters::HLLRedisCounter) + .to receive(:track_event) + .with(test_event_name_1, values: user2.id) + .and_call_original + + expect do + described_class.new.execute(pipeline1) + described_class.new.execute(pipeline2) + end + .to change { + counter.unique_events(event_names: test_event_name_1, + start_date: start_time, + end_date: end_time) + } + .by 2 + end + end + + context 'when pipeline has coverage test reports' do + let_it_be(:pipeline) { create(:ci_pipeline, project: project, user: user1) } + + before do + 2.times do + pipeline.builds << build(:ci_build, :coverage_reports, pipeline: pipeline, project: pipeline.project) + end + end + + it 'tracks the coverage test event using HLLRedisCounter' do + expect(Gitlab::UsageDataCounters::HLLRedisCounter) .to receive(:track_event) - .with(test_event_name, values: user1.id) + .with(test_event_name_2, values: user1.id) .and_call_original - allow(Gitlab::UsageDataCounters::HLLRedisCounter) + expect { track_artifact_report } + .to change { + counter.unique_events(event_names: test_event_name_2, + start_date: start_time, + end_date: end_time) + } + .by 1 + end + end + + context 'when a single user started multiple pipelines with coverage reports' do + let_it_be(:pipeline1) { create(:ci_pipeline, :with_coverage_reports, project: project, user: user1) } + let_it_be(:pipeline2) { create(:ci_pipeline, :with_coverage_reports, project: project, user: user1) } + + it 'tracks all pipelines using HLLRedisCounter by one user_id for the coverage test event' do + expect(Gitlab::UsageDataCounters::HLLRedisCounter) .to receive(:track_event) - .with(test_event_name, values: user1.id) + .with(test_event_name_2, values: user1.id) + .twice .and_call_original - allow(Gitlab::UsageDataCounters::HLLRedisCounter) - .to receive(:track_event) - .with(test_event_name, values: user2.id) - .and_call_original + expect do + described_class.new.execute(pipeline1) + described_class.new.execute(pipeline2) + end + .to change { + counter.unique_events(event_names: test_event_name_2, + start_date: start_time, + end_date: end_time) + } + .by 1 + end + end + + context 'when multiple users started multiple pipelines with coverage test reports' do + let_it_be(:pipeline1) { create(:ci_pipeline, :with_coverage_reports, project: project, user: user1) } + let_it_be(:pipeline2) { create(:ci_pipeline, :with_coverage_reports, project: project, user: user2) } + + it 'tracks all pipelines using HLLRedisCounter by multiple users for coverage test reports' do + expect(Gitlab::UsageDataCounters::HLLRedisCounter) + .to receive(:track_event) + .with(test_event_name_2, values: user1.id) + .and_call_original - allow(Gitlab::UsageDataCounters::HLLRedisCounter) + expect(Gitlab::UsageDataCounters::HLLRedisCounter) .to receive(:track_event) - .with(test_event_name, values: user2.id) + .with(test_event_name_2, values: user2.id) .and_call_original expect do @@ -111,7 +188,7 @@ RSpec.describe Ci::JobArtifacts::TrackArtifactReportService do described_class.new.execute(pipeline2) end .to change { - counter.unique_events(event_names: test_event_name, + counter.unique_events(event_names: test_event_name_2, start_date: start_time, end_date: end_time) } diff --git a/spec/services/ci/pipeline_processing/atomic_processing_service/status_collection_spec.rb b/spec/services/ci/pipeline_processing/atomic_processing_service/status_collection_spec.rb index 7578afa7c50..d0aa1ba4c6c 100644 --- a/spec/services/ci/pipeline_processing/atomic_processing_service/status_collection_spec.rb +++ b/spec/services/ci/pipeline_processing/atomic_processing_service/status_collection_spec.rb @@ -104,7 +104,7 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService::StatusCollection describe '#processing_processables' do it 'returns processables marked as processing' do - expect(collection.processing_processables.map { |processable| processable[:id] } ) + expect(collection.processing_processables.map { |processable| processable[:id] }) .to contain_exactly(build_a.id, build_b.id, test_a.id, test_b.id, deploy.id) end end diff --git a/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb b/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb index 06bb6d39fe5..1fbefc1fa22 100644 --- a/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb +++ b/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb @@ -25,7 +25,7 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do check_expectation(test_file.dig('init', 'expect'), "init") test_file['transitions'].each_with_index do |transition, idx| - event_on_jobs(transition['event'], transition['jobs']) + process_events(transition) Sidekiq::Worker.drain_all # ensure that all async jobs are executed check_expectation(transition['expect'], "transition:#{idx}") end @@ -48,20 +48,37 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do } end + def process_events(transition) + if transition['jobs'] + event_on_jobs(transition['event'], transition['jobs']) + else + event_on_pipeline(transition['event']) + end + end + def event_on_jobs(event, job_names) statuses = pipeline.latest_statuses.by_name(job_names).to_a expect(statuses.count).to eq(job_names.count) # ensure that we have the same counts statuses.each do |status| - if event == 'play' + case event + when 'play' status.play(user) - elsif event == 'retry' + when 'retry' ::Ci::RetryJobService.new(project, user).execute(status) else status.public_send("#{event}!") end end end + + def event_on_pipeline(event) + if event == 'retry' + pipeline.retry_failed(user) + else + pipeline.public_send("#{event}!") + end + end end end diff --git a/spec/services/ci/pipeline_processing/test_cases/stage_one_test_succeeds_one_manual_test_fails_and_retry_manual_build.yml b/spec/services/ci/pipeline_processing/test_cases/stage_one_test_succeeds_one_manual_test_fails_and_retry_manual_build.yml new file mode 100644 index 00000000000..a50fe56f8d4 --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/stage_one_test_succeeds_one_manual_test_fails_and_retry_manual_build.yml @@ -0,0 +1,54 @@ +config: + test1: + script: exit 0 + + test2: + when: manual + script: exit 1 + +init: + expect: + pipeline: pending + stages: + test: pending + jobs: + test1: pending + test2: manual + +transitions: + - event: success + jobs: [test1] + expect: + pipeline: success + stages: + test: success + jobs: + test1: success + test2: manual + - event: play + jobs: [test2] + expect: + pipeline: running + stages: + test: running + jobs: + test1: success + test2: pending + - event: drop + jobs: [test2] + expect: + pipeline: success + stages: + test: success + jobs: + test1: success + test2: failed + - event: retry + jobs: [test2] + expect: + pipeline: running + stages: + test: running + jobs: + test1: success + test2: pending diff --git a/spec/services/ci/pipeline_processing/test_cases/stage_one_test_succeeds_one_manual_test_fails_and_retry_pipeline.yml b/spec/services/ci/pipeline_processing/test_cases/stage_one_test_succeeds_one_manual_test_fails_and_retry_pipeline.yml new file mode 100644 index 00000000000..a6112a95a12 --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/stage_one_test_succeeds_one_manual_test_fails_and_retry_pipeline.yml @@ -0,0 +1,53 @@ +config: + test1: + script: exit 0 + + test2: + when: manual + script: exit 1 + +init: + expect: + pipeline: pending + stages: + test: pending + jobs: + test1: pending + test2: manual + +transitions: + - event: success + jobs: [test1] + expect: + pipeline: success + stages: + test: success + jobs: + test1: success + test2: manual + - event: play + jobs: [test2] + expect: + pipeline: running + stages: + test: running + jobs: + test1: success + test2: pending + - event: drop + jobs: [test2] + expect: + pipeline: success + stages: + test: success + jobs: + test1: success + test2: failed + - event: retry + expect: + pipeline: success + stages: + test: success + jobs: + test1: success + test2: manual diff --git a/spec/services/ci/pipeline_schedules/take_ownership_service_spec.rb b/spec/services/ci/pipeline_schedules/take_ownership_service_spec.rb new file mode 100644 index 00000000000..9a3aad20d89 --- /dev/null +++ b/spec/services/ci/pipeline_schedules/take_ownership_service_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::PipelineSchedules::TakeOwnershipService do + let_it_be(:user) { create(:user) } + let_it_be(:owner) { create(:user) } + let_it_be(:reporter) { create(:user) } + let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project, owner: owner) } + + before_all do + project.add_maintainer(user) + project.add_maintainer(owner) + project.add_reporter(reporter) + end + + describe '#execute' do + context 'when user does not have permission' do + subject(:service) { described_class.new(pipeline_schedule, reporter) } + + it 'returns ServiceResponse.error' do + result = service.execute + + expect(result).to be_a(ServiceResponse) + expect(result.error?).to be(true) + expect(result.message).to eq(_('Failed to change the owner')) + end + end + + context 'when user has permission' do + subject(:service) { described_class.new(pipeline_schedule, user) } + + it 'returns ServiceResponse.success' do + result = service.execute + + expect(result).to be_a(ServiceResponse) + expect(result.success?).to be(true) + expect(result.payload).to eq(pipeline_schedule) + end + + context 'when schedule update fails' do + subject(:service) { described_class.new(pipeline_schedule, owner) } + + before do + allow(pipeline_schedule).to receive(:update).and_return(false) + + errors = ActiveModel::Errors.new(pipeline_schedule) + errors.add(:base, 'An error occurred') + allow(pipeline_schedule).to receive(:errors).and_return(errors) + end + + it 'returns ServiceResponse.error' do + result = service.execute + + expect(result).to be_a(ServiceResponse) + expect(result.error?).to be(true) + expect(result.message).to eq(['An error occurred']) + end + end + end + end +end diff --git a/spec/services/ci/play_build_service_spec.rb b/spec/services/ci/play_build_service_spec.rb index 85ef8b60af4..fc07801b672 100644 --- a/spec/services/ci/play_build_service_spec.rb +++ b/spec/services/ci/play_build_service_spec.rb @@ -87,11 +87,15 @@ RSpec.describe Ci::PlayBuildService, '#execute' do expect(build.reload.job_variables.map(&:key)).to contain_exactly('first', 'second') end - context 'when variables are invalid' do + context 'and variables are invalid' do let(:job_variables) { [{}] } - it 'raises an error' do - expect { subject }.to raise_error(ActiveRecord::RecordInvalid) + it 'resets the attributes of the build' do + build.update!(job_variables_attributes: [{ key: 'old', value: 'old variable' }]) + + subject + + expect(build.job_variables.map(&:key)).to contain_exactly('old') end end diff --git a/spec/services/ci/process_build_service_spec.rb b/spec/services/ci/process_build_service_spec.rb index 2fcb4ce73ff..9301098b083 100644 --- a/spec/services/ci/process_build_service_spec.rb +++ b/spec/services/ci/process_build_service_spec.rb @@ -2,147 +2,118 @@ require 'spec_helper' RSpec.describe Ci::ProcessBuildService, '#execute' do - let(:user) { create(:user) } - let(:project) { create(:project) } + using RSpec::Parameterized::TableSyntax + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:pipeline) { create(:ci_pipeline, ref: 'master', project: project) } subject { described_class.new(project, user).execute(build, current_status) } - before do + before_all do project.add_maintainer(user) end - context 'when build has on_success option' do - let(:build) { create(:ci_build, :created, when: :on_success, user: user, project: project) } - - context 'when current status is success' do - let(:current_status) { 'success' } - - it 'changes the build status' do - expect { subject }.to change { build.status }.to('pending') - end - end - - context 'when current status is skipped' do - let(:current_status) { 'skipped' } - - it 'changes the build status' do - expect { subject }.to change { build.status }.to('pending') - end - end - - context 'when current status is failed' do - let(:current_status) { 'failed' } - - it 'does not change the build status' do - expect { subject }.to change { build.status }.to('skipped') - end + shared_context 'with enqueue_immediately set' do + before do + build.set_enqueue_immediately! end end - context 'when build has on_failure option' do - let(:build) { create(:ci_build, :created, when: :on_failure, user: user, project: project) } - - context 'when current status is success' do - let(:current_status) { 'success' } - - it 'changes the build status' do - expect { subject }.to change { build.status }.to('skipped') - end - end - - context 'when current status is failed' do - let(:current_status) { 'failed' } - - it 'does not change the build status' do - expect { subject }.to change { build.status }.to('pending') - end + shared_context 'with ci_retry_job_fix disabled' do + before do + stub_feature_flags(ci_retry_job_fix: false) end end - context 'when build has always option' do - let(:build) { create(:ci_build, :created, when: :always, user: user, project: project) } - - context 'when current status is success' do - let(:current_status) { 'success' } - - it 'changes the build status' do - expect { subject }.to change { build.status }.to('pending') - end + context 'for single build' do + let!(:build) { create(:ci_build, *[trait].compact, :created, **conditions, pipeline: pipeline) } + + where(:trait, :conditions, :current_status, :after_status, :retry_after_status, :retry_disabled_after_status) do + nil | { when: :on_success } | 'success' | 'pending' | 'pending' | 'pending' + nil | { when: :on_success } | 'skipped' | 'pending' | 'pending' | 'pending' + nil | { when: :on_success } | 'failed' | 'skipped' | 'skipped' | 'skipped' + nil | { when: :on_failure } | 'success' | 'skipped' | 'skipped' | 'skipped' + nil | { when: :on_failure } | 'skipped' | 'skipped' | 'skipped' | 'skipped' + nil | { when: :on_failure } | 'failed' | 'pending' | 'pending' | 'pending' + nil | { when: :always } | 'success' | 'pending' | 'pending' | 'pending' + nil | { when: :always } | 'skipped' | 'pending' | 'pending' | 'pending' + nil | { when: :always } | 'failed' | 'pending' | 'pending' | 'pending' + :actionable | { when: :manual } | 'success' | 'manual' | 'pending' | 'manual' + :actionable | { when: :manual } | 'skipped' | 'manual' | 'pending' | 'manual' + :actionable | { when: :manual } | 'failed' | 'skipped' | 'skipped' | 'skipped' + :schedulable | { when: :delayed } | 'success' | 'scheduled' | 'pending' | 'scheduled' + :schedulable | { when: :delayed } | 'skipped' | 'scheduled' | 'pending' | 'scheduled' + :schedulable | { when: :delayed } | 'failed' | 'skipped' | 'skipped' | 'skipped' end - context 'when current status is failed' do - let(:current_status) { 'failed' } - - it 'does not change the build status' do - expect { subject }.to change { build.status }.to('pending') + with_them do + it 'updates the job status to after_status' do + expect { subject }.to change { build.status }.to(after_status) end - end - end - - context 'when build has manual option' do - let(:build) { create(:ci_build, :created, :actionable, user: user, project: project) } - context 'when current status is success' do - let(:current_status) { 'success' } + context 'when build is set to enqueue immediately' do + include_context 'with enqueue_immediately set' - it 'changes the build status' do - expect { subject }.to change { build.status }.to('manual') - end - end + it 'updates the job status to retry_after_status' do + expect { subject }.to change { build.status }.to(retry_after_status) + end - context 'when current status is failed' do - let(:current_status) { 'failed' } + context 'when feature flag ci_retry_job_fix is disabled' do + include_context 'with ci_retry_job_fix disabled' - it 'does not change the build status' do - expect { subject }.to change { build.status }.to('skipped') + it "updates the job status to retry_disabled_after_status" do + expect { subject }.to change { build.status }.to(retry_disabled_after_status) + end + end end end end - context 'when build has delayed option' do - before do - allow(Ci::BuildScheduleWorker).to receive(:perform_at) {} + context 'when build is scheduled with DAG' do + let!(:build) do + create( + :ci_build, + *[trait].compact, + :dependent, + :created, + when: build_when, + pipeline: pipeline, + needed: other_build + ) end - let(:build) { create(:ci_build, :created, :schedulable, user: user, project: project) } - - context 'when current status is success' do - let(:current_status) { 'success' } + let!(:other_build) { create(:ci_build, :created, when: :on_success, pipeline: pipeline) } - it 'changes the build status' do - expect { subject }.to change { build.status }.to('scheduled') - end + where(:trait, :build_when, :current_status, :after_status, :retry_after_status, :retry_disabled_after_status) do + nil | :on_success | 'success' | 'pending' | 'pending' | 'pending' + nil | :on_success | 'skipped' | 'skipped' | 'skipped' | 'skipped' + nil | :manual | 'success' | 'manual' | 'pending' | 'manual' + nil | :manual | 'skipped' | 'skipped' | 'skipped' | 'skipped' + nil | :delayed | 'success' | 'manual' | 'pending' | 'manual' + nil | :delayed | 'skipped' | 'skipped' | 'skipped' | 'skipped' + :schedulable | :delayed | 'success' | 'scheduled' | 'pending' | 'scheduled' + :schedulable | :delayed | 'skipped' | 'skipped' | 'skipped' | 'skipped' end - context 'when current status is failed' do - let(:current_status) { 'failed' } - - it 'does not change the build status' do - expect { subject }.to change { build.status }.to('skipped') + with_them do + it 'updates the job status to after_status' do + expect { subject }.to change { build.status }.to(after_status) end - end - end - context 'when build is scheduled with DAG' do - using RSpec::Parameterized::TableSyntax + context 'when build is set to enqueue immediately' do + include_context 'with enqueue_immediately set' - let(:pipeline) { create(:ci_pipeline, ref: 'master', project: project) } - let!(:build) { create(:ci_build, :created, when: build_when, pipeline: pipeline, scheduling_type: :dag) } - let!(:other_build) { create(:ci_build, :created, when: :on_success, pipeline: pipeline) } - let!(:build_on_other_build) { create(:ci_build_need, build: build, name: other_build.name) } - - where(:build_when, :current_status, :after_status) do - :on_success | 'success' | 'pending' - :on_success | 'skipped' | 'skipped' - :manual | 'success' | 'manual' - :manual | 'skipped' | 'skipped' - :delayed | 'success' | 'manual' - :delayed | 'skipped' | 'skipped' - end + it 'updates the job status to retry_after_status' do + expect { subject }.to change { build.status }.to(retry_after_status) + end - with_them do - it 'proceeds the build' do - expect { subject }.to change { build.status }.to(after_status) + context 'when feature flag ci_retry_job_fix is disabled' do + include_context 'with ci_retry_job_fix disabled' + + it "updates the job status to retry_disabled_after_status" do + expect { subject }.to change { build.status }.to(retry_disabled_after_status) + 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 e2e760b9812..f40f5cc5a62 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -14,25 +14,29 @@ module Ci let!(:pending_job) { create(:ci_build, :pending, :queued, pipeline: pipeline) } describe '#execute' do - context 'checks database loadbalancing stickiness' do - subject { described_class.new(shared_runner).execute } + subject { described_class.new(shared_runner).execute } + context 'checks database loadbalancing stickiness' do before do project.update!(shared_runners_enabled: false) end - it 'result is valid if replica did caught-up' do + it 'result is valid if replica did caught-up', :aggregate_failures do expect(ApplicationRecord.sticking).to receive(:all_caught_up?) .with(:runner, shared_runner.id) { true } expect(subject).to be_valid + expect(subject.build).to be_nil + expect(subject.build_json).to be_nil end - it 'result is invalid if replica did not caught-up' do + it 'result is invalid if replica did not caught-up', :aggregate_failures do expect(ApplicationRecord.sticking).to receive(:all_caught_up?) .with(:runner, shared_runner.id) { false } expect(subject).not_to be_valid + expect(subject.build).to be_nil + expect(subject.build_json).to be_nil end end @@ -954,6 +958,7 @@ module Ci expect(result).not_to be_valid expect(result.build).to be_nil + expect(result.build_json).to be_nil end end diff --git a/spec/services/ci/retry_job_service_spec.rb b/spec/services/ci/retry_job_service_spec.rb index 69f19c5acf2..540e700efa6 100644 --- a/spec/services/ci/retry_job_service_spec.rb +++ b/spec/services/ci/retry_job_service_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' RSpec.describe Ci::RetryJobService do + using RSpec::Parameterized::TableSyntax let_it_be(:reporter) { create(:user) } let_it_be(:developer) { create(:user) } let_it_be(:project) { create(:project, :repository) } @@ -11,11 +12,11 @@ RSpec.describe Ci::RetryJobService do end let_it_be(:stage) do - create(:ci_stage, project: project, - pipeline: pipeline, - name: 'test') + create(:ci_stage, pipeline: pipeline, name: 'test') end + let_it_be(:deploy_stage) { create(:ci_stage, pipeline: pipeline, name: 'deploy', position: stage.position + 1) } + let(:job_variables_attributes) { [{ key: 'MANUAL_VAR', value: 'manual test var' }] } let(:user) { developer } @@ -26,24 +27,11 @@ RSpec.describe Ci::RetryJobService do project.add_reporter(reporter) end - shared_context 'retryable bridge' do - let_it_be(:downstream_project) { create(:project, :repository) } - - let_it_be_with_refind(:job) do - create(:ci_bridge, :success, - pipeline: pipeline, downstream: downstream_project, description: 'a trigger job', ci_stage: stage - ) - end - - let_it_be(:job_to_clone) { job } - - before do - job.update!(retried: false) + shared_context 'retryable build' do + let_it_be_with_reload(:job) do + create(:ci_build, :success, pipeline: pipeline, ci_stage: stage) end - end - shared_context 'retryable build' do - let_it_be_with_refind(:job) { create(:ci_build, :success, pipeline: pipeline, ci_stage: stage) } let_it_be(:another_pipeline) { create(:ci_empty_pipeline, project: project) } let_it_be(:job_to_clone) do @@ -60,6 +48,12 @@ RSpec.describe Ci::RetryJobService do end end + shared_context 'with ci_retry_job_fix disabled' do + before do + stub_feature_flags(ci_retry_job_fix: false) + end + end + shared_examples_for 'clones the job' do let(:job) { job_to_clone } @@ -87,8 +81,7 @@ RSpec.describe Ci::RetryJobService do context 'when the job has needs' do before do - create(:ci_build_need, build: job, name: 'build1') - create(:ci_build_need, build: job, name: 'build2') + create_list(:ci_build_need, 2, build: job) end it 'bulk inserts all the needs' do @@ -123,16 +116,12 @@ RSpec.describe Ci::RetryJobService do end context 'when there are subsequent processables that are skipped' do - let_it_be(:stage) { create(:ci_stage, pipeline: pipeline, name: 'deploy') } - let!(:subsequent_build) do - create(:ci_build, :skipped, stage_idx: 2, - pipeline: pipeline, - ci_stage: stage) + create(:ci_build, :skipped, pipeline: pipeline, ci_stage: deploy_stage) end let!(:subsequent_bridge) do - create(:ci_bridge, :skipped, stage_idx: 2, pipeline: pipeline, ci_stage: stage) + create(:ci_bridge, :skipped, pipeline: pipeline, ci_stage: deploy_stage) end it 'resumes pipeline processing in the subsequent stage' do @@ -152,10 +141,9 @@ RSpec.describe Ci::RetryJobService do end context 'when the pipeline has other jobs' do - let!(:stage2) { create(:ci_stage, project: project, pipeline: pipeline, name: 'deploy') } - let!(:build2) { create(:ci_build, pipeline: pipeline, ci_stage: stage ) } - let!(:deploy) { create(:ci_build, pipeline: pipeline, ci_stage: stage2) } - let!(:deploy_needs_build2) { create(:ci_build_need, build: deploy, name: build2.name) } + let!(:other_test_build) { create(:ci_build, pipeline: pipeline, ci_stage: stage) } + let!(:deploy) { create(:ci_build, pipeline: pipeline, ci_stage: deploy_stage) } + let!(:deploy_needs_build2) { create(:ci_build_need, build: deploy, name: other_test_build.name) } context 'when job has a nil scheduling_type' do before do @@ -166,7 +154,7 @@ RSpec.describe Ci::RetryJobService do it 'populates scheduling_type of processables' do expect(new_job.scheduling_type).to eq('stage') expect(job.reload.scheduling_type).to eq('stage') - expect(build2.reload.scheduling_type).to eq('stage') + expect(other_test_build.reload.scheduling_type).to eq('stage') expect(deploy.reload.scheduling_type).to eq('dag') end end @@ -193,6 +181,13 @@ RSpec.describe Ci::RetryJobService do end end + shared_examples_for 'checks enqueue_immediately?' do + it "returns enqueue_immediately" do + subject + expect(new_job.enqueue_immediately?).to eq enqueue_immediately + end + end + describe '#clone!' do let(:new_job) { service.clone!(job) } @@ -200,20 +195,6 @@ RSpec.describe Ci::RetryJobService do expect { service.clone!(create(:ci_build).present) }.to raise_error(TypeError) end - context 'when the job to be cloned is a bridge' do - include_context 'retryable bridge' - - it_behaves_like 'clones the job' - - context 'when given variables' do - let(:new_job) { service.clone!(job, variables: job_variables_attributes) } - - it 'does not give variables to the new bridge' do - expect { new_job }.not_to raise_error - end - end - end - context 'when the job to be cloned is a build' do include_context 'retryable build' @@ -224,7 +205,7 @@ RSpec.describe Ci::RetryJobService do context 'when a build with a deployment is retried' do let!(:job) do create(:ci_build, :with_deployment, :deploy_to_production, - pipeline: pipeline, ci_stage: stage, project: project) + pipeline: pipeline, ci_stage: stage) end it 'creates a new deployment' do @@ -247,7 +228,6 @@ RSpec.describe Ci::RetryJobService do options: { environment: { name: environment_name } }, pipeline: pipeline, ci_stage: stage, - project: project, user: other_developer) end @@ -282,24 +262,44 @@ RSpec.describe Ci::RetryJobService do end end end - end - describe '#execute' do - let(:new_job) { service.execute(job)[:job] } + context 'when enqueue_if_actionable is provided' do + let!(:job) do + create(:ci_build, *[trait].compact, :failed, pipeline: pipeline, ci_stage: stage) + end - context 'when the job to be retried is a bridge' do - include_context 'retryable bridge' + let(:new_job) { subject } - it_behaves_like 'retries the job' + subject { service.clone!(job, enqueue_if_actionable: enqueue_if_actionable) } - context 'when given variables' do - let(:new_job) { service.clone!(job, variables: job_variables_attributes) } + where(:enqueue_if_actionable, :trait, :enqueue_immediately) do + true | nil | false + true | :manual | true + true | :expired_scheduled | true + + false | nil | false + false | :manual | false + false | :expired_scheduled | false + end + + with_them do + it_behaves_like 'checks enqueue_immediately?' - it 'does not give variables to the new bridge' do - expect { new_job }.not_to raise_error + context 'when feature flag is disabled' do + include_context 'with ci_retry_job_fix disabled' + + it_behaves_like 'checks enqueue_immediately?' do + let(:enqueue_immediately) { false } + end end end end + end + + describe '#execute' do + let(:new_job) { subject[:job] } + + subject { service.execute(job) } context 'when the job to be retried is a build' do include_context 'retryable build' @@ -307,24 +307,18 @@ RSpec.describe Ci::RetryJobService do it_behaves_like 'retries the job' context 'when there are subsequent jobs that are skipped' do - let_it_be(:stage) { create(:ci_stage, pipeline: pipeline, name: 'deploy') } - let!(:subsequent_build) do - create(:ci_build, :skipped, stage_idx: 2, - pipeline: pipeline, - stage_id: stage.id) + create(:ci_build, :skipped, pipeline: pipeline, ci_stage: deploy_stage) end let!(:subsequent_bridge) do - create(:ci_bridge, :skipped, stage_idx: 2, - pipeline: pipeline, - stage_id: stage.id) + create(:ci_bridge, :skipped, pipeline: pipeline, ci_stage: deploy_stage) end it 'does not cause an N+1 when updating the job ownership' do control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { service.execute(job) }.count - create_list(:ci_build, 2, :skipped, stage_idx: job.stage_idx + 1, pipeline: pipeline, stage_id: stage.id) + create_list(:ci_build, 2, :skipped, pipeline: pipeline, ci_stage: deploy_stage) expect { service.execute(job) }.not_to exceed_all_query_limit(control_count) end @@ -352,5 +346,161 @@ RSpec.describe Ci::RetryJobService do end end end + + context 'when job being retried has jobs in previous stages' do + let!(:job) do + create( + :ci_build, + :failed, + name: 'deploy_a', + pipeline: pipeline, + ci_stage: deploy_stage + ) + end + + before do + create( + :ci_build, + previous_stage_job_status, + name: 'test_a', + pipeline: pipeline, + ci_stage: stage + ) + end + + where(:previous_stage_job_status, :after_status) do + :created | 'created' + :pending | 'created' + :running | 'created' + :manual | 'created' + :scheduled | 'created' + :success | 'pending' + :failed | 'skipped' + :skipped | 'pending' + end + + with_them do + it 'updates the new job status to after_status' do + expect(subject).to be_success + expect(new_job.status).to eq after_status + end + + context 'when feature flag is disabled' do + include_context 'with ci_retry_job_fix disabled' + + it 'enqueues the new job' do + expect(subject).to be_success + expect(new_job).to be_pending + end + end + end + end + + context 'when job being retried has DAG dependencies' do + let!(:job) do + create( + :ci_build, + :failed, + :dependent, + name: 'deploy_a', + pipeline: pipeline, + ci_stage: deploy_stage, + needed: dependency + ) + end + + let(:dependency) do + create( + :ci_build, + dag_dependency_status, + name: 'test_a', + pipeline: pipeline, + ci_stage: stage + ) + end + + where(:dag_dependency_status, :after_status) do + :created | 'created' + :pending | 'created' + :running | 'created' + :manual | 'created' + :scheduled | 'created' + :success | 'pending' + :failed | 'skipped' + :skipped | 'skipped' + end + + with_them do + it 'updates the new job status to after_status' do + expect(subject).to be_success + expect(new_job.status).to eq after_status + end + + context 'when feature flag is disabled' do + include_context 'with ci_retry_job_fix disabled' + + it 'enqueues the new job' do + expect(subject).to be_success + expect(new_job).to be_pending + end + end + end + end + + context 'when there are other manual/scheduled jobs' do + let_it_be(:test_manual_build) do + create(:ci_build, :manual, pipeline: pipeline, ci_stage: stage) + end + + let_it_be(:subsequent_manual_build) do + create(:ci_build, :manual, pipeline: pipeline, ci_stage: deploy_stage) + end + + let_it_be(:test_scheduled_build) do + create(:ci_build, :scheduled, pipeline: pipeline, ci_stage: stage) + end + + let_it_be(:subsequent_scheduled_build) do + create(:ci_build, :scheduled, pipeline: pipeline, ci_stage: deploy_stage) + end + + let!(:job) do + create(:ci_build, *[trait].compact, :failed, pipeline: pipeline, ci_stage: stage) + end + + where(:trait, :enqueue_immediately) do + nil | false + :manual | true + :expired_scheduled | true + end + + with_them do + it 'retries the given job but not the other manual/scheduled jobs' do + expect { subject } + .to change { Ci::Build.count }.by(1) + .and not_change { test_manual_build.reload.status } + .and not_change { subsequent_manual_build.reload.status } + .and not_change { test_scheduled_build.reload.status } + .and not_change { subsequent_scheduled_build.reload.status } + + expect(new_job).to be_pending + end + + it_behaves_like 'checks enqueue_immediately?' + + context 'when feature flag is disabled' do + include_context 'with ci_retry_job_fix disabled' + + it 'enqueues the new job' do + expect(subject).to be_success + expect(new_job).to be_pending + end + + it_behaves_like 'checks enqueue_immediately?' do + let(:enqueue_immediately) { false } + end + end + end + end end end diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index 96437290ae3..77345096537 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -5,14 +5,16 @@ require 'spec_helper' RSpec.describe Ci::RetryPipelineService, '#execute' do include ProjectForksHelper - let(:user) { create(:user) } - let(:project) { create(:project) } + let_it_be_with_refind(:user) { create(:user) } + let_it_be_with_refind(:project) { create(:project) } + let(:pipeline) { create(:ci_pipeline, project: project) } - let(:service) { described_class.new(project, user) } let(:build_stage) { create(:ci_stage, name: 'build', position: 0, pipeline: pipeline) } let(:test_stage) { create(:ci_stage, name: 'test', position: 1, pipeline: pipeline) } let(:deploy_stage) { create(:ci_stage, name: 'deploy', position: 2, pipeline: pipeline) } + subject(:service) { described_class.new(project, user) } + context 'when user has full ability to modify pipeline' do before do project.add_developer(user) @@ -272,6 +274,21 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do expect(pipeline.reload).to be_running end end + + context 'when there is a failed manual action' do + before do + create_build('rspec', :success, build_stage) + create_build('manual-rspec', :failed, build_stage, when: :manual, allow_failure: true) + end + + it 'processes the manual action' do + service.execute(pipeline) + + expect(build('rspec')).to be_success + expect(build('manual-rspec')).to be_manual + expect(pipeline.reload).to be_success + end + end end it 'closes all todos about failed jobs for pipeline' do diff --git a/spec/services/ci/runners/bulk_delete_runners_service_spec.rb b/spec/services/ci/runners/bulk_delete_runners_service_spec.rb index 8e9fc4e3012..fa8af1100df 100644 --- a/spec/services/ci/runners/bulk_delete_runners_service_spec.rb +++ b/spec/services/ci/runners/bulk_delete_runners_service_spec.rb @@ -5,78 +5,180 @@ require 'spec_helper' RSpec.describe ::Ci::Runners::BulkDeleteRunnersService, '#execute' do subject(:execute) { described_class.new(**service_args).execute } - let(:service_args) { { runners: runners_arg } } + let_it_be(:admin_user) { create(:user, :admin) } + let_it_be_with_refind(:owner_user) { create(:user) } # discard memoized ci_owned_runners + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + + let(:user) {} + let(:service_args) { { runners: runners_arg, current_user: user } } let(:runners_arg) {} context 'with runners specified' do let!(:instance_runner) { create(:ci_runner) } - let!(:group_runner) { create(:ci_runner, :group) } - let!(:project_runner) { create(:ci_runner, :project) } + let!(:group_runner) { create(:ci_runner, :group, groups: [group]) } + let!(:project_runner) { create(:ci_runner, :project, projects: [project]) } shared_examples 'a service deleting runners in bulk' do + let!(:expected_deleted_ids) { expected_deleted_runners.map(&:id) } + it 'destroys runners', :aggregate_failures do - expect { subject }.to change { Ci::Runner.count }.by(-2) + expect { execute }.to change { Ci::Runner.count }.by(-expected_deleted_ids.count) is_expected.to be_success - expect(execute.payload).to eq({ deleted_count: 2, deleted_ids: [instance_runner.id, project_runner.id] }) - expect(instance_runner[:errors]).to be_nil - expect(project_runner[:errors]).to be_nil + expect(execute.payload).to eq( + { + deleted_count: expected_deleted_ids.count, + deleted_ids: expected_deleted_ids, + errors: [] + }) expect { project_runner.runner_projects.first.reload }.to raise_error(ActiveRecord::RecordNotFound) - expect { group_runner.reload }.not_to raise_error - expect { instance_runner.reload }.to raise_error(ActiveRecord::RecordNotFound) - expect { project_runner.reload }.to raise_error(ActiveRecord::RecordNotFound) + expected_deleted_runners.each do |deleted_runner| + expect(deleted_runner[:errors]).to be_nil + expect { deleted_runner.reload }.to raise_error(ActiveRecord::RecordNotFound) + end end - context 'with some runners already deleted' do + context 'with too many runners specified' do before do - instance_runner.destroy! + stub_const("#{described_class}::RUNNER_LIMIT", 1) end - let(:runners_arg) { [instance_runner.id, project_runner.id] } - - it 'destroys runners and returns only deleted runners', :aggregate_failures do - expect { subject }.to change { Ci::Runner.count }.by(-1) + it 'deletes only first RUNNER_LIMIT runners', :aggregate_failures do + expect { execute }.to change { Ci::Runner.count }.by(-1) is_expected.to be_success - expect(execute.payload).to eq({ deleted_count: 1, deleted_ids: [project_runner.id] }) - expect(instance_runner[:errors]).to be_nil - expect(project_runner[:errors]).to be_nil - expect { project_runner.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect(execute.payload).to eq( + { + deleted_count: 1, + deleted_ids: expected_deleted_ids.take(1), + errors: ["Can only delete up to 1 runners per call. Ignored the remaining runner(s)."] + }) end end + end - context 'with too many runners specified' do + context 'when the user cannot delete runners' do + let(:runners_arg) { Ci::Runner.all } + + context 'when user is not group owner' do before do - stub_const("#{described_class}::RUNNER_LIMIT", 1) + group.add_developer(user) end - it 'deletes only first RUNNER_LIMIT runners' do - expect { subject }.to change { Ci::Runner.count }.by(-1) + let(:user) { create(:user) } - is_expected.to be_success - expect(execute.payload).to eq({ deleted_count: 1, deleted_ids: [instance_runner.id] }) + it 'does not delete any runner and returns error', :aggregate_failures do + expect { execute }.not_to change { Ci::Runner.count } + expect(execute[:errors]).to match_array("User does not have permission to delete any of the runners") end end - end - context 'with runners specified as relation' do - let(:runners_arg) { Ci::Runner.not_group_type } + context 'when user is not part of the group' do + let(:user) { create(:user) } - include_examples 'a service deleting runners in bulk' + it 'does not delete any runner and returns error', :aggregate_failures do + expect { execute }.not_to change { Ci::Runner.count } + expect(execute[:errors]).to match_array("User does not have permission to delete any of the runners") + end + end end - context 'with runners specified as array of IDs' do - let(:runners_arg) { Ci::Runner.not_group_type.ids } + context 'when the user can delete runners' do + context 'when user is an admin', :enable_admin_mode do + include_examples 'a service deleting runners in bulk' do + let(:runners_arg) { Ci::Runner.all } + let!(:expected_deleted_runners) { [instance_runner, group_runner, project_runner] } + let(:user) { admin_user } + end + + context 'with a runner already deleted' do + before do + group_runner.destroy! + end + + include_examples 'a service deleting runners in bulk' do + let(:runners_arg) { Ci::Runner.all } + let!(:expected_deleted_runners) { [instance_runner, project_runner] } + let(:user) { admin_user } + end + end + + context 'when deleting a single runner' do + let(:runners_arg) { Ci::Runner.all } + + it 'avoids N+1 cached queries', :use_sql_query_cache, :request_store do + # Run this once to establish a baseline + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do + execute + end + + additional_runners = 1 + + create_list(:ci_runner, 1 + additional_runners, :instance) + create_list(:ci_runner, 1 + additional_runners, :group, groups: [group]) + create_list(:ci_runner, 1 + additional_runners, :project, projects: [project]) + + service = described_class.new(runners: runners_arg, current_user: user) + + # Base cost per runner is: + # - 1 `SELECT * FROM "taggings"` query + # - 1 `SAVEPOINT` query + # - 1 `DELETE FROM "ci_runners"` query + # - 1 `RELEASE SAVEPOINT` query + # Project runners have an additional query: + # - 1 `DELETE FROM "ci_runner_projects"` query, given the call to `destroy_all` + instance_runner_cost = 4 + group_runner_cost = 4 + project_runner_cost = 5 + expect { service.execute } + .not_to exceed_all_query_limit(control_count) + .with_threshold(additional_runners * (instance_runner_cost + group_runner_cost + project_runner_cost)) + end + end + end - include_examples 'a service deleting runners in bulk' + context 'when user is group owner' do + before do + group.add_owner(user) + end + + include_examples 'a service deleting runners in bulk' do + let(:runners_arg) { Ci::Runner.not_instance_type } + let!(:expected_deleted_runners) { [group_runner, project_runner] } + let(:user) { owner_user } + end + + context 'with a runner non-authorised to be deleted' do + let(:runners_arg) { Ci::Runner.all } + let!(:expected_deleted_runners) { [project_runner] } + let(:user) { owner_user } + + it 'destroys only authorised runners', :aggregate_failures do + allow(Ability).to receive(:allowed?).and_call_original + expect(Ability).to receive(:allowed?).with(user, :delete_runner, instance_runner).and_return(false) + + expect { execute }.to change { Ci::Runner.count }.by(-2) + + is_expected.to be_success + expect(execute.payload).to eq( + { + deleted_count: 2, + deleted_ids: [group_runner.id, project_runner.id], + errors: ["User does not have permission to delete runner(s) ##{instance_runner.id}"] + }) + end + end + end end context 'with no arguments specified' do let(:runners_arg) { nil } + let(:user) { owner_user } it 'returns 0 deleted runners' do is_expected.to be_success - expect(execute.payload).to eq({ deleted_count: 0, deleted_ids: [] }) + expect(execute.payload).to eq({ deleted_count: 0, deleted_ids: [], errors: [] }) end end end diff --git a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb index 1f44612947b..e5cba80d567 100644 --- a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb +++ b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb @@ -67,6 +67,19 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute' do expect(runner.projects.ids).to match_array([owner_project.id] + project_ids) end end + + context 'when disassociating all projects' do + let(:project_ids) { [] } + + it 'reassigns associated projects and returns success response' do + expect(execute).to be_success + + runner.reload + + expect(runner.owner_project).to eq(owner_project) + expect(runner.projects.ids).to contain_exactly(owner_project.id) + end + end end context 'with failing assign_to requests' do diff --git a/spec/services/clusters/applications/check_ingress_ip_address_service_spec.rb b/spec/services/clusters/applications/check_ingress_ip_address_service_spec.rb deleted file mode 100644 index 605d9e67ab6..00000000000 --- a/spec/services/clusters/applications/check_ingress_ip_address_service_spec.rb +++ /dev/null @@ -1,44 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Applications::CheckIngressIpAddressService do - include ExclusiveLeaseHelpers - - let(:application) { create(:clusters_applications_ingress, :installed) } - let(:service) { described_class.new(application) } - let(:kubeclient) { double(::Kubeclient::Client, get_service: kube_service) } - let(:lease_key) { "check_ingress_ip_address_service:#{application.id}" } - - let(:ingress) do - [ - { - ip: '111.222.111.222', - hostname: 'localhost.localdomain' - } - ] - end - - let(:kube_service) do - ::Kubeclient::Resource.new( - { - status: { - loadBalancer: { - ingress: ingress - } - } - } - ) - end - - subject { service.execute } - - before do - stub_exclusive_lease(lease_key, timeout: 15.seconds.to_i) - allow(application.cluster).to receive(:kubeclient).and_return(kubeclient) - end - - include_examples 'check ingress ip executions', :clusters_applications_ingress - - include_examples 'check ingress ip executions', :clusters_applications_knative -end diff --git a/spec/services/clusters/applications/check_installation_progress_service_spec.rb b/spec/services/clusters/applications/check_installation_progress_service_spec.rb deleted file mode 100644 index 698804ff6af..00000000000 --- a/spec/services/clusters/applications/check_installation_progress_service_spec.rb +++ /dev/null @@ -1,204 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Applications::CheckInstallationProgressService, '#execute' do - RESCHEDULE_PHASES = Gitlab::Kubernetes::Pod::PHASES - [Gitlab::Kubernetes::Pod::SUCCEEDED, Gitlab::Kubernetes::Pod::FAILED].freeze - - let(:application) { create(:clusters_applications_helm, :installing) } - let(:service) { described_class.new(application) } - let(:phase) { Gitlab::Kubernetes::Pod::UNKNOWN } - let(:errors) { nil } - - shared_examples 'a not yet terminated installation' do |a_phase| - let(:phase) { a_phase } - - before do - expect(service).to receive(:pod_phase).once.and_return(phase) - end - - context "when phase is #{a_phase}" do - context 'when not timed_out' do - it 'reschedule a new check' do - expect(ClusterWaitForAppInstallationWorker).to receive(:perform_in).once - expect(service).not_to receive(:remove_installation_pod) - - expect do - service.execute - - application.reload - end.not_to change(application, :status) - - expect(application.status_reason).to be_nil - end - end - end - end - - shared_examples 'error handling' do - context 'when installation raises a Kubeclient::HttpError' do - let(:cluster) { create(:cluster, :provided_by_user, :project) } - let(:logger) { service.send(:logger) } - let(:error) { Kubeclient::HttpError.new(401, 'Unauthorized', nil) } - - before do - application.update!(cluster: cluster) - - expect(service).to receive(:pod_phase).and_raise(error) - end - - include_examples 'logs kubernetes errors' do - let(:error_name) { 'Kubeclient::HttpError' } - let(:error_message) { 'Unauthorized' } - let(:error_code) { 401 } - end - - it 'shows the response code from the error' do - service.execute - - expect(application).to be_errored.or(be_update_errored) - expect(application.status_reason).to eq('Kubernetes error: 401') - end - end - end - - before do - allow(service).to receive(:installation_errors).and_return(errors) - allow(service).to receive(:remove_installation_pod).and_return(nil) - end - - context 'when application is updating' do - let(:application) { create(:clusters_applications_helm, :updating) } - - include_examples 'error handling' - - RESCHEDULE_PHASES.each { |phase| it_behaves_like 'a not yet terminated installation', phase } - - context 'when installation POD succeeded' do - let(:phase) { Gitlab::Kubernetes::Pod::SUCCEEDED } - - before do - expect(service).to receive(:pod_phase).once.and_return(phase) - end - - it 'removes the installation POD' do - expect(service).to receive(:remove_installation_pod).once - - service.execute - end - - it 'make the application installed' do - expect(ClusterWaitForAppInstallationWorker).not_to receive(:perform_in) - - service.execute - - expect(application).to be_updated - expect(application.status_reason).to be_nil - end - end - - context 'when installation POD failed' do - let(:phase) { Gitlab::Kubernetes::Pod::FAILED } - let(:errors) { 'test installation failed' } - - before do - expect(service).to receive(:pod_phase).once.and_return(phase) - end - - it 'make the application errored' do - service.execute - - expect(application).to be_update_errored - expect(application.status_reason).to eq('Operation failed. Check pod logs for install-helm for more details.') - end - end - - context 'when timed out' do - let(:application) { create(:clusters_applications_helm, :timed_out, :updating) } - - before do - expect(service).to receive(:pod_phase).once.and_return(phase) - end - - it 'make the application errored' do - expect(ClusterWaitForAppInstallationWorker).not_to receive(:perform_in) - - service.execute - - expect(application).to be_update_errored - expect(application.status_reason).to eq('Operation timed out. Check pod logs for install-helm for more details.') - end - end - end - - context 'when application is installing' do - include_examples 'error handling' - - RESCHEDULE_PHASES.each { |phase| it_behaves_like 'a not yet terminated installation', phase } - - context 'when installation POD succeeded' do - let(:phase) { Gitlab::Kubernetes::Pod::SUCCEEDED } - - before do - expect(service).to receive(:pod_phase).once.and_return(phase) - end - - it 'removes the installation POD' do - expect_next_instance_of(Gitlab::Kubernetes::Helm::API) do |instance| - expect(instance).to receive(:delete_pod!).with(kind_of(String)).once - end - expect(service).to receive(:remove_installation_pod).and_call_original - - service.execute - end - - it 'make the application installed' do - expect(ClusterWaitForAppInstallationWorker).not_to receive(:perform_in) - - service.execute - - expect(application).to be_installed - expect(application.status_reason).to be_nil - end - - it 'tracks application install', :snowplow do - service.execute - - expect_snowplow_event(category: 'cluster:applications', action: 'cluster_application_helm_installed') - end - end - - context 'when installation POD failed' do - let(:phase) { Gitlab::Kubernetes::Pod::FAILED } - let(:errors) { 'test installation failed' } - - before do - expect(service).to receive(:pod_phase).once.and_return(phase) - end - - it 'make the application errored' do - service.execute - - expect(application).to be_errored - expect(application.status_reason).to eq('Operation failed. Check pod logs for install-helm for more details.') - end - end - - context 'when timed out' do - let(:application) { create(:clusters_applications_helm, :timed_out) } - - before do - expect(service).to receive(:pod_phase).once.and_return(phase) - end - - it 'make the application errored' do - expect(ClusterWaitForAppInstallationWorker).not_to receive(:perform_in) - - service.execute - - expect(application).to be_errored - expect(application.status_reason).to eq('Operation timed out. Check pod logs for install-helm for more details.') - end - end - end -end diff --git a/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb b/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb deleted file mode 100644 index 4b8893429cf..00000000000 --- a/spec/services/clusters/applications/check_uninstall_progress_service_spec.rb +++ /dev/null @@ -1,155 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Applications::CheckUninstallProgressService do - reschedule_phases = Gitlab::Kubernetes::Pod::PHASES - [Gitlab::Kubernetes::Pod::SUCCEEDED, Gitlab::Kubernetes::Pod::FAILED].freeze - - let(:application) { create(:clusters_applications_prometheus, :uninstalling) } - let(:service) { described_class.new(application) } - let(:phase) { Gitlab::Kubernetes::Pod::UNKNOWN } - let(:errors) { nil } - let(:worker_class) { Clusters::Applications::WaitForUninstallAppWorker } - - before do - allow(service).to receive(:installation_errors).and_return(errors) - allow(service).to receive(:remove_installation_pod) - end - - shared_examples 'a not yet terminated installation' do |a_phase| - let(:phase) { a_phase } - - before do - expect(service).to receive(:pod_phase).once.and_return(phase) - end - - context "when phase is #{a_phase}" do - context 'when not timed_out' do - it 'reschedule a new check' do - expect(worker_class).to receive(:perform_in).once - expect(service).not_to receive(:remove_installation_pod) - - expect do - service.execute - - application.reload - end.not_to change(application, :status) - - expect(application.status_reason).to be_nil - end - end - end - end - - context 'when application is uninstalling' do - reschedule_phases.each { |phase| it_behaves_like 'a not yet terminated installation', phase } - - context 'when installation POD succeeded' do - let(:phase) { Gitlab::Kubernetes::Pod::SUCCEEDED } - - before do - expect_next_instance_of(Gitlab::Kubernetes::Helm::API) do |instance| - expect(instance).to receive(:delete_pod!).with(kind_of(String)).once - end - expect(service).to receive(:pod_phase).once.and_return(phase) - end - - it 'removes the installation POD' do - expect(service).to receive(:remove_uninstallation_pod).and_call_original - - service.execute - end - - it 'runs application post_uninstall' do - expect(application).to receive(:post_uninstall).and_call_original - - service.execute - end - - it 'destroys the application' do - expect(worker_class).not_to receive(:perform_in) - - service.execute - - expect(application).to be_destroyed - end - - context 'an error occurs while destroying' do - before do - expect(application).to receive(:destroy!).once.and_raise("destroy failed") - end - - it 'still removes the installation POD' do - expect(service).to receive(:remove_uninstallation_pod).and_call_original - - service.execute - end - - it 'makes the application uninstall_errored' do - service.execute - - expect(application).to be_uninstall_errored - expect(application.status_reason).to eq('Application uninstalled but failed to destroy: destroy failed') - end - end - end - - context 'when installation POD failed' do - let(:phase) { Gitlab::Kubernetes::Pod::FAILED } - let(:errors) { 'test installation failed' } - - before do - expect(service).to receive(:pod_phase).once.and_return(phase) - end - - it 'make the application errored' do - service.execute - - expect(application).to be_uninstall_errored - expect(application.status_reason).to eq('Operation failed. Check pod logs for uninstall-prometheus for more details.') - end - end - - context 'when timed out' do - let(:application) { create(:clusters_applications_prometheus, :timed_out, :uninstalling) } - - before do - expect(service).to receive(:pod_phase).once.and_return(phase) - end - - it 'make the application errored' do - expect(worker_class).not_to receive(:perform_in) - - service.execute - - expect(application).to be_uninstall_errored - expect(application.status_reason).to eq('Operation timed out. Check pod logs for uninstall-prometheus for more details.') - end - end - - context 'when installation raises a Kubeclient::HttpError' do - let(:cluster) { create(:cluster, :provided_by_user, :project) } - let(:logger) { service.send(:logger) } - let(:error) { Kubeclient::HttpError.new(401, 'Unauthorized', nil) } - - before do - application.update!(cluster: cluster) - - expect(service).to receive(:pod_phase).and_raise(error) - end - - include_examples 'logs kubernetes errors' do - let(:error_name) { 'Kubeclient::HttpError' } - let(:error_message) { 'Unauthorized' } - let(:error_code) { 401 } - end - - it 'shows the response code from the error' do - service.execute - - expect(application).to be_uninstall_errored - expect(application.status_reason).to eq('Kubernetes error: 401') - end - end - end -end diff --git a/spec/services/clusters/applications/check_upgrade_progress_service_spec.rb b/spec/services/clusters/applications/check_upgrade_progress_service_spec.rb deleted file mode 100644 index dbde8cec9b9..00000000000 --- a/spec/services/clusters/applications/check_upgrade_progress_service_spec.rb +++ /dev/null @@ -1,94 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Applications::CheckUpgradeProgressService do - reschedule_phashes = ::Gitlab::Kubernetes::Pod::PHASES - - [::Gitlab::Kubernetes::Pod::SUCCEEDED, ::Gitlab::Kubernetes::Pod::FAILED, ::Gitlab].freeze - - let(:application) { create(:clusters_applications_prometheus, :updating) } - let(:service) { described_class.new(application) } - let(:phase) { ::Gitlab::Kubernetes::Pod::UNKNOWN } - let(:errors) { nil } - - shared_examples 'a terminated upgrade' do - it 'removes the POD' do - expect(service).to receive(:remove_pod).once - - service.execute - end - end - - shared_examples 'a not yet terminated upgrade' do |a_phase| - let(:phase) { a_phase } - - context "when phase is #{a_phase}" do - context 'when not timed out' do - it 'reschedule a new check' do - expect(::ClusterWaitForAppUpdateWorker).to receive(:perform_in).once - expect(service).not_to receive(:remove_pod) - - service.execute - - expect(application).to be_updating - expect(application.status_reason).to be_nil - end - end - - context 'when timed out' do - let(:application) { create(:clusters_applications_prometheus, :timed_out, :updating) } - - it_behaves_like 'a terminated upgrade' - - it 'make the application update errored' do - expect(::ClusterWaitForAppUpdateWorker).not_to receive(:perform_in) - - service.execute - - expect(application).to be_update_errored - expect(application.status_reason).to eq("Update timed out") - end - end - end - end - - before do - allow(service).to receive(:phase).once.and_return(phase) - - allow(service).to receive(:errors).and_return(errors) - allow(service).to receive(:remove_pod).and_return(nil) - end - - describe '#execute' do - context 'when upgrade pod succeeded' do - let(:phase) { ::Gitlab::Kubernetes::Pod::SUCCEEDED } - - it_behaves_like 'a terminated upgrade' - - it 'make the application upgraded' do - expect(::ClusterWaitForAppUpdateWorker).not_to receive(:perform_in) - - service.execute - - expect(application).to be_updated - expect(application.status_reason).to be_nil - end - end - - context 'when upgrade pod failed' do - let(:phase) { ::Gitlab::Kubernetes::Pod::FAILED } - let(:errors) { 'test installation failed' } - - it_behaves_like 'a terminated upgrade' - - it 'make the application update errored' do - service.execute - - expect(application).to be_update_errored - expect(application.status_reason).to eq(errors) - end - end - - reschedule_phashes.each { |phase| it_behaves_like 'a not yet terminated upgrade', phase } - end -end diff --git a/spec/services/clusters/applications/create_service_spec.rb b/spec/services/clusters/applications/create_service_spec.rb deleted file mode 100644 index 00a67a9b2ef..00000000000 --- a/spec/services/clusters/applications/create_service_spec.rb +++ /dev/null @@ -1,279 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Applications::CreateService do - include TestRequestHelpers - - let(:cluster) { create(:cluster, :project, :provided_by_gcp) } - let(:user) { create(:user) } - let(:params) { { application: 'ingress' } } - let(:service) { described_class.new(cluster, user, params) } - - describe '#execute' do - before do - allow(ClusterInstallAppWorker).to receive(:perform_async) - allow(ClusterUpgradeAppWorker).to receive(:perform_async) - end - - subject { service.execute(test_request) } - - it 'creates an application' do - expect do - subject - - cluster.reload - end.to change(cluster, :application_ingress) - end - - context 'application already installed' do - let!(:application) { create(:clusters_applications_ingress, :installed, cluster: cluster) } - - it 'does not create a new application' do - expect do - subject - end.not_to change(Clusters::Applications::Ingress, :count) - end - - it 'schedules an upgrade for the application' do - expect(ClusterUpgradeAppWorker).to receive(:perform_async) - - subject - end - end - - context 'known applications' do - context 'ingress application' do - let(:params) do - { - application: 'ingress' - } - end - - before do - expect_any_instance_of(Clusters::Applications::Ingress) - .to receive(:make_scheduled!) - .and_call_original - end - - it 'creates the application' do - expect do - subject - - cluster.reload - end.to change(cluster, :application_ingress) - end - end - - context 'cert manager application' do - let(:params) do - { - application: 'cert_manager', - email: 'test@example.com' - } - end - - before do - expect_any_instance_of(Clusters::Applications::CertManager) - .to receive(:make_scheduled!) - .and_call_original - end - - it 'creates the application' do - expect do - subject - - cluster.reload - end.to change(cluster, :application_cert_manager) - end - - it 'sets the email' do - expect(subject.email).to eq('test@example.com') - end - end - - context 'jupyter application' do - let(:params) do - { - application: 'jupyter', - hostname: 'example.com' - } - end - - before do - create(:clusters_applications_ingress, :installed, external_ip: "127.0.0.0", cluster: cluster) - expect_any_instance_of(Clusters::Applications::Jupyter) - .to receive(:make_scheduled!) - .and_call_original - end - - it 'creates the application' do - expect do - subject - - cluster.reload - end.to change(cluster, :application_jupyter) - end - - it 'sets the hostname' do - expect(subject.hostname).to eq('example.com') - end - - it 'sets the oauth_application' do - expect(subject.oauth_application).to be_present - end - end - - context 'knative application' do - let(:params) do - { - application: 'knative', - hostname: 'example.com', - pages_domain_id: domain.id - } - end - - let(:domain) { create(:pages_domain, :instance_serverless) } - let(:associate_domain_service) { double('AssociateDomainService') } - - before do - expect_any_instance_of(Clusters::Applications::Knative) - .to receive(:make_scheduled!) - .and_call_original - end - - it 'creates the application' do - expect do - subject - - cluster.reload - end.to change(cluster, :application_knative) - end - - it 'sets the hostname' do - expect(subject.hostname).to eq('example.com') - end - - it 'executes AssociateDomainService' do - expect(Serverless::AssociateDomainService).to receive(:new) do |knative, args| - expect(knative).to be_a(Clusters::Applications::Knative) - expect(args[:pages_domain_id]).to eq(params[:pages_domain_id]) - expect(args[:creator]).to eq(user) - - associate_domain_service - end - - expect(associate_domain_service).to receive(:execute) - - subject - end - end - end - - context 'invalid application' do - let(:params) { { application: 'non-existent' } } - - it 'raises an error' do - expect { subject }.to raise_error(Clusters::Applications::CreateService::InvalidApplicationError) - end - end - - context 'group cluster' do - let(:cluster) { create(:cluster, :provided_by_gcp, :group) } - - using RSpec::Parameterized::TableSyntax - - where(:application, :association, :allowed, :pre_create_ingress) do - 'ingress' | :application_ingress | true | false - 'runner' | :application_runner | true | false - 'prometheus' | :application_prometheus | true | false - 'jupyter' | :application_jupyter | true | true - end - - with_them do - before do - klass = "Clusters::Applications::#{application.titleize}" - allow_any_instance_of(klass.constantize).to receive(:make_scheduled!).and_call_original - create(:clusters_applications_ingress, :installed, cluster: cluster, external_hostname: 'example.com') if pre_create_ingress - end - - let(:params) { { application: application } } - - it 'executes for each application' do - if allowed - expect do - subject - - cluster.reload - end.to change(cluster, association) - else - expect { subject }.to raise_error(Clusters::Applications::CreateService::InvalidApplicationError) - end - end - end - end - - context 'when application is installable' do - shared_examples 'installable applications' do - it 'makes the application scheduled' do - expect do - subject - end.to change { Clusters::Applications::Ingress.with_status(:scheduled).count }.by(1) - end - - it 'schedules an install via worker' do - expect(ClusterInstallAppWorker) - .to receive(:perform_async) - .with(*worker_arguments) - .once - - subject - end - end - - context 'when application is associated with a cluster' do - let(:application) { create(:clusters_applications_ingress, :installable, cluster: cluster) } - let(:worker_arguments) { [application.name, application.id] } - - it_behaves_like 'installable applications' - end - - context 'when application is not associated with a cluster' do - let(:worker_arguments) { [params[:application], kind_of(Numeric)] } - - it_behaves_like 'installable applications' - end - end - - context 'when installation is already in progress' do - let!(:application) { create(:clusters_applications_ingress, :installing, cluster: cluster) } - - it 'raises an exception' do - expect { subject } - .to raise_exception(StateMachines::InvalidTransition) - .and not_change(application.class.with_status(:scheduled), :count) - end - - it 'does not schedule a cluster worker' do - expect(ClusterInstallAppWorker).not_to receive(:perform_async) - end - end - - context 'when application is installed' do - %i(installed updated).each do |status| - let(:application) { create(:clusters_applications_ingress, status, cluster: cluster) } - - it 'schedules an upgrade via worker' do - expect(ClusterUpgradeAppWorker) - .to receive(:perform_async) - .with(application.name, application.id) - .once - - subject - - expect(application.reload).to be_scheduled - end - end - end - end -end diff --git a/spec/services/clusters/applications/patch_service_spec.rb b/spec/services/clusters/applications/patch_service_spec.rb deleted file mode 100644 index 281da62b80b..00000000000 --- a/spec/services/clusters/applications/patch_service_spec.rb +++ /dev/null @@ -1,80 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Applications::PatchService do - describe '#execute' do - let(:application) { create(:clusters_applications_knative, :scheduled) } - let!(:update_command) { application.update_command } - let(:service) { described_class.new(application) } - let(:helm_client) { instance_double(Gitlab::Kubernetes::Helm::API) } - - before do - allow(service).to receive(:update_command).and_return(update_command) - allow(service).to receive(:helm_api).and_return(helm_client) - end - - context 'when there are no errors' do - before do - expect(helm_client).to receive(:update).with(update_command) - allow(ClusterWaitForAppInstallationWorker).to receive(:perform_in).and_return(nil) - end - - it 'make the application updating' do - expect(application.cluster).not_to be_nil - service.execute - - expect(application).to be_updating - end - - it 'schedule async installation status check' do - expect(ClusterWaitForAppInstallationWorker).to receive(:perform_in).once - - service.execute - end - end - - context 'when kubernetes cluster communication fails' do - let(:error) { Kubeclient::HttpError.new(500, 'system failure', nil) } - - before do - expect(helm_client).to receive(:update).with(update_command).and_raise(error) - end - - include_examples 'logs kubernetes errors' do - let(:error_name) { 'Kubeclient::HttpError' } - let(:error_message) { 'system failure' } - let(:error_code) { 500 } - end - - it 'make the application errored' do - service.execute - - expect(application).to be_update_errored - expect(application.status_reason).to eq(_('Kubernetes error: %{error_code}') % { error_code: 500 }) - end - end - - context 'a non kubernetes error happens' do - let(:application) { create(:clusters_applications_knative, :scheduled) } - let(:error) { StandardError.new('something bad happened') } - - include_examples 'logs kubernetes errors' do - let(:error_name) { 'StandardError' } - let(:error_message) { 'something bad happened' } - let(:error_code) { nil } - end - - before do - expect(helm_client).to receive(:update).with(update_command).and_raise(error) - end - - it 'make the application errored' do - service.execute - - expect(application).to be_update_errored - expect(application.status_reason).to eq(_('Failed to update.')) - end - end - end -end diff --git a/spec/services/clusters/applications/prometheus_update_service_spec.rb b/spec/services/clusters/applications/prometheus_update_service_spec.rb deleted file mode 100644 index 615bfc44045..00000000000 --- a/spec/services/clusters/applications/prometheus_update_service_spec.rb +++ /dev/null @@ -1,111 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Applications::PrometheusUpdateService do - describe '#execute' do - let(:project) { create(:project) } - let(:environment) { create(:environment, project: project) } - let(:cluster) { create(:cluster, :provided_by_user, :with_installed_helm, projects: [project]) } - let(:application) { create(:clusters_applications_prometheus, :installed, cluster: cluster) } - let(:empty_alerts_values_update_yaml) { "---\nalertmanager:\n enabled: false\nserverFiles:\n alerts: {}\n" } - let(:helm_client) { instance_double(::Gitlab::Kubernetes::Helm::API) } - - subject(:service) { described_class.new(application, project) } - - context 'when prometheus is a Clusters::Integrations::Prometheus' do - let(:application) { create(:clusters_integrations_prometheus, cluster: cluster) } - - it 'raises NotImplementedError' do - expect { service.execute }.to raise_error(NotImplementedError) - end - end - - context 'when prometheus is externally installed' do - let(:application) { create(:clusters_applications_prometheus, :externally_installed, cluster: cluster) } - - it 'raises NotImplementedError' do - expect { service.execute }.to raise_error(NotImplementedError) - end - end - - context 'when prometheus is a Clusters::Applications::Prometheus' do - let!(:patch_command) { application.patch_command(empty_alerts_values_update_yaml) } - - before do - allow(service).to receive(:patch_command).with(empty_alerts_values_update_yaml).and_return(patch_command) - allow(service).to receive(:helm_api).and_return(helm_client) - end - - context 'when there are no errors' do - before do - expect(helm_client).to receive(:update).with(patch_command) - - allow(::ClusterWaitForAppUpdateWorker) - .to receive(:perform_in) - .and_return(nil) - end - - it 'make the application updating' do - expect(application.cluster).not_to be_nil - - service.execute - - expect(application).to be_updating - end - - it 'updates current config' do - prometheus_config_service = spy(:prometheus_config_service) - - expect(Clusters::Applications::PrometheusConfigService) - .to receive(:new) - .with(project, cluster, application) - .and_return(prometheus_config_service) - - expect(prometheus_config_service) - .to receive(:execute) - .and_return(YAML.safe_load(empty_alerts_values_update_yaml)) - - service.execute - end - - it 'schedules async update status check' do - expect(::ClusterWaitForAppUpdateWorker).to receive(:perform_in).once - - service.execute - end - end - - context 'when k8s cluster communication fails' do - before do - error = ::Kubeclient::HttpError.new(500, 'system failure', nil) - allow(helm_client).to receive(:update).and_raise(error) - end - - it 'make the application update errored' do - service.execute - - expect(application).to be_update_errored - expect(application.status_reason).to match(/kubernetes error:/i) - end - end - - context 'when application cannot be persisted' do - let(:application) { build(:clusters_applications_prometheus, :installed) } - - before do - allow(application).to receive(:make_updating!).once - .and_raise(ActiveRecord::RecordInvalid.new(application)) - end - - it 'make the application update errored' do - expect(helm_client).not_to receive(:update) - - service.execute - - expect(application).to be_update_errored - end - end - end - end -end diff --git a/spec/services/clusters/applications/update_service_spec.rb b/spec/services/clusters/applications/update_service_spec.rb deleted file mode 100644 index 4c05a12a4a1..00000000000 --- a/spec/services/clusters/applications/update_service_spec.rb +++ /dev/null @@ -1,91 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Applications::UpdateService do - include TestRequestHelpers - - let(:cluster) { create(:cluster, :project, :provided_by_gcp) } - let(:user) { create(:user) } - let(:params) { { application: 'knative', hostname: 'update.example.com', pages_domain_id: domain.id } } - let(:service) { described_class.new(cluster, user, params) } - let(:domain) { create(:pages_domain, :instance_serverless) } - - subject { service.execute(test_request) } - - describe '#execute' do - before do - allow(ClusterPatchAppWorker).to receive(:perform_async) - end - - context 'application is not installed' do - it 'raises Clusters::Applications::BaseService::InvalidApplicationError' do - expect(ClusterPatchAppWorker).not_to receive(:perform_async) - - expect { subject } - .to raise_exception { Clusters::Applications::BaseService::InvalidApplicationError } - .and not_change { Clusters::Applications::Knative.count } - .and not_change { Clusters::Applications::Knative.with_status(:scheduled).count } - end - end - - context 'application is installed' do - context 'application is schedulable' do - let!(:application) do - create(:clusters_applications_knative, status: 3, cluster: cluster) - end - - it 'updates the application data' do - expect do - subject - end.to change { application.reload.hostname }.to(params[:hostname]) - end - - it 'makes application scheduled!' do - subject - - expect(application.reload).to be_scheduled - end - - it 'schedules ClusterPatchAppWorker' do - expect(ClusterPatchAppWorker).to receive(:perform_async) - - subject - end - - context 'knative application' do - let(:associate_domain_service) { double('AssociateDomainService') } - - it 'executes AssociateDomainService' do - expect(Serverless::AssociateDomainService).to receive(:new) do |knative, args| - expect(knative.id).to eq(application.id) - expect(args[:pages_domain_id]).to eq(params[:pages_domain_id]) - expect(args[:creator]).to eq(user) - - associate_domain_service - end - - expect(associate_domain_service).to receive(:execute) - - subject - end - end - end - - context 'application is not schedulable' do - let!(:application) do - create(:clusters_applications_knative, status: 4, cluster: cluster) - end - - it 'raises StateMachines::InvalidTransition' do - expect(ClusterPatchAppWorker).not_to receive(:perform_async) - - expect { subject } - .to raise_exception { StateMachines::InvalidTransition } - .and not_change { application.reload.hostname } - .and not_change { Clusters::Applications::Knative.with_status(:scheduled).count } - end - end - end - end -end diff --git a/spec/services/clusters/gcp/provision_service_spec.rb b/spec/services/clusters/gcp/provision_service_spec.rb index c5778db6001..c8b7f628e5b 100644 --- a/spec/services/clusters/gcp/provision_service_spec.rb +++ b/spec/services/clusters/gcp/provision_service_spec.rb @@ -42,7 +42,7 @@ RSpec.describe Clusters::Gcp::ProvisionService do gcp_project_id, zone, { "status": 'unexpected' - } ) + }) end it_behaves_like 'error' diff --git a/spec/services/clusters/gcp/verify_provision_status_service_spec.rb b/spec/services/clusters/gcp/verify_provision_status_service_spec.rb index ccb4b3b6c15..ffe4516c02b 100644 --- a/spec/services/clusters/gcp/verify_provision_status_service_spec.rb +++ b/spec/services/clusters/gcp/verify_provision_status_service_spec.rb @@ -44,7 +44,7 @@ RSpec.describe Clusters::Gcp::VerifyProvisionStatusService do { "status": 'RUNNING', "startTime": 1.minute.ago.strftime("%FT%TZ") - } ) + }) end it_behaves_like 'continue_creation' @@ -56,7 +56,7 @@ RSpec.describe Clusters::Gcp::VerifyProvisionStatusService do { "status": 'RUNNING', "startTime": 30.minutes.ago.strftime("%FT%TZ") - } ) + }) end it_behaves_like 'error' @@ -70,7 +70,7 @@ RSpec.describe Clusters::Gcp::VerifyProvisionStatusService do { "status": 'PENDING', "startTime": 1.minute.ago.strftime("%FT%TZ") - } ) + }) end it_behaves_like 'continue_creation' @@ -82,7 +82,7 @@ RSpec.describe Clusters::Gcp::VerifyProvisionStatusService do gcp_project_id, zone, operation_id, { "status": 'DONE' - } ) + }) end it_behaves_like 'finalize_creation' @@ -94,7 +94,7 @@ RSpec.describe Clusters::Gcp::VerifyProvisionStatusService do gcp_project_id, zone, operation_id, { "status": 'unexpected' - } ) + }) end it_behaves_like 'error' diff --git a/spec/services/clusters/kubernetes/configure_istio_ingress_service_spec.rb b/spec/services/clusters/kubernetes/configure_istio_ingress_service_spec.rb deleted file mode 100644 index f26177a56d0..00000000000 --- a/spec/services/clusters/kubernetes/configure_istio_ingress_service_spec.rb +++ /dev/null @@ -1,223 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Kubernetes::ConfigureIstioIngressService, '#execute' do - include KubernetesHelpers - - let(:cluster) { create(:cluster, :project, :provided_by_gcp) } - let(:api_url) { 'https://kubernetes.example.com' } - let(:project) { cluster.project } - let(:environment) { create(:environment, project: project) } - let(:cluster_project) { cluster.cluster_project } - let(:namespace) { "#{project.name}-#{project.id}-#{environment.slug}" } - let(:kubeclient) { cluster.kubeclient } - - subject do - described_class.new( - cluster: cluster - ).execute - end - - before do - stub_kubeclient_discover_istio(api_url) - stub_kubeclient_create_secret(api_url, namespace: namespace) - stub_kubeclient_put_secret(api_url, "#{namespace}-token", namespace: namespace) - - stub_kubeclient_get_secret( - api_url, - metadata_name: "#{namespace}-token", - token: Base64.encode64('sample-token'), - namespace: namespace - ) - - stub_kubeclient_get_secret( - api_url, - metadata_name: 'istio-ingressgateway-ca-certs', - namespace: 'istio-system' - ) - - stub_kubeclient_get_secret( - api_url, - metadata_name: 'istio-ingressgateway-certs', - namespace: 'istio-system' - ) - - stub_kubeclient_put_secret(api_url, 'istio-ingressgateway-ca-certs', namespace: 'istio-system') - stub_kubeclient_put_secret(api_url, 'istio-ingressgateway-certs', namespace: 'istio-system') - stub_kubeclient_get_gateway(api_url, 'knative-ingress-gateway', namespace: 'knative-serving') - stub_kubeclient_put_gateway(api_url, 'knative-ingress-gateway', namespace: 'knative-serving') - end - - context 'without a serverless_domain_cluster' do - it 'configures gateway to use PASSTHROUGH' do - subject - - expect(WebMock).to have_requested(:put, api_url + '/apis/networking.istio.io/v1alpha3/namespaces/knative-serving/gateways/knative-ingress-gateway').with( - body: hash_including( - apiVersion: "networking.istio.io/v1alpha3", - kind: "Gateway", - metadata: { - generation: 1, - labels: { - "networking.knative.dev/ingress-provider" => "istio", - "serving.knative.dev/release" => "v0.7.0" - }, - name: "knative-ingress-gateway", - namespace: "knative-serving", - selfLink: "/apis/networking.istio.io/v1alpha3/namespaces/knative-serving/gateways/knative-ingress-gateway" - }, - spec: { - selector: { - istio: "ingressgateway" - }, - servers: [ - { - hosts: ["*"], - port: { - name: "http", - number: 80, - protocol: "HTTP" - } - }, - { - hosts: ["*"], - port: { - name: "https", - number: 443, - protocol: "HTTPS" - }, - tls: { - mode: "PASSTHROUGH" - } - } - ] - } - ) - ) - end - end - - context 'with a serverless_domain_cluster' do - let(:serverless_domain_cluster) { create(:serverless_domain_cluster) } - let(:certificate) { OpenSSL::X509::Certificate.new(serverless_domain_cluster.certificate) } - - before do - cluster.application_knative = serverless_domain_cluster.knative - end - - it 'configures certificates' do - subject - - expect(serverless_domain_cluster.reload.key).not_to be_blank - expect(serverless_domain_cluster.reload.certificate).not_to be_blank - - expect(certificate.subject.to_s).to include(serverless_domain_cluster.knative.hostname) - - expect(certificate.not_before).to be_within(1.minute).of(Time.current) - expect(certificate.not_after).to be_within(1.minute).of(Time.current + 1000.years) - - expect(WebMock).to have_requested(:put, api_url + '/api/v1/namespaces/istio-system/secrets/istio-ingressgateway-ca-certs').with( - body: hash_including( - metadata: { - name: 'istio-ingressgateway-ca-certs', - namespace: 'istio-system' - }, - type: 'Opaque' - ) - ) - - expect(WebMock).to have_requested(:put, api_url + '/api/v1/namespaces/istio-system/secrets/istio-ingressgateway-certs').with( - body: hash_including( - metadata: { - name: 'istio-ingressgateway-certs', - namespace: 'istio-system' - }, - type: 'kubernetes.io/tls' - ) - ) - end - - it 'configures gateway to use MUTUAL' do - subject - - expect(WebMock).to have_requested(:put, api_url + '/apis/networking.istio.io/v1alpha3/namespaces/knative-serving/gateways/knative-ingress-gateway').with( - body: { - apiVersion: "networking.istio.io/v1alpha3", - kind: "Gateway", - metadata: { - generation: 1, - labels: { - "networking.knative.dev/ingress-provider" => "istio", - "serving.knative.dev/release" => "v0.7.0" - }, - name: "knative-ingress-gateway", - namespace: "knative-serving", - selfLink: "/apis/networking.istio.io/v1alpha3/namespaces/knative-serving/gateways/knative-ingress-gateway" - }, - spec: { - selector: { - istio: "ingressgateway" - }, - servers: [ - { - hosts: ["*"], - port: { - name: "http", - number: 80, - protocol: "HTTP" - } - }, - { - hosts: ["*"], - port: { - name: "https", - number: 443, - protocol: "HTTPS" - }, - tls: { - mode: "MUTUAL", - privateKey: "/etc/istio/ingressgateway-certs/tls.key", - serverCertificate: "/etc/istio/ingressgateway-certs/tls.crt", - caCertificates: "/etc/istio/ingressgateway-ca-certs/cert.pem" - } - } - ] - } - } - ) - end - end - - context 'when there is an error' do - before do - cluster.application_knative = create(:clusters_applications_knative) - - allow_next_instance_of(described_class) do |instance| - allow(instance).to receive(:configure_passthrough).and_raise(error) - end - end - - context 'Kubeclient::HttpError' do - let(:error) { Kubeclient::HttpError.new(404, nil, nil) } - - it 'puts Knative into an errored state' do - subject - - expect(cluster.application_knative).to be_errored - expect(cluster.application_knative.status_reason).to eq('Kubernetes error: 404') - end - end - - context 'StandardError' do - let(:error) { RuntimeError.new('something went wrong') } - - it 'puts Knative into an errored state' do - subject - - expect(cluster.application_knative).to be_errored - expect(cluster.application_knative.status_reason).to eq('Failed to update.') - end - end - end -end diff --git a/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb b/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb index 064f9e42e96..37478a0bcd9 100644 --- a/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb +++ b/spec/services/clusters/kubernetes/create_or_update_service_account_service_spec.rb @@ -166,7 +166,7 @@ RSpec.describe Clusters::Kubernetes::CreateOrUpdateServiceAccountService do expect(WebMock).to have_requested(:put, api_url + "/apis/rbac.authorization.k8s.io/v1/namespaces/#{namespace}/rolebindings/#{role_binding_name}").with( body: hash_including( - metadata: { name: "gitlab-#{namespace}", namespace: "#{namespace}" }, + metadata: { name: "gitlab-#{namespace}", namespace: namespace.to_s }, roleRef: { apiGroup: 'rbac.authorization.k8s.io', kind: 'ClusterRole', diff --git a/spec/services/dependency_proxy/find_cached_manifest_service_spec.rb b/spec/services/dependency_proxy/find_cached_manifest_service_spec.rb index 607d67d8efe..470c6eb9e03 100644 --- a/spec/services/dependency_proxy/find_cached_manifest_service_spec.rb +++ b/spec/services/dependency_proxy/find_cached_manifest_service_spec.rb @@ -39,6 +39,14 @@ RSpec.describe DependencyProxy::FindCachedManifestService do end end + shared_examples 'returning an error' do + it 'returns an error', :aggregate_failures do + expect(subject[:status]).to eq(:error) + expect(subject[:http_status]).to eq(503) + expect(subject[:message]).to eq('Failed to download the manifest from the external registry') + end + end + context 'when no manifest exists' do let_it_be(:image) { 'new-image' } @@ -101,7 +109,7 @@ RSpec.describe DependencyProxy::FindCachedManifestService do it_behaves_like 'returning no manifest' end - context 'failed connection' do + context 'when the connection fails' do before do expect(DependencyProxy::HeadManifestService).to receive(:new).and_raise(Net::OpenTimeout) end @@ -111,12 +119,24 @@ RSpec.describe DependencyProxy::FindCachedManifestService do context 'and no manifest is cached' do let_it_be(:image) { 'new-image' } - it 'returns an error', :aggregate_failures do - expect(subject[:status]).to eq(:error) - expect(subject[:http_status]).to eq(503) - expect(subject[:message]).to eq('Failed to download the manifest from the external registry') + it_behaves_like 'returning an error' + end + end + + context 'when the connection is successful but with error in result' do + before do + allow_next_instance_of(DependencyProxy::HeadManifestService) do |service| + allow(service).to receive(:execute).and_return(status: :error, http_status: 401, message: "Not found") end end + + it_behaves_like 'using the cached manifest' + + context 'and no manifest is cached' do + let_it_be(:image) { 'new-image' } + + it_behaves_like 'returning no manifest' + end end end end diff --git a/spec/services/deployments/create_for_build_service_spec.rb b/spec/services/deployments/create_for_build_service_spec.rb index a2e1acadcc1..3748df87d99 100644 --- a/spec/services/deployments/create_for_build_service_spec.rb +++ b/spec/services/deployments/create_for_build_service_spec.rb @@ -30,7 +30,9 @@ RSpec.describe Deployments::CreateForBuildService do context 'when creation failure occures' do before do - allow(build).to receive(:create_deployment!) { raise ActiveRecord::RecordInvalid } + allow_next_instance_of(Deployment) do |deployment| + allow(deployment).to receive(:save!) { raise ActiveRecord::RecordInvalid } + end end it 'trackes the exception' do @@ -79,5 +81,88 @@ RSpec.describe Deployments::CreateForBuildService do expect { subject }.not_to change { Deployment.count } end end + + context 'when build has environment attribute' do + let!(:build) do + create(:ci_build, environment: 'production', project: project, + options: { environment: { name: 'production', **kubernetes_options } }) + end + + let!(:environment) { create(:environment, project: project, name: build.expanded_environment_name) } + + let(:kubernetes_options) { {} } + + it 'returns a deployment object with environment' do + expect(subject).to be_a(Deployment) + expect(subject.iid).to be_present + expect(subject.environment.name).to eq('production') + expect(subject.cluster).to be_nil + expect(subject.deployment_cluster).to be_nil + end + + context 'when environment has deployment platform' do + let!(:cluster) { create(:cluster, :provided_by_gcp, projects: [project], managed: managed_cluster) } + let(:managed_cluster) { true } + + it 'sets the cluster and deployment_cluster' do + expect(subject.cluster).to eq(cluster) # until we stop double writing in 12.9: https://gitlab.com/gitlab-org/gitlab/issues/202628 + expect(subject.deployment_cluster.cluster).to eq(cluster) + end + + context 'when a custom namespace is given' do + let(:kubernetes_options) { { kubernetes: { namespace: 'the-custom-namespace' } } } + + context 'when cluster is managed' do + it 'does not set the custom namespace' do + expect(subject.deployment_cluster.kubernetes_namespace).not_to eq('the-custom-namespace') + end + end + + context 'when cluster is not managed' do + let(:managed_cluster) { false } + + it 'sets the custom namespace' do + expect(subject.deployment_cluster.kubernetes_namespace).to eq('the-custom-namespace') + end + end + end + end + + context 'when build already has deployment' do + let!(:build) { create(:ci_build, :with_deployment, project: project, environment: 'production') } + let!(:environment) {} + + it 'returns the persisted deployment' do + expect { subject }.not_to change { Deployment.count } + + is_expected.to eq(build.deployment) + end + end + end + + context 'when build does not start environment' do + where(:action) do + %w[stop prepare verify access] + end + + with_them do + let!(:build) do + create(:ci_build, environment: 'production', project: project, + options: { environment: { name: 'production', action: action } }) + end + + it 'returns nothing' do + is_expected.to be_nil + end + end + end + + context 'when build does not have environment attribute' do + let!(:build) { create(:ci_build, project: project) } + + it 'returns nothing' do + is_expected.to be_nil + end + end end end diff --git a/spec/services/environments/create_for_build_service_spec.rb b/spec/services/environments/create_for_build_service_spec.rb new file mode 100644 index 00000000000..721822f355b --- /dev/null +++ b/spec/services/environments/create_for_build_service_spec.rb @@ -0,0 +1,304 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Environments::CreateForBuildService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let_it_be(:pipeline) { create(:ci_pipeline, project: project) } + + let!(:job) { build(:ci_build, project: project, pipeline: pipeline, **attributes) } + let(:service) { described_class.new } + let(:merge_request) {} + + describe '#execute' do + subject { service.execute(job, merge_request: merge_request) } + + shared_examples_for 'returning a correct environment' do + let(:expected_auto_stop_in_seconds) do + ChronicDuration.parse(expected_auto_stop_in).seconds if expected_auto_stop_in + end + + it 'returns a persisted environment object' do + freeze_time do + expect { subject }.to change { Environment.count }.by(1) + + expect(subject).to be_a(Environment) + expect(subject).to be_persisted + expect(subject.project).to eq(project) + expect(subject.name).to eq(expected_environment_name) + expect(subject.auto_stop_in).to eq(expected_auto_stop_in_seconds) + end + end + + context 'when environment has already existed' do + let!(:environment) do + create(:environment, + project: project, + name: expected_environment_name + ).tap do |env| + env.auto_stop_in = expected_auto_stop_in + end + end + + it 'returns the existing environment object' do + expect { subject }.not_to change { Environment.count } + expect { subject }.not_to change { environment.auto_stop_at } + + expect(subject).to be_persisted + expect(subject).to eq(environment) + end + end + end + + context 'when job has environment name attribute' do + let(:environment_name) { 'production' } + let(:expected_environment_name) { 'production' } + let(:expected_auto_stop_in) { nil } + + let(:attributes) do + { + environment: environment_name, + options: { environment: { name: environment_name } } + } + end + + it_behaves_like 'returning a correct environment' + + context 'and job environment also has an auto_stop_in attribute' do + let(:environment_auto_stop_in) { '5 minutes' } + let(:expected_auto_stop_in) { '5 minutes' } + + let(:attributes) do + { + environment: environment_name, + options: { + environment: { + name: environment_name, + auto_stop_in: environment_auto_stop_in + } + } + } + end + + it_behaves_like 'returning a correct environment' + end + + context 'and job environment has an auto_stop_in variable attribute' do + let(:environment_auto_stop_in) { '10 minutes' } + let(:expected_auto_stop_in) { '10 minutes' } + + let(:attributes) do + { + environment: environment_name, + options: { + environment: { + name: environment_name, + auto_stop_in: '$TTL' + } + }, + yaml_variables: [ + { key: "TTL", value: environment_auto_stop_in, public: true } + ] + } + end + + it_behaves_like 'returning a correct environment' + end + end + + context 'when job has deployment tier attribute' do + let(:attributes) do + { + environment: 'customer-portal', + options: { + environment: { + name: 'customer-portal', + deployment_tier: deployment_tier + } + } + } + end + + let(:deployment_tier) { 'production' } + + context 'when environment has not been created yet' do + it 'sets the specified deployment tier' do + is_expected.to be_production + end + + context 'when deployment tier is staging' do + let(:deployment_tier) { 'staging' } + + it 'sets the specified deployment tier' do + is_expected.to be_staging + end + end + + context 'when deployment tier is unknown' do + let(:deployment_tier) { 'unknown' } + + it 'raises an error' do + expect { subject }.to raise_error(ArgumentError, "'unknown' is not a valid tier") + end + end + end + + context 'when environment has already been created' do + before do + create(:environment, project: project, name: 'customer-portal', tier: :staging) + end + + it 'does not overwrite the specified deployment tier' do + # This is to be updated when a deployment succeeded i.e. Deployments::UpdateEnvironmentService. + is_expected.to be_staging + end + end + end + + context 'when job starts a review app' do + let(:environment_name) { 'review/$CI_COMMIT_REF_NAME' } + let(:expected_environment_name) { "review/#{job.ref}" } + let(:expected_auto_stop_in) { nil } + + let(:attributes) do + { + environment: environment_name, + options: { environment: { name: environment_name } } + } + end + + it_behaves_like 'returning a correct environment' + end + + context 'when job stops a review app' do + let(:environment_name) { 'review/$CI_COMMIT_REF_NAME' } + let(:expected_environment_name) { "review/#{job.ref}" } + let(:expected_auto_stop_in) { nil } + + let(:attributes) do + { + environment: environment_name, + options: { environment: { name: environment_name, action: 'stop' } } + } + end + + it_behaves_like 'returning a correct environment' + end + + context 'when merge_request is provided' do + let(:environment_name) { 'development' } + let(:attributes) { { environment: environment_name, options: { environment: { name: environment_name } } } } + let(:merge_request) { create(:merge_request, source_project: project) } + let(:seed) { described_class.new(job, merge_request: merge_request) } + + context 'and environment does not exist' do + let(:environment_name) { 'review/$CI_COMMIT_REF_NAME' } + + it 'creates an environment associated with the merge request' do + expect { subject }.to change { Environment.count }.by(1) + + expect(subject.merge_request).to eq(merge_request) + end + end + + context 'and environment already exists' do + before do + create(:environment, project: project, name: environment_name) + end + + it 'does not change the merge request associated with the environment' do + expect { subject }.not_to change { Environment.count } + + expect(subject.merge_request).to be_nil + end + end + end + + context 'when a pipeline contains a deployment job' do + let!(:job) { build(:ci_build, :start_review_app, project: project) } + + context 'and the environment does not exist' do + it 'creates the environment specified by the job' do + expect { subject }.to change { Environment.count }.by(1) + + expect(environment).to be_present + expect(job.persisted_environment.name).to eq('review/master') + expect(job.metadata.expanded_environment_name).to eq('review/master') + end + + context 'and the pipeline is for a merge request' do + let(:merge_request) { create(:merge_request, source_project: project) } + + it 'associates the environment with the merge request' do + expect { subject }.to change { Environment.count }.by(1) + + expect(environment.merge_request).to eq(merge_request) + end + end + end + + context 'when an environment already exists' do + before do + create(:environment, project: project, name: 'review/master') + end + + it 'ensures environment existence for the job' do + expect { subject }.not_to change { Environment.count } + + expect(environment).to be_present + expect(job.persisted_environment.name).to eq('review/master') + expect(job.metadata.expanded_environment_name).to eq('review/master') + end + + context 'and the pipeline is for a merge request' do + let(:merge_request) { create(:merge_request, source_project: project) } + + it 'does not associate the environment with the merge request' do + expect { subject }.not_to change { Environment.count } + + expect(environment.merge_request).to be_nil + end + end + end + + context 'when an environment name contains an invalid character' do + before do + job.pipeline = build(:ci_pipeline, ref: '!!!', project: project) + end + + it 'sets the failure status' do + expect { subject }.not_to change { Environment.count } + + expect(job).to be_failed + expect(job).to be_environment_creation_failure + expect(job.persisted_environment).to be_nil + end + end + end + + context 'when a pipeline contains a teardown job' do + let!(:job) { build(:ci_build, :stop_review_app, project: project) } + + it 'ensures environment existence for the job' do + expect { subject }.to change { Environment.count }.by(1) + + expect(environment).to be_present + expect(job.persisted_environment.name).to eq('review/master') + expect(job.metadata.expanded_environment_name).to eq('review/master') + end + end + + context 'when a pipeline does not contain a deployment job' do + let!(:job) { build(:ci_build, project: project) } + + it 'does not create any environments' do + expect { subject }.not_to change { Environment.count } + end + end + + def environment + project.environments.find_by_name('review/master') + end + end +end diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 06f0eb1efbc..c3ae062a4b2 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -20,33 +20,6 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi end end - shared_examples 'Snowplow event' do - let(:label) { nil } - - it 'is not emitted if FF is disabled' do - stub_feature_flags(feature_flag_name => false) - - subject - - expect_no_snowplow_event - end - - it 'is emitted' do - params = { - category: category, - action: action, - namespace: namespace, - user: user, - project: project, - label: label - }.compact - - subject - - expect_snowplow_event(**params) - end - end - describe 'Issues' do describe '#open_issue' do let(:issue) { create(:issue) } @@ -95,14 +68,17 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION } end - it_behaves_like 'Snowplow event' do - let(:category) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION.to_s } - let(:label) { 'merge_requests_users' } - let(:action) { 'create' } + it_behaves_like 'Snowplow event tracking with RedisHLL context' do + let(:feature_flag_name) { :route_hll_to_snowplow_phase2 } + let(:category) { described_class.name } + let(:action) { 'created' } + let(:label) { 'usage_activity_by_stage_monthly.create.merge_requests_users' } let(:namespace) { project.namespace } let(:project) { merge_request.project } let(:user) { merge_request.author } - let(:feature_flag_name) { :route_hll_to_snowplow_phase2 } + let(:context) do + [Gitlab::Tracking::ServicePingContext.new(data_source: :redis_hll, event: 'merge_requests_users').to_context] + end end end @@ -121,14 +97,17 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION } end - it_behaves_like 'Snowplow event' do - let(:category) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION.to_s } - let(:label) { 'merge_requests_users' } - let(:action) { 'close' } + it_behaves_like 'Snowplow event tracking with RedisHLL context' do + let(:feature_flag_name) { :route_hll_to_snowplow_phase2 } + let(:category) { described_class.name } + let(:action) { 'closed' } + let(:label) { 'usage_activity_by_stage_monthly.create.merge_requests_users' } let(:namespace) { project.namespace } let(:project) { merge_request.project } let(:user) { merge_request.author } - let(:feature_flag_name) { :route_hll_to_snowplow_phase2 } + let(:context) do + [Gitlab::Tracking::ServicePingContext.new(data_source: :redis_hll, event: 'merge_requests_users').to_context] + end end end @@ -147,14 +126,17 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION } end - it_behaves_like 'Snowplow event' do - let(:category) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION.to_s } - let(:label) { 'merge_requests_users' } - let(:action) { 'merge' } + it_behaves_like 'Snowplow event tracking with RedisHLL context' do + let(:feature_flag_name) { :route_hll_to_snowplow_phase2 } + let(:category) { described_class.name } + let(:action) { 'merged' } + let(:label) { 'usage_activity_by_stage_monthly.create.merge_requests_users' } let(:namespace) { project.namespace } let(:project) { merge_request.project } let(:user) { merge_request.author } - let(:feature_flag_name) { :route_hll_to_snowplow_phase2 } + let(:context) do + [Gitlab::Tracking::ServicePingContext.new(data_source: :redis_hll, event: 'merge_requests_users').to_context] + end end end @@ -330,11 +312,16 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::PUSH_ACTION } end - it_behaves_like 'Snowplow event' do + it_behaves_like 'Snowplow event tracking with RedisHLL context' do let(:category) { described_class.to_s } - let(:action) { 'action_active_users_project_repo' } + let(:action) { :push } let(:namespace) { project.namespace } let(:feature_flag_name) { :route_hll_to_snowplow } + let(:label) { 'usage_activity_by_stage_monthly.create.action_monthly_active_users_project_repo' } + let(:context) do + [Gitlab::Tracking::ServicePingContext.new(data_source: :redis_hll, + event: 'action_active_users_project_repo').to_context] + end end end @@ -355,11 +342,16 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi let(:event_action) { Gitlab::UsageDataCounters::TrackUniqueEvents::PUSH_ACTION } end - it_behaves_like 'Snowplow event' do + it_behaves_like 'Snowplow event tracking with RedisHLL context' do let(:category) { described_class.to_s } - let(:action) { 'action_active_users_project_repo' } + let(:action) { :push } let(:namespace) { project.namespace } let(:feature_flag_name) { :route_hll_to_snowplow } + let(:label) { 'usage_activity_by_stage_monthly.create.action_monthly_active_users_project_repo' } + let(:context) do + [Gitlab::Tracking::ServicePingContext.new(data_source: :redis_hll, + event: 'action_active_users_project_repo').to_context] + end end end @@ -495,7 +487,7 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi stub_feature_flags(route_hll_to_snowplow_phase2: false) end - it 'doesnt emit snowwplow events', :snowplow do + it 'doesnt emit snowplow events', :snowplow do subject expect_no_snowplow_event @@ -518,19 +510,22 @@ RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redi end context 'when it is a diff note' do - it_behaves_like "it records the event in the event counter" do - let(:note) { create(:diff_note_on_merge_request) } - end + let(:note) { create(:diff_note_on_merge_request) } - it_behaves_like 'Snowplow event' do + it_behaves_like "it records the event in the event counter" + + it_behaves_like 'Snowplow event tracking with RedisHLL context' do + let(:feature_flag_name) { :route_hll_to_snowplow_phase2 } let(:note) { create(:diff_note_on_merge_request) } - let(:category) { Gitlab::UsageDataCounters::TrackUniqueEvents::MERGE_REQUEST_ACTION.to_s } - let(:label) { 'merge_requests_users' } - let(:action) { 'comment' } - let(:project) { note.project } + let(:category) { described_class.name } + let(:action) { 'commented' } + let(:label) { 'usage_activity_by_stage_monthly.create.merge_requests_users' } let(:namespace) { project.namespace } - let(:feature_flag_name) { :route_hll_to_snowplow_phase2 } + let(:project) { note.project } let(:user) { author } + let(:context) do + [Gitlab::Tracking::ServicePingContext.new(data_source: :redis_hll, event: 'merge_requests_users').to_context] + end end end diff --git a/spec/services/git/base_hooks_service_spec.rb b/spec/services/git/base_hooks_service_spec.rb index a8d753ff124..5afd7b30ab0 100644 --- a/spec/services/git/base_hooks_service_spec.rb +++ b/spec/services/git/base_hooks_service_spec.rb @@ -4,10 +4,10 @@ require 'spec_helper' RSpec.describe Git::BaseHooksService do include RepoHelpers - include GitHelpers - let(:user) { create(:user) } - let(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :repository) } + let(:oldrev) { Gitlab::Git::BLANK_SHA } let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0 let(:ref) { 'refs/tags/v1.1.0' } @@ -150,11 +150,16 @@ RSpec.describe Git::BaseHooksService do end shared_examples 'creates pipeline with params and expected variables' do + let(:pipeline_service) { double(execute: service_response) } + let(:service_response) { double(error?: false, payload: pipeline, message: "Error") } + let(:pipeline) { double(persisted?: true) } + it 'calls the create pipeline service' do expect(Ci::CreatePipelineService) .to receive(:new) .with(project, user, pipeline_params) - .and_return(double(execute!: true)) + .and_return(pipeline_service) + expect(subject).not_to receive(:log_pipeline_errors) subject.execute end @@ -239,4 +244,85 @@ RSpec.describe Git::BaseHooksService do it_behaves_like 'creates pipeline with params and expected variables' end end + + describe "Pipeline creation" do + let(:pipeline_params) do + { + after: newrev, + before: oldrev, + checkout_sha: checkout_sha, + push_options: push_options, + ref: ref, + variables_attributes: variables_attributes + } + end + + let(:pipeline_service) { double(execute: service_response) } + let(:push_options) { {} } + let(:variables_attributes) { [] } + + context "when the pipeline is persisted" do + let(:pipeline) { double(persisted?: true) } + + context "and there are no errors" do + let(:service_response) { double(error?: false, payload: pipeline, message: "Error") } + + it "returns success" do + expect(Ci::CreatePipelineService) + .to receive(:new) + .with(project, user, pipeline_params) + .and_return(pipeline_service) + + expect(subject.execute[:status]).to eq(:success) + end + end + + context "and there are errors" do + let(:service_response) { double(error?: true, payload: pipeline, message: "Error") } + + it "does not log errors and returns success" do + # This behaviour is due to the save_on_errors: true setting that is the default in the execute method. + expect(Ci::CreatePipelineService) + .to receive(:new) + .with(project, user, pipeline_params) + .and_return(pipeline_service) + expect(subject).not_to receive(:log_pipeline_errors).with(service_response.message) + + expect(subject.execute[:status]).to eq(:success) + end + end + end + + context "when the pipeline wasn't persisted" do + let(:pipeline) { double(persisted?: false) } + + context "and there are no errors" do + let(:service_response) { double(error?: false, payload: pipeline, message: nil) } + + it "returns success" do + expect(Ci::CreatePipelineService) + .to receive(:new) + .with(project, user, pipeline_params) + .and_return(pipeline_service) + expect(subject).to receive(:log_pipeline_errors).with(service_response.message) + + expect(subject.execute[:status]).to eq(:success) + end + end + + context "and there are errors" do + let(:service_response) { double(error?: true, payload: pipeline, message: "Error") } + + it "logs errors and returns success" do + expect(Ci::CreatePipelineService) + .to receive(:new) + .with(project, user, pipeline_params) + .and_return(pipeline_service) + expect(subject).to receive(:log_pipeline_errors).with(service_response.message) + + expect(subject.execute[:status]).to eq(:success) + end + end + end + end end diff --git a/spec/services/git/tag_push_service_spec.rb b/spec/services/git/tag_push_service_spec.rb index 87dbf79a245..597254d46fa 100644 --- a/spec/services/git/tag_push_service_spec.rb +++ b/spec/services/git/tag_push_service_spec.rb @@ -4,7 +4,6 @@ require 'spec_helper' RSpec.describe Git::TagPushService do include RepoHelpers - include GitHelpers let(:user) { create(:user) } let(:project) { create(:project, :repository) } diff --git a/spec/services/google_cloud/generate_pipeline_service_spec.rb b/spec/services/google_cloud/generate_pipeline_service_spec.rb index 75494f229b5..a78d8ff6661 100644 --- a/spec/services/google_cloud/generate_pipeline_service_spec.rb +++ b/spec/services/google_cloud/generate_pipeline_service_spec.rb @@ -67,7 +67,7 @@ RSpec.describe GoogleCloud::GeneratePipelineService do let_it_be(:service_params) { { action: GoogleCloud::GeneratePipelineService::ACTION_DEPLOY_TO_CLOUD_RUN } } let_it_be(:service) { described_class.new(project, maintainer, service_params) } - before do + before_all do project.add_maintainer(maintainer) file_name = '.gitlab-ci.yml' @@ -103,6 +103,15 @@ EOF expect(pipeline[:include]).to be_present expect(gitlab_ci_yml).to include('https://gitlab.com/gitlab-org/incubation-engineering/five-minute-production/library/-/raw/main/gcp/cloud-run.gitlab-ci.yml') end + + it 'stringifies keys from the existing pipelines' do + response = service.execute + + branch_name = response[:branch_name] + gitlab_ci_yml = project.repository.gitlab_ci_yml_for(branch_name) + + expect(YAML.safe_load(gitlab_ci_yml).keys).to eq(%w[stages build-java test-java include]) + end end describe 'when there is an existing pipeline with `deploy` stage' do diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index 0a8164c9ca3..0425ba3e631 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -271,33 +271,4 @@ RSpec.describe Groups::CreateService, '#execute' do end end end - - describe 'logged_out_marketing_header experiment', :experiment do - let(:service) { described_class.new(user, group_params) } - - subject { service.execute } - - before do - stub_experiments(logged_out_marketing_header: :candidate) - end - - it 'tracks signed_up event' do - expect(experiment(:logged_out_marketing_header)).to track( - :namespace_created, - namespace: an_instance_of(Group) - ).on_next_instance.with_context(actor: user) - - subject - end - - context 'when group has not been persisted' do - let(:service) { described_class.new(user, group_params.merge(name: '<script>alert("Attack!")</script>')) } - - it 'does not track signed_up event' do - expect(experiment(:logged_out_marketing_header)).not_to track(:namespace_created) - - subject - end - end - end end diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index 36e868fa5f1..f2dbb69f855 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -36,37 +36,17 @@ RSpec.describe Groups::DestroyService do end context 'bot tokens', :sidekiq_inline do - context 'when user_destroy_with_limited_execution_time_worker is enabled' do - it 'initiates group bot removal', :aggregate_failures do - bot = create(:user, :project_bot) - group.add_developer(bot) - create(:personal_access_token, user: bot) - - destroy_group(group, user, async) - - expect( - Users::GhostUserMigration.where(user: bot, - initiator_user: user) - ).to be_exists - end - end - - context 'when user_destroy_with_limited_execution_time_worker is disabled' do - before do - stub_feature_flags(user_destroy_with_limited_execution_time_worker: false) - end - - it 'removes group bot', :aggregate_failures do - bot = create(:user, :project_bot) - group.add_developer(bot) - token = create(:personal_access_token, user: bot) + it 'initiates group bot removal', :aggregate_failures do + bot = create(:user, :project_bot) + group.add_developer(bot) + create(:personal_access_token, user: bot) - destroy_group(group, user, async) + destroy_group(group, user, async) - expect(PersonalAccessToken.find_by(id: token.id)).to be_nil - expect(User.find_by(id: bot.id)).to be_nil - expect(User.find_by(id: user.id)).not_to be_nil - end + expect( + Users::GhostUserMigration.where(user: bot, + initiator_user: user) + ).to be_exists end end @@ -146,7 +126,7 @@ RSpec.describe Groups::DestroyService do end expect { destroy_group(group, user, false) } - .to raise_error(Groups::DestroyService::DestroyError, "Project #{project.id} can't be deleted" ) + .to raise_error(Groups::DestroyService::DestroyError, "Project #{project.id} can't be deleted") end end diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index b543661e9a0..3cf2c875341 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -200,7 +200,7 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do let_it_be(:project2) { create(:project, :private, namespace: group) } it_behaves_like 'project namespace path is in sync with project path' do - let(:group_full_path) { "#{group.path}" } + let(:group_full_path) { group.path.to_s } let(:projects_with_project_namespace) { [project1, project2] } end end @@ -274,7 +274,7 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do end it_behaves_like 'project namespace path is in sync with project path' do - let(:group_full_path) { "#{new_parent_group.full_path}" } + let(:group_full_path) { new_parent_group.full_path.to_s } let(:projects_with_project_namespace) { [project] } end end diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 5c87b9ac8bb..c758d3d5477 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -101,6 +101,15 @@ RSpec.describe Groups::UpdateService do expect(public_group.reload.name).to eq('new-name') end end + + context 'when the path does not change' do + let(:params) { { name: 'new-name', path: public_group.path } } + + it 'allows the update' do + expect(subject).to be true + expect(public_group.reload.name).to eq('new-name') + end + end end context 'within subgroup' do diff --git a/spec/services/groups/update_shared_runners_service_spec.rb b/spec/services/groups/update_shared_runners_service_spec.rb index 6e938984052..98eccedeace 100644 --- a/spec/services/groups/update_shared_runners_service_spec.rb +++ b/spec/services/groups/update_shared_runners_service_spec.rb @@ -127,7 +127,7 @@ RSpec.describe Groups::UpdateSharedRunnersService do end context 'when parent does not allow' do - let_it_be(:parent) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false ) } + let_it_be(:parent) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false) } let_it_be(:group) { create(:group, :shared_runners_disabled, allow_descendants_override_disabled_shared_runners: false, parent: parent) } it 'results error' do diff --git a/spec/services/import/fogbugz_service_spec.rb b/spec/services/import/fogbugz_service_spec.rb index 7b86c5c45b0..027d0240a7a 100644 --- a/spec/services/import/fogbugz_service_spec.rb +++ b/spec/services/import/fogbugz_service_spec.rb @@ -119,7 +119,7 @@ RSpec.describe Import::FogbugzService do let(:error_messages_array) { instance_double(Array, join: "something went wrong") } let(:errors_double) { instance_double(ActiveModel::Errors, full_messages: error_messages_array, :[] => nil) } let(:project_double) { instance_double(Project, persisted?: false, errors: errors_double) } - let(:project_creator) { instance_double(Gitlab::FogbugzImport::ProjectCreator, execute: project_double ) } + let(:project_creator) { instance_double(Gitlab::FogbugzImport::ProjectCreator, execute: project_double) } before do allow(Gitlab::FogbugzImport::ProjectCreator).to receive(:new).and_return(project_creator) diff --git a/spec/services/import/gitlab_projects/file_acquisition_strategies/file_upload_spec.rb b/spec/services/import/gitlab_projects/file_acquisition_strategies/file_upload_spec.rb index 28af6219812..3c788138157 100644 --- a/spec/services/import/gitlab_projects/file_acquisition_strategies/file_upload_spec.rb +++ b/spec/services/import/gitlab_projects/file_acquisition_strategies/file_upload_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe ::Import::GitlabProjects::FileAcquisitionStrategies::FileUpload, :aggregate_failures do - let(:file) { UploadedFile.new( File.join('spec', 'features', 'projects', 'import_export', 'test_project_export.tar.gz') ) } + let(:file) { UploadedFile.new(File.join('spec', 'features', 'projects', 'import_export', 'test_project_export.tar.gz')) } describe 'validation' do it 'validates presence of file' do diff --git a/spec/services/incident_management/timeline_event_tags/create_service_spec.rb b/spec/services/incident_management/timeline_event_tags/create_service_spec.rb new file mode 100644 index 00000000000..c1b993ce3d9 --- /dev/null +++ b/spec/services/incident_management/timeline_event_tags/create_service_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IncidentManagement::TimelineEventTags::CreateService do + let_it_be(:user_with_permissions) { create(:user) } + let_it_be(:user_without_permissions) { create(:user) } + let_it_be_with_reload(:project) { create(:project) } + + let(:current_user) { user_with_permissions } + let(:args) { { 'name': 'Test tag 1', 'project_path': project.full_path } } + + let(:service) { described_class.new(project, current_user, args) } + + before do + project.add_maintainer(user_with_permissions) + project.add_developer(user_without_permissions) + end + + describe '#execute' do + shared_examples 'error response' do |message| + it 'has an informative message' do + expect(execute).to be_error + expect(execute.message).to eq(message) + end + end + + shared_examples 'success response' do + it 'has timeline event tag' do + expect(execute).to be_success + + result = execute.payload[:timeline_event_tag] + expect(result).to be_a(::IncidentManagement::TimelineEventTag) + expect(result.name).to eq(args[:name]) + expect(result.project).to eq(project) + end + end + + subject(:execute) { service.execute } + + context 'when current user is nil' do + let(:current_user) { nil } + + it_behaves_like 'error response', + 'You have insufficient permissions to manage timeline event tags for this project' + end + + context 'when user does not have permissions to create tags' do + let(:current_user) { user_without_permissions } + + it_behaves_like 'error response', + 'You have insufficient permissions to manage timeline event tags for this project' + end + + context 'when error occurs during creation' do + let(:args) { {} } + + it_behaves_like 'error response', "Name can't be blank and Name is invalid" + end + + context 'when user has permissions' do + it_behaves_like 'success response' + + it 'creates database record' do + expect { execute }.to change { + ::IncidentManagement::TimelineEventTag.where(project_id: project.id).count + }.by(1) + end + end + end +end diff --git a/spec/services/incident_management/timeline_events/create_service_spec.rb b/spec/services/incident_management/timeline_events/create_service_spec.rb index a7f448c825f..b10862a78b5 100644 --- a/spec/services/incident_management/timeline_events/create_service_spec.rb +++ b/spec/services/incident_management/timeline_events/create_service_spec.rb @@ -8,6 +8,9 @@ RSpec.describe IncidentManagement::TimelineEvents::CreateService do let_it_be(:project) { create(:project) } let_it_be_with_refind(:incident) { create(:incident, project: project) } let_it_be(:comment) { create(:note, project: project, noteable: incident) } + let_it_be(:timeline_event_tag) do + create(:incident_management_timeline_event_tag, name: 'Test tag 1', project: project) + end let(:args) do { @@ -134,6 +137,67 @@ RSpec.describe IncidentManagement::TimelineEvents::CreateService do end end + context 'when timeline event tag names are passed' do + let(:args) do + { + note: 'note', + occurred_at: Time.current, + action: 'new comment', + promoted_from_note: comment, + timeline_event_tag_names: ['Test tag 1'] + } + end + + it_behaves_like 'success response' + + it 'matches the tag name' do + result = execute.payload[:timeline_event] + expect(result.timeline_event_tags.first).to eq(timeline_event_tag) + end + + context 'when predefined tags are passed' do + let(:args) do + { + note: 'note', + occurred_at: Time.current, + action: 'new comment', + promoted_from_note: comment, + timeline_event_tag_names: ['start time', 'end time'] + } + end + + it_behaves_like 'success response' + + it 'matches the two tags on the event and creates on project' do + result = execute.payload[:timeline_event] + + expect(result.timeline_event_tags.count).to eq(2) + expect(result.timeline_event_tags.by_names(['Start time', 'End time']).pluck_names) + .to match_array(['Start time', 'End time']) + expect(project.incident_management_timeline_event_tags.pluck_names) + .to include('Start time', 'End time') + end + end + + context 'when invalid tag names are passed' do + let(:args) do + { + note: 'note', + occurred_at: Time.current, + action: 'new comment', + promoted_from_note: comment, + timeline_event_tag_names: ['some other time'] + } + end + + it_behaves_like 'error response', "Following tags don't exist: [\"some other time\"]" + + it 'does not create timeline event' do + expect { execute }.not_to change(IncidentManagement::TimelineEvent, :count) + end + end + end + context 'with editable param' do let(:args) do { @@ -161,6 +225,38 @@ RSpec.describe IncidentManagement::TimelineEvents::CreateService do it 'successfully creates a database record', :aggregate_failures do expect { execute }.to change { ::IncidentManagement::TimelineEvent.count }.by(1) end + + context 'when note is more than 280 characters long' do + let(:args) do + { + note: 'a' * 281, + occurred_at: Time.current, + action: 'new comment', + promoted_from_note: comment, + auto_created: auto_created + } + end + + let(:auto_created) { false } + + context 'when was not promoted from note' do + let(:comment) { nil } + + context 'when auto_created is true' do + let(:auto_created) { true } + + it_behaves_like 'success response' + end + + context 'when auto_created is false' do + it_behaves_like 'error response', 'Timeline text is too long (maximum is 280 characters)' + end + end + + context 'when promoted from note' do + it_behaves_like 'success response' + end + end end describe 'automatically created timeline events' do @@ -229,6 +325,17 @@ RSpec.describe IncidentManagement::TimelineEvents::CreateService do it_behaves_like 'successfully created timeline event' end + describe '.change_severity' do + subject(:execute) { described_class.change_severity(incident, current_user) } + + let_it_be(:severity) { create(:issuable_severity, severity: :critical, issue: incident) } + + let(:expected_note) { "@#{current_user.username} changed the incident severity to **Critical - S1**" } + let(:expected_action) { 'severity' } + + it_behaves_like 'successfully created timeline event' + end + describe '.change_labels' do subject(:execute) do described_class.change_labels(incident, current_user, added_labels: added, removed_labels: removed) diff --git a/spec/services/incident_management/timeline_events/update_service_spec.rb b/spec/services/incident_management/timeline_events/update_service_spec.rb index 5d8518cf2ef..2373a73e108 100644 --- a/spec/services/incident_management/timeline_events/update_service_spec.rb +++ b/spec/services/incident_management/timeline_events/update_service_spec.rb @@ -87,6 +87,12 @@ RSpec.describe IncidentManagement::TimelineEvents::UpdateService do it_behaves_like 'error response', "Timeline text can't be blank" end + context 'when note is more than 280 characters long' do + let(:params) { { note: 'n' * 281, occurred_at: occurred_at } } + + it_behaves_like 'error response', 'Timeline text is too long (maximum is 280 characters)' + end + context 'when occurred_at is nil' do let(:params) { { note: 'Updated note' } } diff --git a/spec/services/issuable/discussions_list_service_spec.rb b/spec/services/issuable/discussions_list_service_spec.rb new file mode 100644 index 00000000000..2ce47f42a72 --- /dev/null +++ b/spec/services/issuable/discussions_list_service_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Issuable::DiscussionsListService do + let_it_be(:current_user) { create(:user) } + let_it_be(:group) { create(:group, :private) } + let_it_be(:project) { create(:project, :repository, :private, group: group) } + let_it_be(:milestone) { create(:milestone, project: project) } + let_it_be(:label) { create(:label, project: project) } + + let(:finder_params_for_issuable) { {} } + + subject(:discussions_service) { described_class.new(current_user, issuable, finder_params_for_issuable) } + + describe 'fetching notes for issue' do + let_it_be(:issuable) { create(:issue, project: project) } + + it_behaves_like 'listing issuable discussions', :guest, 1, 7 + end + + describe 'fetching notes for merge requests' do + let_it_be(:issuable) { create(:merge_request, source_project: project, target_project: project) } + + it_behaves_like 'listing issuable discussions', :reporter, 0, 6 + end +end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index e88fe1b42f0..ef92b6984d5 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -397,9 +397,26 @@ RSpec.describe Issues::CloseService do end context 'when issue is not confidential' do + let(:expected_payload) do + include( + event_type: 'issue', + object_kind: 'issue', + changes: { + closed_at: { current: kind_of(Time), previous: nil }, + state_id: { current: 2, previous: 1 }, + updated_at: { current: kind_of(Time), previous: kind_of(Time) } + }, + object_attributes: include( + closed_at: kind_of(Time), + state: 'closed', + action: 'close' + ) + ) + end + it 'executes issue hooks' do - expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) - expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :issue_hooks) + expect(project).to receive(:execute_hooks).with(expected_payload, :issue_hooks) + expect(project).to receive(:execute_integrations).with(expected_payload, :issue_hooks) described_class.new(project: project, current_user: user).close_issue(issue) end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 5fe4c693451..5ddf91e167e 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -391,22 +391,61 @@ RSpec.describe Issues::CreateService do end end - it 'executes issue hooks when issue is not confidential' do - opts = { title: 'Title', description: 'Description', confidential: false } + describe 'executing hooks' do + let(:opts) { { title: 'Title', description: 'Description' } } + let(:expected_payload) do + include( + event_type: 'issue', + object_kind: 'issue', + changes: { + author_id: { current: user.id, previous: nil }, + created_at: { current: kind_of(Time), previous: nil }, + description: { current: opts[:description], previous: nil }, + id: { current: kind_of(Integer), previous: nil }, + iid: { current: kind_of(Integer), previous: nil }, + project_id: { current: project.id, previous: nil }, + title: { current: opts[:title], previous: nil }, + updated_at: { current: kind_of(Time), previous: nil } + }, + object_attributes: include( + opts.merge( + author_id: user.id, + project_id: project.id + ) + ) + ) + end + + it 'executes issue hooks' do + expect(project).to receive(:execute_hooks).with(expected_payload, :issue_hooks) + expect(project).to receive(:execute_integrations).with(expected_payload, :issue_hooks) - expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) - expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :issue_hooks) + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute + end - described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute - end + context 'when issue is confidential' do + let(:expected_payload) do + include( + event_type: 'confidential_issue', + object_kind: 'issue', + changes: include( + confidential: { current: true, previous: false } + ), + object_attributes: include(confidential: true) + ) + end - it 'executes confidential issue hooks when issue is confidential' do - opts = { title: 'Title', description: 'Description', confidential: true } + before do + opts[:confidential] = true + end - expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks) - expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :confidential_issue_hooks) + it 'executes confidential issue hooks' do + expect(project).to receive(:execute_hooks).with(expected_payload, :confidential_issue_hooks) + expect(project).to receive(:execute_integrations).with(expected_payload, :confidential_issue_hooks) - described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute + end + end end context 'after_save callback to store_mentions' do diff --git a/spec/services/issues/export_csv_service_spec.rb b/spec/services/issues/export_csv_service_spec.rb index d04480bec18..66d017464bf 100644 --- a/spec/services/issues/export_csv_service_spec.rb +++ b/spec/services/issues/export_csv_service_spec.rb @@ -185,7 +185,7 @@ RSpec.describe Issues::ExportCsvService do labeled_rows = csv.select { |entry| labeled_issues.map(&:iid).include?(entry['Issue ID'].to_i) } expect(labeled_rows.count).to eq(2) - expect(labeled_rows.map { |entry| entry['Labels'] }).to all( eq("Feature,Idea") ) + expect(labeled_rows.map { |entry| entry['Labels'] }).to all(eq("Feature,Idea")) end end end diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index 23180f75eb3..655c5085fdc 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -228,18 +228,48 @@ RSpec.describe Issues::MoveService do end context 'project issue hooks' do - let!(:hook) { create(:project_hook, project: old_project, issues_events: true) } + let_it_be(:old_project_hook) { create(:project_hook, project: old_project, issues_events: true) } + let_it_be(:new_project_hook) { create(:project_hook, project: new_project, issues_events: true) } + + let(:expected_new_project_hook_payload) do + hash_including( + event_type: 'issue', + object_kind: 'issue', + object_attributes: include( + project_id: new_project.id, + state: 'opened', + action: 'open' + ) + ) + end + + let(:expected_old_project_hook_payload) do + hash_including( + event_type: 'issue', + object_kind: 'issue', + changes: { + state_id: { current: 2, previous: 1 }, + closed_at: { current: kind_of(Time), previous: nil }, + updated_at: { current: kind_of(Time), previous: kind_of(Time) } + }, + object_attributes: include( + id: old_issue.id, + closed_at: kind_of(Time), + state: 'closed', + action: 'close' + ) + ) + end - it 'executes project issue hooks' do - allow_next_instance_of(WebHookService) do |instance| - allow(instance).to receive(:execute) + it 'executes project issue hooks for both projects' do + expect_next_instance_of(WebHookService, new_project_hook, expected_new_project_hook_payload, 'issue_hooks') do |service| + expect(service).to receive(:async_execute).once + end + expect_next_instance_of(WebHookService, old_project_hook, expected_old_project_hook_payload, 'issue_hooks') do |service| + expect(service).to receive(:async_execute).once end - # Ideally, we'd test that `WebHookWorker.jobs.size` increased by 1, - # but since the entire spec run takes place in a transaction, we never - # actually get to the `after_commit` hook that queues these jobs. - expect { move_service.execute(old_issue, new_project) } - .not_to raise_error # Sidekiq::Worker::EnqueueFromTransactionError + move_service.execute(old_issue, new_project) end end diff --git a/spec/services/issues/relative_position_rebalancing_service_spec.rb b/spec/services/issues/relative_position_rebalancing_service_spec.rb index 37a94e1d6a2..27c0394ac8b 100644 --- a/spec/services/issues/relative_position_rebalancing_service_spec.rb +++ b/spec/services/issues/relative_position_rebalancing_service_spec.rb @@ -34,10 +34,6 @@ RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_s end end - before do - stub_feature_flags(issue_rebalancing_with_retry: false) - end - def issues_in_position_order project.reload.issues.order_by_relative_position.to_a end @@ -97,8 +93,12 @@ RSpec.describe Issues::RelativePositionRebalancingService, :clean_gitlab_redis_s it 'resumes a started rebalance even if there are already too many rebalances running' do Gitlab::Redis::SharedState.with do |redis| - redis.sadd("gitlab:issues-position-rebalances:running_rebalances", "#{::Gitlab::Issues::Rebalancing::State::PROJECT}/#{project.id}") - redis.sadd("gitlab:issues-position-rebalances:running_rebalances", "1/100") + redis.sadd("gitlab:issues-position-rebalances:running_rebalances", + [ + "#{::Gitlab::Issues::Rebalancing::State::PROJECT}/#{project.id}", + "1/100" + ] + ) end caching = service.send(:caching) diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb index 477b44f4c2c..6013826f9b1 100644 --- a/spec/services/issues/reopen_service_spec.rb +++ b/spec/services/issues/reopen_service_spec.rb @@ -85,9 +85,25 @@ RSpec.describe Issues::ReopenService do end context 'when issue is not confidential' do + let(:expected_payload) do + include( + event_type: 'issue', + object_kind: 'issue', + changes: { + closed_at: { current: nil, previous: kind_of(Time) }, + state_id: { current: 1, previous: 2 }, + updated_at: { current: kind_of(Time), previous: kind_of(Time) } + }, + object_attributes: include( + state: 'opened', + action: 'reopen' + ) + ) + end + it 'executes issue hooks' do - expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) - expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :issue_hooks) + expect(project).to receive(:execute_hooks).with(expected_payload, :issue_hooks) + expect(project).to receive(:execute_integrations).with(expected_payload, :issue_hooks) execute end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 20b1a1f58bb..f1ee62fd589 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -104,10 +104,33 @@ RSpec.describe Issues::UpdateService, :mailer do expect(issue.issue_customer_relations_contacts.last.contact).to eq contact end - it 'updates issue milestone when passing `milestone` param' do - update_issue(milestone: milestone) + context 'when updating milestone' do + before do + update_issue({ milestone: nil }) + end - expect(issue.milestone).to eq milestone + it 'updates issue milestone when passing `milestone` param' do + expect { update_issue({ milestone: milestone }) } + .to change(issue, :milestone).to(milestone).from(nil) + end + + it "triggers 'issuableMilestoneUpdated'" do + expect(GraphqlTriggers).to receive(:issuable_milestone_updated).with(issue).and_call_original + + update_issue({ milestone: milestone }) + end + + context 'when milestone remains unchanged' do + before do + update_issue({ title: 'abc', milestone: milestone }) + end + + it "does not trigger 'issuableMilestoneUpdated'" do + expect(GraphqlTriggers).not_to receive(:issuable_milestone_updated) + + update_issue({ milestone: milestone }) + end + end end context 'when sentry identifier is given' do @@ -520,7 +543,7 @@ RSpec.describe Issues::UpdateService, :mailer do end end - context 'when decription is not changed' do + context 'when description is not changed' do it 'does not trigger GraphQL description updated subscription' do expect(GraphqlTriggers).not_to receive(:issuable_description_updated) @@ -1379,7 +1402,7 @@ RSpec.describe Issues::UpdateService, :mailer do end end - include_examples 'issuable update service' do + it_behaves_like 'issuable update service' do let(:open_issuable) { issue } let(:closed_issuable) { create(:closed_issue, project: project) } end diff --git a/spec/services/labels/promote_service_spec.rb b/spec/services/labels/promote_service_spec.rb index a10aaa14030..3af6cf4c8f4 100644 --- a/spec/services/labels/promote_service_spec.rb +++ b/spec/services/labels/promote_service_spec.rb @@ -171,7 +171,7 @@ RSpec.describe Labels::PromoteService do end context 'when there is an existing identical group label' do - let!(:existing_group_label) { create(:group_label, group: group_1, title: project_label_1_1.title ) } + let!(:existing_group_label) { create(:group_label, group: group_1, title: project_label_1_1.title) } it 'uses the existing group label' do expect { service.execute(project_label_1_1) } diff --git a/spec/services/loose_foreign_keys/process_deleted_records_service_spec.rb b/spec/services/loose_foreign_keys/process_deleted_records_service_spec.rb new file mode 100644 index 00000000000..1824f822ba8 --- /dev/null +++ b/spec/services/loose_foreign_keys/process_deleted_records_service_spec.rb @@ -0,0 +1,198 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe LooseForeignKeys::ProcessDeletedRecordsService do + include MigrationsHelpers + + def create_table_structure + migration = ActiveRecord::Migration.new.extend(Gitlab::Database::MigrationHelpers::LooseForeignKeyHelpers) + + migration.create_table :_test_loose_fk_parent_table_1 + migration.create_table :_test_loose_fk_parent_table_2 + + migration.create_table :_test_loose_fk_child_table_1_1 do |t| + t.bigint :parent_id + end + + migration.create_table :_test_loose_fk_child_table_1_2 do |t| + t.bigint :parent_id_with_different_column + end + + migration.create_table :_test_loose_fk_child_table_2_1 do |t| + t.bigint :parent_id + end + + migration.track_record_deletions(:_test_loose_fk_parent_table_1) + migration.track_record_deletions(:_test_loose_fk_parent_table_2) + end + + let(:all_loose_foreign_key_definitions) do + { + '_test_loose_fk_parent_table_1' => [ + ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( + '_test_loose_fk_child_table_1_1', + '_test_loose_fk_parent_table_1', + { + column: 'parent_id', + on_delete: :async_delete, + gitlab_schema: :gitlab_main + } + ), + ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( + '_test_loose_fk_child_table_1_2', + '_test_loose_fk_parent_table_1', + { + column: 'parent_id_with_different_column', + on_delete: :async_nullify, + gitlab_schema: :gitlab_main + } + ) + ], + '_test_loose_fk_parent_table_2' => [ + ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.new( + '_test_loose_fk_child_table_2_1', + '_test_loose_fk_parent_table_2', + { + column: 'parent_id', + on_delete: :async_delete, + gitlab_schema: :gitlab_main + } + ) + ] + } + end + + let(:connection) { ::ApplicationRecord.connection } + + let(:loose_fk_parent_table_1) { table(:_test_loose_fk_parent_table_1) } + let(:loose_fk_parent_table_2) { table(:_test_loose_fk_parent_table_2) } + let(:loose_fk_child_table_1_1) { table(:_test_loose_fk_child_table_1_1) } + let(:loose_fk_child_table_1_2) { table(:_test_loose_fk_child_table_1_2) } + let(:loose_fk_child_table_2_1) { table(:_test_loose_fk_child_table_2_1) } + + before(:all) do + create_table_structure + end + + after(:all) do + migration = ActiveRecord::Migration.new + + migration.drop_table :_test_loose_fk_parent_table_1 + migration.drop_table :_test_loose_fk_parent_table_2 + migration.drop_table :_test_loose_fk_child_table_1_1 + migration.drop_table :_test_loose_fk_child_table_1_2 + migration.drop_table :_test_loose_fk_child_table_2_1 + end + + before do + allow(Gitlab::Database::LooseForeignKeys).to receive(:definitions_by_table) + .and_return(all_loose_foreign_key_definitions) + + parent_record_1 = loose_fk_parent_table_1.create! + loose_fk_child_table_1_1.create!(parent_id: parent_record_1.id) + loose_fk_child_table_1_2.create!(parent_id_with_different_column: parent_record_1.id) + + parent_record_2 = loose_fk_parent_table_1.create! + 2.times { loose_fk_child_table_1_1.create!(parent_id: parent_record_2.id) } + 3.times { loose_fk_child_table_1_2.create!(parent_id_with_different_column: parent_record_2.id) } + + parent_record_3 = loose_fk_parent_table_2.create! + 5.times { loose_fk_child_table_2_1.create!(parent_id: parent_record_3.id) } + + loose_fk_parent_table_1.delete_all + loose_fk_parent_table_2.delete_all + end + + describe '#execute' do + def execute + ::Gitlab::Database::SharedModel.using_connection(connection) do + described_class.new(connection: connection).execute + end + end + + it 'cleans up all rows' do + execute + + expect(loose_fk_child_table_1_1.count).to eq(0) + expect(loose_fk_child_table_1_2.where(parent_id_with_different_column: nil).count).to eq(4) + expect(loose_fk_child_table_2_1.count).to eq(0) + end + + it 'returns stats for records cleaned up' do + stats = execute + + expect(stats[:delete_count]).to eq(8) + expect(stats[:update_count]).to eq(4) + end + + it 'records the Apdex as success: true' do + expect(::Gitlab::Metrics::LooseForeignKeysSlis).to receive(:record_apdex) + .with(success: true, db_config_name: 'main') + + execute + end + + it 'records the error rate as error: false' do + expect(::Gitlab::Metrics::LooseForeignKeysSlis).to receive(:record_error_rate) + .with(error: false, db_config_name: 'main') + + execute + end + + context 'when the amount of records to clean up exceeds BATCH_SIZE' do + before do + stub_const('LooseForeignKeys::CleanupWorker::BATCH_SIZE', 2) + end + + it 'cleans up everything over multiple batches' do + expect(LooseForeignKeys::BatchCleanerService).to receive(:new).exactly(:twice).and_call_original + + execute + + expect(loose_fk_child_table_1_1.count).to eq(0) + expect(loose_fk_child_table_1_2.where(parent_id_with_different_column: nil).count).to eq(4) + expect(loose_fk_child_table_2_1.count).to eq(0) + end + end + + context 'when the amount of records to clean up exceeds the total MAX_DELETES' do + def count_deletable_rows + loose_fk_child_table_1_1.count + loose_fk_child_table_2_1.count + end + + before do + stub_const('LooseForeignKeys::ModificationTracker::MAX_DELETES', 2) + stub_const('LooseForeignKeys::CleanerService::DELETE_LIMIT', 1) + end + + it 'cleans up MAX_DELETES and leaves the rest for the next run' do + expect { execute }.to change { count_deletable_rows }.by(-2) + expect(count_deletable_rows).to be > 0 + end + + it 'records the Apdex as success: false' do + expect(::Gitlab::Metrics::LooseForeignKeysSlis).to receive(:record_apdex) + .with(success: false, db_config_name: 'main') + + execute + end + end + + context 'when cleanup raises an error' do + before do + expect_next_instance_of(::LooseForeignKeys::BatchCleanerService) do |service| + allow(service).to receive(:execute).and_raise("Something broke") + end + end + + it 'records the error rate as error: true and does not increment apdex' do + expect(::Gitlab::Metrics::LooseForeignKeysSlis).to receive(:record_error_rate) + .with(error: true, db_config_name: 'main') + expect(::Gitlab::Metrics::LooseForeignKeysSlis).not_to receive(:record_apdex) + + expect { execute }.to raise_error("Something broke") + end + end + end +end diff --git a/spec/services/markup/rendering_service_spec.rb b/spec/services/markup/rendering_service_spec.rb new file mode 100644 index 00000000000..a5711a8cbc4 --- /dev/null +++ b/spec/services/markup/rendering_service_spec.rb @@ -0,0 +1,163 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Markup::RenderingService do + describe '#execute' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) do + user = create(:user, username: 'gfm') + project.add_maintainer(user) + user + end + + let_it_be(:context) { { project: project } } + let_it_be(:postprocess_context) { { current_user: user } } + + let(:file_name) { nil } + let(:text) { 'Noël' } + + subject do + described_class + .new(text, file_name: file_name, context: context, postprocess_context: postprocess_context) + .execute + end + + context 'when text is missing' do + let(:text) { nil } + + it 'returns an empty string' do + is_expected.to eq('') + end + end + + context 'when file_name is missing' do + it 'returns html (rendered by Banzai)' do + expected_html = '<p data-sourcepos="1:1-1:5" dir="auto">Noël</p>' + + expect(Banzai).to receive(:render).with(text, context) { expected_html } + + is_expected.to eq(expected_html) + end + end + + context 'when postprocess_context is missing' do + let(:file_name) { 'foo.txt' } + let(:postprocess_context) { nil } + + it 'returns html (rendered by Banzai)' do + expected_html = '<pre class="plain-readme">Noël</pre>' + + expect(Banzai).not_to receive(:post_process) { expected_html } + + is_expected.to eq(expected_html) + end + end + + context 'when rendered context is present' do + let(:rendered) { 'rendered text' } + let(:file_name) { 'foo.md' } + + it 'returns an empty string' do + context[:rendered] = rendered + + is_expected.to eq(rendered) + end + end + + context 'when file is a markdown file' do + let(:file_name) { 'foo.md' } + + it 'returns html (rendered by Banzai)' do + expected_html = '<p data-sourcepos="1:1-1:5" dir="auto">Noël</p>' + + expect(Banzai).to receive(:render).with(text, context) { expected_html } + + is_expected.to eq(expected_html) + end + + context 'when renderer returns an error' do + before do + allow(Banzai).to receive(:render).and_raise(StandardError, "An error") + end + + it 'returns html (rendered by ActionView:TextHelper)' do + is_expected.to eq('<p>Noël</p>') + end + + it 'logs the error' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with( + instance_of(StandardError), + project_id: context[:project].id, file_name: 'foo.md' + ) + + subject + end + end + end + + context 'when file is asciidoc file' do + let(:file_name) { 'foo.adoc' } + + it 'returns html (rendered by Gitlab::Asciidoc)' do + expected_html = "<div>\n<p>Noël</p>\n</div>" + + expect(Gitlab::Asciidoc).to receive(:render).with(text, context) { expected_html } + + is_expected.to eq(expected_html) + end + end + + context 'when file is a regular text file' do + let(:file_name) { 'foo.txt' } + + it 'returns html (rendered by ActionView::TagHelper)' do + is_expected.to eq('<pre class="plain-readme">Noël</pre>') + end + end + + context 'when file has an unknown type' do + let(:file_name) { 'foo.tex' } + + it 'returns html (rendered by Gitlab::OtherMarkup)' do + expected_html = 'Noël' + + expect(Gitlab::OtherMarkup).to receive(:render).with(file_name, text, context) { expected_html } + + is_expected.to eq(expected_html) + end + end + + context 'when rendering takes too long' do + let(:file_name) { 'foo.bar' } + + before do + stub_const("Markup::RenderingService::RENDER_TIMEOUT", 0.1) + allow(Gitlab::OtherMarkup).to receive(:render) do + sleep(0.2) + text + end + end + + it 'times out' do + expect(Gitlab::RenderTimeout).to receive(:timeout).and_call_original + expect(Gitlab::ErrorTracking).to receive(:track_exception).with( + instance_of(Timeout::Error), + project_id: context[:project].id, file_name: file_name + ) + + is_expected.to eq("<p>#{text}</p>") + end + + context 'when markup_rendering_timeout is disabled' do + it 'waits until the execution completes' do + stub_feature_flags(markup_rendering_timeout: false) + + expect(Gitlab::RenderTimeout).not_to receive(:timeout) + + is_expected.to eq(text) + end + end + end + end +end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 8559c02be57..d0f009f1321 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -77,7 +77,7 @@ RSpec.describe Members::DestroyService do end end - shared_examples 'a service destroying an access requester' do + shared_examples 'a service destroying an access request of another user' do it_behaves_like 'a service destroying a member' it 'calls Member#after_decline_request' do @@ -85,12 +85,16 @@ RSpec.describe Members::DestroyService do described_class.new(current_user).execute(member, **opts) end + end + + shared_examples 'a service destroying an access request of self' do + it_behaves_like 'a service destroying a member' context 'when current user is the member' do it 'does not call Member#after_decline_request' do expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) - described_class.new(member_user).execute(member, **opts) + described_class.new(current_user).execute(member, **opts) end end end @@ -277,11 +281,24 @@ RSpec.describe Members::DestroyService do group.add_owner(current_user) end - it_behaves_like 'a service destroying an access requester' do + it_behaves_like 'a service destroying an access request of another user' do + let(:member) { group_project.requesters.find_by(user_id: member_user.id) } + end + + it_behaves_like 'a service destroying an access request of another user' do + let(:member) { group.requesters.find_by(user_id: member_user.id) } + end + end + + context 'on withdrawing their own access request' do + let(:opts) { { skip_subresources: true } } + let(:current_user) { member_user } + + it_behaves_like 'a service destroying an access request of self' do let(:member) { group_project.requesters.find_by(user_id: member_user.id) } end - it_behaves_like 'a service destroying an access requester' do + it_behaves_like 'a service destroying an access request of self' do let(:member) { group.requesters.find_by(user_id: member_user.id) } end end diff --git a/spec/services/members/invite_service_spec.rb b/spec/services/members/invite_service_spec.rb index 6dbe161ee02..23d4d671afc 100644 --- a/spec/services/members/invite_service_spec.rb +++ b/spec/services/members/invite_service_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ let(:params) { {} } let(:base_params) { { access_level: Gitlab::Access::GUEST, source: project, invite_source: '_invite_source_' } } - subject(:result) { described_class.new(user, base_params.merge(params) ).execute } + subject(:result) { described_class.new(user, base_params.merge(params)).execute } context 'when there is a valid member invited' do let(:params) { { email: 'email@example.org' } } @@ -393,7 +393,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ context 'when email is already a member with a user on the project' do let!(:existing_member) { create(:project_member, :guest, project: project) } - let(:params) { { email: "#{existing_member.user.email}", access_level: ProjectMember::MAINTAINER } } + let(:params) { { email: existing_member.user.email.to_s, access_level: ProjectMember::MAINTAINER } } it 'allows re-invite of an already invited email and updates the access_level' do expect { result }.not_to change(ProjectMember, :count) @@ -403,7 +403,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ context 'when email belongs to an existing user as a confirmed secondary email' do let(:secondary_email) { create(:email, :confirmed, email: 'secondary@example.com', user: existing_member.user) } - let(:params) { { email: "#{secondary_email.email}" } } + let(:params) { { email: secondary_email.email.to_s } } it 'allows re-invite to an already invited email' do expect_to_create_members(count: 0) diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index f919d6d1516..eb8fae03c39 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -3,18 +3,34 @@ require 'spec_helper' RSpec.describe Members::UpdateService do - let(:project) { create(:project, :public) } - let(:group) { create(:group, :public) } - let(:current_user) { create(:user) } - let(:member_user) { create(:user) } - let(:permission) { :update } - let(:member) { source.members_and_requesters.find_by!(user_id: member_user.id) } - let(:access_level) { Gitlab::Access::MAINTAINER } - let(:params) do - { access_level: access_level } + let_it_be(:project) { create(:project, :public) } + let_it_be(:group) { create(:group, :public) } + let_it_be(:current_user) { create(:user) } + let_it_be(:member_user1) { create(:user) } + let_it_be(:member_user2) { create(:user) } + let_it_be(:member_users) { [member_user1, member_user2] } + let_it_be(:permission) { :update } + let_it_be(:access_level) { Gitlab::Access::MAINTAINER } + let(:members) { source.members_and_requesters.where(user_id: member_users).to_a } + let(:update_service) { described_class.new(current_user, params) } + let(:params) { { access_level: access_level } } + let(:updated_members) do + result = subject + Array.wrap(result[:members] || result[:member]) end - subject { described_class.new(current_user, params).execute(member, permission: permission) } + before do + member_users.first.tap do |member_user| + expires_at = 10.days.from_now + project.add_member(member_user, Gitlab::Access::DEVELOPER, expires_at: expires_at) + group.add_member(member_user, Gitlab::Access::DEVELOPER, expires_at: expires_at) + end + + member_users[1..].each do |member_user| + project.add_developer(member_user) + group.add_developer(member_user) + end + end shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do it 'raises Gitlab::Access::AccessDeniedError' do @@ -23,164 +39,326 @@ RSpec.describe Members::UpdateService do end end - shared_examples 'a service updating a member' do - it 'updates the member' do - expect(TodosDestroyer::EntityLeaveWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name) + shared_examples 'current user cannot update the given members' do + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do + let_it_be(:source) { project } + end + + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do + let_it_be(:source) { group } + end + end + + shared_examples 'returns error status when params are invalid' do + let_it_be(:params) { { expires_at: 2.days.ago } } - updated_member = subject.fetch(:member) + specify do + expect(subject[:status]).to eq(:error) + end + end - expect(updated_member).to be_valid - expect(updated_member.access_level).to eq(access_level) + shared_examples 'a service updating members' do + it 'updates the members' do + new_access_levels = updated_members.map(&:access_level) + + expect(updated_members).not_to be_empty + expect(updated_members).to all(be_valid) + expect(new_access_levels).to all(be access_level) end it 'returns success status' do - result = subject.fetch(:status) + expect(subject.fetch(:status)).to eq(:success) + end - expect(result).to eq(:success) + it 'invokes after_execute with correct args' do + members.each do |member| + expect(update_service).to receive(:after_execute).with( + action: permission, + old_access_level: member.human_access, + old_expiry: member.expires_at, + member: member + ) + end + + subject end - context 'when member is downgraded to guest' do - shared_examples 'schedules to delete confidential todos' do - it do - expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once + it 'authorization update callback is triggered' do + expect(members).to all(receive(:refresh_member_authorized_projects).once) - updated_member = subject.fetch(:member) + subject + end + + it 'does not enqueues todos for deletion' do + members.each do |member| + expect(TodosDestroyer::EntityLeaveWorker) + .not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name) + end - expect(updated_member).to be_valid - expect(updated_member.access_level).to eq(Gitlab::Access::GUEST) + subject + end + + context 'when members are downgraded to guest' do + shared_examples 'schedules to delete confidential todos' do + it do + members.each do |member| + expect(TodosDestroyer::EntityLeaveWorker) + .to receive(:perform_in) + .with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once + end + + new_access_levels = updated_members.map(&:access_level) + expect(updated_members).to all(be_valid) + expect(new_access_levels).to all(be Gitlab::Access::GUEST) end end context 'with Gitlab::Access::GUEST level as a string' do - let(:params) { { access_level: Gitlab::Access::GUEST.to_s } } + let_it_be(:params) { { access_level: Gitlab::Access::GUEST.to_s } } it_behaves_like 'schedules to delete confidential todos' end context 'with Gitlab::Access::GUEST level as an integer' do - let(:params) { { access_level: Gitlab::Access::GUEST } } + let_it_be(:params) { { access_level: Gitlab::Access::GUEST } } it_behaves_like 'schedules to delete confidential todos' end end context 'when access_level is invalid' do - let(:params) { { access_level: 'invalid' } } + let_it_be(:params) { { access_level: 'invalid' } } it 'raises an error' do - expect { described_class.new(current_user, params) }.to raise_error(ArgumentError, 'invalid value for Integer(): "invalid"') + expect { described_class.new(current_user, params) } + .to raise_error(ArgumentError, 'invalid value for Integer(): "invalid"') end end - context 'when member is not valid' do - let(:params) { { expires_at: 2.days.ago } } + context 'when members update results in no change' do + let(:params) { { access_level: members.first.access_level } } - it 'returns error status' do - result = subject + it 'does not invoke update! and post_update' do + expect(update_service).not_to receive(:save!) + expect(update_service).not_to receive(:post_update) - expect(result[:status]).to eq(:error) + expect(subject[:status]).to eq(:success) end - end - end - - before do - project.add_developer(member_user) - group.add_developer(member_user) - end - context 'when current user cannot update the given member' do - it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do - let(:source) { project } - end + it 'authorization update callback is not triggered' do + members.each { |member| expect(member).not_to receive(:refresh_member_authorized_projects) } - it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do - let(:source) { group } + subject + end end end - context 'when current user can update the given member' do + shared_examples 'updating a project' do + let_it_be(:group_project) { create(:project, group: create(:group)) } + let_it_be(:source) { group_project } + before do - project.add_maintainer(current_user) - group.add_owner(current_user) + member_users.each { |member_user| group_project.add_developer(member_user) } end - it_behaves_like 'a service updating a member' do - let(:source) { project } - end + context 'as a project maintainer' do + before do + group_project.add_maintainer(current_user) + end - it_behaves_like 'a service updating a member' do - let(:source) { group } - end - end + it_behaves_like 'a service updating members' - context 'in a project' do - let_it_be(:group_project) { create(:project, group: create(:group)) } + context 'when member update results in an error' do + it_behaves_like 'a service returning an error' + end - let(:source) { group_project } + context 'and updating members to OWNER' do + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do + let_it_be(:access_level) { Gitlab::Access::OWNER } + end + end - context 'a project maintainer' do - before do - group_project.add_maintainer(current_user) + context 'and updating themselves to OWNER' do + let(:members) { source.members_and_requesters.find_by!(user_id: current_user.id) } + + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do + let_it_be(:access_level) { Gitlab::Access::OWNER } + end end - context 'cannot update a member to OWNER' do + context 'and downgrading members from OWNER' do before do - group_project.add_developer(member_user) + member_users.each { |member_user| group_project.add_owner(member_user) } end it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do - let(:access_level) { Gitlab::Access::OWNER } + let_it_be(:access_level) { Gitlab::Access::MAINTAINER } end end + end - context 'cannot update themselves to OWNER' do - let(:member) { source.members_and_requesters.find_by!(user_id: current_user.id) } + context 'when current_user is considered an owner in the project via inheritance' do + before do + group_project.group.add_owner(current_user) + end + context 'and can update members to OWNER' do before do - group_project.add_developer(member_user) + member_users.each { |member_user| group_project.add_developer(member_user) } end - it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do - let(:access_level) { Gitlab::Access::OWNER } + it_behaves_like 'a service updating members' do + let_it_be(:access_level) { Gitlab::Access::OWNER } end end - context 'cannot downgrade a member from OWNER' do + context 'and can downgrade members from OWNER' do before do - group_project.add_owner(member_user) + member_users.each { |member_user| group_project.add_owner(member_user) } end - it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do - let(:access_level) { Gitlab::Access::MAINTAINER } + it_behaves_like 'a service updating members' do + let_it_be(:access_level) { Gitlab::Access::MAINTAINER } end end end + end + + shared_examples 'updating a group' do + let_it_be(:source) { group } + + before do + group.add_owner(current_user) + end + + it_behaves_like 'a service updating members' + + context 'when member update results in an error' do + it_behaves_like 'a service returning an error' + end + + context 'when group members expiration date is updated' do + let_it_be(:params) { { expires_at: 20.days.from_now } } + let(:notification_service) { instance_double(NotificationService) } - context 'owners' do before do - # so that `current_user` is considered an `OWNER` in the project via inheritance. - group_project.group.add_owner(current_user) + allow(NotificationService).to receive(:new).and_return(notification_service) end - context 'can update a member to OWNER' do - before do - group_project.add_developer(member_user) + it 'emails the users that their group membership expiry has changed' do + members.each do |member| + expect(notification_service).to receive(:updated_group_member_expiration).with(member) end - it_behaves_like 'a service updating a member' do - let(:access_level) { Gitlab::Access::OWNER } - end + subject end + end + end - context 'can downgrade a member from OWNER' do - before do - group_project.add_owner(member_user) + context 'when :bulk_update_membership_roles feature flag is disabled' do + let(:member) { source.members_and_requesters.find_by!(user_id: member_user1.id) } + let(:members) { [member] } + + subject { update_service.execute(member, permission: permission) } + + shared_examples 'a service returning an error' do + before do + allow(member).to receive(:save) do + member.errors.add(:user_id) + member.errors.add(:access_level) end + .and_return(false) + end + + it_behaves_like 'returns error status when params are invalid' + + it 'returns the error' do + response = subject + + expect(response[:status]).to eq(:error) + expect(response[:message]).to eq('User is invalid and Access level is invalid') + end + end + + before do + stub_feature_flags(bulk_update_membership_roles: false) + end + + it_behaves_like 'current user cannot update the given members' + it_behaves_like 'updating a project' + it_behaves_like 'updating a group' + end + + subject { update_service.execute(members, permission: permission) } + + shared_examples 'a service returning an error' do + it_behaves_like 'returns error status when params are invalid' - it_behaves_like 'a service updating a member' do - let(:access_level) { Gitlab::Access::MAINTAINER } + context 'when a member update results in invalid record' do + let(:member2) { members.second } + + before do + allow(member2).to receive(:save!) do + member2.errors.add(:user_id) + member2.errors.add(:access_level) + end.and_raise(ActiveRecord::RecordInvalid) + end + + it 'returns the error' do + response = subject + + expect(response[:status]).to eq(:error) + expect(response[:message]).to eq('User is invalid and Access level is invalid') + end + + it 'rollbacks back the entire update' do + old_access_levels = members.pluck(:access_level) + + subject + + expect(members.each(&:reset).pluck(:access_level)).to eq(old_access_levels) + end + end + end + + it_behaves_like 'current user cannot update the given members' + it_behaves_like 'updating a project' + it_behaves_like 'updating a group' + + context 'with a single member' do + let(:member) { create(:group_member, group: group) } + let(:members) { member } + + before do + group.add_owner(current_user) + end + + it 'returns the correct response' do + expect(subject[:member]).to eq(member) + end + end + + context 'when current user is an admin', :enable_admin_mode do + let_it_be(:current_user) { create(:admin) } + let_it_be(:source) { group } + + context 'when all owners are being downgraded' do + before do + member_users.each { |member_user| group.add_owner(member_user) } + end + + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' + end + + context 'when all blocked owners are being downgraded' do + before do + member_users.each do |member_user| + group.add_owner(member_user) + member_user.block end end + + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' end end end diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index 0846ec7f50e..da6492aca95 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -33,9 +33,17 @@ RSpec.describe MergeRequests::ApprovalService do service.execute(merge_request) end + it_behaves_like 'does not trigger GraphQL subscription mergeRequestMergeStatusUpdated' do + let(:action) { service.execute(merge_request) } + end + it 'does not publish MergeRequests::ApprovedEvent' do expect { service.execute(merge_request) }.not_to publish_event(MergeRequests::ApprovedEvent) end + + it_behaves_like 'does not trigger GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { service.execute(merge_request) } + end end context 'with an already approved MR' do @@ -46,6 +54,14 @@ RSpec.describe MergeRequests::ApprovalService do it 'does not create an approval' do expect { service.execute(merge_request) }.not_to change { merge_request.approvals.size } end + + it_behaves_like 'does not trigger GraphQL subscription mergeRequestMergeStatusUpdated' do + let(:action) { service.execute(merge_request) } + end + + it_behaves_like 'does not trigger GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { service.execute(merge_request) } + end end context 'with valid approval' do @@ -67,6 +83,14 @@ RSpec.describe MergeRequests::ApprovalService do .to publish_event(MergeRequests::ApprovedEvent) .with(current_user_id: user.id, merge_request_id: merge_request.id) end + + it_behaves_like 'triggers GraphQL subscription mergeRequestMergeStatusUpdated' do + let(:action) { service.execute(merge_request) } + end + + it_behaves_like 'triggers GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { service.execute(merge_request) } + end end context 'user cannot update the merge request' do @@ -77,6 +101,14 @@ RSpec.describe MergeRequests::ApprovalService do it 'does not update approvals' do expect { service.execute(merge_request) }.not_to change { merge_request.approvals.size } end + + it_behaves_like 'does not trigger GraphQL subscription mergeRequestMergeStatusUpdated' do + let(:action) { service.execute(merge_request) } + end + + it_behaves_like 'does not trigger GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { service.execute(merge_request) } + end end end end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 6a6f01e6a95..4f27ff30da7 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -93,7 +93,7 @@ RSpec.describe MergeRequests::BuildService do shared_examples 'with a Default.md template' do let(:files) { { '.gitlab/merge_request_templates/Default.md' => 'Default template contents' } } - let(:project) { create(:project, :custom_repo, files: files ) } + let(:project) { create(:project, :custom_repo, files: files) } it 'the template description is preferred' do expect(merge_request.description).to eq('Default template contents') @@ -306,7 +306,7 @@ RSpec.describe MergeRequests::BuildService do context 'a Default.md template is defined' do let(:files) { { '.gitlab/merge_request_templates/Default.md' => 'Default template contents' } } - let(:project) { create(:project, :custom_repo, files: files ) } + let(:project) { create(:project, :custom_repo, files: files) } it 'appends the closing description to a Default.md template' do expected_description = ['Default template contents', closing_message].compact.join("\n\n") @@ -386,7 +386,7 @@ RSpec.describe MergeRequests::BuildService do context 'a Default.md template is defined' do let(:files) { { '.gitlab/merge_request_templates/Default.md' => 'Default template contents' } } - let(:project) { create(:project, :custom_repo, files: files ) } + let(:project) { create(:project, :custom_repo, files: files) } it 'keeps the description from the initial params' do expect(merge_request.description).to eq(description) @@ -425,7 +425,7 @@ RSpec.describe MergeRequests::BuildService do context 'a Default.md template is defined' do let(:files) { { '.gitlab/merge_request_templates/Default.md' => 'Default template contents' } } - let(:project) { create(:project, :custom_repo, files: files ) } + let(:project) { create(:project, :custom_repo, files: files) } it 'appends the closing description to a Default.md template' do expected_description = ['Default template contents', closing_message].compact.join("\n\n") @@ -486,7 +486,7 @@ RSpec.describe MergeRequests::BuildService do context 'a Default.md template is defined' do let(:files) { { '.gitlab/merge_request_templates/Default.md' => 'Default template contents' } } - let(:project) { create(:project, :custom_repo, files: files ) } + let(:project) { create(:project, :custom_repo, files: files) } it 'appends the closing description to a Default.md template' do expected_description = ['Default template contents', closing_message].compact.join("\n\n") @@ -715,7 +715,7 @@ RSpec.describe MergeRequests::BuildService do context 'when a Default template is found' do context 'when its contents cannot be retrieved' do let(:files) { { '.gitlab/merge_request_templates/OtherTemplate.md' => 'Other template contents' } } - let(:project) { create(:project, :custom_repo, files: files ) } + let(:project) { create(:project, :custom_repo, files: files) } it 'does not modify the merge request description' do allow(TemplateFinder).to receive(:all_template_names).and_return({ @@ -732,7 +732,7 @@ RSpec.describe MergeRequests::BuildService do context 'when its contents can be retrieved' do let(:files) { { '.gitlab/merge_request_templates/Default.md' => 'Default template contents' } } - let(:project) { create(:project, :custom_repo, files: files ) } + let(:project) { create(:project, :custom_repo, files: files) } it 'modifies the merge request description' do merge_request.description = nil diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 0bc8258af42..da8e8d944d6 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -336,6 +336,12 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do it_behaves_like 'reviewer_ids filter' do let(:execute) { service.execute } end + + context 'when called in a transaction' do + it 'does not raise an error' do + expect { MergeRequest.transaction { described_class.new(project: project, current_user: user, params: opts).execute } }.not_to raise_error + end + end end it_behaves_like 'issuable record that supports quick actions' do @@ -495,15 +501,40 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do project.add_developer(user) end - it 'creates the merge request', :sidekiq_might_not_need_inline do - expect_next_instance_of(MergeRequest) do |instance| - expect(instance).to receive(:eager_fetch_ref!).and_call_original + context 'when async_merge_request_diff_creation is enabled' do + before do + stub_feature_flags(async_merge_request_diff_creation: true) end - merge_request = described_class.new(project: project, current_user: user, params: opts).execute + it 'creates the merge request', :sidekiq_inline do + expect_next_instance_of(MergeRequest) do |instance| + expect(instance).not_to receive(:eager_fetch_ref!) + end - expect(merge_request).to be_persisted - expect(merge_request.iid).to be > 0 + merge_request = described_class.new(project: project, current_user: user, params: opts).execute + + expect(merge_request).to be_persisted + expect(merge_request.iid).to be > 0 + expect(merge_request.merge_request_diff).not_to be_empty + end + end + + context 'when async_merge_request_diff_creation is disabled' do + before do + stub_feature_flags(async_merge_request_diff_creation: false) + end + + it 'creates the merge request' do + expect_next_instance_of(MergeRequest) do |instance| + expect(instance).to receive(:eager_fetch_ref!).and_call_original + end + + merge_request = described_class.new(project: project, current_user: user, params: opts).execute + + expect(merge_request).to be_persisted + expect(merge_request.iid).to be > 0 + expect(merge_request.merge_request_diff).not_to be_empty + end end it 'does not create the merge request when the target project is archived' do diff --git a/spec/services/merge_requests/mergeability/run_checks_service_spec.rb b/spec/services/merge_requests/mergeability/run_checks_service_spec.rb index cf34923795e..c56b38bccc1 100644 --- a/spec/services/merge_requests/mergeability/run_checks_service_spec.rb +++ b/spec/services/merge_requests/mergeability/run_checks_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe MergeRequests::Mergeability::RunChecksService do +RSpec.describe MergeRequests::Mergeability::RunChecksService, :clean_gitlab_redis_cache do subject(:run_checks) { described_class.new(merge_request: merge_request, params: {}) } describe '#execute' do @@ -104,18 +104,6 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService do expect(execute.success?).to eq(true) end end - - context 'when mergeability_caching is turned off' do - before do - stub_feature_flags(mergeability_caching: false) - end - - it 'does not call the results store' do - expect(Gitlab::MergeRequests::Mergeability::ResultsStore).not_to receive(:new) - - expect(execute.success?).to eq(true) - end - end end end @@ -161,11 +149,11 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService do let_it_be(:merge_request) { create(:merge_request) } context 'when the execute method has been executed' do - before do - run_checks.execute - end - context 'when all the checks succeed' do + before do + run_checks.execute + end + it 'returns nil' do expect(failure_reason).to eq(nil) end diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb index c24b83e21a6..ee23238314e 100644 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -190,14 +190,6 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar target_branch: 'conflict-start') end - it 'does not change the merge ref HEAD' do - expect(merge_request.merge_ref_head).to be_nil - - subject - - expect(merge_request.reload.merge_ref_head).not_to be_nil - end - it 'returns ServiceResponse.error and keeps merge status as cannot_be_merged' do result = subject @@ -351,27 +343,5 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar end end end - - context 'merge with conflicts' do - it 'calls MergeToRefService with true allow_conflicts param' do - expect(MergeRequests::MergeToRefService).to receive(:new) - .with(project: project, current_user: merge_request.author, params: { allow_conflicts: true }).and_call_original - - subject - end - - context 'when display_merge_conflicts_in_diff is disabled' do - before do - stub_feature_flags(display_merge_conflicts_in_diff: false) - end - - it 'calls MergeToRefService with false allow_conflicts param' do - expect(MergeRequests::MergeToRefService).to receive(:new) - .with(project: project, current_user: merge_request.author, params: { allow_conflicts: false }).and_call_original - - subject - end - end - end end end diff --git a/spec/services/merge_requests/remove_approval_service_spec.rb b/spec/services/merge_requests/remove_approval_service_spec.rb index 5a319e90a68..7b38f0d1c45 100644 --- a/spec/services/merge_requests/remove_approval_service_spec.rb +++ b/spec/services/merge_requests/remove_approval_service_spec.rb @@ -45,6 +45,14 @@ RSpec.describe MergeRequests::RemoveApprovalService do execute! end + + it_behaves_like 'triggers GraphQL subscription mergeRequestMergeStatusUpdated' do + let(:action) { execute! } + end + + it_behaves_like 'triggers GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { execute! } + end end context 'with a user who has not approved' do @@ -61,6 +69,14 @@ RSpec.describe MergeRequests::RemoveApprovalService do execute! end + + it_behaves_like 'does not trigger GraphQL subscription mergeRequestMergeStatusUpdated' do + let(:action) { execute! } + end + + it_behaves_like 'does not trigger GraphQL subscription mergeRequestReviewersUpdated' do + let(:action) { execute! } + end end end end diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index 9210242a11e..471bb03f18c 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' RSpec.describe MergeRequests::SquashService do - include GitHelpers - let(:service) { described_class.new(project: project, current_user: user, params: { merge_request: merge_request }) } let(:user) { project.first_owner } let(:project) { create(:project, :repository) } @@ -109,11 +107,10 @@ RSpec.describe MergeRequests::SquashService do end it 'has the same diff as the merge request, but a different SHA' do - rugged = rugged_repo(project.repository) - mr_diff = rugged.diff(merge_request.diff_base_sha, merge_request.diff_head_sha) - squash_diff = rugged.diff(merge_request.diff_start_sha, squash_sha) + mr_diff = project.repository.diff(merge_request.diff_base_sha, merge_request.diff_head_sha) + squash_diff = project.repository.diff(merge_request.diff_start_sha, squash_sha) - expect(squash_diff.patch.length).to eq(mr_diff.patch.length) + expect(squash_diff.size).to eq(mr_diff.size) expect(squash_commit.sha).not_to eq(merge_request.diff_head_sha) end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 1d67574b06d..da78f86c7c8 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -794,7 +794,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end it "does not try to mark as unchecked if it's already unchecked" do - expect(merge_request).to receive(:unchecked?).and_return(true) + allow(merge_request).to receive(:unchecked?).twice.and_return(true) expect(merge_request).not_to receive(:mark_as_unchecked) update_merge_request({ target_branch: "target" }) @@ -1148,7 +1148,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end end - include_examples 'issuable update service' do + it_behaves_like 'issuable update service' do let(:open_issuable) { merge_request } let(:closed_issuable) { create(:closed_merge_request, source_project: project) } end diff --git a/spec/services/milestones/transfer_service_spec.rb b/spec/services/milestones/transfer_service_spec.rb index b15d90d685c..de02226661c 100644 --- a/spec/services/milestones/transfer_service_spec.rb +++ b/spec/services/milestones/transfer_service_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Milestones::TransferService do let(:new_group) { create(:group) } let(:old_group) { create(:group) } let(:project) { create(:project, namespace: old_group) } - let(:group_milestone) { create(:milestone, group: old_group) } + let(:group_milestone) { create(:milestone, :closed, group: old_group) } let(:group_milestone2) { create(:milestone, group: old_group) } let(:project_milestone) { create(:milestone, project: project) } let!(:issue_with_group_milestone) { create(:issue, project: project, milestone: group_milestone) } @@ -38,6 +38,7 @@ RSpec.describe Milestones::TransferService do expect(new_milestone).not_to eq(group_milestone) expect(new_milestone.title).to eq(group_milestone.title) expect(new_milestone.project_milestone?).to be_truthy + expect(new_milestone.state).to eq("closed") end context 'when milestone is from an ancestor group' do @@ -88,6 +89,7 @@ RSpec.describe Milestones::TransferService do expect(new_milestone).not_to eq(group_milestone) expect(new_milestone.title).to eq(group_milestone.title) expect(new_milestone.project_milestone?).to be_truthy + expect(new_milestone.state).to eq("closed") end it 'does not apply new project milestone to issuables with project milestone' do diff --git a/spec/services/namespaces/statistics_refresher_service_spec.rb b/spec/services/namespaces/statistics_refresher_service_spec.rb index d3379e843ec..2d5f9235bd4 100644 --- a/spec/services/namespaces/statistics_refresher_service_spec.rb +++ b/spec/services/namespaces/statistics_refresher_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Namespaces::StatisticsRefresherService, '#execute' do let(:group) { create(:group) } + let(:subgroup) { create(:group, parent: group) } let(:projects) { create_list(:project, 5, namespace: group) } let(:service) { described_class.new } @@ -23,6 +24,14 @@ RSpec.describe Namespaces::StatisticsRefresherService, '#execute' do service.execute(group) end + + context 'when given a subgroup' do + it 'does not create statistics for the subgroup' do + service.execute(subgroup) + + expect(subgroup.reload.root_storage_statistics).not_to be_present + end + end end context 'with a root storage statistics relation', :sidekiq_might_not_need_inline do @@ -43,6 +52,16 @@ RSpec.describe Namespaces::StatisticsRefresherService, '#execute' do service.execute(group) end + + context 'when given a subgroup' do + it "recalculates the root namespace's statistics" do + expect(Namespace::RootStorageStatistics) + .to receive(:safe_find_or_create_by!).with({ namespace_id: group.id }) + .and_return(group.root_storage_statistics) + + service.execute(subgroup) + end + end end context 'when something goes wrong' do diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb index c25895d2efa..67d8b37f809 100644 --- a/spec/services/notes/build_service_spec.rb +++ b/spec/services/notes/build_service_spec.rb @@ -189,13 +189,13 @@ RSpec.describe Notes::BuildService do context 'issuable author' do let(:user) { noteable_author } - it_behaves_like 'user allowed to set comment as confidential' + it_behaves_like 'user not allowed to set comment as confidential' end context 'issuable assignee' do let(:user) { issuable_assignee } - it_behaves_like 'user allowed to set comment as confidential' + it_behaves_like 'user not allowed to set comment as confidential' end context 'admin' do @@ -265,13 +265,13 @@ RSpec.describe Notes::BuildService do context 'with noteable author' do let(:user) { note.noteable.author } - it_behaves_like 'confidential set to `true`' + it_behaves_like 'returns `Discussion to reply to cannot be found` error' end context 'with noteable assignee' do let(:user) { issuable_assignee } - it_behaves_like 'confidential set to `true`' + it_behaves_like 'returns `Discussion to reply to cannot be found` error' end context 'with guest access' do diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index 989ca7b8df1..05703ac548d 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -245,7 +245,7 @@ RSpec.describe Notes::UpdateService do context 'for a personal snippet' do let_it_be(:snippet) { create(:personal_snippet, :public) } - let(:note) { create(:note, project: nil, noteable: snippet, author: user, note: "Note on a snippet with reference #{issue.to_reference}" ) } + let(:note) { create(:note, project: nil, noteable: snippet, author: user, note: "Note on a snippet with reference #{issue.to_reference}") } it 'does not create todos' do expect { update_note({ note: "Mentioning user #{user2}" }) }.not_to change { note.todos.count } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 8fbf023cda0..7857bd2263f 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -211,6 +211,23 @@ RSpec.describe NotificationService, :mailer do it_behaves_like 'participating by assignee notification' end + describe '.permitted_actions' do + it 'includes public methods' do + expect(described_class.permitted_actions).to include(:access_token_created) + end + + it 'excludes EXCLUDED_ACTIONS' do + described_class::EXCLUDED_ACTIONS.each do |action| + expect(described_class.permitted_actions).not_to include(action) + end + end + + it 'excludes protected and private methods' do + expect(described_class.permitted_actions).not_to include(:new_resource_email) + expect(described_class.permitted_actions).not_to include(:approve_mr_email) + end + end + describe '#async' do let(:async) { notification.async } diff --git a/spec/services/packages/composer/composer_json_service_spec.rb b/spec/services/packages/composer/composer_json_service_spec.rb index 378016a6ffb..d2187688c4c 100644 --- a/spec/services/packages/composer/composer_json_service_spec.rb +++ b/spec/services/packages/composer/composer_json_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Packages::Composer::ComposerJsonService do subject { described_class.new(project, target).execute } context 'with an existing file' do - let(:project) { create(:project, :custom_repo, files: { 'composer.json' => json } ) } + let(:project) { create(:project, :custom_repo, files: { 'composer.json' => json }) } context 'with a valid file' do let(:json) { '{ "name": "package-name"}' } diff --git a/spec/services/packages/maven/metadata/create_versions_xml_service_spec.rb b/spec/services/packages/maven/metadata/create_versions_xml_service_spec.rb index 39c6feb5d12..70c2bbad87a 100644 --- a/spec/services/packages/maven/metadata/create_versions_xml_service_spec.rb +++ b/spec/services/packages/maven/metadata/create_versions_xml_service_spec.rb @@ -65,6 +65,23 @@ RSpec.describe ::Packages::Maven::Metadata::CreateVersionsXmlService do let(:versions_in_database) { versions_in_xml + additional_versions } it_behaves_like 'returning an xml with versions in the database' + + context 'with an xml without a release version' do + let(:version_release) { nil } + + it_behaves_like 'returning an xml with versions in the database' + + it 'logs a warn with the reason' do + expect(Gitlab::AppJsonLogger).to receive(:warn).with( + message: 'A malformed metadata file has been encountered', + reason: 'Missing release tag', + project_id: package.project_id, + package_id: package.id + ) + + subject + end + end end end diff --git a/spec/services/packages/npm/create_package_service_spec.rb b/spec/services/packages/npm/create_package_service_spec.rb index a3e59913918..ef8cdf2e8ab 100644 --- a/spec/services/packages/npm/create_package_service_spec.rb +++ b/spec/services/packages/npm/create_package_service_spec.rb @@ -148,7 +148,7 @@ RSpec.describe Packages::Npm::CreatePackageService do end context 'when file size is faked by setting the attachment length param to a lower size' do - let(:params) { super().deep_merge!( { _attachments: { "#{package_name}-#{version}.tgz" => { data: encoded_package_data, length: 1 } } }) } + let(:params) { super().deep_merge!({ _attachments: { "#{package_name}-#{version}.tgz" => { data: encoded_package_data, length: 1 } } }) } # TODO (technical debt): Extract the package size calculation outside the service and add separate specs for it. # Right now we have several contexts here to test the calculation's different scenarios. @@ -193,7 +193,7 @@ RSpec.describe Packages::Npm::CreatePackageService do end context 'with empty versions' do - let(:params) { super().merge!({ versions: {} } ) } + let(:params) { super().merge!({ versions: {} }) } it { expect(subject[:http_status]).to eq 400 } it { expect(subject[:message]).to eq 'Version is empty.' } diff --git a/spec/services/packages/rpm/repository_metadata/base_builder_spec.rb b/spec/services/packages/rpm/repository_metadata/base_builder_spec.rb deleted file mode 100644 index 524c224177b..00000000000 --- a/spec/services/packages/rpm/repository_metadata/base_builder_spec.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true -require 'spec_helper' - -RSpec.describe Packages::Rpm::RepositoryMetadata::BaseBuilder do - describe '#execute' do - subject { described_class.new(xml: xml, data: data).execute } - - let(:xml) { nil } - let(:data) { {} } - - before do - stub_const("#{described_class}::ROOT_TAG", 'test') - stub_const("#{described_class}::ROOT_ATTRIBUTES", { foo1: 'bar1', foo2: 'bar2' }) - end - - it 'generate valid xml' do - result = Nokogiri::XML::Document.parse(subject) - - expect(result.children.count).to eq(1) - expect(result.children.first.attributes.count).to eq(2) - expect(result.children.first.attributes['foo1'].value).to eq('bar1') - expect(result.children.first.attributes['foo2'].value).to eq('bar2') - end - - context 'when call with parameters' do - let(:xml) { 'test' } - - it 'raise NotImplementedError' do - expect { subject }.to raise_error NotImplementedError - end - end - end -end diff --git a/spec/services/packages/rpm/repository_metadata/build_filelist_xml_service_spec.rb b/spec/services/packages/rpm/repository_metadata/build_filelist_xml_service_spec.rb new file mode 100644 index 00000000000..d93d6ab9fcb --- /dev/null +++ b/spec/services/packages/rpm/repository_metadata/build_filelist_xml_service_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::Rpm::RepositoryMetadata::BuildFilelistXmlService do + describe '#execute' do + subject { described_class.new(data).execute } + + include_context 'with rpm package data' + + let(:data) { xml_update_params } + let(:file_xpath) { "//package/file" } + + it 'adds all file nodes' do + result = subject + + expect(result.xpath(file_xpath).count).to eq(data[:files].count) + end + + describe 'setting type attribute' do + context 'when all files are directories' do + let(:dirs) do + 3.times.map { generate_directory } # rubocop:disable Performance/TimesMap + end + + let(:files) do + 5.times.map { FFaker::Filesystem.file_name(dirs.sample) } # rubocop:disable Performance/TimesMap + end + + let(:data) do + { + directories: dirs.map { "#{_1}/" }, # Add trailing slash as in original package + files: dirs + files + } + end + + it 'set dir type attribute for directories only' do + result = subject + + result.xpath(file_xpath).each do |tag| + if dirs.include?(tag.content) + expect(tag.attributes['type']&.value).to eq('dir') + else + expect(tag.attributes['type']).to be_nil + end + end + end + end + + def generate_directory + FFaker::Lorem.words(3).join('/') + end + end + end +end diff --git a/spec/services/packages/rpm/repository_metadata/build_filelist_xml_spec.rb b/spec/services/packages/rpm/repository_metadata/build_filelist_xml_spec.rb deleted file mode 100644 index 2feb44c7c1b..00000000000 --- a/spec/services/packages/rpm/repository_metadata/build_filelist_xml_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true -require 'spec_helper' - -RSpec.describe Packages::Rpm::RepositoryMetadata::BuildFilelistXml do - describe '#execute' do - subject { described_class.new.execute } - - context "when generate empty xml" do - let(:expected_xml) do - <<~XML - <?xml version="1.0" encoding="UTF-8"?> - <filelists xmlns="http://linux.duke.edu/metadata/filelists" packages="0"/> - XML - end - - it 'generate expected xml' do - expect(subject).to eq(expected_xml) - end - end - end -end diff --git a/spec/services/packages/rpm/repository_metadata/build_other_xml_service_spec.rb b/spec/services/packages/rpm/repository_metadata/build_other_xml_service_spec.rb new file mode 100644 index 00000000000..201f9e67ce9 --- /dev/null +++ b/spec/services/packages/rpm/repository_metadata/build_other_xml_service_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::Rpm::RepositoryMetadata::BuildOtherXmlService do + describe '#execute' do + subject { described_class.new(data).execute } + + include_context 'with rpm package data' + + let(:data) { xml_update_params } + let(:changelog_xpath) { "//package/changelog" } + + it 'adds all changelog nodes' do + result = subject + + expect(result.xpath(changelog_xpath).count).to eq(data[:changelogs].count) + end + + it 'set required date attribute' do + result = subject + + data[:changelogs].each do |changelog| + expect(result.at("#{changelog_xpath}[@date=\"#{changelog[:changelogtime]}\"]")).not_to be_nil + end + end + end +end diff --git a/spec/services/packages/rpm/repository_metadata/build_other_xml_spec.rb b/spec/services/packages/rpm/repository_metadata/build_other_xml_spec.rb deleted file mode 100644 index 823aa18808a..00000000000 --- a/spec/services/packages/rpm/repository_metadata/build_other_xml_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true -require 'spec_helper' - -RSpec.describe Packages::Rpm::RepositoryMetadata::BuildOtherXml do - describe '#execute' do - subject { described_class.new.execute } - - context "when generate empty xml" do - let(:expected_xml) do - <<~XML - <?xml version="1.0" encoding="UTF-8"?> - <otherdata xmlns="http://linux.duke.edu/metadata/other" packages="0"/> - XML - end - - it 'generate expected xml' do - expect(subject).to eq(expected_xml) - end - end - end -end diff --git a/spec/services/packages/rpm/repository_metadata/build_primary_xml_service_spec.rb b/spec/services/packages/rpm/repository_metadata/build_primary_xml_service_spec.rb new file mode 100644 index 00000000000..9bbfa5c9863 --- /dev/null +++ b/spec/services/packages/rpm/repository_metadata/build_primary_xml_service_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::Rpm::RepositoryMetadata::BuildPrimaryXmlService do + describe '#execute' do + subject { described_class.new(data).execute } + + include_context 'with rpm package data' + + let(:data) { xml_update_params } + let(:required_text_only_attributes) { %i[description summary arch name] } + + it 'adds node with required_text_only_attributes' do + result = subject + + required_text_only_attributes.each do |attribute| + expect( + result.at("//package/#{attribute}").text + ).to eq(data[attribute]) + end + end + end +end diff --git a/spec/services/packages/rpm/repository_metadata/build_primary_xml_spec.rb b/spec/services/packages/rpm/repository_metadata/build_primary_xml_spec.rb deleted file mode 100644 index 147d5862a71..00000000000 --- a/spec/services/packages/rpm/repository_metadata/build_primary_xml_spec.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true -require 'spec_helper' - -RSpec.describe Packages::Rpm::RepositoryMetadata::BuildPrimaryXml do - describe '#execute' do - subject { described_class.new(xml: xml, data: data).execute } - - let(:empty_xml) do - <<~XML - <?xml version="1.0" encoding="UTF-8"?> - <metadata xmlns="http://linux.duke.edu/metadata/common" xmlns:rpm="http://linux.duke.edu/metadata/rpm" packages="0"/> - XML - end - - it_behaves_like 'handling rpm xml file' - - context 'when updating existing xml' do - include_context 'with rpm package data' - - let(:xml) { empty_xml } - let(:data) { xml_update_params } - let(:required_text_only_attributes) { %i[description summary arch name] } - - it 'adds node with required_text_only_attributes' do - result = Nokogiri::XML::Document.parse(subject).remove_namespaces! - - required_text_only_attributes.each do |attribute| - expect( - result.at("//#{described_class::ROOT_TAG}/package/#{attribute}").text - ).to eq(data[attribute]) - end - end - end - end -end diff --git a/spec/services/packages/rpm/repository_metadata/build_repomd_xml_spec.rb b/spec/services/packages/rpm/repository_metadata/build_repomd_xml_service_spec.rb index 0843a983b7e..cf28301fa2c 100644 --- a/spec/services/packages/rpm/repository_metadata/build_repomd_xml_spec.rb +++ b/spec/services/packages/rpm/repository_metadata/build_repomd_xml_service_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -RSpec.describe Packages::Rpm::RepositoryMetadata::BuildRepomdXml do +RSpec.describe Packages::Rpm::RepositoryMetadata::BuildRepomdXmlService do describe '#execute' do subject { described_class.new(data).execute } diff --git a/spec/services/packages/rpm/repository_metadata/update_xml_service_spec.rb b/spec/services/packages/rpm/repository_metadata/update_xml_service_spec.rb new file mode 100644 index 00000000000..e351392ba1c --- /dev/null +++ b/spec/services/packages/rpm/repository_metadata/update_xml_service_spec.rb @@ -0,0 +1,177 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Packages::Rpm::RepositoryMetadata::UpdateXmlService do + describe '#execute' do + subject { described_class.new(filename: filename, xml: xml, data: data).execute } + + let(:xml) { nil } + let(:data) { nil } + + shared_examples 'handling not implemented xml filename' do + let(:filename) { :not_implemented_yet } + let(:empty_xml) { '' } + + it 'raise error' do + expect { subject }.to raise_error(ArgumentError) + end + end + + shared_context 'with primary xml file data' do + let(:filename) { :primary } + let(:empty_xml) do + <<~XML + <?xml version="1.0" encoding="UTF-8"?> + <metadata xmlns="http://linux.duke.edu/metadata/common" xmlns:rpm="http://linux.duke.edu/metadata/rpm" packages="0"/> + XML + end + end + + shared_context 'with other xml file data' do + let(:filename) { :other } + let(:empty_xml) do + <<~XML + <?xml version="1.0" encoding="UTF-8"?> + <otherdata xmlns="http://linux.duke.edu/metadata/other" packages="0"/> + XML + end + end + + shared_context 'with filelist xml file data' do + let(:filename) { :filelist } + let(:empty_xml) do + <<~XML + <?xml version="1.0" encoding="UTF-8"?> + <filelists xmlns="http://linux.duke.edu/metadata/filelists" packages="0"/> + XML + end + end + + context 'when building empty xml' do + shared_examples 'generating empty xml' do + it 'generate expected xml' do + expect(subject).to eq(empty_xml) + end + end + + it_behaves_like 'handling not implemented xml filename' + + context "for 'primary' xml file" do + include_context 'with primary xml file data' + + it_behaves_like 'generating empty xml' + end + + context "for 'other' xml file" do + include_context 'with other xml file data' + + it_behaves_like 'generating empty xml' + end + + context "for 'filelist' xml file" do + include_context 'with filelist xml file data' + + it_behaves_like 'generating empty xml' + end + end + + context 'when updating xml file' do + include_context 'with rpm package data' + + let(:xml) { empty_xml } + let(:data) { xml_update_params } + let(:builder_class) { described_class::BUILDERS[filename] } + + shared_examples 'updating rpm xml file' do + context 'when updating existing xml' do + shared_examples 'changing root tag attribute' do + it "increment previous 'packages' value by 1" do + previous_value = Nokogiri::XML(xml).at(builder_class::ROOT_TAG).attributes["packages"].value.to_i + new_value = Nokogiri::XML(subject).at(builder_class::ROOT_TAG).attributes["packages"].value.to_i + + expect(previous_value + 1).to eq(new_value) + end + end + + it 'generate valid xml add expected xml node to existing xml' do + # Have one root attribute + result = Nokogiri::XML::Document.parse(subject).remove_namespaces! + expect(result.children.count).to eq(1) + + # Root node has 1 child with generated node + expect(result.xpath("//#{builder_class::ROOT_TAG}/package").count).to eq(1) + end + + context 'when empty xml' do + it_behaves_like 'changing root tag attribute' + end + + context 'when xml has children' do + context "when node with given 'pkgid' does not exist yet" do + let(:uniq_node_data) do + xml_update_params.tap do |data| + data[:pkgid] = SecureRandom.uuid + end + end + + let(:xml) { build_xml_from(uniq_node_data) } + + it 'has children nodes' do + existing_xml = Nokogiri::XML::Document.parse(xml).remove_namespaces! + expect(existing_xml.xpath('//package').count).to eq(1) + end + + it_behaves_like 'changing root tag attribute' + end + + context "when node with given 'pkgid' already exist" do + let(:existing_node_data) do + existing_data = data.dup + existing_data[:name] = FFaker::Lorem.word + existing_data + end + + let(:xml) { build_xml_from(existing_node_data) } + + it 'has children nodes' do + existing_xml = Nokogiri::XML::Document.parse(xml).remove_namespaces! + expect(existing_xml.xpath('//package').count).to eq(1) + end + + it 'replace existing node with new data' do + existing_xml = Nokogiri::XML::Document.parse(xml).remove_namespaces! + result = Nokogiri::XML::Document.parse(subject).remove_namespaces! + expect(result.xpath('//package').count).to eq(1) + expect(result.xpath('//package').first.to_xml).not_to eq(existing_xml.xpath('//package').first.to_xml) + end + end + + def build_xml_from(data) + described_class.new(filename: filename, xml: empty_xml, data: data).execute + end + end + end + end + + it_behaves_like 'handling not implemented xml filename' + + context "for 'primary' xml file" do + include_context 'with primary xml file data' + + it_behaves_like 'updating rpm xml file' + end + + context "for 'other' xml file" do + include_context 'with other xml file data' + + it_behaves_like 'updating rpm xml file' + end + + context "for 'filelist' xml file" do + include_context 'with filelist xml file data' + + it_behaves_like 'updating rpm xml file' + end + 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 65da1976dc2..eb8d94ebfa5 100644 --- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' RSpec.describe Projects::HashedStorage::MigrateRepositoryService do - include GitHelpers - let(:gitlab_shell) { Gitlab::Shell.new } let(:project) { create(:project, :legacy_storage, :repository, :wiki_repo, :design_repo) } let(:legacy_storage) { Storage::LegacyProject.new(project) } 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 b67b4d64c1d..6c7164c5e06 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -126,7 +126,7 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do let(:redirect_link) { 'http://external-link' } before do - stub_full_request(download_link).to_return(status: 301, body: 'You are being redirected', headers: { 'Location' => redirect_link } ) + stub_full_request(download_link).to_return(status: 301, body: 'You are being redirected', headers: { 'Location' => redirect_link }) stub_full_request(redirect_link).to_return(body: lfs_content) end diff --git a/spec/services/projects/move_users_star_projects_service_spec.rb b/spec/services/projects/move_users_star_projects_service_spec.rb index 0f766ebd0ec..b580d3d8772 100644 --- a/spec/services/projects/move_users_star_projects_service_spec.rb +++ b/spec/services/projects/move_users_star_projects_service_spec.rb @@ -15,6 +15,9 @@ RSpec.describe Projects::MoveUsersStarProjectsService do end it 'moves the user\'s stars from one project to another' do + project_with_stars.reload + target_project.reload + expect(project_with_stars.users_star_projects.count).to eq 2 expect(project_with_stars.star_count).to eq 2 expect(target_project.users_star_projects.count).to eq 0 @@ -34,6 +37,8 @@ RSpec.describe Projects::MoveUsersStarProjectsService do allow(subject).to receive(:success).and_raise(StandardError) expect { subject.execute(project_with_stars) }.to raise_error(StandardError) + project_with_stars.reload + target_project.reload expect(project_with_stars.users_star_projects.count).to eq 2 expect(project_with_stars.star_count).to eq 2 diff --git a/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 7bf6dfd0fd8..43d23023d83 100644 --- a/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -244,9 +244,10 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do end shared_examples 'process truncated alerts' do - it 'returns 200 but skips processing and logs a warning', :aggregate_failures do + it 'returns 201 but skips processing and logs a warning', :aggregate_failures do expect(subject).to be_success - expect(subject.payload[:alerts].size).to eq(max_alerts) + expect(subject.payload).to eq({}) + expect(subject.http_status).to eq(:created) expect(Gitlab::AppLogger) .to have_received(:warn) .with( @@ -260,9 +261,10 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do end shared_examples 'process all alerts' do - it 'returns 200 and process alerts without warnings', :aggregate_failures do + it 'returns 201 and process alerts without warnings', :aggregate_failures do expect(subject).to be_success - expect(subject.payload[:alerts].size).to eq(2) + expect(subject.payload).to eq({}) + expect(subject.http_status).to eq(:created) expect(Gitlab::AppLogger).not_to have_received(:warn) end end diff --git a/spec/services/protected_branches/api_service_spec.rb b/spec/services/protected_branches/api_service_spec.rb new file mode 100644 index 00000000000..94484f5a7b9 --- /dev/null +++ b/spec/services/protected_branches/api_service_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ProtectedBranches::ApiService do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user, maintainer_projects: [project]) } + + it 'creates a protected branch with prefilled defaults' do + expect(::ProtectedBranches::CreateService).to receive(:new).with( + project, user, hash_including( + push_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }], + merge_access_levels_attributes: [{ access_level: Gitlab::Access::MAINTAINER }] + ) + ).and_call_original + + expect(described_class.new(project, user, { name: 'new name' }).create).to be_valid + end + + it 'updates a protected branch without prefilled defaults' do + protected_branch = create(:protected_branch, project: project, allow_force_push: true) + + expect(::ProtectedBranches::UpdateService).to receive(:new).with( + project, user, hash_including( + push_access_levels_attributes: [], + merge_access_levels_attributes: [] + ) + ).and_call_original + + expect do + expect(described_class.new(project, user, { name: 'new name' }).update(protected_branch)).to be_valid + end.not_to change { protected_branch.reload.allow_force_push } + end +end diff --git a/spec/services/protected_branches/cache_service_spec.rb b/spec/services/protected_branches/cache_service_spec.rb index 00d1e8b5457..d7a3258160b 100644 --- a/spec/services/protected_branches/cache_service_spec.rb +++ b/spec/services/protected_branches/cache_service_spec.rb @@ -111,5 +111,16 @@ RSpec.describe ProtectedBranches::CacheService, :clean_gitlab_redis_cache do expect(service.fetch('not-found') { true }).to eq(true) end end + + describe 'metrics' do + it 'records hit ratio metrics' do + expect_next_instance_of(Gitlab::Cache::Metrics) do |metrics| + expect(metrics).to receive(:increment_cache_miss).once + expect(metrics).to receive(:increment_cache_hit).exactly(4).times + end + + 5.times { service.fetch('main') { true } } + end + end end # rubocop:enable Style/RedundantFetchBlock diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index a43f3bc55bf..f9c16c84121 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -1450,6 +1450,11 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { issue } end + it_behaves_like 'estimate command' do + let(:content) { '/estimate_time 1h' } + let(:issuable) { issue } + end + it_behaves_like 'failed command' do let(:content) { '/estimate' } let(:issuable) { issue } @@ -1470,6 +1475,11 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { issue } end + it_behaves_like 'spend command' do + let(:content) { '/spend_time 1h' } + let(:issuable) { issue } + end + it_behaves_like 'spend command with negative time' do let(:content) { '/spend -120m' } let(:issuable) { issue } @@ -1537,6 +1547,11 @@ RSpec.describe QuickActions::InterpretService do let(:issuable) { issue } end + it_behaves_like 'remove_estimate command' do + let(:content) { '/remove_time_estimate' } + let(:issuable) { issue } + end + it_behaves_like 'remove_time_spent command' do let(:content) { '/remove_time_spent' } let(:issuable) { issue } diff --git a/spec/services/resource_access_tokens/revoke_service_spec.rb b/spec/services/resource_access_tokens/revoke_service_spec.rb index 8f89696cc55..28f173f1bc7 100644 --- a/spec/services/resource_access_tokens/revoke_service_spec.rb +++ b/spec/services/resource_access_tokens/revoke_service_spec.rb @@ -29,35 +29,13 @@ RSpec.describe ResourceAccessTokens::RevokeService do expect(resource.reload.users).not_to include(resource_bot) end - context 'when user_destroy_with_limited_execution_time_worker is enabled' do - it 'initiates user removal' do - subject - - expect( - Users::GhostUserMigration.where(user: resource_bot, - initiator_user: user) - ).to be_exists - end - end - - context 'when user_destroy_with_limited_execution_time_worker is disabled' do - before do - stub_feature_flags(user_destroy_with_limited_execution_time_worker: false) - end - - it 'transfer issuables of bot user to ghost user' do - issue = create(:issue, author: resource_bot) - - subject - - expect(issue.reload.author.ghost?).to be true - end - - it 'deletes project bot user' do - subject + it 'initiates user removal' do + subject - expect(User.exists?(resource_bot.id)).to be_falsy - end + expect( + Users::GhostUserMigration.where(user: resource_bot, + initiator_user: user) + ).to be_exists end it 'logs the event' do diff --git a/spec/services/resource_events/change_milestone_service_spec.rb b/spec/services/resource_events/change_milestone_service_spec.rb index ed234376381..425d5b19907 100644 --- a/spec/services/resource_events/change_milestone_service_spec.rb +++ b/spec/services/resource_events/change_milestone_service_spec.rb @@ -14,4 +14,35 @@ RSpec.describe ResourceEvents::ChangeMilestoneService do let_it_be(:resource) { create(issuable) } # rubocop:disable Rails/SaveBang end end + + describe 'events tracking' do + let_it_be(:user) { create(:user) } + + let(:resource) { create(resource_type, milestone: timebox, project: timebox.project) } + + subject(:service_instance) { described_class.new(resource, user, old_milestone: nil) } + + context 'when the resource is a work item' do + let(:resource_type) { :work_item } + + it 'tracks work item usage data counters' do + expect(Gitlab::UsageDataCounters::WorkItemActivityUniqueCounter) + .to receive(:track_work_item_milestone_changed_action) + .with(author: user) + + service_instance.execute + end + end + + context 'when the resource is not a work item' do + let(:resource_type) { :issue } + + it 'does not track work item usage data counters' do + expect(Gitlab::UsageDataCounters::WorkItemActivityUniqueCounter) + .not_to receive(:track_work_item_milestone_changed_action) + + service_instance.execute + end + end + end end diff --git a/spec/services/resource_events/change_state_service_spec.rb b/spec/services/resource_events/change_state_service_spec.rb index 255ee9eca57..b679943073c 100644 --- a/spec/services/resource_events/change_state_service_spec.rb +++ b/spec/services/resource_events/change_state_service_spec.rb @@ -18,10 +18,11 @@ RSpec.describe ResourceEvents::ChangeStateService do event = resource.resource_state_events.last - if resource.is_a?(Issue) + case resource + when Issue expect(event.issue).to eq(resource) expect(event.merge_request).to be_nil - elsif resource.is_a?(MergeRequest) + when MergeRequest expect(event.issue).to be_nil expect(event.merge_request).to eq(resource) end @@ -91,10 +92,11 @@ RSpec.describe ResourceEvents::ChangeStateService do end def expect_event_source(event, source) - if source.is_a?(MergeRequest) + case source + when MergeRequest expect(event.source_commit).to be_nil expect(event.source_merge_request).to eq(source) - elsif source.is_a?(Commit) + when Commit expect(event.source_commit).to eq(source.id) expect(event.source_merge_request).to be_nil else diff --git a/spec/services/search/group_service_spec.rb b/spec/services/search/group_service_spec.rb index 152d0700cc1..c9bfa7cb7b4 100644 --- a/spec/services/search/group_service_spec.rb +++ b/spec/services/search/group_service_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Search::GroupService do # These projects shouldn't be found let!(:outside_project) { create(:project, :public, name: "Outside #{term}") } - let!(:private_project) { create(:project, :private, namespace: nested_group, name: "Private #{term}" ) } + let!(:private_project) { create(:project, :private, namespace: nested_group, name: "Private #{term}") } let!(:other_project) { create(:project, :public, namespace: nested_group, name: term.reverse) } # These projects should be found diff --git a/spec/services/search_service_spec.rb b/spec/services/search_service_spec.rb index 5edea13afa4..26def474b88 100644 --- a/spec/services/search_service_spec.rb +++ b/spec/services/search_service_spec.rb @@ -399,159 +399,7 @@ RSpec.describe SearchService do end end - context 'redacting search results' do - let(:search) { 'anything' } - - subject(:result) { search_service.search_objects } - - shared_examples "redaction limits N+1 queries" do |limit:| - it 'does not exceed the query limit' do - # issuing the query to remove the data loading call - unredacted_results.to_a - - # only the calls from the redaction are left - query = ActiveRecord::QueryRecorder.new { result } - - # these are the project authorization calls, which are not preloaded - expect(query.count).to be <= limit - end - end - - def found_blob(project) - Gitlab::Search::FoundBlob.new(project: project) - end - - def found_wiki_page(project) - Gitlab::Search::FoundWikiPage.new(found_blob(project)) - end - - before do - expect(search_service) - .to receive(:search_results) - .and_return(double('search results', objects: unredacted_results)) - end - - def ar_relation(klass, *objects) - klass.id_in(objects.map(&:id)) - end - - def kaminari_array(*objects) - Kaminari.paginate_array(objects).page(1).per(20) - end - - context 'issues' do - let(:readable) { create(:issue, project: accessible_project) } - let(:unreadable) { create(:issue, project: inaccessible_project) } - let(:unredacted_results) { ar_relation(Issue, readable, unreadable) } - let(:scope) { 'issues' } - - it 'redacts the inaccessible issue' do - expect(result).to contain_exactly(readable) - end - end - - context 'notes' do - let(:readable) { create(:note_on_commit, project: accessible_project) } - let(:unreadable) { create(:note_on_commit, project: inaccessible_project) } - let(:unredacted_results) { ar_relation(Note, readable, unreadable) } - let(:scope) { 'notes' } - - it 'redacts the inaccessible note' do - expect(result).to contain_exactly(readable) - end - end - - context 'merge_requests' do - let(:readable) { create(:merge_request, source_project: accessible_project, author: user) } - let(:unreadable) { create(:merge_request, source_project: inaccessible_project) } - let(:unredacted_results) { ar_relation(MergeRequest, readable, unreadable) } - let(:scope) { 'merge_requests' } - - it 'redacts the inaccessible merge request' do - expect(result).to contain_exactly(readable) - end - - context 'with :with_api_entity_associations' do - let(:unredacted_results) { ar_relation(MergeRequest.with_api_entity_associations, readable, unreadable) } - - it_behaves_like "redaction limits N+1 queries", limit: 8 - end - end - - context 'project repository blobs' do - let(:readable) { found_blob(accessible_project) } - let(:unreadable) { found_blob(inaccessible_project) } - let(:unredacted_results) { kaminari_array(readable, unreadable) } - let(:scope) { 'blobs' } - - it 'redacts the inaccessible blob' do - expect(result).to contain_exactly(readable) - end - end - - context 'project wiki blobs' do - let(:readable) { found_wiki_page(accessible_project) } - let(:unreadable) { found_wiki_page(inaccessible_project) } - let(:unredacted_results) { kaminari_array(readable, unreadable) } - let(:scope) { 'wiki_blobs' } - - it 'redacts the inaccessible blob' do - expect(result).to contain_exactly(readable) - end - end - - context 'project snippets' do - let(:readable) { create(:project_snippet, project: accessible_project) } - let(:unreadable) { create(:project_snippet, project: inaccessible_project) } - let(:unredacted_results) { ar_relation(ProjectSnippet, readable, unreadable) } - let(:scope) { 'snippet_titles' } - - it 'redacts the inaccessible snippet' do - expect(result).to contain_exactly(readable) - end - - context 'with :with_api_entity_associations' do - it_behaves_like "redaction limits N+1 queries", limit: 14 - end - end - - context 'personal snippets' do - let(:readable) { create(:personal_snippet, :private, author: user) } - let(:unreadable) { create(:personal_snippet, :private) } - let(:unredacted_results) { ar_relation(PersonalSnippet, readable, unreadable) } - let(:scope) { 'snippet_titles' } - - it 'redacts the inaccessible snippet' do - expect(result).to contain_exactly(readable) - end - - context 'with :with_api_entity_associations' do - it_behaves_like "redaction limits N+1 queries", limit: 4 - end - end - - context 'commits' do - let(:readable) { accessible_project.commit } - let(:unreadable) { inaccessible_project.commit } - let(:unredacted_results) { kaminari_array(readable, unreadable) } - let(:scope) { 'commits' } - - it 'redacts the inaccessible commit' do - expect(result).to contain_exactly(readable) - end - end - - context 'users' do - let(:other_user) { create(:user) } - let(:unredacted_results) { ar_relation(User, user, other_user) } - let(:scope) { 'users' } - - it 'passes the users through' do - # Users are always visible to everyone - expect(result).to contain_exactly(user, other_user) - end - end - end + it_behaves_like 'a redacted search results' end describe '#valid_request?' do diff --git a/spec/services/security/ci_configuration/sast_parser_service_spec.rb b/spec/services/security/ci_configuration/sast_parser_service_spec.rb index 7a004e2915c..9211beb76f8 100644 --- a/spec/services/security/ci_configuration/sast_parser_service_spec.rb +++ b/spec/services/security/ci_configuration/sast_parser_service_spec.rb @@ -11,9 +11,9 @@ RSpec.describe Security::CiConfiguration::SastParserService do let(:sast_excluded_paths) { configuration['global'][1] } let(:sast_pipeline_stage) { configuration['pipeline'][0] } let(:sast_search_max_depth) { configuration['pipeline'][1] } - let(:bandit) { configuration['analyzers'][0] } - let(:brakeman) { configuration['analyzers'][1] } + let(:brakeman) { configuration['analyzers'][0] } let(:sast_brakeman_level) { brakeman['variables'][0] } + let(:semgrep) { configuration['analyzers'][1] } let(:secure_analyzers_prefix) { '$CI_TEMPLATE_REGISTRY_HOST/security-products' } it 'parses the configuration for SAST' do @@ -34,7 +34,7 @@ RSpec.describe Security::CiConfiguration::SastParserService do expect(sast_pipeline_stage['value']).to eql('our_custom_security_stage') expect(sast_search_max_depth['value']).to eql('8') expect(brakeman['enabled']).to be(false) - expect(bandit['enabled']).to be(true) + expect(semgrep['enabled']).to be(true) expect(sast_brakeman_level['value']).to eql('2') end @@ -43,7 +43,7 @@ RSpec.describe Security::CiConfiguration::SastParserService do allow(project.repository).to receive(:blob_data_at).and_return(gitlab_ci_yml_excluded_analyzers_content) expect(brakeman['enabled']).to be(false) - expect(bandit['enabled']).to be(true) + expect(semgrep['enabled']).to be(true) end end end diff --git a/spec/services/security/merge_reports_service_spec.rb b/spec/services/security/merge_reports_service_spec.rb index e61977297c5..8415ed8a22f 100644 --- a/spec/services/security/merge_reports_service_spec.rb +++ b/spec/services/security/merge_reports_service_spec.rb @@ -219,10 +219,10 @@ RSpec.describe Security::MergeReportsService, '#execute' do 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(: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, + build(:ci_reports_security_report, type: :sast, scanners: [bandit_scanner], findings: [finding_id_1], diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index b2ccd9dba52..3263e410d3c 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -175,7 +175,7 @@ RSpec.describe ::SystemNotes::IssuablesService do it 'builds a correct phrase when one reviewer removed from a set' do expect(build_note([reviewer, reviewer1, reviewer2], [reviewer, reviewer1])).to( - eq( "removed review request for @#{reviewer2.username}") + eq("removed review request for @#{reviewer2.username}") ) end @@ -681,7 +681,7 @@ RSpec.describe ::SystemNotes::IssuablesService do it 'tracks usage' do expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter) - .to receive(:track_issue_cloned_action).with(author: author, project: project ) + .to receive(:track_issue_cloned_action).with(author: author, project: project) subject end diff --git a/spec/services/tags/create_service_spec.rb b/spec/services/tags/create_service_spec.rb index b1c6623308e..bbf6fe62959 100644 --- a/spec/services/tags/create_service_spec.rb +++ b/spec/services/tags/create_service_spec.rb @@ -27,6 +27,26 @@ RSpec.describe Tags::CreateService do end end + context 'when tag_name is empty' do + it 'returns an error' do + response = service.execute('', 'foo', 'Foo') + + expect(response[:status]).to eq(:error) + expect(response[:http_status]).to eq(400) + expect(response[:message]).to eq('Tag name invalid') + end + end + + context 'when target is empty' do + it 'returns an error' do + response = service.execute('v1.1.0', '', 'Foo') + + expect(response[:status]).to eq(:error) + expect(response[:http_status]).to eq(400) + expect(response[:message]).to eq('Target is empty') + end + end + context 'when tag already exists' do it 'returns an error' do expect(repository).to receive(:add_tag) diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 45a8268043f..774a6ddcfb3 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -1037,7 +1037,7 @@ RSpec.describe TodoService do let_it_be(:noteable) { create(:issue, project: project) } let(:note) { create(:note, project: project, note: mentions, noteable: noteable) } - let(:addressed_note) { create(:note, project: project, note: "#{directly_addressed}", noteable: noteable) } + let(:addressed_note) { create(:note, project: project, note: directly_addressed.to_s, noteable: noteable) } it 'creates a todo for each valid mentioned user not included in skip_users' do service.update_note(note, author, skip_users) diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb index 225e7933d79..9d5ed70e9ef 100644 --- a/spec/services/todos/destroy/entity_leave_service_spec.rb +++ b/spec/services/todos/destroy/entity_leave_service_spec.rb @@ -17,7 +17,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do let!(:todo_issue_user) { create(:todo, user: user, target: issue, project: project) } let!(:todo_issue_c_user) { create(:todo, user: user, target: issue_c, project: project) } let!(:todo_issue_c_user2) { create(:todo, user: user2, target: issue_c, project: project) } - let(:internal_note) { create(:note, noteable: issue, project: project, confidential: true ) } + let(:internal_note) { create(:note, noteable: issue, project: project, confidential: true) } let!(:todo_for_internal_note) do create(:todo, user: user, target: issue, project: project, note: internal_note) end @@ -28,7 +28,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do set_access(group, user, group_access) if group_access end - it "#{params[:method].to_s.humanize(capitalize: false)}" do + it params[:method].to_s.humanize(capitalize: false) do send(method_name) end end @@ -250,7 +250,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) } let!(:todo_subpgroup_user2) { create(:todo, user: user2, group: subgroup) } let!(:todo_parent_group_user) { create(:todo, user: user, group: parent_group) } - let(:subproject_internal_note) { create(:note, noteable: issue, project: project, confidential: true ) } + let(:subproject_internal_note) { create(:note, noteable: issue, project: project, confidential: true) } let!(:todo_for_internal_subproject_note) do create(:todo, user: user, target: issue, project: project, note: subproject_internal_note) end diff --git a/spec/services/topics/merge_service_spec.rb b/spec/services/topics/merge_service_spec.rb index eef31817aa8..98247250a61 100644 --- a/spec/services/topics/merge_service_spec.rb +++ b/spec/services/topics/merge_service_spec.rb @@ -5,10 +5,10 @@ require 'spec_helper' RSpec.describe Topics::MergeService do let_it_be(:source_topic) { create(:topic, name: 'source_topic') } let_it_be(:target_topic) { create(:topic, name: 'target_topic') } - let_it_be(:project_1) { create(:project, :public, topic_list: source_topic.name ) } - let_it_be(:project_2) { create(:project, :private, topic_list: source_topic.name ) } - let_it_be(:project_3) { create(:project, :public, topic_list: target_topic.name ) } - let_it_be(:project_4) { create(:project, :public, topic_list: [source_topic.name, target_topic.name] ) } + let_it_be(:project_1) { create(:project, :public, topic_list: source_topic.name) } + let_it_be(:project_2) { create(:project, :private, topic_list: source_topic.name) } + let_it_be(:project_3) { create(:project, :public, topic_list: target_topic.name) } + let_it_be(:project_4) { create(:project, :public, topic_list: [source_topic.name, target_topic.name]) } subject { described_class.new(source_topic, target_topic).execute } diff --git a/spec/services/users/approve_service_spec.rb b/spec/services/users/approve_service_spec.rb index 078dde546c9..34eb5b18ff6 100644 --- a/spec/services/users/approve_service_spec.rb +++ b/spec/services/users/approve_service_spec.rb @@ -67,7 +67,7 @@ RSpec.describe Users::ApproveService do subject - expect(Gitlab::AppLogger).to have_received(:info).with(message: "User instance access request approved", user: "#{user.username}", email: "#{user.email}", approved_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + expect(Gitlab::AppLogger).to have_received(:info).with(message: "User instance access request approved", user: user.username.to_s, email: user.email.to_s, approved_by: current_user.username.to_s, ip_address: current_user.current_sign_in_ip.to_s) end it 'emails the user on approval' do diff --git a/spec/services/users/ban_service_spec.rb b/spec/services/users/ban_service_spec.rb index 79f3cbeb46d..3f9c7ebf067 100644 --- a/spec/services/users/ban_service_spec.rb +++ b/spec/services/users/ban_service_spec.rb @@ -34,7 +34,7 @@ RSpec.describe Users::BanService do end 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 receive(:info).with(message: "User ban", user: user.username.to_s, email: user.email.to_s, ban_by: current_user.username.to_s, ip_address: current_user.current_sign_in_ip.to_s) ban_user end diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb index 03e1811c8a5..18ad946b289 100644 --- a/spec/services/users/destroy_service_spec.rb +++ b/spec/services/users/destroy_service_spec.rb @@ -10,546 +10,250 @@ RSpec.describe Users::DestroyService do let(:service) { described_class.new(admin) } let(:gitlab_shell) { Gitlab::Shell.new } - shared_examples 'pre-migrate clean-up' do - describe "Deletes a user and all their personal projects", :enable_admin_mode do - context 'no options are given' do - it 'will delete the personal project' do - expect_next_instance_of(Projects::DestroyService) do |destroy_service| - expect(destroy_service).to receive(:execute).once.and_return(true) - end - - service.execute(user) - end + describe "Initiates user deletion and deletes all their personal projects", :enable_admin_mode do + context 'no options are given' do + it 'creates GhostUserMigration record to handle migration in a worker' do + expect { service.execute(user) } + .to( + change do + Users::GhostUserMigration.where(user: user, + initiator_user: admin) + .exists? + end.from(false).to(true)) end - context 'personal projects in pending_delete' do - before do - project.pending_delete = true - project.save! + it 'will delete the personal project' do + expect_next_instance_of(Projects::DestroyService) do |destroy_service| + expect(destroy_service).to receive(:execute).once.and_return(true) end - it 'destroys a personal project in pending_delete' do - expect_next_instance_of(Projects::DestroyService) do |destroy_service| - expect(destroy_service).to receive(:execute).once.and_return(true) - end - - service.execute(user) - end + service.execute(user) end + end - context "solo owned groups present" do - let(:solo_owned) { create(:group) } - let(:member) { create(:group_member) } - let(:user) { member.user } - - before do - solo_owned.group_members = [member] - end - - it 'returns the user with attached errors' do - expect(service.execute(user)).to be(user) - expect(user.errors.full_messages).to( - contain_exactly('You must transfer ownership or delete groups before you can remove user')) - end - - it 'does not delete the user, nor the group' do - service.execute(user) - - expect(User.find(user.id)).to eq user - expect(Group.find(solo_owned.id)).to eq solo_owned - end + context 'personal projects in pending_delete' do + before do + project.pending_delete = true + project.save! end - context "deletions with solo owned groups" do - let(:solo_owned) { create(:group) } - let(:member) { create(:group_member) } - let(:user) { member.user } - - before do - solo_owned.group_members = [member] - service.execute(user, delete_solo_owned_groups: true) + it 'destroys a personal project in pending_delete' do + expect_next_instance_of(Projects::DestroyService) do |destroy_service| + expect(destroy_service).to receive(:execute).once.and_return(true) end - it 'deletes solo owned groups' do - expect { Group.find(solo_owned.id) }.to raise_error(ActiveRecord::RecordNotFound) - end + service.execute(user) end + end - context 'deletions with inherited group owners' do - let(:group) { create(:group, :nested) } - let(:user) { create(:user) } - let(:inherited_owner) { create(:user) } - - before do - group.parent.add_owner(inherited_owner) - group.add_owner(user) + context "solo owned groups present" do + let(:solo_owned) { create(:group) } + let(:member) { create(:group_member) } + let(:user) { member.user } - service.execute(user, delete_solo_owned_groups: true) - end - - it 'does not delete the group' do - expect(Group.exists?(id: group)).to be_truthy - end + before do + solo_owned.group_members = [member] end - describe "user personal's repository removal" do - context 'storages' do - before do - perform_enqueued_jobs { service.execute(user) } - end - - context 'legacy storage' do - let!(:project) { create(:project, :empty_repo, :legacy_storage, namespace: user.namespace) } - - it 'removes repository' do - expect( - gitlab_shell.repository_exists?(project.repository_storage, - "#{project.disk_path}.git") - ).to be_falsey - end - end - - context 'hashed storage' do - let!(:project) { create(:project, :empty_repo, namespace: user.namespace) } - - it 'removes repository' do - expect( - gitlab_shell.repository_exists?(project.repository_storage, - "#{project.disk_path}.git") - ).to be_falsey - end - end - end - - context 'repository removal status is taken into account' do - it 'raises exception' do - expect_next_instance_of(::Projects::DestroyService) do |destroy_service| - expect(destroy_service).to receive(:execute).and_return(false) - end - - expect { service.execute(user) } - .to raise_error(Users::DestroyService::DestroyError, - "Project #{project.id} can't be deleted" ) - end - end + it 'returns the user with attached errors' do + expect(service.execute(user)).to be(user) + expect(user.errors.full_messages).to( + contain_exactly('You must transfer ownership or delete groups before you can remove user')) end - describe "calls the before/after callbacks" do - it 'of project_members' do - expect_any_instance_of(ProjectMember).to receive(:run_callbacks).with(:find).once - expect_any_instance_of(ProjectMember).to receive(:run_callbacks).with(:initialize).once - expect_any_instance_of(ProjectMember).to receive(:run_callbacks).with(:destroy).once - - service.execute(user) - end - - it 'of group_members' do - group_member = create(:group_member) - group_member.group.group_members.create!(user: user, access_level: 40) + it 'does not delete the user, nor the group' do + service.execute(user) - expect_any_instance_of(GroupMember).to receive(:run_callbacks).with(:find).once - expect_any_instance_of(GroupMember).to receive(:run_callbacks).with(:initialize).once - expect_any_instance_of(GroupMember).to receive(:run_callbacks).with(:destroy).once - - service.execute(user) - end + expect(User.find(user.id)).to eq user + expect(Group.find(solo_owned.id)).to eq solo_owned end end - end - - context 'when user_destroy_with_limited_execution_time_worker is disabled' do - before do - stub_feature_flags(user_destroy_with_limited_execution_time_worker: false) - end - - include_examples 'pre-migrate clean-up' - - describe "Deletes a user and all their personal projects", :enable_admin_mode do - context 'no options are given' do - it 'deletes the user' do - user_data = service.execute(user) - expect(user_data['email']).to eq(user.email) - expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound) - expect { Namespace.find(namespace.id) }.to raise_error(ActiveRecord::RecordNotFound) - end - - it 'deletes user associations in batches' do - expect(user).to receive(:destroy_dependent_associations_in_batches) + context "deletions with solo owned groups" do + let(:solo_owned) { create(:group) } + let(:member) { create(:group_member) } + let(:user) { member.user } - service.execute(user) - end - - it 'does not include snippets when deleting in batches' do - expect(user).to receive(:destroy_dependent_associations_in_batches).with({ exclude: [:snippets] }) - - service.execute(user) - end - - it 'calls the bulk snippet destroy service for the user personal snippets' do - repo1 = create(:personal_snippet, :repository, author: user).snippet_repository - repo2 = create(:project_snippet, :repository, project: project, author: user).snippet_repository - - aggregate_failures do - expect(gitlab_shell.repository_exists?(repo1.shard_name, repo1.disk_path + '.git')).to be_truthy - expect(gitlab_shell.repository_exists?(repo2.shard_name, repo2.disk_path + '.git')).to be_truthy - end - - # Call made when destroying user personal projects - expect(Snippets::BulkDestroyService).to receive(:new) - .with(admin, project.snippets).and_call_original - - # Call to remove user personal snippets and for - # project snippets where projects are not user personal - # ones - expect(Snippets::BulkDestroyService).to receive(:new) - .with(admin, user.snippets.only_personal_snippets).and_call_original - - service.execute(user) - - aggregate_failures do - expect(gitlab_shell.repository_exists?(repo1.shard_name, repo1.disk_path + '.git')).to be_falsey - expect(gitlab_shell.repository_exists?(repo2.shard_name, repo2.disk_path + '.git')).to be_falsey - end - end - - it 'calls the bulk snippet destroy service with hard delete option if it is present' do - # this avoids getting into Projects::DestroyService as it would - # call Snippets::BulkDestroyService first! - allow(user).to receive(:personal_projects).and_return([]) - - expect_next_instance_of(Snippets::BulkDestroyService) do |bulk_destroy_service| - expect(bulk_destroy_service).to receive(:execute).with({ skip_authorization: true }).and_call_original - end - - service.execute(user, { hard_delete: true }) - end - - it 'does not delete project snippets that the user is the author of' do - repo = create(:project_snippet, :repository, author: user).snippet_repository - service.execute(user) - expect(gitlab_shell.repository_exists?(repo.shard_name, repo.disk_path + '.git')).to be_truthy - expect(User.ghost.snippets).to include(repo.snippet) - end - - context 'when an error is raised deleting snippets' do - it 'does not delete user' do - snippet = create(:personal_snippet, :repository, author: user) - - bulk_service = double - allow(Snippets::BulkDestroyService).to receive(:new).and_call_original - allow(Snippets::BulkDestroyService).to receive(:new).with(admin, user.snippets).and_return(bulk_service) - allow(bulk_service).to receive(:execute).and_return(ServiceResponse.error(message: 'foo')) - - aggregate_failures do - expect { service.execute(user) } - .to raise_error(Users::DestroyService::DestroyError, 'foo' ) - expect(snippet.reload).not_to be_nil - expect( - gitlab_shell.repository_exists?(snippet.repository_storage, - snippet.disk_path + '.git') - ).to be_truthy - end - end - end + before do + solo_owned.group_members = [member] + service.execute(user, delete_solo_owned_groups: true) end - context 'projects in pending_delete' do - before do - project.pending_delete = true - project.save! - end - - it 'destroys a project in pending_delete' do - expect_next_instance_of(Projects::DestroyService) do |destroy_service| - expect(destroy_service).to receive(:execute).once.and_return(true) - end - - service.execute(user) - - expect { Project.find(project.id) }.to raise_error(ActiveRecord::RecordNotFound) - end + it 'deletes solo owned groups' do + expect { Group.find(solo_owned.id) }.to raise_error(ActiveRecord::RecordNotFound) end + end - context "a deleted user's issues" do - let(:project) { create(:project) } - - before do - project.add_developer(user) - end - - context "for an issue the user was assigned to" do - let!(:issue) { create(:issue, project: project, assignees: [user]) } - - before do - service.execute(user) - end + context 'deletions with inherited group owners' do + let(:group) { create(:group, :nested) } + let(:user) { create(:user) } + let(:inherited_owner) { create(:user) } - it 'does not delete issues the user is assigned to' do - expect(Issue.find_by_id(issue.id)).to be_present - end + before do + group.parent.add_owner(inherited_owner) + group.add_owner(user) - it 'migrates the issue so that it is "Unassigned"' do - migrated_issue = Issue.find_by_id(issue.id) - - expect(migrated_issue.assignees).to be_empty - end - end + service.execute(user, delete_solo_owned_groups: true) end - context "a deleted user's merge_requests" do - let(:project) { create(:project, :repository) } + it 'does not delete the group' do + expect(Group.exists?(id: group)).to be_truthy + end + end + describe "user personal's repository removal" do + context 'storages' do before do - project.add_developer(user) + perform_enqueued_jobs { service.execute(user) } end - context "for an merge request the user was assigned to" do - let!(:merge_request) { create(:merge_request, source_project: project, assignees: [user]) } + context 'legacy storage' do + let!(:project) { create(:project, :empty_repo, :legacy_storage, namespace: user.namespace) } - before do - service.execute(user) - end - - it 'does not delete merge requests the user is assigned to' do - expect(MergeRequest.find_by_id(merge_request.id)).to be_present - end - - it 'migrates the merge request so that it is "Unassigned"' do - migrated_merge_request = MergeRequest.find_by_id(merge_request.id) - - expect(migrated_merge_request.assignees).to be_empty + it 'removes repository' do + expect( + gitlab_shell.repository_exists?(project.repository_storage, + "#{project.disk_path}.git") + ).to be_falsey end end - end - - context 'migrating associated records' do - let!(:issue) { create(:issue, author: user) } - it 'delegates to the `MigrateToGhostUser` service to move associated records to the ghost user' do - expect_any_instance_of(Users::MigrateToGhostUserService).to receive(:execute).once.and_call_original + context 'hashed storage' do + let!(:project) { create(:project, :empty_repo, namespace: user.namespace) } - service.execute(user) - - expect(issue.reload.author).to be_ghost - end - - context 'when hard_delete option is given' do - it 'will not ghost certain records' do - expect_any_instance_of(Users::MigrateToGhostUserService).to receive(:execute).once.and_call_original - - service.execute(user, hard_delete: true) - - expect(Issue.exists?(issue.id)).to be_falsy + it 'removes repository' do + expect( + gitlab_shell.repository_exists?(project.repository_storage, + "#{project.disk_path}.git") + ).to be_falsey end end end - end - - describe "Deletion permission checks" do - it 'does not delete the user when user is not an admin' do - other_user = create(:user) - - expect { described_class.new(other_user).execute(user) }.to raise_error(Gitlab::Access::AccessDeniedError) - expect(User.exists?(user.id)).to be(true) - end - - context 'when admin mode is enabled', :enable_admin_mode do - it 'allows admins to delete anyone' do - described_class.new(admin).execute(user) - - expect(User.exists?(user.id)).to be(false) - end - end - context 'when admin mode is disabled' do - it 'disallows admins to delete anyone' do - expect { described_class.new(admin).execute(user) }.to raise_error(Gitlab::Access::AccessDeniedError) + context 'repository removal status is taken into account' do + it 'raises exception' do + expect_next_instance_of(::Projects::DestroyService) do |destroy_service| + expect(destroy_service).to receive(:execute).and_return(false) + end - expect(User.exists?(user.id)).to be(true) + expect { service.execute(user) } + .to raise_error(Users::DestroyService::DestroyError, + "Project #{project.id} can't be deleted") end end - - it 'allows users to delete their own account' do - described_class.new(user).execute(user) - - expect(User.exists?(user.id)).to be(false) - end - - it 'allows user to be deleted if skip_authorization: true' do - other_user = create(:user) - - described_class.new(user).execute(other_user, skip_authorization: true) - - expect(User.exists?(other_user.id)).to be(false) - end end - context 'batched nullify' do - let(:other_user) { create(:user) } + describe "calls the before/after callbacks" do + it 'of project_members' do + expect_any_instance_of(ProjectMember).to receive(:run_callbacks).with(:find).once + expect_any_instance_of(ProjectMember).to receive(:run_callbacks).with(:initialize).once + expect_any_instance_of(ProjectMember).to receive(:run_callbacks).with(:destroy).once - # rubocop:disable Layout/LineLength - def nullify_in_batches_regexp(table, column, user, batch_size: 100) - %r{^UPDATE "#{table}" SET "#{column}" = NULL WHERE "#{table}"."id" IN \(SELECT "#{table}"."id" FROM "#{table}" WHERE "#{table}"."#{column}" = #{user.id} LIMIT #{batch_size}\)} + service.execute(user) end - def delete_in_batches_regexps(table, column, user, items, batch_size: 1000) - select_query = %r{^SELECT "#{table}".* FROM "#{table}" WHERE "#{table}"."#{column}" = #{user.id}.*ORDER BY "#{table}"."id" ASC LIMIT #{batch_size}} - - [select_query] + items.map { |item| %r{^DELETE FROM "#{table}" WHERE "#{table}"."id" = #{item.id}} } - end - # rubocop:enable Layout/LineLength + it 'of group_members' do + group_member = create(:group_member) + group_member.group.group_members.create!(user: user, access_level: 40) - it 'nullifies related associations in batches' do - expect(other_user).to receive(:nullify_dependent_associations_in_batches).and_call_original + expect_any_instance_of(GroupMember).to receive(:run_callbacks).with(:find).once + expect_any_instance_of(GroupMember).to receive(:run_callbacks).with(:initialize).once + expect_any_instance_of(GroupMember).to receive(:run_callbacks).with(:destroy).once - described_class.new(user).execute(other_user, skip_authorization: true) + service.execute(user) end + end - it 'nullifies issues and resource associations', :aggregate_failures do - issue = create(:issue, closed_by: other_user, updated_by: other_user) - resource_label_event = create(:resource_label_event, user: other_user) - resource_state_event = create(:resource_state_event, user: other_user) - todos = create_list(:todo, 2, project: issue.project, user: other_user, author: other_user, target: issue) - event = create(:event, project: issue.project, author: other_user) + describe 'prometheus metrics', :prometheus do + context 'scheduled records' do + context 'with a single record' do + it 'updates the scheduled records gauge' do + service.execute(user) - query_recorder = ActiveRecord::QueryRecorder.new do - described_class.new(user).execute(other_user, skip_authorization: true) + gauge = Gitlab::Metrics.registry.get(:gitlab_ghost_user_migration_scheduled_records_total) + expect(gauge.get).to eq(1) + end end - issue.reload - resource_label_event.reload - resource_state_event.reload - - expect(issue.closed_by).to be_nil - expect(issue.updated_by).to be_nil - expect(resource_label_event.user).to be_nil - expect(resource_state_event.user).to be_nil - expect(other_user.authored_todos).to be_empty - expect(other_user.todos).to be_empty - expect(other_user.authored_events).to be_empty - - expected_queries = [ - nullify_in_batches_regexp(:issues, :updated_by_id, other_user), - nullify_in_batches_regexp(:issues, :closed_by_id, other_user), - nullify_in_batches_regexp(:resource_label_events, :user_id, other_user), - nullify_in_batches_regexp(:resource_state_events, :user_id, other_user) - ] - - expected_queries += delete_in_batches_regexps(:todos, :user_id, other_user, todos) - expected_queries += delete_in_batches_regexps(:todos, :author_id, other_user, todos) - expected_queries += delete_in_batches_regexps(:events, :author_id, other_user, [event]) - - expect(query_recorder.log).to include(*expected_queries) - end + context 'with approximate count due to large number of records' do + it 'updates the scheduled records gauge' do + allow(Users::GhostUserMigration) + .to(receive_message_chain(:limit, :count).and_return(1001)) + allow(Users::GhostUserMigration).to(receive(:minimum)).and_return(42) + allow(Users::GhostUserMigration).to(receive(:maximum)).and_return(9042) - it 'nullifies merge request associations', :aggregate_failures do - merge_request = create(:merge_request, source_project: project, target_project: project, - assignee: other_user, updated_by: other_user, merge_user: other_user) - merge_request.metrics.update!(merged_by: other_user, latest_closed_by: other_user) - merge_request.reviewers = [other_user] - merge_request.assignees = [other_user] + service.execute(user) - query_recorder = ActiveRecord::QueryRecorder.new do - described_class.new(user).execute(other_user, skip_authorization: true) + gauge = Gitlab::Metrics.registry.get(:gitlab_ghost_user_migration_scheduled_records_total) + expect(gauge.get).to eq(9000) + end end - - merge_request.reload - - expect(merge_request.updated_by).to be_nil - expect(merge_request.assignee).to be_nil - expect(merge_request.assignee_id).to be_nil - expect(merge_request.metrics.merged_by).to be_nil - expect(merge_request.metrics.latest_closed_by).to be_nil - expect(merge_request.reviewers).to be_empty - expect(merge_request.assignees).to be_empty - - expected_queries = [ - nullify_in_batches_regexp(:merge_requests, :updated_by_id, other_user), - nullify_in_batches_regexp(:merge_requests, :assignee_id, other_user), - nullify_in_batches_regexp(:merge_request_metrics, :merged_by_id, other_user), - nullify_in_batches_regexp(:merge_request_metrics, :latest_closed_by_id, other_user) - ] - - expected_queries += delete_in_batches_regexps(:merge_request_assignees, :user_id, other_user, - merge_request.assignees) - expected_queries += delete_in_batches_regexps(:merge_request_reviewers, :user_id, other_user, - merge_request.reviewers) - - expect(query_recorder.log).to include(*expected_queries) end - end - end - context 'when user_destroy_with_limited_execution_time_worker is enabled' do - include_examples 'pre-migrate clean-up' + context 'lag' do + it 'update the lag gauge', :freeze_time do + create(:ghost_user_migration, created_at: 10.minutes.ago) - describe "Deletes a user and all their personal projects", :enable_admin_mode do - context 'no options are given' do - it 'creates GhostUserMigration record to handle migration in a worker' do - expect { service.execute(user) } - .to( - change do - Users::GhostUserMigration.where(user: user, - initiator_user: admin) - .exists? - end.from(false).to(true)) + service.execute(user) + + gauge = Gitlab::Metrics.registry.get(:gitlab_ghost_user_migration_lag_seconds) + expect(gauge.get).to eq(600) end end end + end - describe "Deletion permission checks" do - it 'does not delete the user when user is not an admin' do - other_user = create(:user) - - expect { described_class.new(other_user).execute(user) }.to raise_error(Gitlab::Access::AccessDeniedError) - - expect(Users::GhostUserMigration).not_to be_exists - end - - context 'when admin mode is enabled', :enable_admin_mode do - it 'allows admins to delete anyone' do - expect { described_class.new(admin).execute(user) } - .to( - change do - Users::GhostUserMigration.where(user: user, - initiator_user: admin) - .exists? - end.from(false).to(true)) - end - end + describe "Deletion permission checks" do + it 'does not delete the user when user is not an admin' do + other_user = create(:user) - context 'when admin mode is disabled' do - it 'disallows admins to delete anyone' do - expect { described_class.new(admin).execute(user) }.to raise_error(Gitlab::Access::AccessDeniedError) + expect { described_class.new(other_user).execute(user) }.to raise_error(Gitlab::Access::AccessDeniedError) - expect(Users::GhostUserMigration).not_to be_exists - end - end + expect(Users::GhostUserMigration).not_to be_exists + end - it 'allows users to delete their own account' do - expect { described_class.new(user).execute(user) } + context 'when admin mode is enabled', :enable_admin_mode do + it 'allows admins to delete anyone' do + expect { described_class.new(admin).execute(user) } .to( change do Users::GhostUserMigration.where(user: user, - initiator_user: user) + initiator_user: admin) .exists? end.from(false).to(true)) end + end - it 'allows user to be deleted if skip_authorization: true' do - other_user = create(:user) + context 'when admin mode is disabled' do + it 'disallows admins to delete anyone' do + expect { described_class.new(admin).execute(user) }.to raise_error(Gitlab::Access::AccessDeniedError) + + expect(Users::GhostUserMigration).not_to be_exists + end + end - expect do - described_class.new(user) - .execute(other_user, skip_authorization: true) - end.to( + it 'allows users to delete their own account' do + expect { described_class.new(user).execute(user) } + .to( change do - Users::GhostUserMigration.where(user: other_user, - initiator_user: user ) + Users::GhostUserMigration.where(user: user, + initiator_user: user) .exists? end.from(false).to(true)) - end + end + + it 'allows user to be deleted if skip_authorization: true' do + other_user = create(:user) + + expect do + described_class.new(user) + .execute(other_user, skip_authorization: true) + end.to( + change do + Users::GhostUserMigration.where(user: other_user, + initiator_user: user ) + .exists? + end.from(false).to(true)) end end end diff --git a/spec/services/users/migrate_records_to_ghost_user_in_batches_service_spec.rb b/spec/services/users/migrate_records_to_ghost_user_in_batches_service_spec.rb index 7366b1646b9..107ff82016c 100644 --- a/spec/services/users/migrate_records_to_ghost_user_in_batches_service_spec.rb +++ b/spec/services/users/migrate_records_to_ghost_user_in_batches_service_spec.rb @@ -27,5 +27,34 @@ RSpec.describe Users::MigrateRecordsToGhostUserInBatchesService do service.execute end + + it 'process jobs ordered by the consume_after timestamp' do + older_ghost_user_migration = create(:ghost_user_migration, user: create(:user), + consume_after: 5.minutes.ago) + + # setup execution tracker to only allow a single job to be processed + allow_next_instance_of(::Gitlab::Utils::ExecutionTracker) do |tracker| + allow(tracker).to receive(:over_limit?).and_return(false, true) + end + + expect(Users::MigrateRecordsToGhostUserService).to( + receive(:new).with(older_ghost_user_migration.user, + older_ghost_user_migration.initiator_user, + any_args) + ).and_call_original + + service.execute + end + + it 'reschedules job in case of an error', :freeze_time do + expect_next_instance_of(Users::MigrateRecordsToGhostUserService) do |service| + expect(service).to(receive(:execute)).and_raise(ActiveRecord::QueryCanceled) + end + expect(Gitlab::ErrorTracking).to receive(:track_exception) + + expect { service.execute }.to( + change { ghost_user_migration.reload.consume_after } + .to(30.minutes.from_now)) + end end end diff --git a/spec/services/users/migrate_records_to_ghost_user_service_spec.rb b/spec/services/users/migrate_records_to_ghost_user_service_spec.rb index 766be51ae13..6082c7bd10e 100644 --- a/spec/services/users/migrate_records_to_ghost_user_service_spec.rb +++ b/spec/services/users/migrate_records_to_ghost_user_service_spec.rb @@ -146,24 +146,106 @@ RSpec.describe Users::MigrateRecordsToGhostUserService do end context 'for batched nullify' do + # rubocop:disable Layout/LineLength + def nullify_in_batches_regexp(table, column, user, batch_size: 100) + %r{^UPDATE "#{table}" SET "#{column}" = NULL WHERE "#{table}"."id" IN \(SELECT "#{table}"."id" FROM "#{table}" WHERE "#{table}"."#{column}" = #{user.id} LIMIT #{batch_size}\)} + end + + def delete_in_batches_regexps(table, column, user, items, batch_size: 1000) + select_query = %r{^SELECT "#{table}".* FROM "#{table}" WHERE "#{table}"."#{column}" = #{user.id}.*ORDER BY "#{table}"."id" ASC LIMIT #{batch_size}} + + [select_query] + items.map { |item| %r{^DELETE FROM "#{table}" WHERE "#{table}"."id" = #{item.id}} } + end + # rubocop:enable Layout/LineLength + it 'nullifies related associations in batches' do expect(user).to receive(:nullify_dependent_associations_in_batches).and_call_original service.execute end - it 'nullifies last_updated_issues, closed_issues, resource_label_events' do + it 'nullifies associations marked as `dependent: :nullify` and'\ + 'destroys the associations marked as `dependent: :destroy`, in batches', :aggregate_failures do + # associations to be nullified issue = create(:issue, closed_by: user, updated_by: user) resource_label_event = create(:resource_label_event, user: user) + resource_state_event = create(:resource_state_event, user: user) + created_project = create(:project, creator: user) - service.execute + # associations to be destroyed + todos = create_list(:todo, 2, project: issue.project, user: user, author: user, target: issue) + event = create(:event, project: issue.project, author: user) + + query_recorder = ActiveRecord::QueryRecorder.new do + service.execute + end issue.reload resource_label_event.reload + resource_state_event.reload + created_project.reload expect(issue.closed_by).to be_nil - expect(issue.updated_by).to be_nil - expect(resource_label_event.user).to be_nil + expect(issue.updated_by_id).to be_nil + expect(resource_label_event.user_id).to be_nil + expect(resource_state_event.user_id).to be_nil + expect(created_project.creator_id).to be_nil + expect(user.authored_todos).to be_empty + expect(user.todos).to be_empty + expect(user.authored_events).to be_empty + + expected_queries = [ + nullify_in_batches_regexp(:issues, :updated_by_id, user), + nullify_in_batches_regexp(:issues, :closed_by_id, user), + nullify_in_batches_regexp(:resource_label_events, :user_id, user), + nullify_in_batches_regexp(:resource_state_events, :user_id, user), + nullify_in_batches_regexp(:projects, :creator_id, user) + ] + + expected_queries += delete_in_batches_regexps(:todos, :user_id, user, todos) + expected_queries += delete_in_batches_regexps(:todos, :author_id, user, todos) + expected_queries += delete_in_batches_regexps(:events, :author_id, user, [event]) + + expect(query_recorder.log).to include(*expected_queries) + end + + it 'nullifies merge request associations', :aggregate_failures do + merge_request = create(:merge_request, source_project: project, + target_project: project, + assignee: user, + updated_by: user, + merge_user: user) + merge_request.metrics.update!(merged_by: user, latest_closed_by: user) + merge_request.reviewers = [user] + merge_request.assignees = [user] + + query_recorder = ActiveRecord::QueryRecorder.new do + service.execute + end + + merge_request.reload + + expect(merge_request.updated_by).to be_nil + expect(merge_request.assignee).to be_nil + expect(merge_request.assignee_id).to be_nil + expect(merge_request.metrics.merged_by).to be_nil + expect(merge_request.metrics.latest_closed_by).to be_nil + expect(merge_request.reviewers).to be_empty + expect(merge_request.assignees).to be_empty + + expected_queries = [ + nullify_in_batches_regexp(:merge_requests, :updated_by_id, user), + nullify_in_batches_regexp(:merge_requests, :assignee_id, user), + nullify_in_batches_regexp(:merge_request_metrics, :merged_by_id, user), + nullify_in_batches_regexp(:merge_request_metrics, :latest_closed_by_id, user) + ] + + expected_queries += delete_in_batches_regexps(:merge_request_assignees, :user_id, user, + merge_request.assignees) + expected_queries += delete_in_batches_regexps(:merge_request_reviewers, :user_id, user, + merge_request.reviewers) + + expect(query_recorder.log).to include(*expected_queries) end end @@ -235,7 +317,7 @@ RSpec.describe Users::MigrateRecordsToGhostUserService do aggregate_failures do expect { service.execute }.to( - raise_error(Users::MigrateRecordsToGhostUserService::DestroyError, 'foo' )) + raise_error(Users::MigrateRecordsToGhostUserService::DestroyError, 'foo')) expect(snippet.reload).not_to be_nil expect( gitlab_shell.repository_exists?(snippet.repository_storage, diff --git a/spec/services/users/migrate_to_ghost_user_service_spec.rb b/spec/services/users/migrate_to_ghost_user_service_spec.rb deleted file mode 100644 index 073ebaae5b0..00000000000 --- a/spec/services/users/migrate_to_ghost_user_service_spec.rb +++ /dev/null @@ -1,97 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Users::MigrateToGhostUserService do - let!(:user) { create(:user) } - let!(:project) { create(:project, :repository) } - let(:service) { described_class.new(user) } - let(:always_ghost) { false } - - context "migrating a user's associated records to the ghost user" do - context 'issues' do - context 'deleted user is present as both author and edited_user' do - include_examples "migrating a deleted user's associated records to the ghost user", Issue, [:author, :last_edited_by] do - let(:created_record) do - create(:issue, project: project, author: user, last_edited_by: user) - end - end - end - - context 'deleted user is present only as edited_user' do - include_examples "migrating a deleted user's associated records to the ghost user", Issue, [:last_edited_by] do - let(:created_record) { create(:issue, project: project, author: create(:user), last_edited_by: user) } - end - end - end - - context 'merge requests' do - context 'deleted user is present as both author and merge_user' do - include_examples "migrating a deleted user's associated records to the ghost user", MergeRequest, [:author, :merge_user] do - let(:created_record) { create(:merge_request, source_project: project, author: user, merge_user: user, target_branch: "first") } - end - end - - context 'deleted user is present only as both merge_user' do - include_examples "migrating a deleted user's associated records to the ghost user", MergeRequest, [:merge_user] do - let(:created_record) { create(:merge_request, source_project: project, merge_user: user, target_branch: "first") } - end - end - end - - context 'notes' do - include_examples "migrating a deleted user's associated records to the ghost user", Note do - let(:created_record) { create(:note, project: project, author: user) } - end - end - - context 'abuse reports' do - include_examples "migrating a deleted user's associated records to the ghost user", AbuseReport do - let(:created_record) { create(:abuse_report, reporter: user, user: create(:user)) } - end - end - - context 'award emoji' do - include_examples "migrating a deleted user's associated records to the ghost user", AwardEmoji, [:user] do - let(:created_record) { create(:award_emoji, user: user) } - - context "when the awardable already has an award emoji of the same name assigned to the ghost user" do - let(:awardable) { create(:issue) } - let!(:existing_award_emoji) { create(:award_emoji, user: User.ghost, name: "thumbsup", awardable: awardable) } - let!(:award_emoji) { create(:award_emoji, user: user, name: "thumbsup", awardable: awardable) } - - it "migrates the award emoji regardless" do - service.execute - - migrated_record = AwardEmoji.find_by_id(award_emoji.id) - - expect(migrated_record.user).to eq(User.ghost) - end - - it "does not leave the migrated award emoji in an invalid state" do - service.execute - - migrated_record = AwardEmoji.find_by_id(award_emoji.id) - - expect(migrated_record).to be_valid - end - end - end - end - - context 'snippets' do - include_examples "migrating a deleted user's associated records to the ghost user", Snippet do - let(:created_record) { create(:snippet, project: project, author: user) } - end - end - - context 'reviews' do - let!(:user) { create(:user) } - let(:service) { described_class.new(user) } - - include_examples "migrating a deleted user's associated records to the ghost user", Review, [:author] do - let(:created_record) { create(:review, author: user) } - end - end - end -end diff --git a/spec/services/users/reject_service_spec.rb b/spec/services/users/reject_service_spec.rb index abff6b1e023..37d003c5dac 100644 --- a/spec/services/users/reject_service_spec.rb +++ b/spec/services/users/reject_service_spec.rb @@ -35,29 +35,14 @@ RSpec.describe Users::RejectService do context 'success' do context 'when the executor user is an admin in admin mode', :enable_admin_mode do - context 'when user_destroy_with_limited_execution_time_worker is enabled' do - it 'initiates user removal', :sidekiq_inline do - subject - - expect(subject[:status]).to eq(:success) - expect( - Users::GhostUserMigration.where(user: user, - initiator_user: current_user) - ).to be_exists - end - end - - context 'when user_destroy_with_limited_execution_time_worker is disabled' do - before do - stub_feature_flags(user_destroy_with_limited_execution_time_worker: false) - end - - it 'deletes the user', :sidekiq_inline do - subject + it 'initiates user removal', :sidekiq_inline do + subject - expect(subject[:status]).to eq(:success) - expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound) - end + expect(subject[:status]).to eq(:success) + expect( + Users::GhostUserMigration.where(user: user, + initiator_user: current_user) + ).to be_exists end it 'emails the user on rejection' do @@ -73,7 +58,7 @@ RSpec.describe Users::RejectService do subject - expect(Gitlab::AppLogger).to have_received(:info).with(message: "User instance access request rejected", user: "#{user.username}", email: "#{user.email}", rejected_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + expect(Gitlab::AppLogger).to have_received(:info).with(message: "User instance access request rejected", user: user.username.to_s, email: user.email.to_s, rejected_by: current_user.username.to_s, ip_address: current_user.current_sign_in_ip.to_s) end end end diff --git a/spec/services/users/unban_service_spec.rb b/spec/services/users/unban_service_spec.rb index d536baafdcc..3dcb8450e7b 100644 --- a/spec/services/users/unban_service_spec.rb +++ b/spec/services/users/unban_service_spec.rb @@ -38,7 +38,7 @@ RSpec.describe Users::UnbanService do 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}") + expect(Gitlab::AppLogger).to receive(:info).with(message: "User unban", user: user.username.to_s, email: user.email.to_s, unban_by: current_user.username.to_s, ip_address: current_user.current_sign_in_ip.to_s) unban_user end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 551c3dbcc82..c081b20d95f 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -175,22 +175,6 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state ).once end - context 'when webhooks_gitlab_instance_header flag is disabled' do - before do - stub_feature_flags(webhooks_gitlab_instance_header: false) - end - - it 'excludes the X-Gitlab-Instance header' do - stub_full_request(project_hook.url, method: :post) - - service_instance.execute - - expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with( - headers: headers.except('X-Gitlab-Instance') - ).once - end - end - context 'when the data is a Gitlab::DataBuilder::Pipeline' do let(:pipeline) { create(:ci_pipeline, project: project) } let(:data) { ::Gitlab::DataBuilder::Pipeline.new(pipeline) } @@ -245,7 +229,7 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state it 'does not execute disabled hooks' do allow(service_instance).to receive(:disabled?).and_return(true) - expect(service_instance.execute).to eq({ status: :error, message: 'Hook disabled' }) + expect(service_instance.execute).to have_attributes(status: :error, message: 'Hook disabled') end it 'executes and registers the hook with the recursion detection', :aggregate_failures do @@ -317,7 +301,8 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state project_hook.enable! stub_full_request(project_hook.url, method: :post).to_raise(exception) - expect(service_instance.execute).to eq({ status: :error, message: exception.to_s }) + + expect(service_instance.execute).to have_attributes(status: :error, message: exception.to_s) expect { service_instance.execute }.not_to raise_error end end @@ -326,7 +311,10 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state let_it_be(:project_hook) { create(:project_hook, url: 'http://server.com/my path/') } it 'handles exceptions' do - expect(service_instance.execute).to eq(status: :error, message: 'bad URI(is not URI?): "http://server.com/my path/"') + expect(service_instance.execute).to have_attributes( + status: :error, + message: 'bad URI(is not URI?): "http://server.com/my path/"' + ) expect { service_instance.execute }.not_to raise_error end end @@ -335,20 +323,31 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state it 'does not perform the request' do stub_const("#{described_class}::REQUEST_BODY_SIZE_LIMIT", 10.bytes) - expect(service_instance.execute).to eq({ status: :error, message: "Gitlab::Json::LimitedEncoder::LimitExceeded" }) + expect(service_instance.execute).to have_attributes( + status: :error, + message: 'Gitlab::Json::LimitedEncoder::LimitExceeded' + ) end end it 'handles 200 status code' do stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success') - expect(service_instance.execute).to include({ status: :success, http_status: 200, message: 'Success' }) + expect(service_instance.execute).to have_attributes( + status: :success, + payload: { http_status: 200 }, + message: 'Success' + ) end it 'handles 2xx status codes' do stub_full_request(project_hook.url, method: :post).to_return(status: 201, body: 'Success') - expect(service_instance.execute).to include({ status: :success, http_status: 201, message: 'Success' }) + expect(service_instance.execute).to have_attributes( + status: :success, + payload: { http_status: 201 }, + message: 'Success' + ) end context 'execution logging' do diff --git a/spec/services/work_items/create_service_spec.rb b/spec/services/work_items/create_service_spec.rb index c0bcf9b606d..1bd7e15db67 100644 --- a/spec/services/work_items/create_service_spec.rb +++ b/spec/services/work_items/create_service_spec.rb @@ -179,16 +179,6 @@ RSpec.describe WorkItems::CreateService do let(:error_message) { 'only Issue and Incident can be parent of Task.' } end end - - context 'when hierarchy feature flag is disabled' do - before do - stub_feature_flags(work_items_hierarchy: false) - end - - it_behaves_like 'fails creating work item and returns errors' do - let(:error_message) { '`work_items_hierarchy` feature flag disabled for this project' } - end - end end context 'when user cannot admin parent link' do diff --git a/spec/services/work_items/update_service_spec.rb b/spec/services/work_items/update_service_spec.rb index 1761d1104dd..68efb4c220b 100644 --- a/spec/services/work_items/update_service_spec.rb +++ b/spec/services/work_items/update_service_spec.rb @@ -311,6 +311,34 @@ RSpec.describe WorkItems::UpdateService do end end end + + context 'for milestone widget' do + let_it_be(:milestone) { create(:milestone, project: project) } + + let(:widget_params) { { milestone_widget: { milestone_id: milestone.id } } } + + context 'when milestone is updated' do + it "triggers 'issuableMilestoneUpdated'" do + expect(work_item.milestone).to eq(nil) + expect(GraphqlTriggers).to receive(:issuable_milestone_updated).with(work_item).and_call_original + + update_work_item + end + end + + context 'when milestone remains unchanged' do + before do + update_work_item + end + + it "does not trigger 'issuableMilestoneUpdated'" do + expect(work_item.milestone).to eq(milestone) + expect(GraphqlTriggers).not_to receive(:issuable_milestone_updated) + + update_work_item + end + end + end end describe 'label updates' do diff --git a/spec/services/work_items/widgets/hierarchy_service/update_service_spec.rb b/spec/services/work_items/widgets/hierarchy_service/update_service_spec.rb index 9a425d5308c..1b8c4c5f15f 100644 --- a/spec/services/work_items/widgets/hierarchy_service/update_service_spec.rb +++ b/spec/services/work_items/widgets/hierarchy_service/update_service_spec.rb @@ -42,18 +42,6 @@ RSpec.describe WorkItems::Widgets::HierarchyService::UpdateService do let_it_be(:child_work_item3) { create(:work_item, :task, project: project) } let_it_be(:child_work_item4) { create(:work_item, :task, project: project) } - context 'when work_items_hierarchy feature flag is disabled' do - let(:params) { { children: [child_work_item4] } } - - before do - stub_feature_flags(work_items_hierarchy: false) - end - - it_behaves_like 'raises a WidgetError' do - let(:message) { '`work_items_hierarchy` feature flag disabled for this project' } - end - end - context 'when user has insufficient permissions to link work items' do let(:params) { { children: [child_work_item4] } } @@ -105,16 +93,6 @@ RSpec.describe WorkItems::Widgets::HierarchyService::UpdateService do let(:params) { { parent: parent_work_item } } - context 'when work_items_hierarchy feature flag is disabled' do - before do - stub_feature_flags(work_items_hierarchy: false) - end - - it_behaves_like 'raises a WidgetError' do - let(:message) { '`work_items_hierarchy` feature flag disabled for this project' } - end - end - context 'when user has insufficient permissions to link work items' do it_behaves_like 'raises a WidgetError' do let(:message) { not_found_error } diff --git a/spec/services/work_items/widgets/milestone_service/create_service_spec.rb b/spec/services/work_items/widgets/milestone_service/create_service_spec.rb new file mode 100644 index 00000000000..3f90784b703 --- /dev/null +++ b/spec/services/work_items/widgets/milestone_service/create_service_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::Widgets::MilestoneService::CreateService do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :private, group: group) } + let_it_be(:project_milestone) { create(:milestone, project: project) } + let_it_be(:group_milestone) { create(:milestone, group: group) } + let_it_be(:guest) { create(:user) } + + let(:current_user) { guest } + let(:work_item) { build(:work_item, project: project, updated_at: 1.day.ago) } + let(:widget) { work_item.widgets.find { |widget| widget.is_a?(WorkItems::Widgets::Milestone) } } + let(:service) { described_class.new(widget: widget, current_user: current_user) } + + before do + project.add_guest(guest) + end + + describe '#before_create_callback' do + it_behaves_like "setting work item's milestone" do + subject(:execute_callback) do + service.before_create_callback(params: params) + end + end + end +end diff --git a/spec/services/work_items/widgets/milestone_service/update_service_spec.rb b/spec/services/work_items/widgets/milestone_service/update_service_spec.rb new file mode 100644 index 00000000000..f3a7fc156b9 --- /dev/null +++ b/spec/services/work_items/widgets/milestone_service/update_service_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::Widgets::MilestoneService::UpdateService do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :private, group: group) } + let_it_be(:project_milestone) { create(:milestone, project: project) } + let_it_be(:group_milestone) { create(:milestone, group: group) } + let_it_be(:reporter) { create(:user) } + let_it_be(:guest) { create(:user) } + + let(:work_item) { create(:work_item, project: project, updated_at: 1.day.ago) } + let(:widget) { work_item.widgets.find { |widget| widget.is_a?(WorkItems::Widgets::Milestone) } } + let(:service) { described_class.new(widget: widget, current_user: current_user) } + + before do + project.add_reporter(reporter) + project.add_guest(guest) + end + + describe '#before_update_callback' do + context 'when current user is not allowed to set work item metadata' do + let(:current_user) { guest } + let(:params) { { milestone_id: group_milestone.id } } + + it "does not set the work item's milestone" do + expect { service.before_update_callback(params: params) } + .to not_change(work_item, :milestone) + end + end + + context "when current user is allowed to set work item metadata" do + let(:current_user) { reporter } + + it_behaves_like "setting work item's milestone" do + subject(:execute_callback) do + service.before_update_callback(params: params) + end + end + + context 'when unsetting a milestone' do + let(:params) { { milestone_id: nil } } + + before do + work_item.update!(milestone: project_milestone) + end + + it "sets the work item's milestone" do + expect { service.before_update_callback(params: params) } + .to change(work_item, :milestone) + .from(project_milestone) + .to(nil) + end + end + end + end +end diff --git a/spec/services/x509_certificate_revoke_service_spec.rb b/spec/services/x509_certificate_revoke_service_spec.rb index adad3281c13..ff5d2dc058b 100644 --- a/spec/services/x509_certificate_revoke_service_spec.rb +++ b/spec/services/x509_certificate_revoke_service_spec.rb @@ -5,11 +5,11 @@ require 'spec_helper' RSpec.describe X509CertificateRevokeService do describe '#execute' do let(:service) { described_class.new } - let!(:x509_signature_1) { create(:x509_commit_signature, x509_certificate: x509_certificate, verification_status: :verified ) } - let!(:x509_signature_2) { create(:x509_commit_signature, x509_certificate: x509_certificate, verification_status: :verified ) } + let!(:x509_signature_1) { create(:x509_commit_signature, x509_certificate: x509_certificate, verification_status: :verified) } + let!(:x509_signature_2) { create(:x509_commit_signature, x509_certificate: x509_certificate, verification_status: :verified) } context 'for revoked certificates' do - let(:x509_certificate) { create(:x509_certificate, certificate_status: :revoked ) } + let(:x509_certificate) { create(:x509_certificate, certificate_status: :revoked) } it 'update all commit signatures' do expect do |