diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-08-20 18:42:06 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-08-20 18:42:06 +0000 |
commit | 6e4e1050d9dba2b7b2523fdd1768823ab85feef4 (patch) | |
tree | 78be5963ec075d80116a932011d695dd33910b4e /spec/services | |
parent | 1ce776de4ae122aba3f349c02c17cebeaa8ecf07 (diff) | |
download | gitlab-ce-6e4e1050d9dba2b7b2523fdd1768823ab85feef4.tar.gz |
Add latest changes from gitlab-org/gitlab@13-3-stable-ee
Diffstat (limited to 'spec/services')
145 files changed, 3165 insertions, 1625 deletions
diff --git a/spec/services/admin/propagate_integration_service_spec.rb b/spec/services/admin/propagate_integration_service_spec.rb index 843b78a41e9..2e879cf06d1 100644 --- a/spec/services/admin/propagate_integration_service_spec.rb +++ b/spec/services/admin/propagate_integration_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Admin::PropagateIntegrationService do describe '.propagate' do - let(:excluded_attributes) { %w[id project_id inherit_from_id instance created_at updated_at title description] } + let(:excluded_attributes) { %w[id project_id inherit_from_id instance created_at updated_at default] } let!(:project) { create(:project) } let!(:instance_integration) do JiraService.create!( diff --git a/spec/services/alert_management/alerts/update_service_spec.rb b/spec/services/alert_management/alerts/update_service_spec.rb index 91b02325bad..ee04fc55984 100644 --- a/spec/services/alert_management/alerts/update_service_spec.rb +++ b/spec/services/alert_management/alerts/update_service_spec.rb @@ -147,8 +147,7 @@ RSpec.describe AlertManagement::Alerts::UpdateService do end it_behaves_like 'does not add a system note' - # TODO: We should not add another todo in this scenario - it_behaves_like 'adds a todo' + it_behaves_like 'does not add a todo' end context 'with multiple users included' do diff --git a/spec/services/alert_management/create_alert_issue_service_spec.rb b/spec/services/alert_management/create_alert_issue_service_spec.rb index a8f2b4ee09c..cf24188a738 100644 --- a/spec/services/alert_management/create_alert_issue_service_spec.rb +++ b/spec/services/alert_management/create_alert_issue_service_spec.rb @@ -88,7 +88,6 @@ RSpec.describe AlertManagement::CreateAlertIssueService do it_behaves_like 'creating an alert issue' it_behaves_like 'setting an issue attributes' - it_behaves_like 'create alert issue sets issue labels' end context 'when the alert is generic' do @@ -97,7 +96,6 @@ RSpec.describe AlertManagement::CreateAlertIssueService do it_behaves_like 'creating an alert issue' it_behaves_like 'setting an issue attributes' - it_behaves_like 'create alert issue sets issue labels' end context 'when issue cannot be created' do diff --git a/spec/services/award_emojis/copy_service_spec.rb b/spec/services/award_emojis/copy_service_spec.rb new file mode 100644 index 00000000000..e85c548968e --- /dev/null +++ b/spec/services/award_emojis/copy_service_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AwardEmojis::CopyService do + let_it_be(:from_awardable) do + create(:issue, award_emoji: [ + build(:award_emoji, name: 'thumbsup'), + build(:award_emoji, name: 'thumbsdown') + ]) + end + + describe '#initialize' do + it 'validates that we cannot copy AwardEmoji to the same Awardable' do + expect { described_class.new(from_awardable, from_awardable) }.to raise_error(ArgumentError) + end + end + + describe '#execute' do + let(:to_awardable) { create(:issue) } + + subject(:execute_service) { described_class.new(from_awardable, to_awardable).execute } + + it 'copies AwardEmojis', :aggregate_failures do + expect { execute_service }.to change { AwardEmoji.count }.by(2) + expect(to_awardable.award_emoji.map(&:name)).to match_array(%w(thumbsup thumbsdown)) + end + + it 'returns success', :aggregate_failures do + expect(execute_service).to be_kind_of(ServiceResponse) + expect(execute_service).to be_success + end + end +end diff --git a/spec/services/boards/lists/create_service_spec.rb b/spec/services/boards/lists/create_service_spec.rb index f3d4e62eeca..88b6c3098d1 100644 --- a/spec/services/boards/lists/create_service_spec.rb +++ b/spec/services/boards/lists/create_service_spec.rb @@ -59,6 +59,21 @@ RSpec.describe Boards::Lists::CreateService do expect { service.execute(board) }.to raise_error(ActiveRecord::RecordNotFound) end end + + context 'when backlog param is sent' do + it 'creates one and only one backlog list' do + service = described_class.new(parent, user, 'backlog' => true) + list = service.execute(board) + + expect(list.list_type).to eq('backlog') + expect(list.position).to be_nil + expect(list).to be_valid + + another_backlog = service.execute(board) + + expect(another_backlog).to eq list + end + end end context 'when board parent is a project' do diff --git a/spec/services/boards/lists/list_service_spec.rb b/spec/services/boards/lists/list_service_spec.rb index 3d71c467e96..dfe65f3d241 100644 --- a/spec/services/boards/lists/list_service_spec.rb +++ b/spec/services/boards/lists/list_service_spec.rb @@ -6,12 +6,14 @@ RSpec.describe Boards::Lists::ListService do let(:user) { create(:user) } describe '#execute' do + let(:service) { described_class.new(parent, user) } + context 'when board parent is a project' do let(:project) { create(:project) } let(:board) { create(:board, project: project) } let(:label) { create(:label, project: project) } let!(:list) { create(:list, board: board, label: label) } - let(:service) { described_class.new(project, user) } + let(:parent) { project } it_behaves_like 'lists list service' end @@ -21,7 +23,7 @@ RSpec.describe Boards::Lists::ListService do let(:board) { create(:board, group: group) } let(:label) { create(:group_label, group: group) } let!(:list) { create(:list, board: board, label: label) } - let(:service) { described_class.new(group, user) } + let(:parent) { group } it_behaves_like 'lists list service' end diff --git a/spec/services/branches/create_service_spec.rb b/spec/services/branches/create_service_spec.rb index b682a3f26ec..5cf0d5af75f 100644 --- a/spec/services/branches/create_service_spec.rb +++ b/spec/services/branches/create_service_spec.rb @@ -44,5 +44,25 @@ RSpec.describe Branches::CreateService do expect(result[:message]).to eq('Invalid reference name: unknown') 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}" + pre_receive_error = Gitlab::Git::PreReceiveError.new(raw_message) + + allow(project.repository).to receive(:add_branch).and_raise(pre_receive_error) + + expect(Gitlab::ErrorTracking).to receive(:track_exception).with( + pre_receive_error, + pre_receive_message: raw_message, + branch_name: 'new-feature', + ref: 'unknown' + ) + + result = service.execute('new-feature', 'unknown') + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq(error_message) + end end end diff --git a/spec/services/ci/build_report_result_service_spec.rb b/spec/services/ci/build_report_result_service_spec.rb index 3c1ef5301fc..70bcf74ba43 100644 --- a/spec/services/ci/build_report_result_service_spec.rb +++ b/spec/services/ci/build_report_result_service_spec.rb @@ -19,16 +19,6 @@ RSpec.describe Ci::BuildReportResultService do expect(Ci::BuildReportResult.count).to eq(1) end - context 'when feature is disable' do - it 'does not persist the data' do - stub_feature_flags(build_report_summary: false) - - subject - - expect(Ci::BuildReportResult.count).to eq(0) - end - end - context 'when data has already been persisted' do it 'raises an error and do not persist the same data twice' do expect { 2.times { described_class.new.execute(build) } }.to raise_error(ActiveRecord::RecordNotUnique) diff --git a/spec/services/ci/change_variable_service_spec.rb b/spec/services/ci/change_variable_service_spec.rb new file mode 100644 index 00000000000..7acdd4e834f --- /dev/null +++ b/spec/services/ci/change_variable_service_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::ChangeVariableService do + let(:service) { described_class.new(container: group, current_user: user, params: params) } + + let_it_be(:user) { create(:user) } + let(:group) { create(:group) } + + describe '#execute' do + subject(:execute) { service.execute } + + context 'when creating a variable' do + let(:params) { { variable_params: { key: 'new_variable', value: 'variable_value' }, action: :create } } + + it 'persists a variable' do + expect { execute }.to change(Ci::GroupVariable, :count).from(0).to(1) + end + end + + context 'when updating a variable' do + let!(:variable) { create(:ci_group_variable, value: 'old_value') } + let(:params) { { variable_params: { key: variable.key, value: 'new_value' }, action: :update } } + + before do + group.variables << variable + end + + it 'updates a variable' do + expect { execute }.to change { variable.reload.value }.from('old_value').to('new_value') + end + + context 'when the variable does not exist' do + before do + variable.destroy! + end + + it 'raises a record not found error' do + expect { execute }.to raise_error(::ActiveRecord::RecordNotFound) + end + end + end + + context 'when destroying a variable' do + let!(:variable) { create(:ci_group_variable) } + let(:params) { { variable_params: { key: variable.key }, action: :destroy } } + + before do + group.variables << variable + end + + it 'destroys a variable' do + expect { execute }.to change { Ci::GroupVariable.exists?(variable.id) }.from(true).to(false) + end + + context 'when the variable does not exist' do + before do + variable.destroy! + end + + it 'raises a record not found error' do + expect { execute }.to raise_error(::ActiveRecord::RecordNotFound) + end + end + end + end +end diff --git a/spec/services/ci/change_variables_service_spec.rb b/spec/services/ci/change_variables_service_spec.rb new file mode 100644 index 00000000000..5f1207eaf58 --- /dev/null +++ b/spec/services/ci/change_variables_service_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::ChangeVariablesService do + let(:service) { described_class.new(container: group, current_user: user, params: params) } + + let_it_be(:user) { create(:user) } + let(:group) { spy(:group, variables: []) } + let(:params) { { variables_attributes: [{ key: 'new_variable', value: 'variable_value' }] } } + + describe '#execute' do + subject(:execute) { service.execute } + + it 'delegates to ActiveRecord update' do + execute + + expect(group).to have_received(:update).with(params) + end + end +end diff --git a/spec/services/ci/create_job_artifacts_service_spec.rb b/spec/services/ci/create_job_artifacts_service_spec.rb index 3f5cf079025..72b0d220b11 100644 --- a/spec/services/ci/create_job_artifacts_service_spec.rb +++ b/spec/services/ci/create_job_artifacts_service_spec.rb @@ -73,7 +73,7 @@ RSpec.describe Ci::CreateJobArtifactsService do expect(metadata_artifact.expire_at).to be_within(1.minute).of(expected_expire_at) end - context 'when expire_in params is set' do + context 'when expire_in params is set to a specific value' do before do params.merge!('expire_in' => '2 hours') end @@ -89,6 +89,23 @@ RSpec.describe Ci::CreateJobArtifactsService do expect(metadata_artifact.expire_at).to be_within(1.minute).of(expected_expire_at) end end + + context 'when expire_in params is set to `never`' do + before do + params.merge!('expire_in' => 'never') + end + + it 'sets expiration date according to the parameter' do + expected_expire_at = nil + + expect(subject).to be_truthy + archive_artifact, metadata_artifact = job.job_artifacts.last(2) + + expect(job.artifacts_expire_at).to eq(expected_expire_at) + expect(archive_artifact.expire_at).to eq(expected_expire_at) + expect(metadata_artifact.expire_at).to eq(expected_expire_at) + end + end end end diff --git a/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb b/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb index 16205529f1c..3be5ac1f739 100644 --- a/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb +++ b/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb @@ -24,7 +24,7 @@ RSpec.describe Ci::CreatePipelineService do test: script: rspec rules: - - if: '$CI_COMMIT_BRANCH' + - when: always YAML end @@ -32,7 +32,7 @@ RSpec.describe Ci::CreatePipelineService do expect(pipeline.error_messages.map(&:content)).to be_empty expect(pipeline.warning_messages.map(&:content)).to contain_exactly( - 'jobs:test uses `rules` without defining `workflow:rules`' + /jobs:test may allow multiple pipelines to run/ ) end @@ -77,13 +77,13 @@ RSpec.describe Ci::CreatePipelineService do stage: test script: echo rules: - - if: '$CI_COMMIT_BRANCH' + - when: on_success YAML end it 'contains both errors and warnings' do error_message = 'build job: need test is not defined in prior stages' - warning_message = 'jobs:test uses `rules` without defining `workflow:rules`' + warning_message = /jobs:test may allow multiple pipelines to run/ expect(pipeline.yaml_errors).to eq(error_message) expect(pipeline.error_messages.map(&:content)).to contain_exactly(error_message) diff --git a/spec/services/ci/create_pipeline_service/dry_run_spec.rb b/spec/services/ci/create_pipeline_service/dry_run_spec.rb new file mode 100644 index 00000000000..93378df80f0 --- /dev/null +++ b/spec/services/ci/create_pipeline_service/dry_run_spec.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::CreatePipelineService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:admin) } + let(:ref) { 'refs/heads/master' } + let(:service) { described_class.new(project, user, { ref: ref }) } + + subject { service.execute(:push, dry_run: true) } + + before do + stub_ci_pipeline_yaml_file(config) + end + + describe 'dry run' do + shared_examples 'returns a non persisted pipeline' do + it 'does not persist the pipeline' do + expect(subject).not_to be_persisted + expect(subject.id).to be_nil + end + + it 'does not process the pipeline' do + expect(Ci::ProcessPipelineService).not_to receive(:new) + + subject + end + + it 'does not schedule merge request head pipeline update' do + expect(service).not_to receive(:schedule_head_pipeline_update) + + subject + end + end + + context 'when pipeline is valid' do + let(:config) { gitlab_ci_yaml } + + it_behaves_like 'returns a non persisted pipeline' + + it 'returns a valid pipeline' do + expect(subject.error_messages).to be_empty + expect(subject.yaml_errors).to be_nil + expect(subject.errors).to be_empty + end + end + + context 'when pipeline is not valid' do + context 'when there are syntax errors' do + let(:config) do + <<~YAML + rspec: + script: echo + something: wrong + YAML + end + + it_behaves_like 'returns a non persisted pipeline' + + it 'returns a pipeline with errors', :aggregate_failures do + error_message = 'jobs:rspec config contains unknown keys: something' + + expect(subject.error_messages.map(&:content)).to eq([error_message]) + expect(subject.errors).not_to be_empty + expect(subject.yaml_errors).to eq(error_message) + end + end + + context 'when there are logical errors' do + let(:config) do + <<~YAML + build: + script: echo + stage: build + needs: [test] + test: + script: echo + stage: test + YAML + end + + it_behaves_like 'returns a non persisted pipeline' + + it 'returns a pipeline with errors', :aggregate_failures do + error_message = 'build job: need test is not defined in prior stages' + + expect(subject.error_messages.map(&:content)).to eq([error_message]) + expect(subject.errors).not_to be_empty + end + end + + context 'when there are errors at the seeding stage' do + let(:config) do + <<~YAML + build: + stage: build + script: echo + rules: + - if: '$CI_MERGE_REQUEST_ID' + test: + stage: test + script: echo + needs: ['build'] + YAML + end + + it_behaves_like 'returns a non persisted pipeline' + + it 'returns a pipeline with errors', :aggregate_failures do + error_message = "test: needs 'build'" + + expect(subject.error_messages.map(&:content)).to eq([error_message]) + expect(subject.errors).not_to be_empty + end + end + end + end +end diff --git a/spec/services/ci/create_pipeline_service/parameter_content_spec.rb b/spec/services/ci/create_pipeline_service/parameter_content_spec.rb index 5157574ea04..f656ad52ac8 100644 --- a/spec/services/ci/create_pipeline_service/parameter_content_spec.rb +++ b/spec/services/ci/create_pipeline_service/parameter_content_spec.rb @@ -49,14 +49,5 @@ RSpec.describe Ci::CreatePipelineService do end end end - - context 'when source is not a dangling build' do - subject { service.execute(:web, content: content) } - - it 'raises an exception' do - klass = Gitlab::Ci::Pipeline::Chain::Config::Content::Parameter::UnsupportedSourceError - expect { subject }.to raise_error(klass) - end - end end end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 9dc518be996..db4c2f5a047 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -93,6 +93,7 @@ RSpec.describe Ci::CreatePipelineService do let(:merge_request_1) do create(:merge_request, source_branch: 'feature', target_branch: "master", source_project: project) end + let(:merge_request_2) do create(:merge_request, source_branch: 'feature', target_branch: "v1.1.0", source_project: project) end @@ -512,7 +513,7 @@ RSpec.describe Ci::CreatePipelineService do it 'pull it from Auto-DevOps' do pipeline = execute_service expect(pipeline).to be_auto_devops_source - expect(pipeline.builds.map(&:name)).to match_array(%w[build code_quality eslint-sast test]) + expect(pipeline.builds.map(&:name)).to match_array(%w[build code_quality eslint-sast secret_detection_default_branch secrets-sast test]) end end @@ -905,6 +906,7 @@ RSpec.describe Ci::CreatePipelineService do stub_ci_pipeline_yaml_file(YAML.dump({ rspec: { script: 'rspec', retry: retry_value } })) + rspec_job.update!(options: { retry: retry_value }) end context 'as an integer' do @@ -912,8 +914,6 @@ RSpec.describe Ci::CreatePipelineService do it 'correctly creates builds with auto-retry value configured' do expect(pipeline).to be_persisted - expect(rspec_job.options_retry_max).to eq 2 - expect(rspec_job.options_retry_when).to eq ['always'] end end @@ -922,8 +922,6 @@ RSpec.describe Ci::CreatePipelineService do it 'correctly creates builds with auto-retry value configured' do expect(pipeline).to be_persisted - expect(rspec_job.options_retry_max).to eq 2 - expect(rspec_job.options_retry_when).to eq ['runner_system_failure'] end end end @@ -985,7 +983,6 @@ RSpec.describe Ci::CreatePipelineService do context 'with release' do shared_examples_for 'a successful release pipeline' do before do - stub_feature_flags(ci_release_generation: true) stub_ci_pipeline_yaml_file(YAML.dump(config)) end @@ -1695,16 +1692,23 @@ RSpec.describe Ci::CreatePipelineService do context 'when pipeline on feature is created' do let(:ref_name) { 'refs/heads/feature' } + shared_examples 'has errors' do + it 'contains the expected errors' do + expect(pipeline.builds).to be_empty + expect(pipeline.yaml_errors).to eq("test_a: needs 'build_a'") + expect(pipeline.error_messages.map(&:content)).to contain_exactly("test_a: needs 'build_a'") + expect(pipeline.errors[:base]).to contain_exactly("test_a: needs 'build_a'") + end + end + context 'when save_on_errors is enabled' do let(:pipeline) { execute_service(save_on_errors: true) } it 'does create a pipeline as test_a depends on build_a' do expect(pipeline).to be_persisted - expect(pipeline.builds).to be_empty - expect(pipeline.yaml_errors).to eq("test_a: needs 'build_a'") - expect(pipeline.messages.pluck(:content)).to contain_exactly("test_a: needs 'build_a'") - expect(pipeline.errors[:base]).to contain_exactly("test_a: needs 'build_a'") end + + it_behaves_like 'has errors' end context 'when save_on_errors is disabled' do @@ -1712,11 +1716,9 @@ RSpec.describe Ci::CreatePipelineService do it 'does not create a pipeline as test_a depends on build_a' do expect(pipeline).not_to be_persisted - expect(pipeline.builds).to be_empty - expect(pipeline.yaml_errors).to be_nil - expect(pipeline.messages).not_to be_empty - expect(pipeline.errors[:base]).to contain_exactly("test_a: needs 'build_a'") end + + it_behaves_like 'has errors' end end diff --git a/spec/services/ci/daily_build_group_report_result_service_spec.rb b/spec/services/ci/daily_build_group_report_result_service_spec.rb index 7d181a5c2ba..f196afb05e8 100644 --- a/spec/services/ci/daily_build_group_report_result_service_spec.rb +++ b/spec/services/ci/daily_build_group_report_result_service_spec.rb @@ -65,6 +65,7 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do created_at: '2020-02-06 00:02:20' ) end + let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: '4/4 rspec', coverage: 84) } let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: '3/3 karma', coverage: 92) } @@ -104,6 +105,7 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do created_at: '2020-02-06 00:02:20' ) end + let!(:new_rspec_job) { create(:ci_build, pipeline: new_pipeline, name: '4/4 rspec', coverage: 84) } let!(:new_karma_job) { create(:ci_build, pipeline: new_pipeline, name: '3/3 karma', coverage: 92) } @@ -149,6 +151,7 @@ RSpec.describe Ci::DailyBuildGroupReportResultService, '#execute' do created_at: '2020-02-06 00:02:20' ) end + let!(:some_job) { create(:ci_build, pipeline: new_pipeline, name: 'foo') } it 'does nothing' do 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 a10a333b462..bc8b6b2d113 100644 --- a/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb +++ b/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb @@ -5,20 +5,12 @@ require_relative 'shared_processing_service.rb' require_relative 'shared_processing_service_tests_with_yaml.rb' RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do - before do - stub_feature_flags(ci_atomic_processing: true) - - # This feature flag is implicit - # Atomic Processing does not process statuses differently - stub_feature_flags(ci_composite_status: true) - end - it_behaves_like 'Pipeline Processing Service' it_behaves_like 'Pipeline Processing Service Tests With Yaml' private - def process_pipeline(initial_process: false) + def process_pipeline described_class.new(pipeline).execute end end diff --git a/spec/services/ci/pipeline_processing/legacy_processing_service_spec.rb b/spec/services/ci/pipeline_processing/legacy_processing_service_spec.rb deleted file mode 100644 index 569a6d62dc1..00000000000 --- a/spec/services/ci/pipeline_processing/legacy_processing_service_spec.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' -require_relative 'shared_processing_service.rb' -require_relative 'shared_processing_service_tests_with_yaml.rb' - -RSpec.describe Ci::PipelineProcessing::LegacyProcessingService do - before do - stub_feature_flags(ci_atomic_processing: false) - end - - context 'when ci_composite_status is enabled' do - before do - stub_feature_flags(ci_composite_status: true) - end - - it_behaves_like 'Pipeline Processing Service' - it_behaves_like 'Pipeline Processing Service Tests With Yaml' - end - - context 'when ci_composite_status is disabled' do - before do - stub_feature_flags(ci_composite_status: false) - end - - it_behaves_like 'Pipeline Processing Service' - it_behaves_like 'Pipeline Processing Service Tests With Yaml' - end - - private - - def process_pipeline(initial_process: false) - described_class.new(pipeline).execute(initial_process: initial_process) - end -end diff --git a/spec/services/ci/pipeline_processing/shared_processing_service.rb b/spec/services/ci/pipeline_processing/shared_processing_service.rb index 224066885b6..7de22b6a4cc 100644 --- a/spec/services/ci/pipeline_processing/shared_processing_service.rb +++ b/spec/services/ci/pipeline_processing/shared_processing_service.rb @@ -788,8 +788,7 @@ RSpec.shared_examples 'Pipeline Processing Service' do let!(:deploy_pages) { create_build('deploy_pages', stage: 'deploy', stage_idx: 2, scheduling_type: :dag) } it 'runs deploy_pages without waiting prior stages' do - # Ci::PipelineProcessing::LegacyProcessingService requires :initial_process parameter - expect(process_pipeline(initial_process: true)).to be_truthy + expect(process_pipeline).to be_truthy expect(stages).to eq(%w(pending created pending)) expect(builds.pending).to contain_exactly(linux_build, mac_build, deploy_pages) diff --git a/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb b/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb index 17d254ba48e..77645298bc7 100644 --- a/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb +++ b/spec/services/ci/pipeline_processing/shared_processing_service_tests_with_yaml.rb @@ -42,7 +42,7 @@ RSpec.shared_context 'Pipeline Processing Service Tests With Yaml' do { pipeline: pipeline.status, - stages: pipeline.ordered_stages.pluck(:name, :status).to_h, + stages: pipeline.stages.pluck(:name, :status).to_h, jobs: pipeline.statuses.latest.pluck(:name, :status).to_h } end diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds.yml b/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds_deploy_needs_one_build_and_test.yml index a133023b12d..a133023b12d 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds_deploy_needs_one_build_and_test.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds_deploy_always.yml b/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds_deploy_needs_one_build_and_test_when_always.yml index 4c676761e5c..4c676761e5c 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds_deploy_always.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_other_build_succeeds_deploy_needs_one_build_and_test_when_always.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_test_allow_failure.yml b/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_test_fails_with_allow_failure.yml index ea7046262c3..ea7046262c3 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_test_allow_failure.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_test_fails_with_allow_failure.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_on_failure_with_failure.yml b/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_test_on_failure_deploy_needs_test.yml index 5ace621e89c..5ace621e89c 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_test_on_failure_with_failure.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_test_on_failure_deploy_needs_test.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_build_allow_failure_test_on_failure.yml b/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_with_allow_failure_test_on_failure.yml index cfc456387ff..cfc456387ff 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_build_allow_failure_test_on_failure.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_build_fails_with_allow_failure_test_on_failure.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_build_succeds_test_manual_allow_failure_true_deploy_needs_both.yml b/spec/services/ci/pipeline_processing/test_cases/dag_build_succeds_test_manual_allow_failure_true_deploy_needs_both.yml new file mode 100644 index 00000000000..60f803bc3d0 --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/dag_build_succeds_test_manual_allow_failure_true_deploy_needs_both.yml @@ -0,0 +1,41 @@ +config: + build: + stage: build + script: exit 0 + + test: + stage: test + when: manual + allow_failure: true + script: exit 1 + + deploy: + stage: deploy + script: exit 0 + needs: [build, test] + +init: + expect: + pipeline: pending + stages: + build: pending + test: created + deploy: created + jobs: + build: pending + test: created + deploy: created + +transitions: + - event: success + jobs: [build] + expect: + pipeline: running + stages: + build: success + test: skipped + deploy: created + jobs: + build: success + test: manual + deploy: created diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_build_succeds_test_manual_allow_failure_true_deploy_needs_test.yml b/spec/services/ci/pipeline_processing/test_cases/dag_build_succeds_test_manual_allow_failure_true_deploy_needs_test.yml new file mode 100644 index 00000000000..4e4b2f22224 --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/dag_build_succeds_test_manual_allow_failure_true_deploy_needs_test.yml @@ -0,0 +1,41 @@ +config: + build: + stage: build + script: exit 0 + + test: + stage: test + when: manual + allow_failure: true + script: exit 0 + + deploy: + stage: deploy + script: exit 0 + needs: [test] + +init: + expect: + pipeline: pending + stages: + build: pending + test: created + deploy: created + jobs: + build: pending + test: created + deploy: created + +transitions: + - event: success + jobs: [build] + expect: + pipeline: running + stages: + build: success + test: skipped + deploy: created + jobs: + build: success + test: manual + deploy: created diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_on_failure_with_success.yml b/spec/services/ci/pipeline_processing/test_cases/dag_build_succeeds_test_on_failure_deploy_needs_test.yml index 19524cfd3e4..19524cfd3e4 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_test_on_failure_with_success.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_build_succeeds_test_on_failure_deploy_needs_test.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_build_test_manual_review_deploy.yml b/spec/services/ci/pipeline_processing/test_cases/dag_build_test_manual_review_deploy.yml new file mode 100644 index 00000000000..fef28dcfbbe --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/dag_build_test_manual_review_deploy.yml @@ -0,0 +1,82 @@ +config: + stages: [build, test, review, deploy] + + build: + stage: build + script: exit 0 + + test: + stage: test + script: exit 0 + + release_test: + stage: test + when: manual + allow_failure: true + script: exit 0 + + review: + stage: review + script: exit 0 + needs: [test, release_test] + + staging: + stage: deploy + script: exit 0 + needs: [test, release_test] + + production: + stage: deploy + script: exit 0 + needs: [review] + +init: + expect: + pipeline: pending + stages: + build: pending + test: created + review: created + deploy: created + jobs: + build: pending + test: created + release_test: created + review: created + staging: created + production: created + +transitions: + - event: success + jobs: [build] + expect: + pipeline: running + stages: + build: success + test: pending + review: created + deploy: created + jobs: + build: success + test: pending + release_test: manual + review: created + staging: created + production: created + + - event: success + jobs: [test] + expect: + pipeline: running + stages: + build: success + test: success + review: created + deploy: created + jobs: + build: success + test: success + release_test: manual + review: created + staging: created + production: created diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure.yml b/spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure_deploy_needs_one_build_and_test.yml index f324525bd56..f324525bd56 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure_deploy_needs_one_build_and_test.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure_deploy_always.yml b/spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure_deploy_needs_one_build_and_test_when_always.yml index 9986dbaa215..9986dbaa215 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure_deploy_always.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_builds_succeed_test_on_failure_deploy_needs_one_build_and_test_when_always.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_deploy_needs_empty.yml b/spec/services/ci/pipeline_processing/test_cases/dag_deploy_needs_empty.yml new file mode 100644 index 00000000000..1783c0acb11 --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/dag_deploy_needs_empty.yml @@ -0,0 +1,27 @@ +config: + build: + stage: build + script: exit 0 + + test: + stage: test + script: exit 0 + + deploy: + stage: deploy + script: exit 0 + needs: [] + +init: + expect: + pipeline: pending + stages: + build: pending + test: created + deploy: pending + jobs: + build: pending + test: created + deploy: pending + +transitions: [] diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_allow_failure_true.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_fails_with_allow_failure.yml index 8d4d9d403f1..8d4d9d403f1 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_test_allow_failure_true.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_fails_with_allow_failure.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_always.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_always.yml new file mode 100644 index 00000000000..bb8723aa303 --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_always.yml @@ -0,0 +1,45 @@ +config: + test: + stage: test + when: manual + allow_failure: false + script: exit 1 + + deploy: + stage: deploy + when: always + script: exit 0 + needs: [test] + +init: + expect: + pipeline: manual + stages: + test: manual + deploy: created + jobs: + test: manual + deploy: created + +transitions: + - event: enqueue + jobs: [test] + expect: + pipeline: pending + stages: + test: pending + deploy: created + jobs: + test: pending + deploy: created + + - event: drop + jobs: [test] + expect: + pipeline: running + stages: + test: failed + deploy: pending + jobs: + test: failed + deploy: pending diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_on_failure.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_on_failure.yml new file mode 100644 index 00000000000..3099a94befb --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_deploy_on_failure.yml @@ -0,0 +1,45 @@ +config: + test: + stage: test + when: manual + allow_failure: false + script: exit 1 + + deploy: + stage: deploy + when: on_failure + script: exit 0 + needs: [test] + +init: + expect: + pipeline: manual + stages: + test: manual + deploy: created + jobs: + test: manual + deploy: created + +transitions: + - event: enqueue + jobs: [test] + expect: + pipeline: pending + stages: + test: pending + deploy: created + jobs: + test: pending + deploy: created + + - event: drop + jobs: [test] + expect: + pipeline: running + stages: + test: failed + deploy: pending + jobs: + test: failed + deploy: pending diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_other_test_succeeds_deploy_needs_both.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_other_test_succeeds_deploy_needs_both.yml new file mode 100644 index 00000000000..7330a73b5a3 --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_false_other_test_succeeds_deploy_needs_both.yml @@ -0,0 +1,40 @@ +config: + test1: + stage: test + script: exit 0 + + test2: + stage: test + when: manual + allow_failure: false + script: exit 1 + + deploy: + stage: deploy + script: exit 0 + needs: [test1, test2] + +init: + expect: + pipeline: running + stages: + test: running + deploy: created + jobs: + test1: pending + test2: manual + deploy: created + +transitions: + - event: success + jobs: [test1] + expect: + pipeline: manual + stages: + test: manual + deploy: created + jobs: + test1: success + test2: manual + deploy: created + diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_other_test_succeeds.yml b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_other_test_succeeds_deploy_needs_both.yml index 34073b92ccc..34073b92ccc 100644 --- a/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_other_test_succeeds.yml +++ b/spec/services/ci/pipeline_processing/test_cases/dag_test_manual_allow_failure_true_other_test_succeeds_deploy_needs_both.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/stage_test_on_failure_with_failure.yml b/spec/services/ci/pipeline_processing/test_cases/stage_build_fails_test_on_failure.yml index 1751cbb2023..1751cbb2023 100644 --- a/spec/services/ci/pipeline_processing/test_cases/stage_test_on_failure_with_failure.yml +++ b/spec/services/ci/pipeline_processing/test_cases/stage_build_fails_test_on_failure.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/stage_build_allow_failure_test_on_failure.yml b/spec/services/ci/pipeline_processing/test_cases/stage_build_fails_with_allow_failure_test_on_failure.yml index 3e081d4411b..3e081d4411b 100644 --- a/spec/services/ci/pipeline_processing/test_cases/stage_build_allow_failure_test_on_failure.yml +++ b/spec/services/ci/pipeline_processing/test_cases/stage_build_fails_with_allow_failure_test_on_failure.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/stage_build_fails_test_allow_failure.yml b/spec/services/ci/pipeline_processing/test_cases/stage_build_succeeds_test_manual_allow_failure_true.yml index 362ac6e4239..2fd85b74d4d 100644 --- a/spec/services/ci/pipeline_processing/test_cases/stage_build_fails_test_allow_failure.yml +++ b/spec/services/ci/pipeline_processing/test_cases/stage_build_succeeds_test_manual_allow_failure_true.yml @@ -1,10 +1,11 @@ config: build: stage: build - script: exit 1 + script: exit 0 test: stage: test + when: manual allow_failure: true script: exit 1 @@ -25,15 +26,15 @@ init: deploy: created transitions: - - event: drop + - event: success jobs: [build] expect: - pipeline: failed + pipeline: running stages: - build: failed + build: success test: skipped - deploy: skipped + deploy: pending jobs: - build: failed - test: skipped - deploy: skipped + build: success + test: manual + deploy: pending diff --git a/spec/services/ci/pipeline_processing/test_cases/stage_test_on_failure_with_success.yml b/spec/services/ci/pipeline_processing/test_cases/stage_build_succeeds_test_on_failure.yml index 15afe1ce8e1..15afe1ce8e1 100644 --- a/spec/services/ci/pipeline_processing/test_cases/stage_test_on_failure_with_success.yml +++ b/spec/services/ci/pipeline_processing/test_cases/stage_build_succeeds_test_on_failure.yml diff --git a/spec/services/ci/pipeline_processing/test_cases/stage_build_test_manual_review_deploy.yml b/spec/services/ci/pipeline_processing/test_cases/stage_build_test_manual_review_deploy.yml new file mode 100644 index 00000000000..2829765fd95 --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/stage_build_test_manual_review_deploy.yml @@ -0,0 +1,79 @@ +config: + stages: [build, test, review, deploy] + + build: + stage: build + script: exit 0 + + test: + stage: test + script: exit 0 + + release_test: + stage: test + when: manual + allow_failure: true + script: exit 0 + + review: + stage: review + script: exit 0 + + staging: + stage: deploy + script: exit 0 + + production: + stage: deploy + script: exit 0 + +init: + expect: + pipeline: pending + stages: + build: pending + test: created + review: created + deploy: created + jobs: + build: pending + test: created + release_test: created + review: created + staging: created + production: created + +transitions: + - event: success + jobs: [build] + expect: + pipeline: running + stages: + build: success + test: pending + review: created + deploy: created + jobs: + build: success + test: pending + release_test: manual + review: created + staging: created + production: created + + - event: success + jobs: [test] + expect: + pipeline: running + stages: + build: success + test: success + review: pending + deploy: created + jobs: + build: success + test: success + release_test: manual + review: pending + staging: created + production: created diff --git a/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_true_deploy_always.yml b/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_true_deploy_always.yml new file mode 100644 index 00000000000..9181c8adf50 --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/stage_test_manual_allow_failure_true_deploy_always.yml @@ -0,0 +1,23 @@ +config: + test: + stage: test + when: manual + allow_failure: true + script: exit 1 + + deploy: + stage: deploy + when: always + script: exit 0 + +init: + expect: + pipeline: pending + stages: + test: skipped + deploy: pending + jobs: + test: manual + deploy: pending + +transitions: [] diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index fa46d6c4d1d..212c8f99865 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -364,7 +364,7 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do stage: "stage_#{stage_num}", stage_idx: stage_num, pipeline: pipeline, **opts) do |build| - pipeline.update_legacy_status + ::Ci::ProcessPipelineService.new(pipeline).execute end end end diff --git a/spec/services/clusters/aws/authorize_role_service_spec.rb b/spec/services/clusters/aws/authorize_role_service_spec.rb index 530268340b7..3d12400a47b 100644 --- a/spec/services/clusters/aws/authorize_role_service_spec.rb +++ b/spec/services/clusters/aws/authorize_role_service_spec.rb @@ -53,6 +53,12 @@ RSpec.describe Clusters::Aws::AuthorizeRoleService do expect(subject.status).to eq(:unprocessable_entity) expect(subject.body).to eq({}) end + + it 'logs the error' do + expect(::Gitlab::ErrorTracking).to receive(:track_exception) + + subject + end end context 'cannot create role' do diff --git a/spec/services/clusters/cleanup/project_namespace_service_spec.rb b/spec/services/clusters/cleanup/project_namespace_service_spec.rb index 761ad8dd8c8..605aaea17e4 100644 --- a/spec/services/clusters/cleanup/project_namespace_service_spec.rb +++ b/spec/services/clusters/cleanup/project_namespace_service_spec.rb @@ -16,6 +16,7 @@ RSpec.describe Clusters::Cleanup::ProjectNamespaceService do execution_count: 0 } end + let(:kubeclient_instance_double) do instance_double(Gitlab::Kubernetes::KubeClient, delete_namespace: nil, delete_service_account: nil) end diff --git a/spec/services/clusters/cleanup/service_account_service_spec.rb b/spec/services/clusters/cleanup/service_account_service_spec.rb index 6fe3d0c286e..f256df1b2fc 100644 --- a/spec/services/clusters/cleanup/service_account_service_spec.rb +++ b/spec/services/clusters/cleanup/service_account_service_spec.rb @@ -16,6 +16,7 @@ RSpec.describe Clusters::Cleanup::ServiceAccountService do execution_count: 0 } end + let(:kubeclient_instance_double) do instance_double(Gitlab::Kubernetes::KubeClient, delete_namespace: nil, delete_service_account: nil) end diff --git a/spec/services/clusters/parse_cluster_applications_artifact_service_spec.rb b/spec/services/clusters/parse_cluster_applications_artifact_service_spec.rb index 3b155d95345..1f6ad218927 100644 --- a/spec/services/clusters/parse_cluster_applications_artifact_service_spec.rb +++ b/spec/services/clusters/parse_cluster_applications_artifact_service_spec.rb @@ -36,94 +36,89 @@ RSpec.describe Clusters::ParseClusterApplicationsArtifactService do let(:job) { deployment.deployable } let(:artifact) { create(:ci_job_artifact, :cluster_applications, job: job) } - context 'when cluster_applications_artifact feature flag is disabled' do - before do - stub_feature_flags(cluster_applications_artifact: false) - end - - it 'does not call Gitlab::Kubernetes::Helm::Parsers::ListV2 and returns success immediately' do - expect(Gitlab::Kubernetes::Helm::Parsers::ListV2).not_to receive(:new) + it 'calls Gitlab::Kubernetes::Helm::Parsers::ListV2' do + expect(Gitlab::Kubernetes::Helm::Parsers::ListV2).to receive(:new).and_call_original - result = described_class.new(job, user).execute(artifact) + result = described_class.new(job, user).execute(artifact) - expect(result[:status]).to eq(:success) - end + expect(result[:status]).to eq(:success) end - context 'when cluster_applications_artifact feature flag is enabled for project' do - before do - stub_feature_flags(cluster_applications_artifact: job.project) + context 'artifact is not of cluster_applications type' do + let(:artifact) { create(:ci_job_artifact, :archive) } + let(:job) { artifact.job } + + it 'raise ArgumentError' do + expect do + described_class.new(job, user).execute(artifact) + end.to raise_error(ArgumentError, 'Artifact is not cluster_applications file type') end + end - it 'calls Gitlab::Kubernetes::Helm::Parsers::ListV2' do - expect(Gitlab::Kubernetes::Helm::Parsers::ListV2).to receive(:new).and_call_original + context 'artifact exceeds acceptable size' do + it 'returns an error' do + stub_const("#{described_class}::MAX_ACCEPTABLE_ARTIFACT_SIZE", 1.byte) result = described_class.new(job, user).execute(artifact) - expect(result[:status]).to eq(:success) + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('Cluster_applications artifact too big. Maximum allowable size: 1 Byte') end + end - context 'artifact is not of cluster_applications type' do - let(:artifact) { create(:ci_job_artifact, :archive) } - let(:job) { artifact.job } + context 'job has no deployment' do + let(:job) { build(:ci_build) } - it 'raise ArgumentError' do - expect do - described_class.new(job, user).execute(artifact) - end.to raise_error(ArgumentError, 'Artifact is not cluster_applications file type') - end + it 'returns an error' do + result = described_class.new(job, user).execute(artifact) + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('No deployment found for this job') end + end - context 'artifact exceeds acceptable size' do - it 'returns an error' do - stub_const("#{described_class}::MAX_ACCEPTABLE_ARTIFACT_SIZE", 1.byte) + context 'job has no deployment cluster' do + let(:deployment) { create(:deployment) } + let(:job) { deployment.deployable } - result = described_class.new(job, user).execute(artifact) + it 'returns an error' do + result = described_class.new(job, user).execute(artifact) - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Cluster_applications artifact too big. Maximum allowable size: 1 Byte') - end + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('No deployment cluster found for this job') end + end - context 'job has no deployment' do - let(:job) { build(:ci_build) } + context 'blob is empty' do + let(:file) { fixture_file_upload(Rails.root.join("spec/fixtures/helm/helm_list_v2_empty_blob.json.gz")) } + let(:artifact) { create(:ci_job_artifact, :cluster_applications, job: job, file: file) } - it 'returns an error' do - result = described_class.new(job, user).execute(artifact) + it 'returns success' do + result = described_class.new(job, user).execute(artifact) - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('No deployment found for this job') - end + expect(result[:status]).to eq(:success) end + end - context 'job has no deployment cluster' do - let(:deployment) { create(:deployment) } - let(:job) { deployment.deployable } + context 'job has deployment cluster' do + context 'current user does not have access to deployment cluster' do + let(:other_user) { create(:user) } it 'returns an error' do - result = described_class.new(job, user).execute(artifact) + result = described_class.new(job, other_user).execute(artifact) expect(result[:status]).to eq(:error) expect(result[:message]).to eq('No deployment cluster found for this job') end end - context 'job has deployment cluster' do - context 'current user does not have access to deployment cluster' do - let(:other_user) { create(:user) } - - it 'returns an error' do - result = described_class.new(job, other_user).execute(artifact) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('No deployment cluster found for this job') - end - end + it 'does not affect unpermitted cluster applications' do + expect(Clusters::ParseClusterApplicationsArtifactService::RELEASE_NAMES).to contain_exactly('cilium') + end - Clusters::ParseClusterApplicationsArtifactService::RELEASE_NAMES.each do |release_name| - context release_name do - include_examples 'parse cluster applications artifact', release_name - end + Clusters::ParseClusterApplicationsArtifactService::RELEASE_NAMES.each do |release_name| + context release_name do + include_examples 'parse cluster applications artifact', release_name end end end diff --git a/spec/services/commits/commit_patch_service_spec.rb b/spec/services/commits/commit_patch_service_spec.rb index c8c0cbe23b2..55cbd0e5d66 100644 --- a/spec/services/commits/commit_patch_service_spec.rb +++ b/spec/services/commits/commit_patch_service_spec.rb @@ -10,6 +10,7 @@ RSpec.describe Commits::CommitPatchService do [content_1, content_2] end + let(:user) { project.creator } let(:branch_name) { 'branch-with-patches' } let(:project) { create(:project, :repository) } diff --git a/spec/services/design_management/move_designs_service_spec.rb b/spec/services/design_management/move_designs_service_spec.rb new file mode 100644 index 00000000000..a05518dc28d --- /dev/null +++ b/spec/services/design_management/move_designs_service_spec.rb @@ -0,0 +1,147 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe DesignManagement::MoveDesignsService do + include DesignManagementTestHelpers + + let_it_be(:issue) { create(:issue) } + let_it_be(:developer) { create(:user, developer_projects: [issue.project]) } + let_it_be(:designs) { create_list(:design, 3, :with_relative_position, issue: issue) } + + let(:project) { issue.project } + + let(:service) { described_class.new(current_user, params) } + + let(:params) do + { + current_design: current_design, + previous_design: previous_design, + next_design: next_design + } + end + + let(:current_user) { developer } + let(:current_design) { nil } + let(:previous_design) { nil } + let(:next_design) { nil } + + before do + enable_design_management + end + + describe '#execute' do + subject { service.execute } + + context 'the feature is unavailable' do + let(:current_design) { designs.first } + let(:previous_design) { designs.second } + let(:next_design) { designs.third } + + before do + stub_feature_flags(reorder_designs: false) + end + + it 'raises cannot_move' do + expect(subject).to be_error.and(have_attributes(message: :cannot_move)) + end + + context 'but it is available on the current project' do + before do + stub_feature_flags(reorder_designs: issue.project) + end + + it 'is successful' do + expect(subject).to be_success + end + end + end + + context 'the user cannot move designs' do + let(:current_design) { designs.first } + let(:current_user) { build_stubbed(:user) } + + it 'raises cannot_move' do + expect(subject).to be_error.and(have_attributes(message: :cannot_move)) + end + end + + context 'the designs are not distinct' do + let(:current_design) { designs.first } + let(:previous_design) { designs.first } + + it 'raises not_distinct' do + expect(subject).to be_error.and(have_attributes(message: :not_distinct)) + end + end + + context 'the designs are not on the same issue' do + let(:current_design) { designs.first } + let(:previous_design) { create(:design) } + + it 'raises not_same_issue' do + expect(subject).to be_error.and(have_attributes(message: :not_same_issue)) + end + end + + context 'no focus is passed' do + let(:previous_design) { designs.second } + let(:next_design) { designs.third } + + it 'raises no_focus' do + expect(subject).to be_error.and(have_attributes(message: :no_focus)) + end + end + + context 'no neighbours are passed' do + let(:current_design) { designs.first } + + it 'raises no_neighbors' do + expect(subject).to be_error.and(have_attributes(message: :no_neighbors)) + end + end + + context 'the designs are not adjacent' do + let(:current_design) { designs.first } + let(:previous_design) { designs.second } + let(:next_design) { designs.third } + + it 'raises not_adjacent' do + create(:design, issue: issue, relative_position: next_design.relative_position - 1) + + expect(subject).to be_error.and(have_attributes(message: :not_adjacent)) + end + end + + context 'moving a design with neighbours' do + let(:current_design) { designs.first } + let(:previous_design) { designs.second } + let(:next_design) { designs.third } + + it 'repositions existing designs and correctly places the given design' do + other_design1 = create(:design, issue: issue, relative_position: 10) + other_design2 = create(:design, issue: issue, relative_position: 20) + other_design3, other_design4 = create_list(:design, 2, issue: issue) + + expect(subject).to be_success + + expect(issue.designs.ordered(issue.project)).to eq([ + # Existing designs which already had a relative_position set. + # These should stay at the beginning, in the same order. + other_design1, + other_design2, + + # The designs we're passing into the service. + # These should be placed between the existing designs, in the correct order. + previous_design, + current_design, + next_design, + + # Existing designs which didn't have a relative_position set. + # These should be placed at the end, in the order of their IDs. + other_design3, + other_design4 + ]) + end + end + end +end diff --git a/spec/services/design_management/save_designs_service_spec.rb b/spec/services/design_management/save_designs_service_spec.rb index 24639632566..abba5de2c27 100644 --- a/spec/services/design_management/save_designs_service_spec.rb +++ b/spec/services/design_management/save_designs_service_spec.rb @@ -5,9 +5,9 @@ RSpec.describe DesignManagement::SaveDesignsService do include DesignManagementTestHelpers include ConcurrentHelpers - let_it_be(:developer) { create(:user) } + let_it_be_with_reload(:issue) { create(:issue) } + let_it_be(:developer) { create(:user, developer_projects: [issue.project]) } let(:project) { issue.project } - let(:issue) { create(:issue) } let(:user) { developer } let(:files) { [rails_sample] } let(:design_repository) { ::Gitlab::GlRepository::DESIGN.repository_resolver.call(project) } @@ -19,8 +19,20 @@ RSpec.describe DesignManagement::SaveDesignsService do fixture_file_upload("spec/fixtures/#{filename}") end + def commit_count + design_repository.expire_statistics_caches + design_repository.expire_root_ref_cache + design_repository.commit_count + end + before do - project.add_developer(developer) + if issue.design_collection.repository.exists? + issue.design_collection.repository.expire_all_method_caches + issue.design_collection.repository.raw.delete_all_refs_except([Gitlab::Git::BLANK_SHA]) + end + + allow(::DesignManagement::NewVersionWorker) + .to receive(:perform_async).with(Integer).and_return(nil) end def run_service(files_to_upload = nil) @@ -83,24 +95,20 @@ RSpec.describe DesignManagement::SaveDesignsService do design_repository.exists? end - it 'creates a design repository when it did not exist' do - expect { run_service }.to change { repository_exists }.from(false).to(true) + it 'is ensured when the service runs' do + run_service + + expect(repository_exists).to be true end end - it 'updates the creation count' do + it 'creates a commit, an event in the activity stream and updates the creation count' do counter = Gitlab::UsageDataCounters::DesignsCounter - expect { run_service }.to change { counter.read(:create) }.by(1) - end - it 'creates an event in the activity stream' do expect { run_service } .to change { Event.count }.by(1) .and change { Event.for_design.created_action.count }.by(1) - end - - it 'creates a commit in the repository' do - run_service + .and change { counter.read(:create) }.by(1) expect(design_repository.commit).to have_attributes( author: user, @@ -109,35 +117,26 @@ RSpec.describe DesignManagement::SaveDesignsService do end it 'can run the same command in parallel' do - blocks = Array.new(10).map do - unique_files = %w(rails_sample.jpg dk.png) - .map { |name| RenameableUpload.unique_file(name) } + parellism = 4 + + blocks = Array.new(parellism).map do + unique_files = [RenameableUpload.unique_file('rails_sample.jpg')] -> { run_service(unique_files) } end - expect { run_parallel(blocks) }.to change(DesignManagement::Version, :count).by(10) + expect { run_parallel(blocks) }.to change(DesignManagement::Version, :count).by(parellism) end - it 'causes diff_refs not to be nil' do - expect(response).to include( - designs: all(have_attributes(diff_refs: be_present)) - ) - end - - it 'creates a design & a version for the filename if it did not exist' do - expect(issue.designs.size).to eq(0) - - updated_designs = response[:designs] - - expect(updated_designs.size).to eq(1) - expect(updated_designs.first.versions.size).to eq(1) - end - - it 'saves the user as the author' do - updated_designs = response[:designs] + describe 'the response' do + it 'includes designs with the expected properties' do + updated_designs = response[:designs] - expect(updated_designs.first.versions.first.author).to eq(user) + expect(updated_designs).to all(have_attributes(diff_refs: be_present)) + expect(updated_designs.size).to eq(1) + expect(updated_designs.first.versions.size).to eq(1) + expect(updated_designs.first.versions.first.author).to eq(user) + end end describe 'saving the file to LFS' do @@ -147,14 +146,10 @@ RSpec.describe DesignManagement::SaveDesignsService do end end - it 'saves the design to LFS' do - expect { run_service }.to change { LfsObject.count }.by(1) - end - - it 'saves the repository_type of the LfsObjectsProject as design' do - expect do - run_service - end.to change { project.lfs_objects_projects.count }.from(0).to(1) + it 'saves the design to LFS and saves the repository_type of the LfsObjectsProject as design' do + expect { run_service } + .to change { LfsObject.count }.by(1) + .and change { project.lfs_objects_projects.count }.from(0).to(1) expect(project.lfs_objects_projects.first.repository_type).to eq('design') end @@ -202,12 +197,10 @@ RSpec.describe DesignManagement::SaveDesignsService do run_service end - it 'does not create a new version' do - expect { run_service }.not_to change { issue.design_versions.count } - end + it 'does not create a new version, and returns the design in `skipped_designs`' do + response = nil - it 'returns the design in `skipped_designs` instead of `designs`' do - response = run_service + expect { response = run_service }.not_to change { issue.design_versions.count } expect(response[:designs]).to be_empty expect(response[:skipped_designs].size).to eq(1) @@ -223,35 +216,20 @@ RSpec.describe DesignManagement::SaveDesignsService do touch_files([files.first]) end - it 'counts one creation and one update' do + it 'has the correct side-effects' do counter = Gitlab::UsageDataCounters::DesignsCounter - expect { run_service } - .to change { counter.read(:create) }.by(1) - .and change { counter.read(:update) }.by(1) - end - it 'creates the correct activity stream events' do + expect(::DesignManagement::NewVersionWorker) + .to receive(:perform_async).once.with(Integer).and_return(nil) + expect { run_service } .to change { Event.count }.by(2) .and change { Event.for_design.count }.by(2) .and change { Event.created_action.count }.by(1) .and change { Event.updated_action.count }.by(1) - end - - it 'creates a single commit' do - commit_count = -> do - design_repository.expire_all_method_caches - design_repository.commit_count - end - - expect { run_service }.to change { commit_count.call }.by(1) - end - - it 'enqueues just one new version worker' do - expect(::DesignManagement::NewVersionWorker) - .to receive(:perform_async).once.with(Integer) - - run_service + .and change { counter.read(:create) }.by(1) + .and change { counter.read(:update) }.by(1) + .and change { commit_count }.by(1) end end @@ -262,45 +240,28 @@ RSpec.describe DesignManagement::SaveDesignsService do expect(response).to include(designs: have_attributes(size: 2), status: :success) end - it 'creates 2 designs with a single version' do - expect { run_service }.to change { issue.designs.count }.from(0).to(2) - - expect(DesignManagement::Version.for_designs(issue.designs).size).to eq(1) - end - - it 'increments the creation count by 2' do + it 'has the correct side-effects', :request_store do counter = Gitlab::UsageDataCounters::DesignsCounter - expect { run_service }.to change { counter.read(:create) }.by 2 - end - - it 'enqueues a new version worker' do - expect(::DesignManagement::NewVersionWorker) - .to receive(:perform_async).once.with(Integer) - - run_service - end - - it 'creates a single commit' do - commit_count = -> do - design_repository.expire_all_method_caches - design_repository.commit_count - end - - expect { run_service }.to change { commit_count.call }.by(1) - end - - it 'only does 5 gitaly calls', :request_store, :sidekiq_might_not_need_inline do - allow(::DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer) service = described_class.new(project, user, issue: issue, files: files) + # Some unrelated calls that are usually cached or happen only once - service.__send__(:repository).create_if_not_exists - service.__send__(:repository).has_visible_content? + # We expect: + # - An exists? + # - a check for existing blobs + # - default branch + # - an after_commit callback on LfsObjectsProject + design_repository.create_if_not_exists + design_repository.has_visible_content? - request_count = -> { Gitlab::GitalyClient.get_request_count } + expect(::DesignManagement::NewVersionWorker) + .to receive(:perform_async).once.with(Integer).and_return(nil) - # An exists?, a check for existing blobs, default branch, an after_commit - # callback on LfsObjectsProject - expect { service.execute }.to change(&request_count).by(4) + expect { service.execute } + .to change { issue.designs.count }.from(0).to(2) + .and change { DesignManagement::Version.count }.by(1) + .and change { counter.read(:create) }.by(2) + .and change { Gitlab::GitalyClient.get_request_count }.by(3) + .and change { commit_count }.by(1) end context 'when uploading too many files' do @@ -313,7 +274,7 @@ RSpec.describe DesignManagement::SaveDesignsService do end context 'when the user is not allowed to upload designs' do - let(:user) { create(:user) } + let(:user) { build_stubbed(:user) } it_behaves_like 'a service error' end diff --git a/spec/services/discussions/capture_diff_note_positions_service_spec.rb b/spec/services/discussions/capture_diff_note_positions_service_spec.rb index dede5a4c354..be53b02a4c1 100644 --- a/spec/services/discussions/capture_diff_note_positions_service_spec.rb +++ b/spec/services/discussions/capture_diff_note_positions_service_spec.rb @@ -12,24 +12,24 @@ RSpec.describe Discussions::CaptureDiffNotePositionsService do let(:offset) { 30 } let(:first_new_line) { 508 } let(:second_new_line) { 521 } + let(:third_removed_line) { 1240 } let(:service) { described_class.new(merge_request) } - def build_position(new_line, diff_refs) + def build_position(diff_refs, new_line: nil, old_line: nil) path = 'files/markdown/ruby-style-guide.md' Gitlab::Diff::Position.new(old_path: path, new_path: path, - new_line: new_line, diff_refs: diff_refs) + new_line: new_line, old_line: old_line, diff_refs: diff_refs) end - def note_for(new_line) - position = build_position(new_line, merge_request.diff_refs) + def note_for(new_line: nil, old_line: nil) + position = build_position(merge_request.diff_refs, new_line: new_line, old_line: old_line) create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) end - def verify_diff_note_position!(note, line) - id, old_line, new_line = note.line_code.split('_') + def verify_diff_note_position!(note, new_line: nil, old_line: nil) + id, removed_line, added_line = note.line_code.split('_') - expect(new_line).to eq(line.to_s) expect(note.diff_note_positions.size).to eq(1) diff_position = note.diff_note_positions.last @@ -38,12 +38,13 @@ RSpec.describe Discussions::CaptureDiffNotePositionsService do start_sha: merge_request.target_branch_sha, head_sha: merge_request.merge_ref_head.sha) - expect(diff_position.line_code).to eq("#{id}_#{old_line.to_i - offset}_#{new_line}") - expect(diff_position.position).to eq(build_position(new_line.to_i, diff_refs)) + expect(diff_position.line_code).to eq("#{id}_#{removed_line.to_i - offset}_#{added_line}") + expect(diff_position.position).to eq(build_position(diff_refs, new_line: new_line, old_line: old_line)) end - let!(:first_discussion_note) { note_for(first_new_line) } - let!(:second_discussion_note) { note_for(second_new_line) } + let!(:first_discussion_note) { note_for(new_line: first_new_line) } + let!(:second_discussion_note) { note_for(new_line: second_new_line) } + let!(:third_discussion_note) { note_for(old_line: third_removed_line) } let!(:second_discussion_another_note) do create(:diff_note_on_merge_request, project: project, @@ -57,8 +58,9 @@ RSpec.describe Discussions::CaptureDiffNotePositionsService do MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) service.execute - verify_diff_note_position!(first_discussion_note, first_new_line) - verify_diff_note_position!(second_discussion_note, second_new_line) + verify_diff_note_position!(first_discussion_note, new_line: first_new_line) + verify_diff_note_position!(second_discussion_note, new_line: second_new_line) + verify_diff_note_position!(third_discussion_note, old_line: third_removed_line - offset) expect(second_discussion_another_note.diff_note_positions).to be_empty end diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index d10ed7d6640..a91519a710f 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -171,45 +171,52 @@ RSpec.describe EventCreateService do let_it_be(:wiki_page) { create(:wiki_page) } let_it_be(:meta) { create(:wiki_page_meta, :for_wiki_page, wiki_page: wiki_page) } - Event::WIKI_ACTIONS.each do |action| - context "The action is #{action}" do - let(:event) { service.wiki_event(meta, user, action) } - - it 'creates the event', :aggregate_failures do - expect(event).to have_attributes( - wiki_page?: true, - valid?: true, - persisted?: true, - action: action.to_s, - wiki_page: wiki_page, - author: user - ) - end + let(:fingerprint) { generate(:sha) } - it 'records the event in the event counter' do - stub_feature_flags(Gitlab::UsageDataCounters::TrackUniqueActions::FEATURE_FLAG => true) - counter_class = Gitlab::UsageDataCounters::TrackUniqueActions - tracking_params = { event_action: counter_class::WIKI_ACTION, date_from: Date.yesterday, date_to: Date.today } + def create_event + service.wiki_event(meta, user, action, fingerprint) + end - expect { event } - .to change { counter_class.count_unique_events(tracking_params) } - .from(0).to(1) - end + where(:action) { Event::WIKI_ACTIONS.map { |action| [action] } } + + with_them do + it 'creates the event' do + expect(create_event).to have_attributes( + wiki_page?: true, + valid?: true, + persisted?: true, + action: action.to_s, + wiki_page: wiki_page, + author: user, + fingerprint: fingerprint + ) + end - it 'is idempotent', :aggregate_failures do - expect { event }.to change(Event, :count).by(1) - duplicate = nil - expect { duplicate = service.wiki_event(meta, user, action) }.not_to change(Event, :count) + it 'is idempotent', :aggregate_failures do + event = nil + expect { event = create_event }.to change(Event, :count).by(1) + duplicate = nil + expect { duplicate = create_event }.not_to change(Event, :count) - expect(duplicate).to eq(event) - end + expect(duplicate).to eq(event) + end + + it 'records the event in the event counter' do + counter_class = Gitlab::UsageDataCounters::TrackUniqueActions + tracking_params = { event_action: counter_class::WIKI_ACTION, date_from: Date.yesterday, date_to: Date.today } + + expect { create_event } + .to change { counter_class.count_unique(tracking_params) } + .by(1) end end (Event.actions.keys - Event::WIKI_ACTIONS).each do |bad_action| context "The action is #{bad_action}" do + let(:action) { bad_action } + it 'raises an error' do - expect { service.wiki_event(meta, user, bad_action) }.to raise_error(described_class::IllegalActionError) + expect { create_event }.to raise_error(described_class::IllegalActionError) end end end @@ -236,12 +243,11 @@ RSpec.describe EventCreateService do it_behaves_like 'service for creating a push event', PushEventPayloadService it 'records the event in the event counter' do - stub_feature_flags(Gitlab::UsageDataCounters::TrackUniqueActions::FEATURE_FLAG => true) counter_class = Gitlab::UsageDataCounters::TrackUniqueActions tracking_params = { event_action: counter_class::PUSH_ACTION, date_from: Date.yesterday, date_to: Date.today } expect { subject } - .to change { counter_class.count_unique_events(tracking_params) } + .to change { counter_class.count_unique(tracking_params) } .from(0).to(1) end end @@ -260,12 +266,11 @@ RSpec.describe EventCreateService do it_behaves_like 'service for creating a push event', BulkPushEventPayloadService it 'records the event in the event counter' do - stub_feature_flags(Gitlab::UsageDataCounters::TrackUniqueActions::FEATURE_FLAG => true) counter_class = Gitlab::UsageDataCounters::TrackUniqueActions tracking_params = { event_action: counter_class::PUSH_ACTION, date_from: Date.yesterday, date_to: Date.today } expect { subject } - .to change { counter_class.count_unique_events(tracking_params) } + .to change { counter_class.count_unique(tracking_params) } .from(0).to(1) end end @@ -315,12 +320,11 @@ RSpec.describe EventCreateService do end it 'records the event in the event counter' do - stub_feature_flags(Gitlab::UsageDataCounters::TrackUniqueActions::FEATURE_FLAG => true) counter_class = Gitlab::UsageDataCounters::TrackUniqueActions tracking_params = { event_action: counter_class::DESIGN_ACTION, date_from: Date.yesterday, date_to: Date.today } expect { result } - .to change { counter_class.count_unique_events(tracking_params) } + .to change { counter_class.count_unique(tracking_params) } .from(0).to(1) end end @@ -343,12 +347,11 @@ RSpec.describe EventCreateService do end it 'records the event in the event counter' do - stub_feature_flags(Gitlab::UsageDataCounters::TrackUniqueActions::FEATURE_FLAG => true) counter_class = Gitlab::UsageDataCounters::TrackUniqueActions tracking_params = { event_action: counter_class::DESIGN_ACTION, date_from: Date.yesterday, date_to: Date.today } expect { result } - .to change { counter_class.count_unique_events(tracking_params) } + .to change { counter_class.count_unique(tracking_params) } .from(0).to(1) end end diff --git a/spec/services/git/base_hooks_service_spec.rb b/spec/services/git/base_hooks_service_spec.rb index 661c77b56bb..4ab27c7ab05 100644 --- a/spec/services/git/base_hooks_service_spec.rb +++ b/spec/services/git/base_hooks_service_spec.rb @@ -171,6 +171,7 @@ RSpec.describe Git::BaseHooksService do } } end + let(:variables_attributes) { [] } before do @@ -192,6 +193,7 @@ RSpec.describe Git::BaseHooksService do } } end + let(:variables_attributes) do [ { "key" => "FOO", "variable_type" => "env_var", "secret_value" => "123" }, @@ -219,6 +221,7 @@ RSpec.describe Git::BaseHooksService do } } end + let(:variables_attributes) do [ { "key" => "FOO", "variable_type" => "env_var", "secret_value" => "123" } diff --git a/spec/services/git/process_ref_changes_service_spec.rb b/spec/services/git/process_ref_changes_service_spec.rb index c2fb40a0ed0..fc313bf6eb9 100644 --- a/spec/services/git/process_ref_changes_service_spec.rb +++ b/spec/services/git/process_ref_changes_service_spec.rb @@ -167,6 +167,7 @@ RSpec.describe Git::ProcessRefChangesService do { index: 0, oldrev: Gitlab::Git::BLANK_SHA, newrev: '789012', ref: "refs/tags/v10.0.0" } ] end + let(:branch_changes) do [ { index: 0, oldrev: Gitlab::Git::BLANK_SHA, newrev: '789012', ref: "#{ref_prefix}/create1" }, @@ -174,6 +175,7 @@ RSpec.describe Git::ProcessRefChangesService do { index: 2, oldrev: Gitlab::Git::BLANK_SHA, newrev: '789014', ref: "#{ref_prefix}/create3" } ] end + let(:git_changes) { double(branch_changes: branch_changes, tag_changes: tag_changes) } it 'schedules job for existing merge requests' do @@ -190,18 +192,6 @@ RSpec.describe Git::ProcessRefChangesService do subject.execute end - - context 'refresh_only_existing_merge_requests_on_push disabled' do - before do - stub_feature_flags(refresh_only_existing_merge_requests_on_push: false) - end - - it 'refreshes all merge requests' do - expect(UpdateMergeRequestsWorker).to receive(:perform_async).exactly(3).times - - subject.execute - end - end end end diff --git a/spec/services/git/wiki_push_service_spec.rb b/spec/services/git/wiki_push_service_spec.rb index f338b7a5709..7f709be8593 100644 --- a/spec/services/git/wiki_push_service_spec.rb +++ b/spec/services/git/wiki_push_service_spec.rb @@ -218,7 +218,7 @@ RSpec.describe Git::WikiPushService, services: true do message = 'something went very very wrong' allow_next_instance_of(WikiPages::EventCreateService, current_user) do |service| allow(service).to receive(:execute) - .with(String, WikiPage, Symbol) + .with(String, WikiPage, Symbol, String) .and_return(ServiceResponse.error(message: message)) end diff --git a/spec/services/groups/group_links/create_service_spec.rb b/spec/services/groups/group_links/create_service_spec.rb index bca03863d1e..fb88433d8f6 100644 --- a/spec/services/groups/group_links/create_service_spec.rb +++ b/spec/services/groups/group_links/create_service_spec.rb @@ -25,6 +25,7 @@ RSpec.describe Groups::GroupLinks::CreateService, '#execute' do expires_at: nil } end + let(:user) { group_user } subject { described_class.new(group, user, opts) } diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index c87fc7d941e..89e4d091ff7 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -8,6 +8,75 @@ RSpec.describe Groups::TransferService do let!(:group_member) { create(:group_member, :owner, group: group, user: user) } let(:transfer_service) { described_class.new(group, user) } + context 'handling packages' do + let_it_be(:group) { create(:group, :public) } + let(:project) { create(:project, :public, namespace: group) } + let(:new_group) { create(:group, :public) } + + before do + group.add_owner(user) + new_group&.add_owner(user) + end + + context 'with an npm package' do + before do + create(:npm_package, project: project) + end + + shared_examples 'transfer not allowed' do + it 'does not allow transfer when there is a root namespace change' do + transfer_service.execute(new_group) + + expect(transfer_service.error).to eq('Transfer failed: Group contains projects with NPM packages.') + expect(group.parent).not_to eq(new_group) + end + end + + it_behaves_like 'transfer not allowed' + + context 'with a project within subgroup' do + let(:root_group) { create(:group) } + let(:group) { create(:group, parent: root_group) } + + before do + root_group.add_owner(user) + end + + it_behaves_like 'transfer not allowed' + + context 'without a root namespace change' do + let(:new_group) { create(:group, parent: root_group) } + + it 'allows transfer' do + transfer_service.execute(new_group) + + expect(transfer_service.error).to be nil + expect(group.parent).to eq(new_group) + end + end + + context 'when transferring a group into a root group' do + let(:new_group) { nil } + + it_behaves_like 'transfer not allowed' + end + end + end + + context 'without an npm package' do + context 'when transferring a group into a root group' do + let(:group) { create(:group, parent: create(:group)) } + + it 'allows transfer' do + transfer_service.execute(nil) + + expect(transfer_service.error).to be nil + expect(group.parent).to be_nil + end + end + end + end + shared_examples 'ensuring allowed transfer for a group' do context "when there's an exception on GitLab shell directories" do let(:new_parent_group) { create(:group, :public) } diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 25c79d9e600..1e6a8d53354 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -9,6 +9,50 @@ RSpec.describe Groups::UpdateService do let!(:public_group) { create(:group, :public) } describe "#execute" do + shared_examples 'with packages' do + before do + group.add_owner(user) + end + + context 'with npm packages' do + let!(:package) { create(:npm_package, project: project) } + + it 'does not allow a path update' do + expect(update_group(group, user, path: 'updated')).to be false + expect(group.errors[:path]).to include('cannot change when group contains projects with NPM packages') + end + + it 'allows name update' do + expect(update_group(group, user, name: 'Updated')).to be true + expect(group.errors).to be_empty + expect(group.name).to eq('Updated') + end + end + end + + context 'with project' do + let!(:group) { create(:group, :public) } + let(:project) { create(:project, namespace: group) } + + it_behaves_like 'with packages' + + context 'located in a subgroup' do + let(:subgroup) { create(:group, parent: group) } + let!(:project) { create(:project, namespace: subgroup) } + + before do + subgroup.add_owner(user) + end + + it_behaves_like 'with packages' + + it 'does allow a path update if there is not a root namespace change' do + expect(update_group(subgroup, user, path: 'updated')).to be true + expect(subgroup.errors[:path]).to be_empty + end + end + end + context "project visibility_level validation" do context "public group with public projects" do let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } @@ -238,4 +282,8 @@ RSpec.describe Groups::UpdateService do end end end + + def update_group(group, user, opts) + Groups::UpdateService.new(group, user, opts).execute + end end diff --git a/spec/services/import/github_service_spec.rb b/spec/services/import/github_service_spec.rb index 266ff309662..408d7767254 100644 --- a/spec/services/import/github_service_spec.rb +++ b/spec/services/import/github_service_spec.rb @@ -6,7 +6,6 @@ RSpec.describe Import::GithubService do let_it_be(:user) { create(:user) } let_it_be(:token) { 'complex-token' } let_it_be(:access_params) { { github_access_token: 'github-complex-token' } } - let_it_be(:client) { Gitlab::LegacyGithubImport::Client.new(token) } let_it_be(:params) { { repo_id: 123, new_name: 'new_repo', target_namespace: 'root' } } let(:subject) { described_class.new(client, user, params) } @@ -15,41 +14,61 @@ RSpec.describe Import::GithubService do allow(subject).to receive(:authorized?).and_return(true) end - context 'do not raise an exception on input error' do - let(:exception) { Octokit::ClientError.new(status: 404, body: 'Not Found') } + shared_examples 'handles errors' do |klass| + let(:client) { klass.new(token) } - before do - expect(client).to receive(:repo).and_raise(exception) - end + context 'do not raise an exception on input error' do + let(:exception) { Octokit::ClientError.new(status: 404, body: 'Not Found') } + + before do + expect(client).to receive(:repository).and_raise(exception) + end - it 'logs the original error' do - expect(Gitlab::Import::Logger).to receive(:error).with({ - message: 'Import failed due to a GitHub error', - status: 404, - error: 'Not Found' - }).and_call_original + it 'logs the original error' do + expect(Gitlab::Import::Logger).to receive(:error).with({ + message: 'Import failed due to a GitHub error', + status: 404, + error: 'Not Found' + }).and_call_original - subject.execute(access_params, :github) + subject.execute(access_params, :github) + end + + it 'returns an error' do + result = subject.execute(access_params, :github) + + expect(result).to include( + message: 'Import failed due to a GitHub error: Not Found', + status: :error, + http_status: :unprocessable_entity + ) + end end - it 'returns an error' do - result = subject.execute(access_params, :github) + it 'raises an exception for unknown error causes' do + exception = StandardError.new('Not Implemented') + + expect(client).to receive(:repository).and_raise(exception) - expect(result).to include( - message: 'Import failed due to a GitHub error: Not Found', - status: :error, - http_status: :unprocessable_entity - ) + expect(Gitlab::Import::Logger).not_to receive(:error) + + expect { subject.execute(access_params, :github) }.to raise_error(exception) end end - it 'raises an exception for unknown error causes' do - exception = StandardError.new('Not Implemented') + context 'when remove_legacy_github_client feature flag is enabled' do + before do + stub_feature_flags(remove_legacy_github_client: true) + end - expect(client).to receive(:repo).and_raise(exception) + include_examples 'handles errors', Gitlab::GithubImport::Client + end - expect(Gitlab::Import::Logger).not_to receive(:error) + context 'when remove_legacy_github_client feature flag is enabled' do + before do + stub_feature_flags(remove_legacy_github_client: false) + end - expect { subject.execute(access_params, :github) }.to raise_error(exception) + include_examples 'handles errors', Gitlab::LegacyGithubImport::Client end end diff --git a/spec/services/incident_management/create_incident_label_service_spec.rb b/spec/services/incident_management/create_incident_label_service_spec.rb index 2f11bcf397e..18a7c019497 100644 --- a/spec/services/incident_management/create_incident_label_service_spec.rb +++ b/spec/services/incident_management/create_incident_label_service_spec.rb @@ -52,7 +52,15 @@ RSpec.describe IncidentManagement::CreateIncidentLabelService do end context 'without label' do - it_behaves_like 'new label' + context 'when user has permissions to create labels' do + it_behaves_like 'new label' + end + + context 'when user has no permissions to create labels' do + let_it_be(:user) { create(:user) } + + it_behaves_like 'new label' + end end end end diff --git a/spec/services/incident_management/create_issue_service_spec.rb b/spec/services/incident_management/create_issue_service_spec.rb deleted file mode 100644 index dab9a149458..00000000000 --- a/spec/services/incident_management/create_issue_service_spec.rb +++ /dev/null @@ -1,239 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe IncidentManagement::CreateIssueService do - let(:project) { create(:project, :repository, :private) } - let_it_be(:user) { User.alert_bot } - let(:service) { described_class.new(project, alert_payload) } - let(:alert_starts_at) { Time.current } - let(:alert_title) { 'TITLE' } - let(:alert_annotations) { { title: alert_title } } - - let(:alert_payload) do - build_alert_payload( - annotations: alert_annotations, - starts_at: alert_starts_at - ) - end - - let(:alert_presenter) do - Gitlab::Alerting::Alert.new(project: project, payload: alert_payload).present - end - - let!(:setting) do - create(:project_incident_management_setting, project: project) - end - - subject { service.execute } - - context 'when create_issue enabled' do - let(:issue) { subject[:issue] } - - before do - setting.update!(create_issue: true) - end - - context 'without issue_template_content' do - it 'creates an issue with alert summary only' do - expect(subject).to include(status: :success) - - expect(issue.author).to eq(user) - expect(issue.title).to eq(alert_title) - expect(issue.description).to include(alert_presenter.issue_summary_markdown.strip) - expect(separator_count(issue.description)).to eq(0) - end - end - - context 'with erroneous issue service' do - let(:invalid_issue) do - build(:issue, project: project, title: nil).tap(&:valid?) - end - - let(:issue_error) { invalid_issue.errors.full_messages.to_sentence } - - it 'returns and logs the issue error' do - expect_next_instance_of(Issues::CreateService) do |issue_service| - expect(issue_service).to receive(:execute).and_return(invalid_issue) - end - - expect(service) - .to receive(:log_error) - .with(error_message(issue_error)) - - expect(subject).to include(status: :error, message: issue_error) - end - end - - shared_examples 'GFM template' do - context 'plain content' do - let(:template_content) { 'some content' } - - it 'creates an issue appending issue template' do - expect(subject).to include(status: :success) - - expect(issue.description).to include(alert_presenter.issue_summary_markdown) - expect(separator_count(issue.description)).to eq(1) - expect(issue.description).to include(template_content) - end - end - - context 'quick actions' do - let(:user) { create(:user) } - let(:plain_text) { 'some content' } - - let(:template_content) do - <<~CONTENT - #{plain_text} - /due tomorrow - /assign @#{user.username} - CONTENT - end - - before do - project.add_maintainer(user) - end - - it 'creates an issue interpreting quick actions' do - expect(subject).to include(status: :success) - - expect(issue.description).to include(plain_text) - expect(issue.due_date).to be_present - expect(issue.assignees).to eq([user]) - end - end - end - - context 'with gitlab_incident_markdown' do - let(:alert_annotations) do - { title: alert_title, gitlab_incident_markdown: template_content } - end - - it_behaves_like 'GFM template' - end - - context 'with issue_template_content' do - before do - create_issue_template('bug', template_content) - setting.update!(issue_template_key: 'bug') - end - - it_behaves_like 'GFM template' - - context 'and gitlab_incident_markdown' do - let(:template_content) { 'plain text'} - let(:alt_template) { 'alternate text' } - let(:alert_annotations) do - { title: alert_title, gitlab_incident_markdown: alt_template } - end - - it 'includes both templates' do - expect(subject).to include(status: :success) - - expect(issue.description).to include(alert_presenter.issue_summary_markdown) - expect(issue.description).to include(template_content) - expect(issue.description).to include(alt_template) - expect(separator_count(issue.description)).to eq(2) - end - end - - private - - def create_issue_template(name, content) - project.repository.create_file( - project.creator, - ".gitlab/issue_templates/#{name}.md", - content, - message: 'message', - branch_name: 'master' - ) - end - end - - context 'with gitlab alert' do - let(:gitlab_alert) { create(:prometheus_alert, project: project) } - - before do - alert_payload['labels'] = { - 'gitlab_alert_id' => gitlab_alert.prometheus_metric_id.to_s - } - end - - it 'creates an issue' do - query_title = "#{gitlab_alert.title} #{gitlab_alert.computed_operator} #{gitlab_alert.threshold}" - - expect(subject).to include(status: :success) - - expect(issue.author).to eq(user) - expect(issue.title).to eq(alert_presenter.full_title) - expect(issue.title).to include(gitlab_alert.environment.name) - expect(issue.title).to include(query_title) - expect(issue.title).to include('for 5 minutes') - expect(issue.description).to include(alert_presenter.issue_summary_markdown.strip) - expect(separator_count(issue.description)).to eq(0) - end - end - - describe 'with invalid alert payload' do - shared_examples 'invalid alert' do - it 'does not create an issue' do - expect(service) - .to receive(:log_error) - .with(error_message('invalid alert')) - - expect(subject).to eq(status: :error, message: 'invalid alert') - end - end - - context 'without title' do - let(:alert_annotations) { {} } - - it_behaves_like 'invalid alert' - end - - context 'without startsAt' do - let(:alert_starts_at) { nil } - - it_behaves_like 'invalid alert' - end - end - - describe "label `incident`" do - it_behaves_like 'create alert issue sets issue labels' - end - end - - context 'when create_issue disabled' do - before do - setting.update!(create_issue: false) - end - - it 'returns an error' do - expect(service) - .to receive(:log_error) - .with(error_message('setting disabled')) - - expect(subject).to eq(status: :error, message: 'setting disabled') - end - end - - private - - def build_alert_payload(annotations: {}, starts_at: Time.current) - { - 'annotations' => annotations.stringify_keys - }.tap do |payload| - payload['startsAt'] = starts_at.rfc3339 if starts_at - end - end - - def error_message(message) - %{Cannot create incident issue for "#{project.full_name}": #{message}} - end - - def separator_count(text) - summary_separator = "\n\n---\n\n" - - text.scan(summary_separator).size - end -end diff --git a/spec/services/incident_management/incidents/create_service_spec.rb b/spec/services/incident_management/incidents/create_service_spec.rb new file mode 100644 index 00000000000..404c428cd94 --- /dev/null +++ b/spec/services/incident_management/incidents/create_service_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IncidentManagement::Incidents::CreateService do + let_it_be(:project) { create(:project) } + let_it_be(:user) { User.alert_bot } + let(:description) { 'Incident description' } + + describe '#execute' do + subject(:create_incident) { described_class.new(project, user, title: title, description: description).execute } + + context 'when incident has title and description' do + let(:title) { 'Incident title' } + let(:new_issue) { Issue.last! } + let(:label_title) { IncidentManagement::CreateIncidentLabelService::LABEL_PROPERTIES[:title] } + + it 'responds with success' do + expect(create_incident).to be_success + end + + it 'creates an incident issue' do + expect { create_incident }.to change(Issue, :count).by(1) + end + + it 'created issue has correct attributes' do + create_incident + aggregate_failures do + expect(new_issue.title).to eq(title) + expect(new_issue.description).to eq(description) + expect(new_issue.author).to eq(user) + expect(new_issue.issue_type).to eq('incident') + expect(new_issue.labels.map(&:title)).to eq([label_title]) + end + end + + context 'when incident label does not exists' do + it 'creates incident label' do + expect { create_incident }.to change { project.labels.where(title: label_title).count }.by(1) + end + end + + context 'when incident label already exists' do + let!(:label) { create(:label, project: project, title: label_title) } + + it 'does not create new labels' do + expect { create_incident }.not_to change(Label, :count) + end + end + end + + context 'when incident has no title' do + let(:title) { '' } + + it 'does not create an issue' do + expect { create_incident }.not_to change(Issue, :count) + end + + it 'responds with errors' do + expect(create_incident).to be_error + expect(create_incident.message).to eq("Title can't be blank") + end + + it 'result payload contains an Issue object' do + expect(create_incident.payload[:issue]).to be_kind_of(Issue) + end + end + end +end diff --git a/spec/services/incident_management/pager_duty/create_incident_issue_service_spec.rb b/spec/services/incident_management/pager_duty/create_incident_issue_service_spec.rb index cf43ed2411d..73ad0532e07 100644 --- a/spec/services/incident_management/pager_duty/create_incident_issue_service_spec.rb +++ b/spec/services/incident_management/pager_duty/create_incident_issue_service_spec.rb @@ -12,84 +12,63 @@ RSpec.describe IncidentManagement::PagerDuty::CreateIncidentIssueService do subject(:execute) { described_class.new(project, incident_payload).execute } describe '#execute' do - context 'when pagerduty_webhook feature enabled' do - before do - stub_feature_flags(pagerduty_webhook: project) - end + context 'when PagerDuty webhook setting is active' do + let_it_be(:incident_management_setting) { create(:project_incident_management_setting, project: project, pagerduty_active: true) } - context 'when PagerDuty webhook setting is active' do - let_it_be(:incident_management_setting) { create(:project_incident_management_setting, project: project, pagerduty_active: true) } - - context 'when issue can be created' do - it 'creates a new issue' do - expect { execute }.to change(Issue, :count).by(1) - end - - it 'responds with success' do - response = execute - - expect(response).to be_success - expect(response.payload[:issue]).to be_kind_of(Issue) - end - - it 'the issue author is Alert bot' do - expect(execute.payload[:issue].author).to eq(User.alert_bot) - end - - it 'issue has a correct title' do - expect(execute.payload[:issue].title).to eq(incident_payload['title']) - end - - it 'issue has a correct description' do - markdown_line_break = ' ' - - expect(execute.payload[:issue].description).to eq( - <<~MARKDOWN.chomp - **Incident:** [My new incident](https://webdemo.pagerduty.com/incidents/PRORDTY)#{markdown_line_break} - **Incident number:** 33#{markdown_line_break} - **Urgency:** high#{markdown_line_break} - **Status:** triggered#{markdown_line_break} - **Incident key:** #{markdown_line_break} - **Created at:** 26 September 2017, 3:14PM (UTC)#{markdown_line_break} - **Assignees:** [Laura Haley](https://webdemo.pagerduty.com/users/P553OPV)#{markdown_line_break} - **Impacted services:** [Production XDB Cluster](https://webdemo.pagerduty.com/services/PN49J75) - MARKDOWN - ) - end + context 'when issue can be created' do + it 'creates a new issue' do + expect { execute }.to change(Issue, :count).by(1) end - context 'when the payload does not contain a title' do - let(:incident_payload) { {} } + it 'responds with success' do + response = execute + + expect(response).to be_success + expect(response.payload[:issue]).to be_kind_of(Issue) + end - it 'does not create a GitLab issue' do - expect { execute }.not_to change(Issue, :count) - end + it 'the issue author is Alert bot' do + expect(execute.payload[:issue].author).to eq(User.alert_bot) + end - it 'responds with error' do - expect(execute).to be_error - expect(execute.message).to eq("Title can't be blank") - end + it 'issue has a correct title' do + expect(execute.payload[:issue].title).to eq(incident_payload['title']) + end + + it 'issue has a correct description' do + markdown_line_break = ' ' + + expect(execute.payload[:issue].description).to eq( + <<~MARKDOWN.chomp + **Incident:** [My new incident](https://webdemo.pagerduty.com/incidents/PRORDTY)#{markdown_line_break} + **Incident number:** 33#{markdown_line_break} + **Urgency:** high#{markdown_line_break} + **Status:** triggered#{markdown_line_break} + **Incident key:** #{markdown_line_break} + **Created at:** 26 September 2017, 3:14PM (UTC)#{markdown_line_break} + **Assignees:** [Laura Haley](https://webdemo.pagerduty.com/users/P553OPV)#{markdown_line_break} + **Impacted services:** [Production XDB Cluster](https://webdemo.pagerduty.com/services/PN49J75) + MARKDOWN + ) end end - context 'when PagerDuty webhook setting is not active' do - let_it_be(:incident_management_setting) { create(:project_incident_management_setting, project: project, pagerduty_active: false) } + context 'when the payload does not contain a title' do + let(:incident_payload) { {} } it 'does not create a GitLab issue' do expect { execute }.not_to change(Issue, :count) end - it 'responds with forbidden' do + it 'responds with error' do expect(execute).to be_error - expect(execute.http_status).to eq(:forbidden) + expect(execute.message).to eq("Title can't be blank") end end end - context 'when pagerduty_webhook feature disabled' do - before do - stub_feature_flags(pagerduty_webhook: false) - end + context 'when PagerDuty webhook setting is not active' do + let_it_be(:incident_management_setting) { create(:project_incident_management_setting, project: project, pagerduty_active: false) } it 'does not create a GitLab issue' do expect { execute }.not_to change(Issue, :count) diff --git a/spec/services/incident_management/pager_duty/process_webhook_service_spec.rb b/spec/services/incident_management/pager_duty/process_webhook_service_spec.rb index 11ce8388427..4c8aebe5fe2 100644 --- a/spec/services/incident_management/pager_duty/process_webhook_service_spec.rb +++ b/spec/services/incident_management/pager_duty/process_webhook_service_spec.rb @@ -19,92 +19,68 @@ RSpec.describe IncidentManagement::PagerDuty::ProcessWebhookService do subject(:execute) { described_class.new(project, nil, webhook_payload).execute(token) } - context 'when pagerduty_webhook feature is enabled' do - before do - stub_feature_flags(pagerduty_webhook: project) - end - - context 'when PagerDuty webhook setting is active' do - let_it_be(:incident_management_setting) { create(:project_incident_management_setting, project: project, pagerduty_active: true) } - - context 'when token is valid' do - let(:token) { incident_management_setting.pagerduty_token } + context 'when PagerDuty webhook setting is active' do + let_it_be(:incident_management_setting) { create(:project_incident_management_setting, project: project, pagerduty_active: true) } - context 'when webhook payload has acceptable size' do - it 'responds with Accepted' do - result = execute + context 'when token is valid' do + let(:token) { incident_management_setting.pagerduty_token } - expect(result).to be_success - expect(result.http_status).to eq(:accepted) - end - - it 'processes issues' do - incident_payload = ::PagerDuty::WebhookPayloadParser.call(webhook_payload).first['incident'] - - expect(::IncidentManagement::PagerDuty::ProcessIncidentWorker) - .to receive(:perform_async) - .with(project.id, incident_payload) - .once + context 'when webhook payload has acceptable size' do + it 'responds with Accepted' do + result = execute - execute - end + expect(result).to be_success + expect(result.http_status).to eq(:accepted) end - context 'when webhook payload is too big' do - let(:deep_size) { instance_double(Gitlab::Utils::DeepSize, valid?: false) } - - before do - allow(Gitlab::Utils::DeepSize) - .to receive(:new) - .with(webhook_payload, max_size: described_class::PAGER_DUTY_PAYLOAD_SIZE_LIMIT) - .and_return(deep_size) - end + it 'processes issues' do + incident_payload = ::PagerDuty::WebhookPayloadParser.call(webhook_payload).first['incident'] - it 'responds with Bad Request' do - result = execute + expect(::IncidentManagement::PagerDuty::ProcessIncidentWorker) + .to receive(:perform_async) + .with(project.id, incident_payload) + .once - expect(result).to be_error - expect(result.http_status).to eq(:bad_request) - end - - it_behaves_like 'does not process incidents' + execute end + end - context 'when webhook payload is blank' do - let(:webhook_payload) { nil } + context 'when webhook payload is too big' do + let(:deep_size) { instance_double(Gitlab::Utils::DeepSize, valid?: false) } - it 'responds with Accepted' do - result = execute + before do + allow(Gitlab::Utils::DeepSize) + .to receive(:new) + .with(webhook_payload, max_size: described_class::PAGER_DUTY_PAYLOAD_SIZE_LIMIT) + .and_return(deep_size) + end - expect(result).to be_success - expect(result.http_status).to eq(:accepted) - end + it 'responds with Bad Request' do + result = execute - it_behaves_like 'does not process incidents' + expect(result).to be_error + expect(result.http_status).to eq(:bad_request) end + + it_behaves_like 'does not process incidents' end - context 'when token is invalid' do - let(:token) { 'invalid-token' } + context 'when webhook payload is blank' do + let(:webhook_payload) { nil } - it 'responds with Unauthorized' do + it 'responds with Accepted' do result = execute - expect(result).to be_error - expect(result.http_status).to eq(:unauthorized) + expect(result).to be_success + expect(result.http_status).to eq(:accepted) end it_behaves_like 'does not process incidents' end end - context 'when both tokens are nil' do - let_it_be(:incident_management_setting) { create(:project_incident_management_setting, project: project, pagerduty_active: false) } - let(:token) { nil } - - before do - incident_management_setting.update_column(:pagerduty_active, true) - end + context 'when token is invalid' do + let(:token) { 'invalid-token' } it 'responds with Unauthorized' do result = execute @@ -115,25 +91,28 @@ RSpec.describe IncidentManagement::PagerDuty::ProcessWebhookService do it_behaves_like 'does not process incidents' end + end - context 'when PagerDuty webhook setting is not active' do - let_it_be(:incident_management_setting) { create(:project_incident_management_setting, project: project, pagerduty_active: false) } + context 'when both tokens are nil' do + let_it_be(:incident_management_setting) { create(:project_incident_management_setting, project: project, pagerduty_active: false) } + let(:token) { nil } - it 'responds with Forbidden' do - result = execute + before do + incident_management_setting.update_column(:pagerduty_active, true) + end - expect(result).to be_error - expect(result.http_status).to eq(:forbidden) - end + it 'responds with Unauthorized' do + result = execute - it_behaves_like 'does not process incidents' + expect(result).to be_error + expect(result.http_status).to eq(:unauthorized) end + + it_behaves_like 'does not process incidents' end - context 'when pagerduty_webhook feature is disabled' do - before do - stub_feature_flags(pagerduty_webhook: false) - end + context 'when PagerDuty webhook setting is not active' do + let_it_be(:incident_management_setting) { create(:project_incident_management_setting, project: project, pagerduty_active: false) } it 'responds with Forbidden' do result = execute diff --git a/spec/services/issuable/clone/content_rewriter_spec.rb b/spec/services/issuable/clone/content_rewriter_spec.rb deleted file mode 100644 index f39439b7c2f..00000000000 --- a/spec/services/issuable/clone/content_rewriter_spec.rb +++ /dev/null @@ -1,182 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Issuable::Clone::ContentRewriter do - let(:user) { create(:user) } - let(:group) { create(:group) } - let(:project1) { create(:project, :public, group: group) } - let(:project2) { create(:project, :public, group: group) } - - let(:other_issue) { create(:issue, project: project1) } - let(:merge_request) { create(:merge_request) } - - subject { described_class.new(user, original_issue, new_issue)} - - let(:description) { 'Simple text' } - let(:original_issue) { create(:issue, description: description, project: project1) } - let(:new_issue) { create(:issue, project: project2) } - - context 'rewriting award emojis' do - it 'copies the award emojis' do - create(:award_emoji, awardable: original_issue, name: 'thumbsup') - create(:award_emoji, awardable: original_issue, name: 'thumbsdown') - - expect { subject.execute }.to change { AwardEmoji.count }.by(2) - - expect(new_issue.award_emoji.map(&:name)).to match_array(%w(thumbsup thumbsdown)) - end - end - - context 'rewriting description' do - before do - subject.execute - end - - context 'when description is a simple text' do - it 'does not rewrite the description' do - expect(new_issue.reload.description).to eq(original_issue.description) - end - end - - context 'when description contains a local reference' do - let(:description) { "See ##{other_issue.iid}" } - - it 'rewrites the local reference correctly' do - expected_description = "See #{project1.path}##{other_issue.iid}" - - expect(new_issue.reload.description).to eq(expected_description) - end - end - - context 'when description contains a cross reference' do - let(:description) { "See #{merge_request.project.full_path}!#{merge_request.iid}" } - - it 'rewrites the cross reference correctly' do - expected_description = "See #{merge_request.project.full_path}!#{merge_request.iid}" - - expect(new_issue.reload.description).to eq(expected_description) - end - end - - context 'when description contains a user reference' do - let(:description) { "FYU #{user.to_reference}" } - - it 'works with a user reference' do - expect(new_issue.reload.description).to eq("FYU #{user.to_reference}") - end - end - - context 'when description contains uploads' do - let(:uploader) { build(:file_uploader, project: project1) } - let(:description) { "Text and #{uploader.markdown_link}" } - - it 'rewrites uploads in the description' do - upload = Upload.last - - expect(new_issue.description).not_to eq(description) - expect(new_issue.description).to match(/Text and #{FileUploader::MARKDOWN_PATTERN}/) - expect(upload.secret).not_to eq(uploader.secret) - expect(new_issue.description).to include(upload.secret) - expect(new_issue.description).to include(upload.path) - end - end - end - - context 'rewriting notes' do - context 'simple notes' do - let!(:notes) do - [ - create(:note, noteable: original_issue, project: project1, - created_at: 2.weeks.ago, updated_at: 1.week.ago), - create(:note, noteable: original_issue, project: project1), - create(:note, system: true, noteable: original_issue, project: project1) - ] - end - let!(:system_note_metadata) { create(:system_note_metadata, note: notes.last) } - let!(:award_emoji) { create(:award_emoji, awardable: notes.first, name: 'thumbsup')} - - before do - subject.execute - end - - it 'rewrites existing notes in valid order' do - expect(new_issue.notes.order('id ASC').pluck(:note).first(3)).to eq(notes.map(&:note)) - end - - it 'copies all the issue notes' do - expect(new_issue.notes.count).to eq(3) - end - - it 'does not change the note attributes' do - subject.execute - - new_note = new_issue.notes.first - - expect(new_note.note).to eq(notes.first.note) - expect(new_note.author).to eq(notes.first.author) - end - - it 'copies the award emojis' do - subject.execute - - new_note = new_issue.notes.first - new_note.award_emoji.first.name = 'thumbsup' - end - - it 'copies system_note_metadata for system note' do - new_note = new_issue.notes.last - - expect(new_note.system_note_metadata.action).to eq(system_note_metadata.action) - expect(new_note.system_note_metadata.id).not_to eq(system_note_metadata.id) - end - end - - context 'notes with reference' do - let(:text) do - "See ##{other_issue.iid} and #{merge_request.project.full_path}!#{merge_request.iid}" - end - let!(:note) { create(:note, noteable: original_issue, note: text, project: project1) } - - it 'rewrites the references correctly' do - subject.execute - - new_note = new_issue.notes.first - - expected_text = "See #{other_issue.project.path}##{other_issue.iid} and #{merge_request.project.full_path}!#{merge_request.iid}" - - expect(new_note.note).to eq(expected_text) - expect(new_note.author).to eq(note.author) - end - end - - context 'notes with upload' do - let(:uploader) { build(:file_uploader, project: project1) } - let(:text) { "Simple text with image: #{uploader.markdown_link} "} - let!(:note) { create(:note, noteable: original_issue, note: text, project: project1) } - - it 'rewrites note content correctly' do - subject.execute - new_note = new_issue.notes.first - - expect(note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/) - expect(new_note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/) - expect(note.note).not_to eq(new_note.note) - expect(note.note_html).not_to eq(new_note.note_html) - end - end - - context "discussion notes" do - let(:note) { create(:note, noteable: original_issue, note: "sample note", project: project1) } - let!(:discussion) { create(:discussion_note_on_issue, in_reply_to: note, note: "reply to sample note") } - - it 'rewrites discussion correctly' do - subject.execute - - expect(new_issue.notes.count).to eq(original_issue.notes.count) - expect(new_issue.notes.where(discussion_id: discussion.discussion_id).count).to eq(0) - expect(original_issue.notes.where(discussion_id: discussion.discussion_id).count).to eq(1) - end - end - end -end diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb index 68b226b02da..93eef8a2732 100644 --- a/spec/services/issues/build_service_spec.rb +++ b/spec/services/issues/build_service_spec.rb @@ -147,5 +147,19 @@ RSpec.describe Issues::BuildService do expect(issue.milestone).to be_nil end + + context 'setting issue type' do + it 'sets the issue_type on the issue' do + issue = build_issue(issue_type: 'incident') + + expect(issue.issue_type).to eq('incident') + end + + it 'defaults to issue if issue_type not given' do + issue = build_issue + + expect(issue.issue_type).to eq('issue') + end + end end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 6678d831775..7ca7d3be99c 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -252,6 +252,41 @@ RSpec.describe Issues::CloseService do expect(todo.reload).to be_done end + context 'when there is an associated Alert Management Alert' do + context 'when alert can be resolved' do + let!(:alert) { create(:alert_management_alert, issue: issue, project: project) } + + it 'resolves an alert and sends a system note' do + expect_next_instance_of(SystemNotes::AlertManagementService) do |notes_service| + expect(notes_service).to receive(:closed_alert_issue).with(issue) + end + + close_issue + + expect(alert.reload.resolved?).to eq(true) + end + end + + context 'when alert cannot be resolved' do + let!(:alert) { create(:alert_management_alert, :with_validation_errors, issue: issue, project: project) } + + before do + allow(Gitlab::AppLogger).to receive(:warn).and_call_original + end + + it 'writes a warning into the log' do + close_issue + + expect(Gitlab::AppLogger).to have_received(:warn).with( + message: 'Cannot resolve an associated Alert Management alert', + issue_id: issue.id, + alert_id: alert.id, + alert_errors: { hosts: ['hosts array is over 255 chars'] } + ) + end + end + end + it 'deletes milestone issue counters cache' do issue.update(milestone: create(:milestone, project: project)) diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index 8929907a179..5f944d1213b 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -3,15 +3,15 @@ require 'spec_helper' RSpec.describe Issues::MoveService do - let(:user) { create(:user) } - let(:author) { create(:user) } - let(:title) { 'Some issue' } - let(:description) { "Some issue description with mention to #{user.to_reference}" } - let(:group) { create(:group, :private) } - let(:sub_group_1) { create(:group, :private, parent: group) } - let(:sub_group_2) { create(:group, :private, parent: group) } - let(:old_project) { create(:project, namespace: sub_group_1) } - let(:new_project) { create(:project, namespace: sub_group_2) } + let_it_be(:user) { create(:user) } + let_it_be(:author) { create(:user) } + let_it_be(:title) { 'Some issue' } + let_it_be(:description) { "Some issue description with mention to #{user.to_reference}" } + let_it_be(:group) { create(:group, :private) } + let_it_be(:sub_group_1) { create(:group, :private, parent: group) } + let_it_be(:sub_group_2) { create(:group, :private, parent: group) } + let_it_be(:old_project) { create(:project, namespace: sub_group_1) } + let_it_be(:new_project) { create(:project, namespace: sub_group_2) } let(:old_issue) do create(:issue, title: title, description: description, project: old_project, author: author) @@ -30,15 +30,10 @@ RSpec.describe Issues::MoveService do describe '#execute' do shared_context 'issue move executed' do - let!(:award_emoji) { create(:award_emoji, awardable: old_issue) } - let!(:new_issue) { move_service.execute(old_issue, new_project) } end context 'issue movable' do - let!(:note_with_mention) { create(:note, noteable: old_issue, author: author, project: old_project, note: "note with mention #{user.to_reference}") } - let!(:note_with_no_mention) { create(:note, noteable: old_issue, author: author, project: old_project, note: "note without mention") } - include_context 'user can move issue' context 'generic issue' do @@ -48,11 +43,11 @@ RSpec.describe Issues::MoveService do expect(new_issue.project).to eq new_project end - it 'rewrites issue title' do + it 'copies issue title' do expect(new_issue.title).to eq title end - it 'rewrites issue description' do + it 'copies issue description' do expect(new_issue.description).to eq description end @@ -93,23 +88,21 @@ RSpec.describe Issues::MoveService do it 'preserves create time' do expect(old_issue.created_at).to eq new_issue.created_at end + end - it 'moves the award emoji' do - expect(old_issue.award_emoji.first.name).to eq new_issue.reload.award_emoji.first.name - end + context 'issue with award emoji' do + let!(:award_emoji) { create(:award_emoji, awardable: old_issue) } - context 'when issue has notes with mentions' do - it 'saves user mentions with actual mentions for new issue' do - expect(new_issue.user_mentions.find_by(note_id: nil).mentioned_users_ids).to match_array([user.id]) - expect(new_issue.user_mentions.where.not(note_id: nil).first.mentioned_users_ids).to match_array([user.id]) - expect(new_issue.user_mentions.where.not(note_id: nil).count).to eq 1 - expect(new_issue.user_mentions.count).to eq 2 - end + it 'copies the award emoji' do + old_issue.reload + new_issue = move_service.execute(old_issue, new_project) + + expect(old_issue.award_emoji.first.name).to eq new_issue.reload.award_emoji.first.name end end context 'issue with assignee' do - let(:assignee) { create(:user) } + let_it_be(:assignee) { create(:user) } before do old_issue.assignees = [assignee] @@ -154,6 +147,25 @@ RSpec.describe Issues::MoveService do .not_to raise_error # Sidekiq::Worker::EnqueueFromTransactionError end end + + # These tests verify that notes are copied. More thorough tests are in + # the unit test for Notes::CopyService. + context 'issue with notes' do + let!(:notes) do + [ + create(:note, noteable: old_issue, project: old_project, created_at: 2.weeks.ago, updated_at: 1.week.ago), + create(:note, noteable: old_issue, project: old_project) + ] + end + + let(:copied_notes) { new_issue.notes.limit(notes.size) } # Remove the system note added by the copy itself + + include_context 'issue move executed' + + it 'copies existing notes in order' do + expect(copied_notes.order('id ASC').pluck(:note)).to eq(notes.map(&:note)) + end + end end describe 'move permissions' do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 77bd540e22f..42452e95f6b 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -840,27 +840,27 @@ RSpec.describe Issues::UpdateService, :mailer do end context 'real-time updates' do - let(:update_params) { { assignee_ids: [user2.id] } } + using RSpec::Parameterized::TableSyntax - context 'when broadcast_issue_updates is enabled' do - before do - stub_feature_flags(broadcast_issue_updates: true) - end - - it 'broadcasts to the issues channel' do - expect(IssuesChannel).to receive(:broadcast_to).with(issue, event: 'updated') + let(:update_params) { { assignee_ids: [user2.id] } } - update_issue(update_params) - end + where(:action_cable_in_app_enabled, :feature_flag_enabled, :should_broadcast) do + true | true | true + true | false | true + false | true | true + false | false | false end - context 'when broadcast_issue_updates is disabled' do - before do - stub_feature_flags(broadcast_issue_updates: false) - end + with_them do + it 'broadcasts to the issues channel based on ActionCable and feature flag values' do + expect(Gitlab::ActionCable::Config).to receive(:in_app?).and_return(action_cable_in_app_enabled) + stub_feature_flags(broadcast_issue_updates: feature_flag_enabled) - it 'does not broadcast to the issues channel' do - expect(IssuesChannel).not_to receive(:broadcast_to) + if should_broadcast + expect(IssuesChannel).to receive(:broadcast_to).with(issue, event: 'updated') + else + expect(IssuesChannel).not_to receive(:broadcast_to) + end update_issue(update_params) end diff --git a/spec/services/jira/requests/projects/list_service_spec.rb b/spec/services/jira/requests/projects/list_service_spec.rb index 51e67dd821d..b4db77f8104 100644 --- a/spec/services/jira/requests/projects/list_service_spec.rb +++ b/spec/services/jira/requests/projects/list_service_spec.rb @@ -66,16 +66,28 @@ RSpec.describe Jira::Requests::Projects::ListService do context 'when the request returns values' do before do - expect(client).to receive(:get).and_return([{ "key" => 'project1' }, { "key" => 'project2' }]) + expect(client).to receive(:get).and_return([{ 'key' => 'pr1', 'name' => 'First Project' }, { 'key' => 'pr2', 'name' => 'Second Project' }]) end - it 'returns a paylod with jira projets' do + it 'returns a paylod with Jira projets' do payload = subject.payload expect(subject.success?).to be_truthy - expect(payload[:projects].map(&:key)).to eq(%w(project1 project2)) + expect(payload[:projects].map(&:key)).to eq(%w(pr1 pr2)) expect(payload[:is_last]).to be_truthy end + + context 'when filtering projects by name' do + let(:params) { { query: 'first' } } + + it 'returns a paylod with Jira projets' do + payload = subject.payload + + expect(subject.success?).to be_truthy + expect(payload[:projects].map(&:key)).to eq(%w(pr1)) + expect(payload[:is_last]).to be_truthy + end + end end end end diff --git a/spec/services/jira_import/cloud_users_mapper_service_spec.rb b/spec/services/jira_import/cloud_users_mapper_service_spec.rb new file mode 100644 index 00000000000..591f80f3efc --- /dev/null +++ b/spec/services/jira_import/cloud_users_mapper_service_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe JiraImport::CloudUsersMapperService do + let(:start_at) { 7 } + let(:url) { "/rest/api/2/users?maxResults=50&startAt=#{start_at}" } + let(:jira_users) do + [ + { 'accountId' => 'abcd', 'displayName' => 'user1' }, + { 'accountId' => 'efg' }, + { 'accountId' => 'hij', 'displayName' => 'user3', 'emailAddress' => 'user3@example.com' } + ] + end + + describe '#execute' do + it_behaves_like 'mapping jira users' + end +end diff --git a/spec/services/jira_import/server_users_mapper_service_spec.rb b/spec/services/jira_import/server_users_mapper_service_spec.rb new file mode 100644 index 00000000000..22cb0327cc5 --- /dev/null +++ b/spec/services/jira_import/server_users_mapper_service_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe JiraImport::ServerUsersMapperService do + let(:start_at) { 7 } + let(:url) { "/rest/api/2/user/search?username=''&maxResults=50&startAt=#{start_at}" } + let(:jira_users) do + [ + { 'key' => 'abcd', 'name' => 'user1' }, + { 'key' => 'efg' }, + { 'key' => 'hij', 'name' => 'user3', 'emailAddress' => 'user3@example.com' } + ] + end + + describe '#execute' do + it_behaves_like 'mapping jira users' + end +end diff --git a/spec/services/jira_import/users_importer_spec.rb b/spec/services/jira_import/users_importer_spec.rb index 64cdc70f612..efb303dab9f 100644 --- a/spec/services/jira_import/users_importer_spec.rb +++ b/spec/services/jira_import/users_importer_spec.rb @@ -14,6 +14,27 @@ RSpec.describe JiraImport::UsersImporter do subject { importer.execute } describe '#execute' do + let(:mapped_users) do + [ + { + jira_account_id: 'acc1', + jira_display_name: 'user1', + jira_email: 'sample@jira.com', + gitlab_id: nil, + gitlab_username: nil, + gitlab_name: nil + }, + { + jira_account_id: 'acc2', + jira_display_name: 'user2', + jira_email: nil, + gitlab_id: nil, + gitlab_username: nil, + gitlab_name: nil + } + ] + end + before do stub_jira_service_test project.add_maintainer(user) @@ -25,53 +46,83 @@ RSpec.describe JiraImport::UsersImporter do end end - context 'when Jira import is configured correctly' do - let_it_be(:jira_service) { create(:jira_service, project: project, active: true) } - let(:client) { double } + RSpec.shared_examples 'maps jira users to gitlab users' do + context 'when Jira import is configured correctly' do + let_it_be(:jira_service) { create(:jira_service, project: project, active: true) } + let(:client) { double } - before do - expect(importer).to receive(:client).and_return(client) - end - - context 'when jira client raises an error' do - it 'returns an error response' do - expect(client).to receive(:get).and_raise(Timeout::Error) - - expect(subject.error?).to be_truthy - expect(subject.message).to include('There was an error when communicating to Jira') - end - end - - context 'when jira client returns result' do before do - allow(client).to receive(:get).with('/rest/api/2/users?maxResults=50&startAt=7') - .and_return(jira_users) + expect(importer).to receive(:client).at_least(1).and_return(client) + allow(client).to receive_message_chain(:ServerInfo, :all, :deploymentType).and_return(deployment_type) end - context 'when jira client returns an empty array' do - let(:jira_users) { [] } + context 'when jira client raises an error' do + it 'returns an error response' do + expect(client).to receive(:get).and_raise(Timeout::Error) - it 'retturns nil payload' do - expect(subject.success?).to be_truthy - expect(subject.payload).to be_nil + expect(subject.error?).to be_truthy + expect(subject.message).to include('There was an error when communicating to Jira') end end - context 'when jira client returns an results' do - let(:jira_users) { [{ 'name' => 'user1' }, { 'name' => 'user2' }] } - let(:mapped_users) { [{ jira_display_name: 'user1', gitlab_id: 5 }] } + context 'when jira client returns result' do + context 'when jira client returns an empty array' do + let(:jira_users) { [] } - before do - expect(JiraImport::UsersMapper).to receive(:new).with(project, jira_users) - .and_return(double(execute: mapped_users)) + it 'retturns nil payload' do + expect(subject.success?).to be_truthy + expect(subject.payload).to be_empty + end end - it 'returns the mapped users' do - expect(subject.success?).to be_truthy - expect(subject.payload).to eq(mapped_users) + context 'when jira client returns an results' do + it 'returns the mapped users' do + expect(subject.success?).to be_truthy + expect(subject.payload).to eq(mapped_users) + end end end end end + + context 'when Jira instance is of Server deployment type' do + let(:deployment_type) { 'Server' } + let(:url) { "/rest/api/2/user/search?username=''&maxResults=50&startAt=#{start_at}" } + let(:jira_users) do + [ + { 'key' => 'acc1', 'name' => 'user1', 'emailAddress' => 'sample@jira.com' }, + { 'key' => 'acc2', 'name' => 'user2' } + ] + end + + before do + allow_next_instance_of(JiraImport::ServerUsersMapperService) do |instance| + allow(instance).to receive(:client).and_return(client) + allow(client).to receive(:get).with(url).and_return(jira_users) + end + end + + it_behaves_like 'maps jira users to gitlab users' + end + + context 'when Jira instance is of Cloud deploymet type' do + let(:deployment_type) { 'Cloud' } + let(:url) { "/rest/api/2/users?maxResults=50&startAt=#{start_at}" } + let(:jira_users) do + [ + { 'accountId' => 'acc1', 'displayName' => 'user1', 'emailAddress' => 'sample@jira.com' }, + { 'accountId' => 'acc2', 'displayName' => 'user2' } + ] + end + + before do + allow_next_instance_of(JiraImport::CloudUsersMapperService) do |instance| + allow(instance).to receive(:client).and_return(client) + allow(client).to receive(:get).with(url).and_return(jira_users) + end + end + + it_behaves_like 'maps jira users to gitlab users' + end end end diff --git a/spec/services/jira_import/users_mapper_spec.rb b/spec/services/jira_import/users_mapper_spec.rb deleted file mode 100644 index e5e8279a6fb..00000000000 --- a/spec/services/jira_import/users_mapper_spec.rb +++ /dev/null @@ -1,43 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe JiraImport::UsersMapper do - let_it_be(:project) { create(:project) } - - subject { described_class.new(project, jira_users).execute } - - describe '#execute' do - context 'jira_users is nil' do - let(:jira_users) { nil } - - it 'returns an empty array' do - expect(subject).to be_empty - end - end - - context 'when jira_users is present' do - let(:jira_users) do - [ - { 'accountId' => 'abcd', 'displayName' => 'user1' }, - { 'accountId' => 'efg' }, - { 'accountId' => 'hij', 'displayName' => 'user3', 'emailAddress' => 'user3@example.com' } - ] - end - - # TODO: now we only create an array in a proper format - # mapping is tracked in https://gitlab.com/gitlab-org/gitlab/-/issues/219023 - let(:mapped_users) do - [ - { jira_account_id: 'abcd', jira_display_name: 'user1', jira_email: nil, gitlab_id: nil, gitlab_username: nil, gitlab_name: nil }, - { jira_account_id: 'efg', jira_display_name: nil, jira_email: nil, gitlab_id: nil, gitlab_username: nil, gitlab_name: nil }, - { jira_account_id: 'hij', jira_display_name: 'user3', jira_email: 'user3@example.com', gitlab_id: nil, gitlab_username: nil, gitlab_name: nil } - ] - end - - it 'returns users mapped to Gitlab' do - expect(subject).to eq(mapped_users) - end - end - end -end diff --git a/spec/services/labels/available_labels_service_spec.rb b/spec/services/labels/available_labels_service_spec.rb index 9912f2cf469..9ee0b80edcd 100644 --- a/spec/services/labels/available_labels_service_spec.rb +++ b/spec/services/labels/available_labels_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Labels::AvailableLabelsService do let(:other_project_label) { create(:label) } let(:group_label) { create(:group_label, group: group) } let(:other_group_label) { create(:group_label) } - let(:labels) { [project_label, other_project_label, group_label, other_group_label] } + let!(:labels) { [project_label, other_project_label, group_label, other_group_label] } describe '#find_or_create_by_titles' do let(:label_titles) { labels.map(&:title).push('non existing title') } @@ -88,5 +88,33 @@ RSpec.describe Labels::AvailableLabelsService do expect(result).to match_array([group_label.id]) end end + + it 'accepts a single id parameter' do + result = described_class.new(user, project, label_id: project_label.id).filter_labels_ids_in_param(:label_id) + + expect(result).to match_array([project_label.id]) + end + end + + describe '#available_labels' do + context 'when parent is a project' do + it 'returns only relevant labels' do + result = described_class.new(user, project, {}).available_labels + + expect(result.count).to eq(2) + expect(result).to include(project_label, group_label) + expect(result).not_to include(other_project_label, other_group_label) + end + end + + context 'when parent is a group' do + it 'returns only relevant labels' do + result = described_class.new(user, group, {}).available_labels + + expect(result.count).to eq(1) + expect(result).to include(group_label) + expect(result).not_to include(project_label, other_project_label, other_group_label) + end + end end end diff --git a/spec/services/markdown_content_rewriter_service_spec.rb b/spec/services/markdown_content_rewriter_service_spec.rb new file mode 100644 index 00000000000..47332bec319 --- /dev/null +++ b/spec/services/markdown_content_rewriter_service_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MarkdownContentRewriterService do + let_it_be(:user) { create(:user) } + let_it_be(:source_parent) { create(:project, :public) } + let_it_be(:target_parent) { create(:project, :public) } + let(:content) { 'My content' } + + describe '#initialize' do + it 'raises an error if source_parent is not a Project' do + expect do + described_class.new(user, content, create(:group), target_parent) + end.to raise_error(ArgumentError, 'The rewriter classes require that `source_parent` is a `Project`') + end + end + + describe '#execute' do + subject { described_class.new(user, content, source_parent, target_parent).execute } + + it 'calls the rewriter classes successfully', :aggregate_failures do + [Gitlab::Gfm::ReferenceRewriter, Gitlab::Gfm::UploadsRewriter].each do |rewriter_class| + service = double + + expect(service).to receive(:rewrite).with(target_parent) + expect(rewriter_class).to receive(:new).and_return(service) + end + + subject + end + + # Perform simple integration-style tests for each rewriter class. + # to prove they run correctly. + context 'when content contains a reference' do + let_it_be(:issue) { create(:issue, project: source_parent) } + let(:content) { "See ##{issue.iid}" } + + it 'rewrites content' do + expect(subject).to eq("See #{source_parent.full_path}##{issue.iid}") + end + end + + context 'when content contains an upload' do + let(:image_uploader) { build(:file_uploader, project: source_parent) } + let(:content) { "Text and #{image_uploader.markdown_link}" } + + it 'rewrites content' do + new_content = subject + + expect(new_content).not_to eq(content) + expect(new_content.length).to eq(content.length) + end + end + end +end diff --git a/spec/services/merge_requests/conflicts/resolve_service_spec.rb b/spec/services/merge_requests/conflicts/resolve_service_spec.rb index c4d50124ca9..0abc70f71b0 100644 --- a/spec/services/merge_requests/conflicts/resolve_service_spec.rb +++ b/spec/services/merge_requests/conflicts/resolve_service_spec.rb @@ -205,6 +205,7 @@ RSpec.describe MergeRequests::Conflicts::ResolveService do let(:resolver) do MergeRequests::Conflicts::ListService.new(merge_request).conflicts.resolver end + let(:regex_conflict) do resolver.conflict_for_path(resolver.conflicts, 'files/ruby/regex.rb', 'files/ruby/regex.rb') end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index a8661f027e8..bb62e594e7a 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -347,6 +347,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do target_branch: 'master' } end + let(:issuable) { described_class.new(project, user, params).execute } end diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb index c3da02273a4..55856deeaca 100644 --- a/spec/services/merge_requests/ff_merge_service_spec.rb +++ b/spec/services/merge_requests/ff_merge_service_spec.rb @@ -12,6 +12,7 @@ RSpec.describe MergeRequests::FfMergeService do assignees: [user2], author: create(:user)) end + let(:project) { merge_request.project } let(:valid_merge_params) { { sha: merge_request.diff_head_sha } } @@ -113,9 +114,16 @@ RSpec.describe MergeRequests::FfMergeService do it 'logs and saves error if there is an PreReceiveError exception' do error_message = 'error message' + raw_message = 'The truth is out there' - allow(service).to receive(:repository).and_raise(Gitlab::Git::PreReceiveError, "GitLab: #{error_message}") + pre_receive_error = Gitlab::Git::PreReceiveError.new(raw_message, "GitLab: #{error_message}") + allow(service).to receive(:repository).and_raise(pre_receive_error) allow(service).to receive(:execute_hooks) + expect(Gitlab::ErrorTracking).to receive(:track_exception).with( + pre_receive_error, + pre_receive_message: raw_message, + merge_request_id: merge_request.id + ) service.execute(merge_request) diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index 55f92d6bd0a..85bcf4562b1 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -5,9 +5,10 @@ require 'spec_helper' RSpec.describe MergeRequests::PushOptionsHandlerService do include ProjectForksHelper - let(:user) { create(:user) } - let(:project) { create(:project, :public, :repository) } - let(:forked_project) { fork_project(project, user, repository: true) } + let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:user) { create(:user, developer_projects: [project]) } + let_it_be(:forked_project) { fork_project(project, user, repository: true) } + let(:service) { described_class.new(project, user, changes, push_options) } let(:source_branch) { 'fix' } let(:target_branch) { 'feature' } @@ -21,28 +22,14 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" } let(:default_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{project.default_branch}" } - before do - project.add_developer(user) - end - shared_examples_for 'a service that can create a merge request' do subject(:last_mr) { MergeRequest.last } - it 'creates a merge request' do - expect { service.execute }.to change { MergeRequest.count }.by(1) - end - - it 'sets the correct target branch' do + it 'creates a merge request with the correct target branch and assigned user' do branch = push_options[:target] || project.default_branch - service.execute - + expect { service.execute }.to change { MergeRequest.count }.by(1) expect(last_mr.target_branch).to eq(branch) - end - - it 'assigns the MR to the user' do - service.execute - expect(last_mr.assignees).to contain_exactly(user) end @@ -54,15 +41,10 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do allow(forked_project).to receive(:empty_repo?).and_return(false) end - it 'sets the correct source project' do + it 'sets the correct source and target project' do service.execute expect(last_mr.source_project).to eq(forked_project) - end - - it 'sets the correct target project' do - service.execute - expect(last_mr.target_project).to eq(project) end end @@ -746,6 +728,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do end describe 'when MRs are not enabled' do + let(:project) { create(:project, :public, :repository).tap { |pr| pr.add_developer(user) } } let(:push_options) { { create: true } } let(:changes) { new_branch_changes } diff --git a/spec/services/merge_requests/pushed_branches_service_spec.rb b/spec/services/merge_requests/pushed_branches_service_spec.rb index 6e9c77bd3b6..cd6af4c275e 100644 --- a/spec/services/merge_requests/pushed_branches_service_spec.rb +++ b/spec/services/merge_requests/pushed_branches_service_spec.rb @@ -8,19 +8,24 @@ RSpec.describe MergeRequests::PushedBranchesService do context 'when branches pushed' do let(:pushed_branches) do - %w(branch1 branch2 extra1 extra2 extra3).map do |branch| + %w(branch1 branch2 closed-branch1 closed-branch2 extra1 extra2).map do |branch| { ref: "refs/heads/#{branch}" } end end - it 'returns only branches which have a merge request' do + it 'returns only branches which have a open and closed merge request' do create(:merge_request, source_branch: 'branch1', source_project: project) create(:merge_request, source_branch: 'branch2', source_project: project) create(:merge_request, target_branch: 'branch2', source_project: project) - create(:merge_request, :closed, target_branch: 'extra1', source_project: project) - create(:merge_request, source_branch: 'extra2') - - expect(service.execute).to contain_exactly('branch1', 'branch2') + create(:merge_request, :closed, target_branch: 'closed-branch1', source_project: project) + create(:merge_request, :closed, source_branch: 'closed-branch2', source_project: project) + create(:merge_request, source_branch: 'extra1') + + expect(service.execute).to contain_exactly( + 'branch1', + 'branch2', + 'closed-branch2' + ) end end diff --git a/spec/services/merge_requests/rebase_service_spec.rb b/spec/services/merge_requests/rebase_service_spec.rb index 2e525f2ed01..653fcf12a76 100644 --- a/spec/services/merge_requests/rebase_service_spec.rb +++ b/spec/services/merge_requests/rebase_service_spec.rb @@ -13,6 +13,7 @@ RSpec.describe MergeRequests::RebaseService do target_branch: 'master', rebase_jid: rebase_jid end + let(:project) { merge_request.project } let(:repository) { project.repository.raw } let(:skip_ci) { false } diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 18c4cef7087..0696e8a247f 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -555,11 +555,13 @@ RSpec.describe MergeRequests::RefreshService do message: 'Test commit', branch_name: 'master') end + let!(:second_commit) do @fork_project.repository.create_file(@user, 'test2.txt', 'More test data', message: 'Second test commit', branch_name: 'master') end + let!(:forked_master_mr) do create(:merge_request, source_project: @fork_project, @@ -567,6 +569,7 @@ RSpec.describe MergeRequests::RefreshService do target_branch: 'master', target_project: @project) end + let(:force_push_commit) { @project.commit('feature').id } it 'reloads a new diff for a push to the forked project' do diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index 1ec1dc0f6eb..acbd0a42fcd 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -13,6 +13,7 @@ RSpec.describe MergeRequests::SquashService do let(:squash_dir_path) do File.join(Gitlab.config.shared.path, 'tmp/squash', repository.gl_repository, merge_request.id.to_s) end + let(:merge_request_with_one_commit) do create(:merge_request, source_branch: 'feature', source_project: project, diff --git a/spec/services/metrics/dashboard/clone_dashboard_service_spec.rb b/spec/services/metrics/dashboard/clone_dashboard_service_spec.rb index 4a226fe386c..728b343b801 100644 --- a/spec/services/metrics/dashboard/clone_dashboard_service_spec.rb +++ b/spec/services/metrics/dashboard/clone_dashboard_service_spec.rb @@ -84,14 +84,12 @@ RSpec.describe Metrics::Dashboard::CloneDashboardService, :use_clean_rails_memor it_behaves_like 'valid dashboard cloning process', ::Metrics::Dashboard::SystemDashboardService::DASHBOARD_PATH, [ ::Gitlab::Metrics::Dashboard::Stages::CommonMetricsInserter, - ::Gitlab::Metrics::Dashboard::Stages::CustomMetricsInserter, - ::Gitlab::Metrics::Dashboard::Stages::Sorter + ::Gitlab::Metrics::Dashboard::Stages::CustomMetricsInserter ] it_behaves_like 'valid dashboard cloning process', ::Metrics::Dashboard::ClusterDashboardService::DASHBOARD_PATH, [ - ::Gitlab::Metrics::Dashboard::Stages::CommonMetricsInserter, - ::Gitlab::Metrics::Dashboard::Stages::Sorter + ::Gitlab::Metrics::Dashboard::Stages::CommonMetricsInserter ] it_behaves_like 'valid dashboard cloning process', diff --git a/spec/services/metrics/dashboard/cluster_metrics_embed_service_spec.rb b/spec/services/metrics/dashboard/cluster_metrics_embed_service_spec.rb index e80911d6265..dbb89af45d0 100644 --- a/spec/services/metrics/dashboard/cluster_metrics_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/cluster_metrics_embed_service_spec.rb @@ -52,6 +52,7 @@ RSpec.describe Metrics::Dashboard::ClusterMetricsEmbedService, :use_clean_rails_ } ] end + let(:service_call) { described_class.new(*service_params).get_dashboard } let(:panel_groups) { service_call[:dashboard][:panel_groups] } let(:panel) { panel_groups.first[:panels].first } diff --git a/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb b/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb index d4391ecb6b9..aea9c25d104 100644 --- a/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb +++ b/spec/services/metrics/dashboard/custom_dashboard_service_spec.rb @@ -57,6 +57,16 @@ RSpec.describe Metrics::Dashboard::CustomDashboardService, :use_clean_rails_memo described_class.new(*service_params).get_dashboard end + it 'tracks panel type' do + allow(::Gitlab::Tracking).to receive(:event).and_call_original + + described_class.new(*service_params).get_dashboard + + expect(::Gitlab::Tracking).to have_received(:event) + .with('MetricsDashboard::Chart', 'chart_rendered', { label: 'area-chart' }) + .at_least(:once) + end + context 'and the dashboard is then deleted' do it 'does not return the previously cached dashboard' do described_class.new(*service_params).get_dashboard @@ -104,6 +114,16 @@ RSpec.describe Metrics::Dashboard::CustomDashboardService, :use_clean_rails_memo }] ) end + + it 'caches repo file list' do + expect(Gitlab::Metrics::Dashboard::RepoDashboardFinder).to receive(:list_dashboards) + .with(project) + .once + .and_call_original + + described_class.all_dashboard_paths(project) + described_class.all_dashboard_paths(project) + end end end diff --git a/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb b/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb index a5f7c2ab8ab..82321dbc822 100644 --- a/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb @@ -111,7 +111,8 @@ RSpec.describe Metrics::Dashboard::CustomMetricEmbedService do it_behaves_like 'valid embedded dashboard service response' it 'does not cache the unprocessed dashboard' do - expect(Gitlab::Metrics::Dashboard::Cache).not_to receive(:fetch) + # Fail spec if any method of Cache class is called. + stub_const('Gitlab::Metrics::Dashboard::Cache', double) described_class.new(*service_params).get_dashboard end diff --git a/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb b/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb index 72b356be60f..3c533b0c464 100644 --- a/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb @@ -132,7 +132,7 @@ RSpec.describe Metrics::Dashboard::DynamicEmbedService, :use_clean_rails_memory_ end shared_examples 'uses system dashboard' do - it 'uses the default dashboard' do + it 'uses the overview dashboard' do expect(Gitlab::Metrics::Dashboard::Finder) .to receive(:find_raw) .with(project, dashboard_path: system_dashboard_path) diff --git a/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb b/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb index 29c941826b5..dd9d498e307 100644 --- a/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb @@ -72,12 +72,18 @@ RSpec.describe Metrics::Dashboard::GitlabAlertEmbedService do it_behaves_like 'valid embedded dashboard service response' it_behaves_like 'raises error for users with insufficient permissions' - it 'uses the metric info corresponding to the alert' do + it 'generates an panel based on the alert' do result = service_call - metrics = result[:dashboard][:panel_groups][0][:panels][0][:metrics] - - expect(metrics.length).to eq 1 - expect(metrics.first[:metric_id]).to eq alert.prometheus_metric_id + panel = result[:dashboard][:panel_groups][0][:panels][0] + metric = panel[:metrics].first + + expect(panel[:metrics].length).to eq 1 + expect(panel).to include( + title: alert.prometheus_metric.title, + y_label: alert.prometheus_metric.y_label, + type: 'area-chart' + ) + expect(metric[:metric_id]).to eq alert.prometheus_metric_id end context 'when the metric does not exist' do @@ -87,7 +93,8 @@ RSpec.describe Metrics::Dashboard::GitlabAlertEmbedService do end it 'does not cache the unprocessed dashboard' do - expect(Gitlab::Metrics::Dashboard::Cache).not_to receive(:fetch) + # Fail spec if any method of Cache class is called. + stub_const('Gitlab::Metrics::Dashboard::Cache', double) described_class.new(*service_params).get_dashboard end diff --git a/spec/services/metrics/dashboard/grafana_metric_embed_service_spec.rb b/spec/services/metrics/dashboard/grafana_metric_embed_service_spec.rb index ee3c55cb642..5263fd40a40 100644 --- a/spec/services/metrics/dashboard/grafana_metric_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/grafana_metric_embed_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Metrics::Dashboard::GrafanaMetricEmbedService do include ReactiveCachingHelpers include GrafanaApiHelpers - let_it_be(:project) { build(:project) } + let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } let_it_be(:grafana_integration) { create(:grafana_integration, project: project) } @@ -15,7 +15,7 @@ RSpec.describe Metrics::Dashboard::GrafanaMetricEmbedService do valid_grafana_dashboard_link(grafana_integration.grafana_url) end - before do + before_all do project.add_maintainer(user) end @@ -58,6 +58,31 @@ RSpec.describe Metrics::Dashboard::GrafanaMetricEmbedService do expect(subject.current_user).to eq(user) expect(subject.params[:grafana_url]).to eq(grafana_url) end + + context 'with unknown users' do + let(:params) { [project.id, current_user_id, grafana_url] } + + context 'when anonymous' do + where(:current_user_id) do + [nil, ''] + end + + with_them do + it 'sets current_user as nil' do + expect(subject.current_user).to be_nil + end + end + end + + context 'when invalid' do + let(:current_user_id) { non_existing_record_id } + + it 'raise record not found error' do + expect { subject } + .to raise_error(ActiveRecord::RecordNotFound, /Couldn't find User/) + end + end + end end describe '#get_dashboard', :use_clean_rails_memory_store_caching do @@ -145,7 +170,17 @@ RSpec.describe Metrics::Dashboard::GrafanaMetricEmbedService do stub_datasource_request(grafana_integration.grafana_url) end - it_behaves_like 'valid embedded dashboard service response' + context 'when project is private and user is member' do + it_behaves_like 'valid embedded dashboard service response' + end + + context 'when project is public and user is anonymous' do + let(:project) { create(:project, :public) } + let(:user) { nil } + let(:grafana_integration) { create(:grafana_integration, project: project) } + + it_behaves_like 'valid embedded dashboard service response' + end end end diff --git a/spec/services/metrics/dashboard/panel_preview_service_spec.rb b/spec/services/metrics/dashboard/panel_preview_service_spec.rb new file mode 100644 index 00000000000..d58dee3e7a3 --- /dev/null +++ b/spec/services/metrics/dashboard/panel_preview_service_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Metrics::Dashboard::PanelPreviewService do + let_it_be(:project) { create(:project) } + let_it_be(:environment) { create(:environment, project: project) } + let_it_be(:panel_yml) do + <<~YML + --- + title: test panel + YML + end + let_it_be(:dashboard) do + { + panel_groups: [ + { + panels: [{ 'title' => 'test panel' }] + } + ] + } + end + + describe '#execute' do + subject(:service_response) { described_class.new(project, panel_yml, environment).execute } + + context "valid panel's yaml" do + before do + allow_next_instance_of(::Gitlab::Metrics::Dashboard::Processor) do |processor| + allow(processor).to receive(:process).and_return(dashboard) + end + end + + it 'returns success service response' do + expect(service_response.success?).to be_truthy + end + + it 'returns processed panel' do + expect(service_response.payload).to eq('title' => 'test panel') + end + + it 'uses dashboard processor' do + sequence = [ + ::Gitlab::Metrics::Dashboard::Stages::CommonMetricsInserter, + ::Gitlab::Metrics::Dashboard::Stages::MetricEndpointInserter, + ::Gitlab::Metrics::Dashboard::Stages::PanelIdsInserter, + ::Gitlab::Metrics::Dashboard::Stages::AlertsInserter, + ::Gitlab::Metrics::Dashboard::Stages::UrlValidator + ] + processor_params = [project, dashboard, sequence, environment: environment] + + expect_next_instance_of(::Gitlab::Metrics::Dashboard::Processor, *processor_params) do |processor| + expect(processor).to receive(:process).and_return(dashboard) + end + + service_response + end + end + + context "invalid panel's yaml" do + [ + Gitlab::Metrics::Dashboard::Errors::DashboardProcessingError, + Gitlab::Config::Loader::Yaml::NotHashError, + Gitlab::Config::Loader::Yaml::DataTooLargeError, + Gitlab::Config::Loader::FormatError + ].each do |error_class| + before do + allow_next_instance_of(::Gitlab::Metrics::Dashboard::Processor) do |processor| + allow(processor).to receive(:process).and_raise(error_class.new('error')) + end + end + + it 'returns error service response' do + expect(service_response.error?).to be_truthy + end + + it 'returns error message' do + expect(service_response.message).to eq('error') + end + end + end + end +end diff --git a/spec/services/metrics/dashboard/pod_dashboard_service_spec.rb b/spec/services/metrics/dashboard/pod_dashboard_service_spec.rb index ae0e38a04b2..0ea812e93ee 100644 --- a/spec/services/metrics/dashboard/pod_dashboard_service_spec.rb +++ b/spec/services/metrics/dashboard/pod_dashboard_service_spec.rb @@ -54,4 +54,20 @@ RSpec.describe Metrics::Dashboard::PodDashboardService, :use_clean_rails_memory_ let(:dashboard_version) { subject.send(:dashboard_version) } end end + + describe '.all_dashboard_paths' do + it 'returns the dashboard attributes' do + all_dashboards = described_class.all_dashboard_paths(project) + + expect(all_dashboards).to eq( + [{ + path: described_class::DASHBOARD_PATH, + display_name: described_class::DASHBOARD_NAME, + default: false, + system_dashboard: false, + out_of_the_box_dashboard: true + }] + ) + end + end end diff --git a/spec/services/metrics/dashboard/self_monitoring_dashboard_service_spec.rb b/spec/services/metrics/dashboard/self_monitoring_dashboard_service_spec.rb index aea3e41a013..33b7e3c85cd 100644 --- a/spec/services/metrics/dashboard/self_monitoring_dashboard_service_spec.rb +++ b/spec/services/metrics/dashboard/self_monitoring_dashboard_service_spec.rb @@ -50,7 +50,7 @@ RSpec.describe Metrics::Dashboard::SelfMonitoringDashboardService, :use_clean_ra path: described_class::DASHBOARD_PATH, display_name: described_class::DASHBOARD_NAME, default: true, - system_dashboard: false, + system_dashboard: true, out_of_the_box_dashboard: true }] ) diff --git a/spec/services/notes/copy_service_spec.rb b/spec/services/notes/copy_service_spec.rb new file mode 100644 index 00000000000..fd44aa7cf40 --- /dev/null +++ b/spec/services/notes/copy_service_spec.rb @@ -0,0 +1,157 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Notes::CopyService do + describe '#initialize' do + let_it_be(:noteable) { create(:issue) } + + it 'validates that we cannot copy notes to the same Noteable' do + expect { described_class.new(noteable, noteable) }.to raise_error(ArgumentError) + end + end + + describe '#execute' do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:from_project) { create(:project, :public, group: group) } + let_it_be(:to_project) { create(:project, :public, group: group) } + let(:from_noteable) { create(:issue, project: from_project) } + let(:to_noteable) { create(:issue, project: to_project) } + + subject(:execute_service) { described_class.new(user, from_noteable, to_noteable).execute } + + context 'rewriting the note body' do + context 'simple notes' do + let!(:notes) do + [ + create(:note, noteable: from_noteable, project: from_noteable.project, + created_at: 2.weeks.ago, updated_at: 1.week.ago), + create(:note, noteable: from_noteable, project: from_noteable.project), + create(:note, system: true, noteable: from_noteable, project: from_noteable.project) + ] + end + + it 'rewrites existing notes in valid order' do + execute_service + + expect(to_noteable.notes.order('id ASC').pluck(:note).first(3)).to eq(notes.map(&:note)) + end + + it 'copies all the issue notes' do + execute_service + + expect(to_noteable.notes.count).to eq(3) + end + + it 'does not change the note attributes' do + execute_service + + new_note = to_noteable.notes.first + + expect(new_note).to have_attributes( + note: notes.first.note, + author: notes.first.author + ) + end + + it 'copies the award emojis' do + create(:award_emoji, awardable: notes.first, name: 'thumbsup') + + execute_service + + new_award_emoji = to_noteable.notes.first.award_emoji.first + + expect(new_award_emoji.name).to eq('thumbsup') + end + + it 'copies system_note_metadata for system note' do + system_note_metadata = create(:system_note_metadata, note: notes.last) + + execute_service + + new_note = to_noteable.notes.last + + aggregate_failures do + expect(new_note.system_note_metadata.action).to eq(system_note_metadata.action) + expect(new_note.system_note_metadata.id).not_to eq(system_note_metadata.id) + end + end + + it 'returns success' do + aggregate_failures do + expect(execute_service).to be_kind_of(ServiceResponse) + expect(execute_service).to be_success + end + end + end + + context 'notes with mentions' do + let!(:note_with_mention) { create(:note, noteable: from_noteable, author: from_noteable.author, project: from_noteable.project, note: "note with mention #{user.to_reference}") } + let!(:note_with_no_mention) { create(:note, noteable: from_noteable, author: from_noteable.author, project: from_noteable.project, note: "note without mention") } + + it 'saves user mentions with actual mentions for new issue' do + execute_service + + aggregate_failures do + expect(to_noteable.user_mentions.first.mentioned_users_ids).to match_array([user.id]) + expect(to_noteable.user_mentions.count).to eq(1) + end + end + end + + context 'notes with reference' do + let(:other_issue) { create(:issue, project: from_noteable.project) } + let(:merge_request) { create(:merge_request) } + let(:text) { "See ##{other_issue.iid} and #{merge_request.project.full_path}!#{merge_request.iid}" } + let!(:note) { create(:note, noteable: from_noteable, note: text, project: from_noteable.project) } + + it 'rewrites the references correctly' do + execute_service + + new_note = to_noteable.notes.first + + expected_text = "See #{other_issue.project.path}##{other_issue.iid} and #{merge_request.project.full_path}!#{merge_request.iid}" + + aggregate_failures do + expect(new_note.note).to eq(expected_text) + expect(new_note.author).to eq(note.author) + end + end + end + + context 'notes with upload' do + let(:uploader) { build(:file_uploader, project: from_noteable.project) } + let(:text) { "Simple text with image: #{uploader.markdown_link} "} + let!(:note) { create(:note, noteable: from_noteable, note: text, project: from_noteable.project) } + + it 'rewrites note content correctly' do + execute_service + new_note = to_noteable.notes.first + + aggregate_failures do + expect(note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/) + expect(new_note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/) + expect(note.note).not_to eq(new_note.note) + expect(note.note_html).not_to eq(new_note.note_html) + end + end + end + + context 'discussion notes' do + let(:note) { create(:note, noteable: from_noteable, note: 'sample note', project: from_noteable.project) } + let!(:discussion) { create(:discussion_note_on_issue, in_reply_to: note, note: 'reply to sample note') } + + it 'rewrites discussion correctly' do + execute_service + + aggregate_failures do + expect(to_noteable.notes.count).to eq(from_noteable.notes.count) + expect(to_noteable.notes.where(discussion_id: discussion.discussion_id).count).to eq(0) + expect(from_noteable.notes.where(discussion_id: discussion.discussion_id).count).to eq(1) + end + end + end + end + end +end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index fd824621db7..f087f72ca46 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -117,6 +117,7 @@ RSpec.describe Notes::CreateService do source_project: project_with_repo, target_project: project_with_repo) end + let(:line_number) { 14 } let(:position) do Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", @@ -125,6 +126,7 @@ RSpec.describe Notes::CreateService do new_line: line_number, diff_refs: merge_request.diff_refs) end + let(:previous_note) do create(:diff_note_on_merge_request, noteable: merge_request, project: project_with_repo) end @@ -432,6 +434,13 @@ RSpec.describe Notes::CreateService do .and change { existing_note.updated_at } end + it 'returns a DiscussionNote with its parent discussion refreshed correctly' do + discussion_notes = subject.discussion.notes + + expect(discussion_notes.size).to eq(2) + expect(discussion_notes.first).to be_a(DiscussionNote) + end + context 'discussion to reply cannot be found' do before do existing_note.delete diff --git a/spec/services/notes/quick_actions_service_spec.rb b/spec/services/notes/quick_actions_service_spec.rb index d20824efaaa..e9decd44730 100644 --- a/spec/services/notes/quick_actions_service_spec.rb +++ b/spec/services/notes/quick_actions_service_spec.rb @@ -58,14 +58,29 @@ RSpec.describe Notes::QuickActionsService do end describe '/spend' do - let(:note_text) { '/spend 1h' } + context 'when note is not persisted' do + let(:note_text) { '/spend 1h' } - it 'updates the spent time on the noteable' do - content, update_params = service.execute(note) - service.apply_updates(update_params, note) + it 'adds time to noteable, adds timelog with nil note_id and has no content' do + content, update_params = service.execute(note) + service.apply_updates(update_params, note) - expect(content).to eq '' - expect(note.noteable.time_spent).to eq(3600) + expect(content).to eq '' + expect(note.noteable.time_spent).to eq(3600) + expect(Timelog.last.note_id).to be_nil + end + end + + context 'when note is persisted' do + let(:note_text) { "a note \n/spend 1h" } + + it 'updates the spent time and populates timelog with note_id' do + new_content, update_params = service.execute(note) + note.update!(note: new_content) + service.apply_updates(update_params, note) + + expect(Timelog.last.note_id).to eq(note.id) + end end end end diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index 70dea99de4a..47b8ba0cd72 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -13,6 +13,17 @@ RSpec.describe Notes::UpdateService do let(:issue) { create(:issue, project: project) } let(:issue2) { create(:issue, project: private_project) } let(:note) { create(:note, project: project, noteable: issue, author: user, note: "Old note #{user2.to_reference}") } + let(:markdown) do + <<-MARKDOWN.strip_heredoc + ```suggestion + foo + ``` + + ```suggestion + bar + ``` + MARKDOWN + end before do project.add_maintainer(user) @@ -36,18 +47,18 @@ RSpec.describe Notes::UpdateService do end end - context 'suggestions' do - it 'refreshes note suggestions' do - markdown = <<-MARKDOWN.strip_heredoc - ```suggestion - foo - ``` + context 'with system note' do + before do + note.update_column(:system, true) + end - ```suggestion - bar - ``` - MARKDOWN + it 'does not update the note' do + expect { update_note(note: 'new text') }.not_to change { note.reload.note } + end + end + context 'suggestions' do + it 'refreshes note suggestions' do suggestion = create(:suggestion) note = suggestion.note @@ -191,5 +202,24 @@ RSpec.describe Notes::UpdateService do end end end + + 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}" ) } + + it 'does not create todos' do + expect { update_note({ note: "Mentioning user #{user2}" }) }.not_to change { note.todos.count } + end + + it 'does not create suggestions' do + expect { update_note({ note: "Updated snippet with markdown suggestion #{markdown}" }) } + .not_to change { note.suggestions.count } + end + + it 'does not create mentions' do + expect(note).not_to receive(:create_new_cross_references!) + update_note({ note: "Updated with new reference: #{issue.to_reference}" }) + end + end end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 2fe7a46de4b..8186bc40bc0 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -238,6 +238,26 @@ RSpec.describe NotificationService, :mailer do expect { subject }.to have_enqueued_email(user, mail: "access_token_about_to_expire_email") end end + + describe '#access_token_expired' do + let_it_be(:user) { create(:user) } + + subject { notification.access_token_expired(user) } + + it 'sends email to the token owner' do + expect { subject }.to have_enqueued_email(user, mail: "access_token_expired_email") + end + + context 'when user is not allowed to receive notifications' do + before do + user.block! + end + + it 'does not send email to the token owner' do + expect { subject }.not_to have_enqueued_email(user, mail: "access_token_expired_email") + end + end + end end describe '#unknown_sign_in' do @@ -2054,16 +2074,66 @@ RSpec.describe NotificationService, :mailer do end describe '#project_was_moved' do - it 'notifies the expected users' do - notification.project_was_moved(project, "gitlab/gitlab") + context 'when notifications are disabled' do + before do + @u_custom_global.global_notification_setting.update!(moved_project: false) + end - should_email(@u_watcher) - should_email(@u_participating) - should_email(@u_lazy_participant) - should_email(@u_custom_global) - should_not_email(@u_guest_watcher) - should_not_email(@u_guest_custom) - should_not_email(@u_disabled) + it 'does not send a notification' do + notification.project_was_moved(project, "gitlab/gitlab") + + should_not_email(@u_custom_global) + end + end + + context 'with users at both project and group level' do + let(:maintainer) { create(:user) } + let(:developer) { create(:user) } + let(:group_owner) { create(:user) } + let(:group_maintainer) { create(:user) } + let(:group_developer) { create(:user) } + let(:blocked_user) { create(:user, :blocked) } + let(:invited_user) { create(:user) } + + let!(:group) do + create(:group, :public) do |group| + project.group = group + project.save! + + group.add_owner(group_owner) + group.add_maintainer(group_maintainer) + group.add_developer(group_developer) + # This is to check for dupes + group.add_maintainer(maintainer) + group.add_maintainer(blocked_user) + end + end + + before do + project.add_maintainer(maintainer) + project.add_developer(developer) + project.add_maintainer(blocked_user) + reset_delivered_emails! + end + + it 'notifies the expected users' do + notification.project_was_moved(project, "gitlab/gitlab") + + should_email(@u_watcher) + should_email(@u_participating) + should_email(@u_lazy_participant) + should_email(@u_custom_global) + should_not_email(@u_guest_watcher) + should_not_email(@u_guest_custom) + should_not_email(@u_disabled) + + should_email(maintainer) + should_email(group_owner) + should_email(group_maintainer) + should_not_email(group_developer) + should_not_email(developer) + should_not_email(blocked_user) + end end it_behaves_like 'project emails are disabled' do diff --git a/spec/services/packages/create_dependency_service_spec.rb b/spec/services/packages/create_dependency_service_spec.rb index 00e5e5c6d96..3ad59b31b2c 100644 --- a/spec/services/packages/create_dependency_service_spec.rb +++ b/spec/services/packages/create_dependency_service_spec.rb @@ -15,6 +15,7 @@ RSpec.describe Packages::CreateDependencyService do .gsub('1.0.1', version)) .with_indifferent_access end + let(:package_version) { params[:versions].each_key.first } let(:dependencies) { params[:versions][package_version] } let(:package) { create(:npm_package) } diff --git a/spec/services/packages/maven/find_or_create_package_service_spec.rb b/spec/services/packages/maven/find_or_create_package_service_spec.rb index c9441324216..4406e4037e2 100644 --- a/spec/services/packages/maven/find_or_create_package_service_spec.rb +++ b/spec/services/packages/maven/find_or_create_package_service_spec.rb @@ -4,34 +4,77 @@ require 'spec_helper' RSpec.describe Packages::Maven::FindOrCreatePackageService do let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } - let_it_be(:app_name) { 'my-app' } - let_it_be(:version) { '1.0-SNAPSHOT' } - let_it_be(:path) { "my/company/app/#{app_name}" } - let_it_be(:path_with_version) { "#{path}/#{version}" } - let_it_be(:params) do - { - path: path_with_version, - name: path, - version: version - } - end + + let(:app_name) { 'my-app' } + let(:path) { "sandbox/test/app/#{app_name}" } + let(:version) { '1.0.0' } + let(:file_name) { 'test.jar' } + let(:param_path) { "#{path}/#{version}" } describe '#execute' do - subject { described_class.new(project, user, params).execute } + using RSpec::Parameterized::TableSyntax + + subject { described_class.new(project, user, { path: param_path, file_name: file_name }).execute } + + RSpec.shared_examples 'reuse existing package' do + it { expect { subject}.not_to change { Packages::Package.count } } + + it { is_expected.to eq(existing_package) } + end + + RSpec.shared_examples 'create package' do + it { expect { subject}.to change { Packages::Package.count }.by(1) } + + it 'sets the proper name and version' do + pkg = subject + + expect(pkg.name).to eq(path) + expect(pkg.version).to eq(version) + end + end - context 'without any existing package' do - it 'creates a package' do - expect { subject }.to change { Packages::Package.count }.by(1) + context 'path with version' do + # Note that "path with version" and "file type maven metadata xml" only exists for snapshot versions + # In other words, we will never have an metadata xml upload on a path with version for a non snapshot version + where(:package_exist, :file_type, :snapshot_version, :shared_example_name) do + true | :jar | false | 'reuse existing package' + false | :jar | false | 'create package' + true | :jar | true | 'reuse existing package' + false | :jar | true | 'create package' + true | :maven_xml | true | 'reuse existing package' + false | :maven_xml | true | 'create package' + end + + with_them do + let(:version) { snapshot_version ? '1.0-SNAPSHOT' : '1.0.0' } + let(:file_name) { file_type == :maven_xml ? 'maven-metadata.xml' : 'test.jar' } + + let!(:existing_package) do + if package_exist + create(:maven_package, name: path, version: version, project: project) + end + end + + it_behaves_like params[:shared_example_name] end end - context 'with an existing package' do - let_it_be(:existing_package) { create(:maven_package, name: path, version: version, project: project) } + context 'path without version' do + let(:param_path) { path } + let(:version) { nil } + + context 'maven-metadata.xml file' do + let(:file_name) { 'maven-metadata.xml' } + + context 'with existing package' do + let!(:existing_package) { create(:maven_package, name: path, version: version, project: project) } + + it_behaves_like 'reuse existing package' + end - it { is_expected.to eq existing_package } - it "doesn't create a new package" do - expect { subject } - .to not_change { Packages::Package.count } + context 'without existing package' do + it_behaves_like 'create package' + 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 25bbbf82bec..c1391746f52 100644 --- a/spec/services/packages/npm/create_package_service_spec.rb +++ b/spec/services/packages/npm/create_package_service_spec.rb @@ -13,6 +13,7 @@ RSpec.describe Packages::Npm::CreatePackageService do .gsub('1.0.1', version)).with_indifferent_access .merge!(override) end + let(:override) { {} } let(:package_name) { "@#{namespace.path}/my-app".freeze } diff --git a/spec/services/packages/pypi/create_package_service_spec.rb b/spec/services/packages/pypi/create_package_service_spec.rb index 250b43d1f75..bfecb32f9ef 100644 --- a/spec/services/packages/pypi/create_package_service_spec.rb +++ b/spec/services/packages/pypi/create_package_service_spec.rb @@ -49,18 +49,11 @@ RSpec.describe Packages::Pypi::CreatePackageService do params[:md5_digest] = 'def' end - it 'replaces the file' do + it 'throws an error' do expect { subject } .to change { Packages::Package.pypi.count }.by(0) - .and change { Packages::PackageFile.count }.by(1) - - expect(created_package.package_files.size).to eq 2 - expect(created_package.package_files.first.file_name).to eq 'foo.tgz' - expect(created_package.package_files.first.file_sha256).to eq '123' - expect(created_package.package_files.first.file_md5).to eq '567' - expect(created_package.package_files.last.file_name).to eq 'foo.tgz' - expect(created_package.package_files.last.file_sha256).to eq 'abc' - expect(created_package.package_files.last.file_md5).to eq 'def' + .and change { Packages::PackageFile.count }.by(0) + .and raise_error(/File name has already been taken/) end end diff --git a/spec/services/personal_access_tokens/revoke_service_spec.rb b/spec/services/personal_access_tokens/revoke_service_spec.rb new file mode 100644 index 00000000000..5afa43cef76 --- /dev/null +++ b/spec/services/personal_access_tokens/revoke_service_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe PersonalAccessTokens::RevokeService do + shared_examples_for 'a successfully revoked token' do + it { expect(subject.success?).to be true } + it { expect(service.token.revoked?).to be true } + end + + shared_examples_for 'an unsuccessfully revoked token' do + it { expect(subject.success?).to be false } + it { expect(service.token.revoked?).to be false } + end + + describe '#execute' do + subject { service.execute } + + let(:service) { described_class.new(current_user, token: token) } + + context 'when current_user is an administrator' do + let_it_be(:current_user) { create(:admin) } + let_it_be(:token) { create(:personal_access_token) } + + it_behaves_like 'a successfully revoked token' + end + + context 'when current_user is not an administrator' do + let_it_be(:current_user) { create(:user) } + + context 'token belongs to a different user' do + let_it_be(:token) { create(:personal_access_token) } + + it_behaves_like 'an unsuccessfully revoked token' + end + + context 'token belongs to current_user' do + let_it_be(:token) { create(:personal_access_token, user: current_user) } + + it_behaves_like 'a successfully revoked token' + end + end + end +end diff --git a/spec/services/pod_logs/elasticsearch_service_spec.rb b/spec/services/pod_logs/elasticsearch_service_spec.rb index 9431e47c6f2..e233abcd96a 100644 --- a/spec/services/pod_logs/elasticsearch_service_spec.rb +++ b/spec/services/pod_logs/elasticsearch_service_spec.rb @@ -243,6 +243,7 @@ RSpec.describe ::PodLogs::ElasticsearchService do cursor: cursor } end + let(:expected_cursor) { '9999934,1572449784442' } before do diff --git a/spec/services/preview_markdown_service_spec.rb b/spec/services/preview_markdown_service_spec.rb index 2509d1300b3..53f8f5b7253 100644 --- a/spec/services/preview_markdown_service_spec.rb +++ b/spec/services/preview_markdown_service_spec.rb @@ -25,12 +25,14 @@ RSpec.describe PreviewMarkdownService do let(:merge_request) do create(:merge_request, target_project: project, source_project: project) end + let(:text) { "```suggestion\nfoo\n```" } let(:params) do suggestion_params.merge(text: text, target_type: 'MergeRequest', target_id: merge_request.iid) end + let(:service) { described_class.new(project, user, params) } context 'when preview markdown param is present' do @@ -106,6 +108,7 @@ RSpec.describe PreviewMarkdownService do target_id: issue.id } end + let(:service) { described_class.new(project, user, params) } it 'removes quick actions from text' do @@ -128,6 +131,7 @@ RSpec.describe PreviewMarkdownService do target_type: 'MergeRequest' } end + let(:service) { described_class.new(project, user, params) } it 'removes quick actions from text' do @@ -153,6 +157,7 @@ RSpec.describe PreviewMarkdownService do target_id: commit.id } end + let(:service) { described_class.new(project, user, params) } it 'removes quick actions from text' do diff --git a/spec/services/product_analytics/build_graph_service_spec.rb b/spec/services/product_analytics/build_graph_service_spec.rb new file mode 100644 index 00000000000..933a2bfee92 --- /dev/null +++ b/spec/services/product_analytics/build_graph_service_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ProductAnalytics::BuildGraphService do + let_it_be(:project) { create(:project) } + + let_it_be(:events) do + [ + create(:product_analytics_event, project: project, platform: 'web'), + create(:product_analytics_event, project: project, platform: 'web'), + create(:product_analytics_event, project: project, platform: 'app'), + create(:product_analytics_event, project: project, platform: 'mobile'), + create(:product_analytics_event, project: project, platform: 'mobile', collector_tstamp: Time.zone.now - 60.days) + ] + end + + let(:params) { { graph: 'platform', timerange: 5 } } + + subject { described_class.new(project, params).execute } + + it 'returns a valid graph hash' do + expect(subject[:id]).to eq(:platform) + expect(subject[:keys]).to eq(%w(app mobile web)) + expect(subject[:values]).to eq([1, 1, 2]) + end +end diff --git a/spec/services/projects/alerting/notify_service_spec.rb b/spec/services/projects/alerting/notify_service_spec.rb index 123b0bad2a8..3e74a15c3c0 100644 --- a/spec/services/projects/alerting/notify_service_spec.rb +++ b/spec/services/projects/alerting/notify_service_spec.rb @@ -30,21 +30,6 @@ RSpec.describe Projects::Alerting::NotifyService do end end - shared_examples 'sends notification email' do - let(:notification_service) { spy } - - it 'sends a notification for firing alerts only' do - expect(NotificationService) - .to receive(:new) - .and_return(notification_service) - - expect(notification_service) - .to receive_message_chain(:async, :prometheus_alerts_fired) - - expect(subject).to be_success - end - end - shared_examples 'does not process incident issues' do it 'does not process issues' do expect(IncidentManagement::ProcessAlertWorker) @@ -81,6 +66,7 @@ RSpec.describe Projects::Alerting::NotifyService do fingerprint: fingerprint }.with_indifferent_access end + let(:payload) { ActionController::Parameters.new(payload_raw).permit! } subject { service.execute(token) } @@ -234,7 +220,7 @@ RSpec.describe Projects::Alerting::NotifyService do context 'with emails turned on' do let(:email_enabled) { true } - it_behaves_like 'sends notification email' + it_behaves_like 'Alert Notification Service sends notification email' end end diff --git a/spec/services/projects/cleanup_service_spec.rb b/spec/services/projects/cleanup_service_spec.rb index 528f31456a9..7c28b729e84 100644 --- a/spec/services/projects/cleanup_service_spec.rb +++ b/spec/services/projects/cleanup_service_spec.rb @@ -19,7 +19,7 @@ RSpec.describe Projects::CleanupService do it 'runs garbage collection on the repository' do expect_next_instance_of(GitGarbageCollectWorker) do |worker| - expect(worker).to receive(:perform) + expect(worker).to receive(:perform).with(project.id, :gc, "project_cleanup:gc:#{project.id}") end service.execute diff --git a/spec/services/projects/container_repository/delete_tags_service_spec.rb b/spec/services/projects/container_repository/delete_tags_service_spec.rb index 3d065deefdf..3014ccbd7ba 100644 --- a/spec/services/projects/container_repository/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb @@ -3,21 +3,15 @@ require 'spec_helper' RSpec.describe Projects::ContainerRepository::DeleteTagsService do - let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, :private) } - let_it_be(:repository) { create(:container_repository, :root, project: project) } + include_context 'container repository delete tags service shared context' - let(:params) { { tags: tags } } let(:service) { described_class.new(project, user, params) } - before do - stub_container_registry_config(enabled: true, - api_url: 'http://registry.gitlab', - host_port: 'registry.gitlab') - - stub_container_registry_tags( - repository: repository.path, - tags: %w(latest A Ba Bb C D E)) + let_it_be(:available_service_classes) do + [ + ::Projects::ContainerRepository::Gitlab::DeleteTagsService, + ::Projects::ContainerRepository::ThirdParty::DeleteTagsService + ] end RSpec.shared_examples 'logging a success response' do @@ -45,8 +39,54 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do end end + RSpec.shared_examples 'calling the correct delete tags service' do |expected_service_class| + let(:service_response) { { status: :success, deleted: tags } } + let(:excluded_service_class) { available_service_classes.excluding(expected_service_class).first } + + before do + service_double = double + expect(expected_service_class).to receive(:new).with(repository, tags).and_return(service_double) + expect(excluded_service_class).not_to receive(:new) + expect(service_double).to receive(:execute).and_return(service_response) + end + + it { is_expected.to include(status: :success) } + + it_behaves_like 'logging a success response' + + context 'with an error service response' do + let(:service_response) { { status: :error, message: 'could not delete tags' } } + + it { is_expected.to include(status: :error) } + + it_behaves_like 'logging an error response' + end + end + + RSpec.shared_examples 'handling invalid params' do + context 'with invalid params' do + before do + expect(::Projects::ContainerRepository::Gitlab::DeleteTagsService).not_to receive(:new) + expect(::Projects::ContainerRepository::ThirdParty::DeleteTagsService).not_to receive(:new) + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) + end + + context 'when no params are specified' do + let_it_be(:params) { {} } + + it { is_expected.to include(status: :error) } + end + + context 'with empty tags' do + let_it_be(:tags) { [] } + + it { is_expected.to include(status: :error) } + end + end + end + describe '#execute' do - let(:tags) { %w[A] } + let(:tags) { %w[A Ba] } subject { service.execute(repository) } @@ -61,247 +101,58 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do context 'when the registry supports fast delete' do context 'and the feature is enabled' do - let_it_be(:project) { create(:project, :private) } - let_it_be(:repository) { create(:container_repository, :root, project: project) } - before do allow(repository.client).to receive(:supports_tag_delete?).and_return(true) end - context 'with tags to delete' do - let_it_be(:tags) { %w[A Ba] } - - it 'deletes the tags by name' do - stub_delete_reference_request('A') - stub_delete_reference_request('Ba') - - expect_delete_tag_by_name('A') - expect_delete_tag_by_name('Ba') - - is_expected.to include(status: :success) - end - - it 'succeeds when tag delete returns 404' do - stub_delete_reference_request('A') - stub_delete_reference_request('Ba', 404) - - is_expected.to include(status: :success) - end - - it_behaves_like 'logging a success response' do - before do - stub_delete_reference_request('A') - stub_delete_reference_request('Ba') - end - end - - context 'with failures' do - context 'when the delete request fails' do - before do - stub_delete_reference_request('A', 500) - stub_delete_reference_request('Ba', 500) - end - - it { is_expected.to include(status: :error) } - - it_behaves_like 'logging an error response' - end - end - end - - context 'when no params are specified' do - let_it_be(:params) { {} } + it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::Gitlab::DeleteTagsService - it 'does not remove anything' do - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) + it_behaves_like 'handling invalid params' - is_expected.to include(status: :error) + context 'with the real service' do + before do + stub_delete_reference_requests(tags) + expect_delete_tag_by_names(tags) end - end - context 'with empty tags' do - let_it_be(:tags) { [] } + it { is_expected.to include(status: :success) } - it 'does not remove anything' do - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) - - is_expected.to include(status: :error) - end + it_behaves_like 'logging a success response' end end context 'and the feature is disabled' do - let_it_be(:tags) { %w[A Ba] } - before do stub_feature_flags(container_registry_fast_tag_delete: false) - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') - stub_put_manifest_request('A') - stub_put_manifest_request('Ba') end - it 'fallbacks to slow delete' do - expect(service).not_to receive(:fast_delete) - expect(service).to receive(:slow_delete).with(repository, tags).and_call_original - - expect_delete_tag_by_digest('sha256:dummy') + it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::ThirdParty::DeleteTagsService - subject - end + it_behaves_like 'handling invalid params' - it_behaves_like 'logging a success response' do + context 'with the real service' do before do - allow(service).to receive(:slow_delete).and_call_original + stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + tags.each { |tag| stub_put_manifest_request(tag) } expect_delete_tag_by_digest('sha256:dummy') end + + it { is_expected.to include(status: :success) } + + it_behaves_like 'logging a success response' end end end context 'when the registry does not support fast delete' do - let_it_be(:project) { create(:project, :private) } - let_it_be(:repository) { create(:container_repository, :root, project: project) } - before do - stub_tag_digest('latest', 'sha256:configA') - stub_tag_digest('A', 'sha256:configA') - stub_tag_digest('Ba', 'sha256:configB') - allow(repository.client).to receive(:supports_tag_delete?).and_return(false) end - context 'when no params are specified' do - let_it_be(:params) { {} } - - it 'does not remove anything' do - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_digest) + it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::ThirdParty::DeleteTagsService - is_expected.to include(status: :error) - end - end - - context 'with empty tags' do - let_it_be(:tags) { [] } - - it 'does not remove anything' do - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_digest) - - is_expected.to include(status: :error) - end - end - - context 'with tags to delete' do - let_it_be(:tags) { %w[A Ba] } - - it 'deletes the tags using a dummy image' do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') - - stub_put_manifest_request('A') - stub_put_manifest_request('Ba') - - expect_delete_tag_by_digest('sha256:dummy') - - is_expected.to include(status: :success) - end - - it 'succeeds when tag delete returns 404' do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') - - stub_put_manifest_request('A') - stub_put_manifest_request('Ba') - - stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:dummy") - .to_return(status: 404, body: "", headers: {}) - - is_expected.to include(status: :success) - end - - it_behaves_like 'logging a success response' do - before do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') - stub_put_manifest_request('A') - stub_put_manifest_request('Ba') - expect_delete_tag_by_digest('sha256:dummy') - end - end - - context 'with failures' do - context 'when the dummy manifest generation fails' do - before do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3', success: false) - end - - it { is_expected.to include(status: :error) } - - it_behaves_like 'logging an error response', message: 'could not generate manifest' - end - - context 'when updating the tags fails' do - before do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') - - stub_put_manifest_request('A', 500) - stub_put_manifest_request('Ba', 500) - - stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3") - .to_return(status: 200, body: "", headers: {}) - end - - it { is_expected.to include(status: :error) } - it_behaves_like 'logging an error response' - end - end - end + it_behaves_like 'handling invalid params' end end end - - private - - def stub_delete_reference_request(tag, status = 200) - stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/#{tag}") - .to_return(status: status, body: '') - end - - def stub_put_manifest_request(tag, status = 200, headers = { 'docker-content-digest' => 'sha256:dummy' }) - stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/#{tag}") - .to_return(status: status, body: '', headers: headers) - end - - def stub_tag_digest(tag, digest) - stub_request(:head, "http://registry.gitlab/v2/#{repository.path}/manifests/#{tag}") - .to_return(status: 200, body: "", headers: { 'docker-content-digest' => digest }) - end - - def stub_digest_config(digest, created_at) - allow_any_instance_of(ContainerRegistry::Client) - .to receive(:blob) - .with(repository.path, digest, nil) do - { 'created' => created_at.to_datetime.rfc3339 }.to_json if created_at - end - end - - def stub_upload(content, digest, success: true) - expect_any_instance_of(ContainerRegistry::Client) - .to receive(:upload_blob) - .with(repository.path, content, digest) { double(success?: success ) } - end - - def expect_delete_tag_by_digest(digest) - expect_any_instance_of(ContainerRegistry::Client) - .to receive(:delete_repository_tag_by_digest) - .with(repository.path, digest) { true } - - expect_any_instance_of(ContainerRegistry::Client) - .not_to receive(:delete_repository_tag_by_name) - end - - def expect_delete_tag_by_name(name) - expect_any_instance_of(ContainerRegistry::Client) - .to receive(:delete_repository_tag_by_name) - .with(repository.path, name) { true } - - expect_any_instance_of(ContainerRegistry::Client) - .not_to receive(:delete_repository_tag_by_digest) - end end diff --git a/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb b/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb new file mode 100644 index 00000000000..68c232e5d83 --- /dev/null +++ b/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do + include_context 'container repository delete tags service shared context' + + let(:service) { described_class.new(repository, tags) } + + describe '#execute' do + let(:tags) { %w[A Ba] } + + subject { service.execute } + + context 'with tags to delete' do + it 'deletes the tags by name' do + stub_delete_reference_requests(tags) + expect_delete_tag_by_names(tags) + + is_expected.to eq(status: :success, deleted: tags) + end + + it 'succeeds when tag delete returns 404' do + stub_delete_reference_requests('A' => 200, 'Ba' => 404) + + is_expected.to eq(status: :success, deleted: tags) + end + + it 'succeeds when a tag delete returns 500' do + stub_delete_reference_requests('A' => 200, 'Ba' => 500) + + is_expected.to eq(status: :success, deleted: ['A']) + end + + context 'with failures' do + context 'when the delete request fails' do + before do + stub_delete_reference_requests('A' => 500, 'Ba' => 500) + end + + it { is_expected.to eq(status: :error, message: 'could not delete tags') } + end + end + end + + context 'with empty tags' do + let_it_be(:tags) { [] } + + it 'does not remove anything' do + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) + + is_expected.to eq(status: :success, deleted: []) + end + end + end +end diff --git a/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb b/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb new file mode 100644 index 00000000000..7fc963949eb --- /dev/null +++ b/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::ContainerRepository::ThirdParty::DeleteTagsService do + include_context 'container repository delete tags service shared context' + + let(:service) { described_class.new(repository, tags) } + + describe '#execute' do + let(:tags) { %w[A Ba] } + + subject { service.execute } + + context 'with tags to delete' do + it 'deletes the tags by name' do + stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + + tags.each { |tag| stub_put_manifest_request(tag) } + + expect_delete_tag_by_digest('sha256:dummy') + + is_expected.to eq(status: :success, deleted: tags) + end + + it 'succeeds when tag delete returns 404' do + stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + + stub_put_manifest_request('A') + stub_put_manifest_request('Ba') + + stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:dummy") + .to_return(status: 404, body: '', headers: {}) + + is_expected.to eq(status: :success, deleted: tags) + end + + context 'with failures' do + context 'when the dummy manifest generation fails' do + before do + stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3', success: false) + end + + it { is_expected.to eq(status: :error, message: 'could not generate manifest') } + end + + context 'when updating tags fails' do + before do + stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + + stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3") + .to_return(status: 200, body: '', headers: {}) + end + + context 'all tag updates fail' do + before do + stub_put_manifest_request('A', 500, {}) + stub_put_manifest_request('Ba', 500, {}) + end + + it { is_expected.to eq(status: :error, message: 'could not delete tags') } + end + + context 'a single tag update fails' do + before do + stub_put_manifest_request('A') + stub_put_manifest_request('Ba', 500, {}) + + stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:dummy") + .to_return(status: 404, body: '', headers: {}) + end + + it { is_expected.to eq(status: :success, deleted: ['A']) } + end + end + end + end + + context 'with empty tags' do + let_it_be(:tags) { [] } + + it 'does not remove anything' do + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) + + is_expected.to eq(status: :success, deleted: []) + end + end + end +end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 9eb7cacbbcb..e1df8700795 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -48,6 +48,12 @@ RSpec.describe Projects::CreateService, '#execute' do expect(project.project_setting).to be_new_record end + + it_behaves_like 'storing arguments in the application context' do + let(:expected_params) { { project: subject.full_path, related_class: described_class.to_s } } + + subject { create_project(user, opts) } + end end context "admin creates project with other user's namespace_id" do diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index c49aa42b147..925c2ff5d88 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -105,6 +105,7 @@ RSpec.describe Projects::ForkService do group.add_owner(@to_user) group end + let(:to_project) { fork_project(from_forked_project, @to_user, namespace: other_namespace) } it 'sets the root of the network to the root project' do @@ -439,37 +440,71 @@ RSpec.describe Projects::ForkService do end describe '#valid_fork_target?' do - subject { described_class.new(project, user, params).valid_fork_target? } - let(:project) { Project.new } let(:params) { {} } - context 'when current user is an admin' do - let(:user) { build(:user, :admin) } + context 'when target is not passed' do + subject { described_class.new(project, user, params).valid_fork_target? } - it { is_expected.to be_truthy } - end + context 'when current user is an admin' do + let(:user) { build(:user, :admin) } - context 'when current_user is not an admin' do - let(:user) { create(:user) } + it { is_expected.to be_truthy } + end - let(:finder_mock) { instance_double('ForkTargetsFinder', execute: [user.namespace]) } - let(:project) { create(:project) } + context 'when current_user is not an admin' do + let(:user) { create(:user) } - before do - allow(ForkTargetsFinder).to receive(:new).with(project, user).and_return(finder_mock) + let(:finder_mock) { instance_double('ForkTargetsFinder', execute: [user.namespace]) } + let(:project) { create(:project) } + + before do + allow(ForkTargetsFinder).to receive(:new).with(project, user).and_return(finder_mock) + end + + context 'when target namespace is in valid fork targets' do + let(:params) { { namespace: user.namespace } } + + it { is_expected.to be_truthy } + end + + context 'when target namespace is not in valid fork targets' do + let(:params) { { namespace: create(:group) } } + + it { is_expected.to be_falsey } + end end + end + + context 'when target is passed' do + let(:target) { create(:group) } - context 'when target namespace is in valid fork targets' do - let(:params) { { namespace: user.namespace } } + subject { described_class.new(project, user, params).valid_fork_target?(target) } + + context 'when current user is an admin' do + let(:user) { build(:user, :admin) } it { is_expected.to be_truthy } end - context 'when target namespace is not in valid fork targets' do - let(:params) { { namespace: create(:group) } } + context 'when current user is not an admin' do + let(:user) { create(:user) } - it { is_expected.to be_falsey } + before do + allow(ForkTargetsFinder).to receive(:new).with(project, user).and_return(finder_mock) + end + + context 'when target namespace is in valid fork targets' do + let(:finder_mock) { instance_double('ForkTargetsFinder', execute: [target]) } + + it { is_expected.to be_truthy } + end + + context 'when target namespace is not in valid fork targets' do + let(:finder_mock) { instance_double('ForkTargetsFinder', execute: [create(:group)]) } + + it { is_expected.to be_falsey } + end end end end diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index 3cfc9844d65..8a538bc67ed 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -384,6 +384,7 @@ RSpec.describe Projects::Operations::UpdateService do manual_configuration: "0" }) end + let(:params) do { prometheus_integration_attributes: { diff --git a/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/spec/services/projects/prometheus/alerts/notify_service_spec.rb index aae257e3e3a..efe8e8b9243 100644 --- a/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -21,38 +21,6 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do project.clear_memoization(:licensed_feature_available) end - shared_examples 'sends notification email' do - let(:notification_service) { spy } - - it 'sends a notification for firing alerts only' do - expect(NotificationService) - .to receive(:new) - .and_return(notification_service) - - expect(notification_service) - .to receive_message_chain(:async, :prometheus_alerts_fired) - - expect(subject).to be_success - end - end - - shared_examples 'notifies alerts' do - it_behaves_like 'sends notification email' - end - - shared_examples 'no notifications' do |http_status:| - let(:notification_service) { spy } - let(:create_events_service) { spy } - - it 'does not notify' do - expect(notification_service).not_to receive(:async) - expect(create_events_service).not_to receive(:execute) - - expect(subject).to be_error - expect(subject.http_status).to eq(http_status) - end - end - context 'with valid payload' do let_it_be(:alert_firing) { create(:prometheus_alert, project: project) } let_it_be(:alert_resolved) { create(:prometheus_alert, project: project) } @@ -89,11 +57,11 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do context 'without token' do let(:token_input) { nil } - it_behaves_like 'notifies alerts' + it_behaves_like 'Alert Notification Service sends notification email' end context 'with token' do - it_behaves_like 'no notifications', http_status: :unauthorized + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :unauthorized end end @@ -125,9 +93,9 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do case result = params[:result] when :success - it_behaves_like 'notifies alerts' + it_behaves_like 'Alert Notification Service sends notification email' when :failure - it_behaves_like 'no notifications', http_status: :unauthorized + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :unauthorized else raise "invalid result: #{result.inspect}" end @@ -137,7 +105,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do context 'without project specific cluster' do let!(:cluster) { create(:cluster, enabled: true) } - it_behaves_like 'no notifications', http_status: :unauthorized + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :unauthorized end context 'with manual prometheus installation' do @@ -166,9 +134,9 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do case result = params[:result] when :success - it_behaves_like 'notifies alerts' + it_behaves_like 'Alert Notification Service sends notification email' when :failure - it_behaves_like 'no notifications', http_status: :unauthorized + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :unauthorized else raise "invalid result: #{result.inspect}" end @@ -199,9 +167,9 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do case result = params[:result] when :success - it_behaves_like 'notifies alerts' + it_behaves_like 'Alert Notification Service sends notification email' when :failure - it_behaves_like 'no notifications', http_status: :unauthorized + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :unauthorized else raise "invalid result: #{result.inspect}" end @@ -226,7 +194,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do end context 'when incident_management_setting.send_email is true' do - it_behaves_like 'notifies alerts' + it_behaves_like 'Alert Notification Service sends notification email' end context 'incident_management_setting.send_email is false' do @@ -278,7 +246,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do .and_return(false) end - it_behaves_like 'no notifications', http_status: :unprocessable_entity + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :unprocessable_entity end context 'when the payload is too big' do @@ -289,7 +257,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do allow(Gitlab::Utils::DeepSize).to receive(:new).and_return(deep_size_object) end - it_behaves_like 'no notifications', http_status: :bad_request + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :bad_request it 'does not process Prometheus alerts' do expect(AlertManagement::ProcessPrometheusAlertService) diff --git a/spec/services/projects/propagate_service_template_spec.rb b/spec/services/projects/propagate_service_template_spec.rb index 266bf2cc213..df69e5a29fb 100644 --- a/spec/services/projects/propagate_service_template_spec.rb +++ b/spec/services/projects/propagate_service_template_spec.rb @@ -20,7 +20,7 @@ RSpec.describe Projects::PropagateServiceTemplate do end let!(:project) { create(:project) } - let(:excluded_attributes) { %w[id project_id template created_at updated_at title description] } + let(:excluded_attributes) { %w[id project_id template created_at updated_at default] } it 'creates services for projects' do expect(project.pushover_service).to be_nil @@ -120,7 +120,7 @@ RSpec.describe Projects::PropagateServiceTemplate do describe 'external tracker' do it 'updates the project external tracker' do - service_template.update!(category: 'issue_tracker', default: false) + service_template.update!(category: 'issue_tracker') expect { described_class.propagate(service_template) } .to change { project.reload.has_external_issue_tracker }.to(true) diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 72426a6f6ec..3362b333c6e 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -11,6 +11,39 @@ RSpec.describe Projects::TransferService do subject(:execute_transfer) { described_class.new(project, user).execute(group) } + context 'with npm packages' do + before do + group.add_owner(user) + end + + subject(:transfer_service) { described_class.new(project, user) } + + let!(:package) { create(:npm_package, project: project) } + + context 'with a root namespace change' do + it 'does not allow the transfer' do + expect(transfer_service.execute(group)).to be false + expect(project.errors[:new_namespace]).to include("Root namespace can't be updated if project has NPM packages") + end + end + + context 'without a root namespace change' do + let(:root) { create(:group) } + let(:group) { create(:group, parent: root) } + let(:other_group) { create(:group, parent: root) } + let(:project) { create(:project, :repository, namespace: group) } + + before do + other_group.add_owner(user) + end + + it 'does allow the transfer' do + expect(transfer_service.execute(other_group)).to be true + expect(project.errors[:new_namespace]).to be_empty + end + end + end + context 'namespace -> namespace' do before do allow_next_instance_of(Gitlab::UploadsTransfer) do |service| diff --git a/spec/services/projects/update_pages_configuration_service_spec.rb b/spec/services/projects/update_pages_configuration_service_spec.rb index c4c9fc779fa..9f7ebd40df6 100644 --- a/spec/services/projects/update_pages_configuration_service_spec.rb +++ b/spec/services/projects/update_pages_configuration_service_spec.rb @@ -3,41 +3,75 @@ require 'spec_helper' RSpec.describe Projects::UpdatePagesConfigurationService do - let(:project) { create(:project) } let(:service) { described_class.new(project) } - describe "#update" do - let(:file) { Tempfile.new('pages-test') } - + describe "#execute" do subject { service.execute } - after do - file.close - file.unlink - end + context 'when pages are deployed' do + let_it_be(:project) do + create(:project).tap(&:mark_pages_as_deployed) + end - before do - allow(service).to receive(:pages_config_file).and_return(file.path) - end + let(:file) { Tempfile.new('pages-test') } + + before do + allow(service).to receive(:pages_config_file).and_return(file.path) + end + + after do + file.close + file.unlink + end + + context 'when configuration changes' do + it 'updates the config and reloads the daemon' do + allow(service).to receive(:update_file).and_call_original - context 'when configuration changes' do - it 'updates the .update file' do - expect(service).to receive(:reload_daemon).and_call_original + expect(service).to receive(:update_file).with(file.path, an_instance_of(String)) + .and_call_original + expect(service).to receive(:reload_daemon).and_call_original - expect(subject).to include(status: :success, reload: true) + expect(subject).to include(status: :success) + end + end + + context 'when configuration does not change' do + before do + # we set the configuration + service.execute + end + + it 'does not update the .update file' do + expect(service).not_to receive(:reload_daemon) + + expect(subject).to include(status: :success) + end + end + + context 'when an error occurs' do + it 'returns an error object' do + e = StandardError.new("Failure") + allow(service).to receive(:reload_daemon).and_raise(e) + + expect(subject).to eq(status: :error, message: "Failure", exception: e) + end end end - context 'when configuration does not change' do - before do - # we set the configuration - service.execute + context 'when pages are not deployed' do + let_it_be(:project) do + create(:project).tap(&:mark_pages_as_not_deployed) + end + + it 'returns successfully' do + expect(subject).to eq(status: :success) end - it 'does not update the .update file' do - expect(service).not_to receive(:reload_daemon) + it 'does not update the config' do + expect(service).not_to receive(:update_file) - expect(subject).to include(status: :success, reload: false) + subject end end end diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 2e02cb56668..374ce4f4ce2 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -53,7 +53,7 @@ RSpec.describe Projects::UpdatePagesService do # Check that all expected files are extracted %w[index.html zero .hidden/file].each do |filename| - expect(File.exist?(File.join(project.public_pages_path, filename))).to be_truthy + expect(File.exist?(File.join(project.pages_path, 'public', filename))).to be_truthy end end @@ -65,15 +65,17 @@ RSpec.describe Projects::UpdatePagesService do it 'removes pages after destroy' do expect(PagesWorker).to receive(:perform_in) expect(project.pages_deployed?).to be_falsey + expect(Dir.exist?(File.join(project.pages_path))).to be_falsey expect(execute).to eq(:success) expect(project.pages_metadatum).to be_deployed expect(project.pages_deployed?).to be_truthy + expect(Dir.exist?(File.join(project.pages_path))).to be_truthy project.destroy - expect(project.pages_deployed?).to be_falsey + expect(Dir.exist?(File.join(project.pages_path))).to be_falsey expect(ProjectPagesMetadatum.find_by_project_id(project)).to be_nil end @@ -160,19 +162,9 @@ RSpec.describe Projects::UpdatePagesService do end context 'with background jobs running', :sidekiq_inline do - where(:ci_atomic_processing) do - [true, false] - end - - with_them do - before do - stub_feature_flags(ci_atomic_processing: ci_atomic_processing) - end - - it 'succeeds' do - expect(project.pages_deployed?).to be_falsey - expect(execute).to eq(:success) - end + it 'succeeds' do + expect(project.pages_deployed?).to be_falsey + expect(execute).to eq(:success) end end end diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index f0a8074f46c..09244db8010 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -10,10 +10,6 @@ RSpec.describe Projects::UpdateRemoteMirrorService do subject(:service) { described_class.new(project, project.creator) } - before do - stub_feature_flags(gitaly_ruby_remote_branches_ls_remote: false) - end - describe '#execute' do subject(:execute!) { service.execute(remote_mirror, 0) } @@ -26,17 +22,14 @@ RSpec.describe Projects::UpdateRemoteMirrorService do end it 'ensures the remote exists' do - stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) - expect(remote_mirror).to receive(:ensure_remote!) execute! end - it 'fetches the remote repository' do - expect(project.repository) - .to receive(:fetch_remote) - .with(remote_mirror.remote_name, no_tags: true, ssh_auth: remote_mirror) + it 'does not fetch the remote repository' do + # See https://gitlab.com/gitlab-org/gitaly/-/issues/2670 + expect(project.repository).not_to receive(:fetch_remote) execute! end @@ -48,8 +41,6 @@ RSpec.describe Projects::UpdateRemoteMirrorService do end it 'marks the mirror as successfully finished' do - stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) - result = execute! expect(result[:status]).to eq(:success) @@ -57,7 +48,7 @@ RSpec.describe Projects::UpdateRemoteMirrorService do end it 'marks the mirror as failed and raises the error when an unexpected error occurs' do - allow(project.repository).to receive(:fetch_remote).and_raise('Badly broken') + allow(remote_mirror).to receive(:update_repository).and_raise('Badly broken') expect { execute! }.to raise_error(/Badly broken/) @@ -67,33 +58,30 @@ RSpec.describe Projects::UpdateRemoteMirrorService do context 'when the update fails because of a `Gitlab::Git::CommandError`' do before do - allow(project.repository).to receive(:fetch_remote).and_raise(Gitlab::Git::CommandError.new('fetch failed')) + allow(remote_mirror).to receive(:update_repository) + .and_raise(Gitlab::Git::CommandError.new('update failed')) end it 'wraps `Gitlab::Git::CommandError`s in a service error' do - expect(execute!).to eq(status: :error, message: 'fetch failed') + expect(execute!).to eq(status: :error, message: 'update failed') end it 'marks the mirror as to be retried' do execute! expect(remote_mirror).to be_to_retry - expect(remote_mirror.last_error).to include('fetch failed') + expect(remote_mirror.last_error).to include('update failed') end it "marks the mirror as failed after #{described_class::MAX_TRIES} tries" do service.execute(remote_mirror, described_class::MAX_TRIES) expect(remote_mirror).to be_failed - expect(remote_mirror.last_error).to include('fetch failed') + expect(remote_mirror.last_error).to include('update failed') end end context 'when there are divergent refs' do - before do - stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) - end - it 'marks the mirror as failed and sets an error message' do response = double(divergent_refs: %w[refs/heads/master refs/heads/develop]) expect(remote_mirror).to receive(:update_repository).and_return(response) @@ -106,37 +94,5 @@ RSpec.describe Projects::UpdateRemoteMirrorService do expect(remote_mirror.last_error).to include("refs/heads/develop") end end - - # https://gitlab.com/gitlab-org/gitaly/-/issues/2670 - context 'when `gitaly_ruby_remote_branches_ls_remote` is enabled' do - before do - stub_feature_flags(gitaly_ruby_remote_branches_ls_remote: true) - end - - it 'does not perform a fetch' do - expect(project.repository).not_to receive(:fetch_remote) - - execute! - end - end - end - - def stub_fetch_remote(project, remote_name:, ssh_auth:) - allow(project.repository) - .to receive(:fetch_remote) - .with(remote_name, no_tags: true, ssh_auth: ssh_auth) { fetch_remote(project.repository, remote_name) } - end - - def fetch_remote(repository, remote_name) - local_branch_names(repository).each do |branch| - commit = repository.commit(branch) - repository.write_ref("refs/remotes/#{remote_name}/#{branch}", commit.id) if commit - end - end - - def local_branch_names(repository) - branch_names = repository.branches.map(&:name) - # we want the protected branch to be pushed first - branch_names.unshift(branch_names.delete('master')) end end diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index 57e02c26b71..0fcd14f3bc9 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -21,6 +21,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } let!(:checksum) { project.repository.checksum } let(:project_repository_double) { double(:repository) } + let(:original_project_repository_double) { double(:repository) } before do allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original @@ -29,6 +30,9 @@ RSpec.describe Projects::UpdateRepositoryStorageService do allow(Gitlab::Git::Repository).to receive(:new) .with('test_second_storage', project.repository.raw.relative_path, project.repository.gl_repository, project.repository.full_path) .and_return(project_repository_double) + allow(Gitlab::Git::Repository).to receive(:new) + .with('default', project.repository.raw.relative_path, nil, nil) + .and_return(original_project_repository_double) end context 'when the move succeeds' do @@ -41,8 +45,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do .with(project.repository.raw) expect(project_repository_double).to receive(:checksum) .and_return(checksum) - expect(GitlabShellWorker).to receive(:perform_async).with(:mv_repository, 'default', anything, anything) - .and_call_original + expect(original_project_repository_double).to receive(:remove) result = subject.execute project.reload @@ -74,13 +77,29 @@ RSpec.describe Projects::UpdateRepositoryStorageService do expect(project_repository_double).to receive(:replicate) .with(project.repository.raw) .and_raise(Gitlab::Git::CommandError) - expect(GitlabShellWorker).not_to receive(:perform_async) result = subject.execute expect(result).to be_error expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('default') + expect(repository_storage_move).to be_failed + end + end + + context 'when the cleanup fails' do + it 'sets the correct state' do + expect(project_repository_double).to receive(:replicate) + .with(project.repository.raw) + expect(project_repository_double).to receive(:checksum) + .and_return(checksum) + expect(original_project_repository_double).to receive(:remove) + .and_raise(Gitlab::Git::CommandError) + + result = subject.execute + + expect(result).to be_error + expect(repository_storage_move).to be_cleanup_failed end end @@ -93,7 +112,6 @@ RSpec.describe Projects::UpdateRepositoryStorageService do .with(project.repository.raw) expect(project_repository_double).to receive(:checksum) .and_return('not matching checksum') - expect(GitlabShellWorker).not_to receive(:perform_async) result = subject.execute @@ -114,6 +132,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do .with(project.repository.raw) expect(project_repository_double).to receive(:checksum) .and_return(checksum) + expect(original_project_repository_double).to receive(:remove) result = subject.execute project.reload diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 6620ee6e697..4a613f42556 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -396,6 +396,50 @@ RSpec.describe Projects::UpdateService do end end + shared_examples 'updating pages configuration' do + it 'schedules the `PagesUpdateConfigurationWorker` when pages are deployed' do + project.mark_pages_as_deployed + + expect(PagesUpdateConfigurationWorker).to receive(:perform_async).with(project.id) + + subject + end + + it "does not schedule a job when pages aren't deployed" do + project.mark_pages_as_not_deployed + + expect(PagesUpdateConfigurationWorker).not_to receive(:perform_async).with(project.id) + + subject + end + + context 'when `async_update_pages_config` is disabled' do + before do + stub_feature_flags(async_update_pages_config: false) + end + + it 'calls Projects::UpdatePagesConfigurationService when pages are deployed' do + project.mark_pages_as_deployed + + expect(Projects::UpdatePagesConfigurationService) + .to receive(:new) + .with(project) + .and_call_original + + subject + end + + it "does not update pages config when pages aren't deployed" do + project.mark_pages_as_not_deployed + + expect(Projects::UpdatePagesConfigurationService) + .not_to receive(:new) + + subject + end + end + end + context 'when updating #pages_https_only', :https_pages_enabled do subject(:call_service) do update_project(project, admin, pages_https_only: false) @@ -407,14 +451,7 @@ RSpec.describe Projects::UpdateService do .to(false) end - it 'calls Projects::UpdatePagesConfigurationService' do - expect(Projects::UpdatePagesConfigurationService) - .to receive(:new) - .with(project) - .and_call_original - - call_service - end + it_behaves_like 'updating pages configuration' end context 'when updating #pages_access_level' do @@ -428,14 +465,7 @@ RSpec.describe Projects::UpdateService do .to(ProjectFeature::ENABLED) end - it 'calls Projects::UpdatePagesConfigurationService' do - expect(Projects::UpdatePagesConfigurationService) - .to receive(:new) - .with(project) - .and_call_original - - call_service - end + it_behaves_like 'updating pages configuration' end context 'when updating #emails_disabled' do diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb index 3c0698aa203..ad4696b0074 100644 --- a/spec/services/releases/create_service_spec.rb +++ b/spec/services/releases/create_service_spec.rb @@ -198,6 +198,7 @@ RSpec.describe Releases::CreateService do released_at: released_at }.compact end + let(:last_release) { project.releases.last } around do |example| diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index f22c379cd30..7dbd55a6909 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -34,6 +34,16 @@ RSpec.describe ResourceAccessTokens::CreateService do end end + shared_examples 'fails on gitlab.com' do + before do + allow(Gitlab).to receive(:com?) { true } + end + + it 'returns nil' do + expect(subject).to be nil + end + end + shared_examples 'allows creation of bot with valid params' do it { expect { subject }.to change { User.count }.by(1) } @@ -171,6 +181,7 @@ RSpec.describe ResourceAccessTokens::CreateService do it_behaves_like 'fails when user does not have the permission to create a Resource Bot' it_behaves_like 'fails when flag is disabled' + it_behaves_like 'fails on gitlab.com' context 'user with valid permission' do before_all do diff --git a/spec/services/resource_events/change_milestone_service_spec.rb b/spec/services/resource_events/change_milestone_service_spec.rb index 9c0f9420f7a..3a9dadbd40e 100644 --- a/spec/services/resource_events/change_milestone_service_spec.rb +++ b/spec/services/resource_events/change_milestone_service_spec.rb @@ -3,9 +3,15 @@ require 'spec_helper' RSpec.describe ResourceEvents::ChangeMilestoneService do + let_it_be(:timebox) { create(:milestone) } + + let(:created_at_time) { Time.utc(2019, 12, 30) } + let(:add_timebox_args) { { created_at: created_at_time, old_milestone: nil } } + let(:remove_timebox_args) { { created_at: created_at_time, old_milestone: timebox } } + [:issue, :merge_request].each do |issuable| - it_behaves_like 'a milestone events creator' do - let(:resource) { create(issuable) } + it_behaves_like 'timebox(milestone or iteration) resource events creator', ResourceMilestoneEvent do + let_it_be(:resource) { create(issuable) } end end end diff --git a/spec/services/search_service_spec.rb b/spec/services/search_service_spec.rb index 52aef73ac77..f6bb7acee57 100644 --- a/spec/services/search_service_spec.rb +++ b/spec/services/search_service_spec.rb @@ -374,6 +374,19 @@ RSpec.describe SearchService do 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 @@ -427,6 +440,12 @@ RSpec.describe SearchService do 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: 7 + end end context 'project repository blobs' do @@ -460,6 +479,10 @@ RSpec.describe SearchService do 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: 12 + end end context 'personal snippets' do @@ -471,6 +494,10 @@ RSpec.describe SearchService do 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: 3 + end end context 'commits' do diff --git a/spec/services/service_desk_settings/update_service_spec.rb b/spec/services/service_desk_settings/update_service_spec.rb index 8b920d536b4..fbef587365d 100644 --- a/spec/services/service_desk_settings/update_service_spec.rb +++ b/spec/services/service_desk_settings/update_service_spec.rb @@ -31,6 +31,17 @@ RSpec.describe ServiceDeskSettings::UpdateService do end end + context 'when project_key is an empty string' do + let(:params) { { project_key: '' } } + + it 'sets nil project_key' do + result = described_class.new(settings.project, user, params).execute + + expect(result[:status]).to eq :success + expect(settings.reload.project_key).to be_nil + end + end + context 'with invalid params' do let(:params) { { outgoing_name: 'x' * 256 } } diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index 62eef00b67f..2106a9c2045 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -15,6 +15,7 @@ RSpec.describe Snippets::CreateService do visibility_level: Gitlab::VisibilityLevel::PRIVATE } end + let(:extra_opts) { {} } let(:creator) { admin } diff --git a/spec/services/snippets/update_service_spec.rb b/spec/services/snippets/update_service_spec.rb index 66dddcc49de..638fe1948fd 100644 --- a/spec/services/snippets/update_service_spec.rb +++ b/spec/services/snippets/update_service_spec.rb @@ -15,6 +15,7 @@ RSpec.describe Snippets::UpdateService do visibility_level: visibility_level } end + let(:extra_opts) { {} } let(:options) { base_opts.merge(extra_opts) } let(:updater) { user } diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb index abb8e49ec52..8edd9406bce 100644 --- a/spec/services/spam/spam_action_service_spec.rb +++ b/spec/services/spam/spam_action_service_spec.rb @@ -13,6 +13,7 @@ RSpec.describe Spam::SpamActionService do 'HTTP_USER_AGENT' => fake_user_agent, 'HTTP_REFERRER' => fake_referrer } end + let(:request) { double(:request, env: env) } let_it_be(:project) { create(:project, :public) } diff --git a/spec/services/spam/spam_verdict_service_spec.rb b/spec/services/spam/spam_verdict_service_spec.rb index d775e1bdfb5..14b788e3a86 100644 --- a/spec/services/spam/spam_verdict_service_spec.rb +++ b/spec/services/spam/spam_verdict_service_spec.rb @@ -13,6 +13,7 @@ RSpec.describe Spam::SpamVerdictService do 'HTTP_USER_AGENT' => fake_user_agent, 'HTTP_REFERRER' => fake_referrer } end + let(:request) { double(:request, env: env) } let(:check_for_spam) { true } diff --git a/spec/services/submit_usage_ping_service_spec.rb b/spec/services/submit_usage_ping_service_spec.rb index 4885ef99c13..450af68d383 100644 --- a/spec/services/submit_usage_ping_service_spec.rb +++ b/spec/services/submit_usage_ping_service_spec.rb @@ -49,17 +49,22 @@ RSpec.describe SubmitUsagePingService do let(:with_conv_index_params) { { conv_index: score_params[:score] } } let(:without_dev_ops_score_params) { { dev_ops_score: {} } } - context 'when usage ping is disabled' do - before do - stub_application_setting(usage_ping_enabled: false) - end + shared_examples 'does not run' do + it do + expect(Gitlab::HTTP).not_to receive(:post) + expect(Gitlab::UsageData).not_to receive(:data) - it 'does not run' do - expect(HTTParty).not_to receive(:post) + subject.execute + end + end - result = subject.execute + shared_examples 'does not send a blank usage ping payload' do + it do + expect(Gitlab::HTTP).not_to receive(:post) - expect(result).to eq false + expect { subject.execute }.to raise_error(described_class::SubmissionError) do |error| + expect(error.message).to include('Usage data is blank') + end end end @@ -75,33 +80,47 @@ RSpec.describe SubmitUsagePingService do end end + context 'when usage ping is disabled' do + before do + stub_application_setting(usage_ping_enabled: false) + end + + it_behaves_like 'does not run' + end + context 'when usage ping is enabled' do before do stub_usage_data_connections stub_application_setting(usage_ping_enabled: true) end + context 'and user requires usage stats consent' do + before do + allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: true)) + end + + it_behaves_like 'does not run' + end + it 'sends a POST request' do - response = stub_response(without_dev_ops_score_params) + response = stub_response(body: without_dev_ops_score_params) subject.execute expect(response).to have_been_requested end - it 'refreshes usage data statistics before submitting' do - stub_response(without_dev_ops_score_params) + it 'forces a refresh of usage data statistics before submitting' do + stub_response(body: without_dev_ops_score_params) - expect(Gitlab::UsageData).to receive(:to_json) - .with(force_refresh: true) - .and_call_original + expect(Gitlab::UsageData).to receive(:data).with(force_refresh: true).and_call_original subject.execute end context 'when conv_index data is passed' do before do - stub_response(with_conv_index_params) + stub_response(body: with_conv_index_params) end it_behaves_like 'saves DevOps score data from the response' @@ -109,18 +128,84 @@ RSpec.describe SubmitUsagePingService do context 'when DevOps score data is passed' do before do - stub_response(with_dev_ops_score_params) + stub_response(body: with_dev_ops_score_params) end it_behaves_like 'saves DevOps score data from the response' end + + context 'with save_raw_usage_data feature enabled' do + before do + stub_response(body: with_dev_ops_score_params) + stub_feature_flags(save_raw_usage_data: true) + end + + it 'creates a raw_usage_data record' do + expect { subject.execute }.to change(RawUsageData, :count).by(1) + end + + it 'saves the correct payload' do + recorded_at = Time.current + usage_data = { uuid: 'uuid', recorded_at: recorded_at } + + expect(Gitlab::UsageData).to receive(:data).with(force_refresh: true).and_return(usage_data) + + subject.execute + + raw_usage_data = RawUsageData.find_by(recorded_at: recorded_at) + + expect(raw_usage_data.recorded_at).to be_like_time(recorded_at) + expect(raw_usage_data.payload.to_json).to eq(usage_data.to_json) + end + end + + context 'with save_raw_usage_data feature disabled' do + before do + stub_response(body: with_dev_ops_score_params) + end + + it 'does not create a raw_usage_data record' do + stub_feature_flags(save_raw_usage_data: false) + + expect { subject.execute }.to change(RawUsageData, :count).by(0) + end + end + + context 'and usage ping response has unsuccessful status' do + before do + stub_response(body: nil, status: 504) + end + + it 'raises an exception' do + expect { subject.execute }.to raise_error(described_class::SubmissionError) do |error| + expect(error.message).to include('Unsuccessful response code: 504') + end + end + end + + context 'and usage data is empty string' do + before do + allow(Gitlab::UsageData).to receive(:data).and_return({}) + end + + it_behaves_like 'does not send a blank usage ping payload' + end + + context 'and usage data is nil' do + before do + allow(Gitlab::UsageData).to receive(:data).and_return(nil) + end + + it_behaves_like 'does not send a blank usage ping payload' + end end - def stub_response(body) - stub_full_request('https://version.gitlab.com/usage_data', method: :post) + def stub_response(body:, status: 201) + stub_full_request(SubmitUsagePingService::STAGING_URL, method: :post) .to_return( headers: { 'Content-Type' => 'application/json' }, - body: body.to_json + body: body.to_json, + status: status ) end end diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb index aa9caf35987..d8ade0fbbda 100644 --- a/spec/services/suggestions/apply_service_spec.rb +++ b/spec/services/suggestions/apply_service_spec.rb @@ -609,40 +609,16 @@ RSpec.describe Suggestions::ApplyService do end end - context 'suggestion is eligible to be outdated' do - it 'returns error message' do - expect(suggestion).to receive(:outdated?) { true } - - result = apply_service.new(user, suggestion).execute - - expect(result).to eq(message: 'A suggestion is not applicable.', - status: :error) - end - end - - context 'note is outdated' do - before do - allow(diff_note).to receive(:active?) { false } - end + context 'suggestion is not appliable' do + let(:inapplicable_reason) { "Can't apply this suggestion." } it 'returns error message' do - result = apply_service.new(user, suggestion).execute - - expect(result).to eq(message: 'A suggestion is not applicable.', - status: :error) - end - end + expect(suggestion).to receive(:appliable?).and_return(false) + expect(suggestion).to receive(:inapplicable_reason).and_return(inapplicable_reason) - context 'suggestion was already applied' do - before do - suggestion.update!(applied: true, commit_id: 'sha') - end - - it 'returns error message' do result = apply_service.new(user, suggestion).execute - expect(result).to eq(message: 'A suggestion is not applicable.', - status: :error) + expect(result).to eq(message: inapplicable_reason, status: :error) end end diff --git a/spec/services/suggestions/create_service_spec.rb b/spec/services/suggestions/create_service_spec.rb index 54e7c5cc127..80823364fe8 100644 --- a/spec/services/suggestions/create_service_spec.rb +++ b/spec/services/suggestions/create_service_spec.rb @@ -159,6 +159,7 @@ RSpec.describe Suggestions::CreateService do ``` MARKDOWN end + let(:position) { build_position(new_line: 13) } it 'creates an appliable suggestion' do diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 58fa772fefb..969e5955609 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -699,7 +699,7 @@ RSpec.describe SystemNoteService do it 'calls AlertManagementService' do expect_next_instance_of(SystemNotes::AlertManagementService) do |service| - expect(service).to receive(:new_alert_issue).with(alert, alert.issue) + expect(service).to receive(:new_alert_issue).with(alert.issue) end described_class.new_alert_issue(alert, alert.issue, author) diff --git a/spec/services/system_notes/alert_management_service_spec.rb b/spec/services/system_notes/alert_management_service_spec.rb index 403763d5fd9..943d7f55af4 100644 --- a/spec/services/system_notes/alert_management_service_spec.rb +++ b/spec/services/system_notes/alert_management_service_spec.rb @@ -22,7 +22,7 @@ RSpec.describe ::SystemNotes::AlertManagementService do describe '#new_alert_issue' do let_it_be(:issue) { noteable.issue } - subject { described_class.new(noteable: noteable, project: project, author: author).new_alert_issue(noteable, issue) } + subject { described_class.new(noteable: noteable, project: project, author: author).new_alert_issue(issue) } it_behaves_like 'a system note' do let(:action) { 'alert_issue_added' } @@ -32,4 +32,18 @@ RSpec.describe ::SystemNotes::AlertManagementService do expect(subject.note).to eq("created issue #{issue.to_reference(project)} for this alert") end end + + describe '#closed_alert_issue' do + let_it_be(:issue) { noteable.issue } + + subject { described_class.new(noteable: noteable, project: project, author: author).closed_alert_issue(issue) } + + it_behaves_like 'a system note' do + let(:action) { 'status' } + end + + it 'has the appropriate message' do + expect(subject.note).to eq("changed the status to **Resolved** by closing issue #{issue.to_reference(project)}") + end + end end diff --git a/spec/services/system_notes/design_management_service_spec.rb b/spec/services/system_notes/design_management_service_spec.rb index 6267ad2aaad..19e1f338eb8 100644 --- a/spec/services/system_notes/design_management_service_spec.rb +++ b/spec/services/system_notes/design_management_service_spec.rb @@ -134,6 +134,7 @@ RSpec.describe SystemNotes::DesignManagementService do let(:discussion_note) do create(:diff_note_on_design, noteable: design, author: author) end + let(:action) { 'designs_discussion_added' } it_behaves_like 'a system note' do diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index b187025eb11..94d4b61933d 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -3,22 +3,23 @@ require 'spec_helper' RSpec.describe TodoService do - let(:author) { create(:user) } - let(:assignee) { create(:user) } - let(:non_member) { create(:user) } - let(:member) { create(:user) } - let(:guest) { create(:user) } - let(:admin) { create(:admin) } - let(:john_doe) { create(:user) } - let(:skipped) { create(:user) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:author) { create(:user) } + let_it_be(:assignee) { create(:user) } + let_it_be(:non_member) { create(:user) } + let_it_be(:member) { create(:user) } + let_it_be(:guest) { create(:user) } + let_it_be(:admin) { create(:admin) } + let_it_be(:john_doe) { create(:user) } + let_it_be(:skipped) { create(:user) } + let(:skip_users) { [skipped] } - let(:project) { create(:project, :repository) } let(:mentions) { 'FYI: ' + [author, assignee, john_doe, member, guest, non_member, admin, skipped].map(&:to_reference).join(' ') } let(:directly_addressed) { [author, assignee, john_doe, member, guest, non_member, admin, skipped].map(&:to_reference).join(' ') } let(:directly_addressed_and_mentioned) { member.to_reference + ", what do you think? cc: " + [guest, admin, skipped].map(&:to_reference).join(' ') } let(:service) { described_class.new } - before do + before_all do project.add_guest(guest) project.add_developer(author) project.add_developer(assignee) @@ -58,6 +59,10 @@ RSpec.describe TodoService do should_not_create_todo(user: guest, target: addressed_target_assigned, action: Todo::DIRECTLY_ADDRESSED) end + + it 'does not create a todo if already assigned' do + should_not_create_any_todo { service.send(described_method, target_assigned, author, [john_doe]) } + end end describe 'Issues' do @@ -456,7 +461,16 @@ RSpec.describe TodoService do end context 'leaving a note on a commit in a public project with private code' do - let(:project) { create(:project, :repository, :public, :repository_private) } + let_it_be(:project) { create(:project, :repository, :public, :repository_private) } + + before_all do + project.add_guest(guest) + project.add_developer(author) + project.add_developer(assignee) + project.add_developer(member) + project.add_developer(john_doe) + project.add_developer(skipped) + end it 'creates a todo for each valid mentioned user' do expected_todo = base_commit_todo_attrs.merge( @@ -492,7 +506,16 @@ RSpec.describe TodoService do end context 'leaving a note on a commit in a private project' do - let(:project) { create(:project, :repository, :private) } + let_it_be(:project) { create(:project, :repository, :private) } + + before_all do + project.add_guest(guest) + project.add_developer(author) + project.add_developer(assignee) + project.add_developer(member) + project.add_developer(john_doe) + project.add_developer(skipped) + end it 'creates a todo for each valid mentioned user' do expected_todo = base_commit_todo_attrs.merge( @@ -554,10 +577,10 @@ RSpec.describe TodoService do end end - describe '#reassigned_issuable' do - let(:described_method) { :reassigned_issuable } + describe '#reassigned_assignable' do + let(:described_method) { :reassigned_assignable } - context 'issuable is a merge request' do + context 'assignable is a merge request' do it_behaves_like 'reassigned target' do let(:target_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } let(:addressed_target_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } @@ -565,13 +588,21 @@ RSpec.describe TodoService do end end - context 'issuable is an issue' do + context 'assignable is an issue' do it_behaves_like 'reassigned target' do let(:target_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } let(:addressed_target_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } let(:target_unassigned) { create(:issue, project: project, author: author, assignees: []) } end end + + context 'assignable is an alert' do + it_behaves_like 'reassigned target' do + let(:target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) } + let(:addressed_target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) } + let(:target_unassigned) { create(:alert_management_alert, project: project, assignees: []) } + end + end end describe 'Merge Requests' do @@ -759,16 +790,6 @@ RSpec.describe TodoService do end end - describe '#assign_alert' do - let(:described_method) { :assign_alert } - - it_behaves_like 'reassigned target' do - let(:target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) } - let(:addressed_target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) } - let(:target_unassigned) { create(:alert_management_alert, project: project, assignees: []) } - end - end - describe '#merge_request_build_failed' do let(:merge_participants) { [mr_unassigned.author, admin] } @@ -822,7 +843,17 @@ RSpec.describe TodoService do end describe '#new_note' do - let(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository) } + + before_all do + project.add_guest(guest) + project.add_developer(author) + project.add_developer(assignee) + project.add_developer(member) + project.add_developer(john_doe) + project.add_developer(skipped) + end + let(:mention) { john_doe.to_reference } let(:diff_note_on_merge_request) { create(:diff_note_on_merge_request, project: project, noteable: mr_unassigned, author: author, note: "Hey #{mention}") } let(:addressed_diff_note_on_merge_request) { create(:diff_note_on_merge_request, project: project, noteable: mr_unassigned, author: author, note: "#{mention}, hey!") } diff --git a/spec/services/users/create_service_spec.rb b/spec/services/users/create_service_spec.rb index 69d2d6ca9ff..9040966215c 100644 --- a/spec/services/users/create_service_spec.rb +++ b/spec/services/users/create_service_spec.rb @@ -154,6 +154,7 @@ RSpec.describe Users::CreateService do let(:params) do { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass', skip_confirmation: true } end + let(:service) { described_class.new(nil, params) } it 'persists the given attributes' do diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb index e45cb05a6c5..9404668e3c5 100644 --- a/spec/services/users/refresh_authorized_projects_service_spec.rb +++ b/spec/services/users/refresh_authorized_projects_service_spec.rb @@ -76,6 +76,26 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do service.execute_without_lease end + it 'removes duplicate entries' do + [Gitlab::Access::MAINTAINER, Gitlab::Access::REPORTER].each do |access_level| + user.project_authorizations.create!(project: project, access_level: access_level) + end + + expect(service).to( + receive(:update_authorizations) + .with([project.id], [[user.id, project.id, Gitlab::Access::MAINTAINER]]) + .and_call_original) + + service.execute_without_lease + + expect(user.project_authorizations.count).to eq(1) + project_authorization = ProjectAuthorization.where( + project_id: project.id, + user_id: user.id, + access_level: Gitlab::Access::MAINTAINER) + expect(project_authorization).to exist + end + it 'sets the access level of a project to the highest available level' do user.project_authorizations.delete_all diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 2be481c5b62..b7b81d33c3e 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -13,9 +13,11 @@ RSpec.describe WebHookService do 'X-Gitlab-Event' => 'Push Hook' } end + let(:data) do { before: 'oldrev', after: 'newrev', ref: 'ref' } end + let(:service_instance) { described_class.new(project_hook, data, :push_hooks) } describe '#initialize' do @@ -128,6 +130,14 @@ RSpec.describe WebHookService do end end + context 'when request body size is too big' do + 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" }) + end + end + it 'handles 200 status code' do stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success') diff --git a/spec/services/wiki_pages/event_create_service_spec.rb b/spec/services/wiki_pages/event_create_service_spec.rb index abf3bcb4c4d..974f2591763 100644 --- a/spec/services/wiki_pages/event_create_service_spec.rb +++ b/spec/services/wiki_pages/event_create_service_spec.rb @@ -12,7 +12,8 @@ RSpec.describe WikiPages::EventCreateService do let_it_be(:page) { create(:wiki_page, project: project) } let(:slug) { generate(:sluggified_title) } let(:action) { :created } - let(:response) { subject.execute(slug, page, action) } + let(:fingerprint) { page.sha } + let(:response) { subject.execute(slug, page, action, fingerprint) } context 'the user is nil' do subject { described_class.new(nil) } diff --git a/spec/services/wikis/create_attachment_service_spec.rb b/spec/services/wikis/create_attachment_service_spec.rb index 50cb9ac111c..22e34e1f373 100644 --- a/spec/services/wikis/create_attachment_service_spec.rb +++ b/spec/services/wikis/create_attachment_service_spec.rb @@ -13,6 +13,7 @@ RSpec.describe Wikis::CreateAttachmentService do file_content: 'Content of attachment' } end + let(:opts) { file_opts } subject(:service) { described_class.new(container: container, current_user: user, params: opts) } |