diff options
Diffstat (limited to 'spec/services')
163 files changed, 2352 insertions, 918 deletions
diff --git a/spec/services/admin/propagate_integration_service_spec.rb b/spec/services/admin/propagate_integration_service_spec.rb index 13320528e4f..151658fe429 100644 --- a/spec/services/admin/propagate_integration_service_spec.rb +++ b/spec/services/admin/propagate_integration_service_spec.rb @@ -7,20 +7,20 @@ RSpec.describe Admin::PropagateIntegrationService do include JiraServiceHelper before do - stub_jira_service_test + stub_jira_integration_test end let(:group) { create(:group) } let_it_be(:project) { create(:project) } - let_it_be(:instance_integration) { create(:jira_service, :instance) } - let_it_be(:not_inherited_integration) { create(:jira_service, project: project) } + let_it_be(:instance_integration) { create(:jira_integration, :instance) } + let_it_be(:not_inherited_integration) { create(:jira_integration, project: project) } let_it_be(:inherited_integration) do - create(:jira_service, project: create(:project), inherit_from_id: instance_integration.id) + create(:jira_integration, project: create(:project), inherit_from_id: instance_integration.id) end let_it_be(:different_type_inherited_integration) do - create(:redmine_service, project: project, inherit_from_id: instance_integration.id) + create(:redmine_integration, project: project, inherit_from_id: instance_integration.id) end context 'with inherited integration' do @@ -55,7 +55,7 @@ RSpec.describe Admin::PropagateIntegrationService do end context 'for a group-level integration' do - let(:group_integration) { create(:jira_service, group: group, project: nil) } + let(:group_integration) { create(:jira_integration, group: group, project: nil) } context 'with a project without integration' do let(:another_project) { create(:project, group: group) } @@ -81,7 +81,7 @@ RSpec.describe Admin::PropagateIntegrationService do context 'with a subgroup with integration' do let(:subgroup) { create(:group, parent: group) } - let(:subgroup_integration) { create(:jira_service, group: subgroup, project: nil, inherit_from_id: group_integration.id) } + let(:subgroup_integration) { create(:jira_integration, group: subgroup, project: nil, inherit_from_id: group_integration.id) } it 'calls to PropagateIntegrationInheritDescendantWorker' do expect(PropagateIntegrationInheritDescendantWorker).to receive(:perform_async) diff --git a/spec/services/admin/propagate_service_template_spec.rb b/spec/services/admin/propagate_service_template_spec.rb index 1bcf9af78ce..c8ca3173f99 100644 --- a/spec/services/admin/propagate_service_template_spec.rb +++ b/spec/services/admin/propagate_service_template_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe Admin::PropagateServiceTemplate do describe '.propagate' do let_it_be(:project) { create(:project) } + let!(:service_template) do Integrations::Pushover.create!( template: true, 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 695e90ebd92..55f8e47717c 100644 --- a/spec/services/alert_management/create_alert_issue_service_spec.rb +++ b/spec/services/alert_management/create_alert_issue_service_spec.rb @@ -15,6 +15,7 @@ RSpec.describe AlertManagement::CreateAlertIssueService do let_it_be(:generic_alert, reload: true) { create(:alert_management_alert, :triggered, project: project, payload: payload) } let_it_be(:prometheus_alert, reload: true) { create(:alert_management_alert, :triggered, :prometheus, project: project, payload: payload) } + let(:alert) { generic_alert } let(:alert_presenter) { alert.present } let(:created_issue) { Issue.last! } diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb index 56c1284927d..5f0c02cd521 100644 --- a/spec/services/application_settings/update_service_spec.rb +++ b/spec/services/application_settings/update_service_spec.rb @@ -23,8 +23,8 @@ RSpec.describe ApplicationSettings::UpdateService do context 'when the passed terms are blank' do let(:params) { { terms: '' } } - it 'does not create terms' do - expect { subject.execute }.not_to change { ApplicationSetting::Term.count } + it 'does create terms' do + expect { subject.execute }.to change { ApplicationSetting::Term.count }.by(1) end end diff --git a/spec/services/audit_event_service_spec.rb b/spec/services/audit_event_service_spec.rb index 997f506c269..ce7b43972da 100644 --- a/spec/services/audit_event_service_spec.rb +++ b/spec/services/audit_event_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe AuditEventService do let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user, :with_sign_ins) } let_it_be(:project_member) { create(:project_member, user: user) } + let(:service) { described_class.new(user, project, { action: :destroy }) } let(:logger) { instance_double(Gitlab::AuditJsonLogger) } @@ -78,15 +79,14 @@ RSpec.describe AuditEventService do context 'with IP address', :request_store do using RSpec::Parameterized::TableSyntax - where(:from_caller, :from_context, :from_author_sign_in, :output) do - '192.168.0.1' | '192.168.0.2' | '192.168.0.3' | '192.168.0.1' - nil | '192.168.0.2' | '192.168.0.3' | '192.168.0.2' - nil | nil | '192.168.0.3' | '192.168.0.3' + where(:from_context, :from_author_sign_in, :output) do + '192.168.0.2' | '192.168.0.3' | '192.168.0.2' + nil | '192.168.0.3' | '192.168.0.3' end with_them do let(:user) { create(:user, current_sign_in_ip: from_author_sign_in) } - let(:audit_service) { described_class.new(user, user, with: 'standard', ip_address: from_caller) } + let(:audit_service) { described_class.new(user, user, with: 'standard') } before do allow(Gitlab::RequestContext.instance).to receive(:client_ip).and_return(from_context) diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index ba7acd3d3df..4124696ac08 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -6,4 +6,96 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do include AdminModeHelper it_behaves_like 'a container registry auth service' + + context 'when in migration mode' do + include_context 'container registry auth service context' + + let_it_be(:current_user) { create(:user) } + let_it_be(:project) { create(:project) } + + before do + project.add_developer(current_user) + end + + shared_examples 'an unmodified token' do + it_behaves_like 'a valid token' + it { expect(payload['access']).not_to include(have_key('migration_eligible')) } + end + + shared_examples 'a modified token with migration eligibility' do |eligible| + it_behaves_like 'a valid token' + it { expect(payload['access']).to include(include('migration_eligible' => eligible)) } + end + + shared_examples 'a modified token' do + context 'with a non eligible root ancestor and project' do + before do + stub_feature_flags(container_registry_migration_phase1_deny: project.root_ancestor) + stub_feature_flags(container_registry_migration_phase1_allow: false) + end + + it_behaves_like 'a modified token with migration eligibility', false + end + + context 'with a non eligible root ancestor and eligible project' do + before do + stub_feature_flags(container_registry_migration_phase1_deny: false) + stub_feature_flags(container_registry_migration_phase1_deny: project.root_ancestor) + stub_feature_flags(container_registry_migration_phase1_allow: project) + end + + it_behaves_like 'a modified token with migration eligibility', false + end + + context 'with an eligible root ancestor and non eligible project' do + before do + stub_feature_flags(container_registry_migration_phase1_deny: false) + stub_feature_flags(container_registry_migration_phase1_allow: false) + end + + it_behaves_like 'a modified token with migration eligibility', false + end + + context 'with an eligible root ancestor and project' do + before do + stub_feature_flags(container_registry_migration_phase1_deny: false) + stub_feature_flags(container_registry_migration_phase1_allow: project) + end + + it_behaves_like 'a modified token with migration eligibility', true + end + end + + context 'with pull action' do + let(:current_params) do + { scopes: ["repository:#{project.full_path}:pull"] } + end + + it_behaves_like 'an unmodified token' + end + + context 'with push action' do + let(:current_params) do + { scopes: ["repository:#{project.full_path}:push"] } + end + + it_behaves_like 'a modified token' + end + + context 'with multiple actions including push' do + let(:current_params) do + { scopes: ["repository:#{project.full_path}:pull,push,delete"] } + end + + it_behaves_like 'a modified token' + end + + context 'with multiple actions excluding push' do + let(:current_params) do + { scopes: ["repository:#{project.full_path}:pull,delete"] } + end + + it_behaves_like 'an unmodified token' + end + end end diff --git a/spec/services/auth/dependency_proxy_authentication_service_spec.rb b/spec/services/auth/dependency_proxy_authentication_service_spec.rb index 1fd1677c7da..35e6d59b456 100644 --- a/spec/services/auth/dependency_proxy_authentication_service_spec.rb +++ b/spec/services/auth/dependency_proxy_authentication_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Auth::DependencyProxyAuthenticationService do let_it_be(:user) { create(:user) } + let(:service) { Auth::DependencyProxyAuthenticationService.new(nil, user) } before do diff --git a/spec/services/auto_merge_service_spec.rb b/spec/services/auto_merge_service_spec.rb index 3f7a26aa00e..335c608c206 100644 --- a/spec/services/auto_merge_service_spec.rb +++ b/spec/services/auto_merge_service_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe AutoMergeService do let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user) } + let(:service) { described_class.new(project, user) } before_all do diff --git a/spec/services/branches/create_service_spec.rb b/spec/services/branches/create_service_spec.rb index 5cf0d5af75f..1962aca35e1 100644 --- a/spec/services/branches/create_service_spec.rb +++ b/spec/services/branches/create_service_spec.rb @@ -38,10 +38,23 @@ RSpec.describe Branches::CreateService do end it 'returns an error with a reference name' do + err_msg = 'Failed to create branch \'new-feature\': invalid reference name \'unknown\'' result = service.execute('new-feature', 'unknown') expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Invalid reference name: unknown') + expect(result[:message]).to eq(err_msg) + end + end + + context 'when an ambiguous branch name is provided' do + it 'returns an error that branch could not be created' do + err_msg = 'Failed to create branch \'feature\': 13:reference is ambiguous.' + + service.execute('feature/widget', 'master') + result = service.execute('feature', 'master') + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq(err_msg) end end diff --git a/spec/services/bulk_create_integration_service_spec.rb b/spec/services/bulk_create_integration_service_spec.rb index 8369eb48088..4b0029e27cb 100644 --- a/spec/services/bulk_create_integration_service_spec.rb +++ b/spec/services/bulk_create_integration_service_spec.rb @@ -6,13 +6,14 @@ RSpec.describe BulkCreateIntegrationService do include JiraServiceHelper before_all do - stub_jira_service_test + stub_jira_integration_test end let_it_be(:excluded_group) { create(:group) } let_it_be(:excluded_project) { create(:project, group: excluded_group) } - let(:instance_integration) { create(:jira_service, :instance) } - let(:template_integration) { create(:jira_service, :template) } + + let(:instance_integration) { create(:jira_integration, :instance) } + let(:template_integration) { create(:jira_integration, :template) } let(:excluded_attributes) { %w[id project_id group_id inherit_from_id instance template created_at updated_at] } shared_examples 'creates integration from batch ids' do @@ -49,7 +50,7 @@ RSpec.describe BulkCreateIntegrationService do context 'with a project association' do let!(:project) { create(:project) } - let(:created_integration) { project.jira_service } + let(:created_integration) { project.jira_integration } let(:batch) { Project.where(id: project.id) } let(:association) { 'project' } @@ -73,8 +74,8 @@ RSpec.describe BulkCreateIntegrationService do context 'with a project association' do let!(:project) { create(:project, group: group) } - let(:integration) { create(:jira_service, group: group, project: nil) } - let(:created_integration) { project.jira_service } + let(:integration) { create(:jira_integration, group: group, project: nil) } + let(:created_integration) { project.jira_integration } let(:batch) { Project.where(id: Project.minimum(:id)..Project.maximum(:id)).without_integration(integration).in_namespace(integration.group.self_and_descendants) } let(:association) { 'project' } let(:inherit_from_id) { integration.id } @@ -85,7 +86,7 @@ RSpec.describe BulkCreateIntegrationService do context 'with a group association' do let!(:subgroup) { create(:group, parent: group) } - let(:integration) { create(:jira_service, group: group, project: nil, inherit_from_id: instance_integration.id) } + let(:integration) { create(:jira_integration, group: group, project: nil, inherit_from_id: instance_integration.id) } let(:created_integration) { Integration.find_by(group: subgroup) } let(:batch) { Group.where(id: subgroup.id) } let(:association) { 'group' } @@ -101,7 +102,7 @@ RSpec.describe BulkCreateIntegrationService do context 'with a project association' do let!(:project) { create(:project) } - let(:created_integration) { project.jira_service } + let(:created_integration) { project.jira_integration } let(:batch) { Project.where(id: project.id) } let(:association) { 'project' } let(:inherit_from_id) { integration.id } diff --git a/spec/services/bulk_imports/file_download_service_spec.rb b/spec/services/bulk_imports/file_download_service_spec.rb index 0961ddce553..a24af9ae64d 100644 --- a/spec/services/bulk_imports/file_download_service_spec.rb +++ b/spec/services/bulk_imports/file_download_service_spec.rb @@ -4,26 +4,41 @@ require 'spec_helper' RSpec.describe BulkImports::FileDownloadService do describe '#execute' do + let_it_be(:allowed_content_types) { %w(application/gzip application/octet-stream) } + let_it_be(:file_size_limit) { 5.gigabytes } let_it_be(:config) { build(:bulk_import_configuration) } let_it_be(:content_type) { 'application/octet-stream' } + let_it_be(:content_disposition) { nil } let_it_be(:filename) { 'file_download_service_spec' } let_it_be(:tmpdir) { Dir.tmpdir } let_it_be(:filepath) { File.join(tmpdir, filename) } + let_it_be(:content_length) { 1000 } + + let(:chunk_double) { double('chunk', size: 100, code: 200) } - let(:chunk_double) { double('chunk', size: 1000, code: 200) } let(:response_double) do double( code: 200, success?: true, parsed_response: {}, headers: { - 'content-length' => 100, - 'content-type' => content_type + 'content-length' => content_length, + 'content-type' => content_type, + 'content-disposition' => content_disposition } ) end - subject { described_class.new(configuration: config, relative_url: '/test', dir: tmpdir, filename: filename) } + subject do + described_class.new( + configuration: config, + relative_url: '/test', + dir: tmpdir, + filename: filename, + file_size_limit: file_size_limit, + allowed_content_types: allowed_content_types + ) + end before do allow_next_instance_of(BulkImports::Clients::HTTP) do |client| @@ -54,7 +69,14 @@ RSpec.describe BulkImports::FileDownloadService do stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) double = instance_double(BulkImports::Configuration, url: 'https://localhost', access_token: 'token') - service = described_class.new(configuration: double, relative_url: '/test', dir: tmpdir, filename: filename) + service = described_class.new( + configuration: double, + relative_url: '/test', + dir: tmpdir, + filename: filename, + file_size_limit: file_size_limit, + allowed_content_types: allowed_content_types + ) expect { service.execute }.to raise_error(Gitlab::UrlBlocker::BlockedUrlError) end @@ -70,31 +92,46 @@ RSpec.describe BulkImports::FileDownloadService do context 'when content-length is not valid' do context 'when content-length exceeds limit' do - before do - stub_const("#{described_class}::FILE_SIZE_LIMIT", 1) - end + let(:file_size_limit) { 1 } it 'raises an error' do - expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid content length') + expect { subject.execute }.to raise_error( + described_class::ServiceError, + 'File size 1000 Bytes exceeds limit of 1 Byte' + ) end end context 'when content-length is missing' do - let(:response_double) { double(success?: true, headers: { 'content-type' => content_type }) } + let(:content_length) { nil } it 'raises an error' do - expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid content length') + expect { subject.execute }.to raise_error( + described_class::ServiceError, + 'Missing content-length header' + ) end end end - context 'when partially downloaded file exceeds limit' do - before do - stub_const("#{described_class}::FILE_SIZE_LIMIT", 150) + context 'when content-length is equals the file size limit' do + let(:content_length) { 150 } + let(:file_size_limit) { 150 } + + it 'does not raise an error' do + expect { subject.execute }.not_to raise_error end + end + + context 'when partially downloaded file exceeds limit' do + let(:content_length) { 151 } + let(:file_size_limit) { 150 } it 'raises an error' do - expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid downloaded file') + expect { subject.execute }.to raise_error( + described_class::ServiceError, + 'File size 151 Bytes exceeds limit of 150 Bytes' + ) end end @@ -102,7 +139,10 @@ RSpec.describe BulkImports::FileDownloadService do let(:chunk_double) { double('chunk', size: 1000, code: 307) } it 'raises an error' do - expect { subject.execute }.to raise_error(described_class::ServiceError, 'File download error 307') + expect { subject.execute }.to raise_error( + described_class::ServiceError, + 'File download error 307' + ) end end @@ -110,23 +150,88 @@ RSpec.describe BulkImports::FileDownloadService do let_it_be(:symlink) { File.join(tmpdir, 'symlink') } before do - FileUtils.ln_s(File.join(tmpdir, filename), symlink) + FileUtils.ln_s(File.join(tmpdir, filename), symlink, force: true) end - subject { described_class.new(configuration: config, relative_url: '/test', dir: tmpdir, filename: 'symlink') } + subject do + described_class.new( + configuration: config, + relative_url: '/test', + dir: tmpdir, + filename: 'symlink', + file_size_limit: file_size_limit, + allowed_content_types: allowed_content_types + ) + end it 'raises an error and removes the file' do - expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid downloaded file') + expect { subject.execute }.to raise_error( + described_class::ServiceError, + 'Invalid downloaded file' + ) expect(File.exist?(symlink)).to eq(false) end end context 'when dir is not in tmpdir' do - subject { described_class.new(configuration: config, relative_url: '/test', dir: '/etc', filename: filename) } + subject do + described_class.new( + configuration: config, + relative_url: '/test', + dir: '/etc', + filename: filename, + file_size_limit: file_size_limit, + allowed_content_types: allowed_content_types + ) + end it 'raises an error' do - expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid target directory') + expect { subject.execute }.to raise_error( + described_class::ServiceError, + 'Invalid target directory' + ) + end + end + + context 'when using the remote filename' do + let_it_be(:filename) { nil } + + context 'when no filename is given' do + it 'raises an error when the filename is not provided in the request header' do + expect { subject.execute }.to raise_error( + described_class::ServiceError, + 'Remote filename not provided in content-disposition header' + ) + end + end + + context 'with a given filename' do + let_it_be(:content_disposition) { 'filename="avatar.png"' } + + it 'uses the given filename' do + expect(subject.execute).to eq(File.join(tmpdir, "avatar.png")) + end + end + + context 'when the filename is a path' do + let_it_be(:content_disposition) { 'filename="../../avatar.png"' } + + it 'raises an error when the filename is not provided in the request header' do + expect(subject.execute).to eq(File.join(tmpdir, "avatar.png")) + end + end + + context 'when the filename is longer the the limit' do + let_it_be(:content_disposition) { 'filename="../../xxx.b"' } + + before do + stub_const("#{described_class}::FILENAME_SIZE_LIMIT", 1) + end + + it 'raises an error when the filename is not provided in the request header' do + expect(subject.execute).to eq(File.join(tmpdir, "x.b")) + end end end end diff --git a/spec/services/bulk_update_integration_service_spec.rb b/spec/services/bulk_update_integration_service_spec.rb index a866e0852bc..b6b7d1936a2 100644 --- a/spec/services/bulk_update_integration_service_spec.rb +++ b/spec/services/bulk_update_integration_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe BulkUpdateIntegrationService do include JiraServiceHelper before_all do - stub_jira_service_test + stub_jira_integration_test end let(:excluded_attributes) { %w[id project_id group_id inherit_from_id instance template created_at updated_at] } diff --git a/spec/services/captcha/captcha_verification_service_spec.rb b/spec/services/captcha/captcha_verification_service_spec.rb index 245e06703f5..fe2199fb53e 100644 --- a/spec/services/captcha/captcha_verification_service_spec.rb +++ b/spec/services/captcha/captcha_verification_service_spec.rb @@ -4,21 +4,31 @@ require 'spec_helper' RSpec.describe Captcha::CaptchaVerificationService do describe '#execute' do - let(:captcha_response) { nil } - let(:request) { double(:request) } - let(:service) { described_class.new } + let(:captcha_response) { 'abc123' } + let(:fake_ip) { '1.2.3.4' } + let(:spam_params) do + ::Spam::SpamParams.new( + captcha_response: captcha_response, + spam_log_id: double, + ip_address: fake_ip, + user_agent: double, + referer: double + ) + end + + let(:service) { described_class.new(spam_params: spam_params) } - subject { service.execute(captcha_response: captcha_response, request: request) } + subject { service.execute } context 'when there is no captcha_response' do + let(:captcha_response) { nil } + it 'returns false' do expect(subject).to eq(false) end end context 'when there is a captcha_response' do - let(:captcha_response) { 'abc123' } - before do expect(Gitlab::Recaptcha).to receive(:load_configurations!) end @@ -29,10 +39,12 @@ RSpec.describe Captcha::CaptchaVerificationService do expect(subject).to eq(true) end - it 'has a request method which returns the request' do + it 'has a request method which returns an object with the ip address #remote_ip' do subject - expect(service.send(:request)).to eq(request) + request_struct = service.send(:request) + expect(request_struct).to respond_to(:remote_ip) + expect(request_struct.remote_ip).to eq(fake_ip) end end end diff --git a/spec/services/ci/after_requeue_job_service_spec.rb b/spec/services/ci/after_requeue_job_service_spec.rb index a2147759dba..f8c49060ce0 100644 --- a/spec/services/ci/after_requeue_job_service_spec.rb +++ b/spec/services/ci/after_requeue_job_service_spec.rb @@ -8,9 +8,9 @@ RSpec.describe Ci::AfterRequeueJobService do let(:pipeline) { create(:ci_pipeline, project: project) } - let!(:build) { create(:ci_build, pipeline: pipeline, stage_idx: 0) } let!(:test1) { create(:ci_build, :success, pipeline: pipeline, stage_idx: 1) } let!(:test2) { create(:ci_build, :skipped, pipeline: pipeline, stage_idx: 1) } + let!(:build) { create(:ci_build, pipeline: pipeline, stage_idx: 0, name: 'build') } subject(:execute_service) { described_class.new(project, user).execute(build) } @@ -24,6 +24,34 @@ RSpec.describe Ci::AfterRequeueJobService do expect(test2.reload).to be_created end + context 'when there is a job need from the same stage' do + let!(:test3) do + create(:ci_build, + :skipped, + pipeline: pipeline, + stage_idx: 0, + scheduling_type: :dag) + end + + before do + create(:ci_build_need, build: test3, name: 'build') + end + + it 'marks subsequent skipped jobs as processable' do + expect { execute_service }.to change { test3.reload.status }.from('skipped').to('created') + end + + context 'with ci_same_stage_job_needs FF disabled' do + before do + stub_feature_flags(ci_same_stage_job_needs: false) + end + + it 'does nothing with the build' do + expect { execute_service }.not_to change { test3.reload.status } + end + end + end + context 'when the pipeline is a downstream pipeline and the bridge is depended' do let!(:trigger_job) { create(:ci_bridge, :strategy_depend, status: 'success') } diff --git a/spec/services/ci/append_build_trace_service_spec.rb b/spec/services/ci/append_build_trace_service_spec.rb index 8812680b665..b251f00158f 100644 --- a/spec/services/ci/append_build_trace_service_spec.rb +++ b/spec/services/ci/append_build_trace_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Ci::AppendBuildTraceService do let_it_be(:project) { create(:project) } let_it_be(:pipeline) { create(:ci_pipeline, project: project) } - let_it_be(:build) { create(:ci_build, :running, pipeline: pipeline) } + let_it_be_with_reload(:build) { create(:ci_build, :running, pipeline: pipeline) } before do stub_feature_flags(ci_enable_live_trace: true) @@ -54,4 +54,46 @@ RSpec.describe Ci::AppendBuildTraceService do expect(result.stream_size).to eq 4 end end + + context 'when the trace size is exceeded' do + before do + project.actual_limits.update!(ci_jobs_trace_size_limit: 1) + end + + it 'returns 403 status code' do + stream_size = 1.25.megabytes + body_data = 'x' * stream_size + content_range = "0-#{stream_size}" + + result = described_class + .new(build, content_range: content_range) + .execute(body_data) + + expect(result.status).to eq 403 + expect(result.stream_size).to be_nil + expect(build.trace_chunks.count).to eq 0 + expect(build.reload).to be_failed + expect(build.failure_reason).to eq 'trace_size_exceeded' + end + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(ci_jobs_trace_size_limit: false) + end + + it 'appends trace chunks' do + stream_size = 1.25.megabytes + body_data = 'x' * stream_size + content_range = "0-#{stream_size}" + + result = described_class + .new(build, content_range: content_range) + .execute(body_data) + + expect(result.status).to eq 202 + expect(result.stream_size).to eq stream_size + expect(build.trace_chunks.count).to eq 10 + end + end + end end diff --git a/spec/services/ci/archive_trace_service_spec.rb b/spec/services/ci/archive_trace_service_spec.rb index a4f498f17c3..12804efc28c 100644 --- a/spec/services/ci/archive_trace_service_spec.rb +++ b/spec/services/ci/archive_trace_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Ci::ArchiveTraceService, '#execute' do - subject { described_class.new.execute(job, worker_name: ArchiveTraceWorker.name) } + subject { described_class.new.execute(job, worker_name: Ci::ArchiveTraceWorker.name) } context 'when job is finished' do let(:job) { create(:ci_build, :success, :trace_live) } @@ -30,43 +30,17 @@ RSpec.describe Ci::ArchiveTraceService, '#execute' do create(:ci_build_trace_chunk, build: job) end - context 'when the feature flag `erase_traces_from_already_archived_jobs_when_archiving_again` is enabled' do - before do - stub_feature_flags(erase_traces_from_already_archived_jobs_when_archiving_again: true) - end - - it 'removes the trace chunks' do - expect { subject }.to change { job.trace_chunks.count }.to(0) - end - - context 'when associated data does not exist' do - before do - job.job_artifacts_trace.file.remove! - end - - it 'removes the trace artifact' do - expect { subject }.to change { job.reload.job_artifacts_trace }.to(nil) - end - end + it 'removes the trace chunks' do + expect { subject }.to change { job.trace_chunks.count }.to(0) end - context 'when the feature flag `erase_traces_from_already_archived_jobs_when_archiving_again` is disabled' do + context 'when associated data does not exist' do before do - stub_feature_flags(erase_traces_from_already_archived_jobs_when_archiving_again: false) + job.job_artifacts_trace.file.remove! end - it 'does not remove the trace chunks' do - expect { subject }.not_to change { job.trace_chunks.count } - end - - context 'when associated data does not exist' do - before do - job.job_artifacts_trace.file.remove! - end - - it 'does not remove the trace artifact' do - expect { subject }.not_to change { job.reload.job_artifacts_trace } - end + it 'removes the trace artifact' do + expect { subject }.to change { job.reload.job_artifacts_trace }.to(nil) end end end @@ -77,7 +51,7 @@ RSpec.describe Ci::ArchiveTraceService, '#execute' do it 'leaves a warning message in sidekiq log' do expect(Sidekiq.logger).to receive(:warn).with( - class: ArchiveTraceWorker.name, + class: Ci::ArchiveTraceWorker.name, message: 'The job does not have live trace but going to be archived.', job_id: job.id) @@ -94,7 +68,7 @@ RSpec.describe Ci::ArchiveTraceService, '#execute' do it 'leaves a warning message in sidekiq log' do expect(Sidekiq.logger).to receive(:warn).with( - class: ArchiveTraceWorker.name, + class: Ci::ArchiveTraceWorker.name, message: 'The job does not have archived trace after archiving.', job_id: job.id) @@ -114,7 +88,7 @@ RSpec.describe Ci::ArchiveTraceService, '#execute' do job_id: job.id).once expect(Sidekiq.logger).to receive(:warn).with( - class: ArchiveTraceWorker.name, + class: Ci::ArchiveTraceWorker.name, message: "Failed to archive trace. message: Job is not finished yet.", job_id: job.id).and_call_original diff --git a/spec/services/ci/create_pipeline_service/cache_spec.rb b/spec/services/ci/create_pipeline_service/cache_spec.rb index 5f74c2f1cef..f9767a794db 100644 --- a/spec/services/ci/create_pipeline_service/cache_spec.rb +++ b/spec/services/ci/create_pipeline_service/cache_spec.rb @@ -33,11 +33,11 @@ RSpec.describe Ci::CreatePipelineService do it 'uses the provided key' do expected = { - 'key' => 'a-key', - 'paths' => ['logs/', 'binaries/'], - 'policy' => 'pull-push', - 'untracked' => true, - 'when' => 'on_success' + key: 'a-key', + paths: ['logs/', 'binaries/'], + policy: 'pull-push', + untracked: true, + when: 'on_success' } expect(pipeline).to be_persisted @@ -66,10 +66,10 @@ RSpec.describe Ci::CreatePipelineService do it 'builds a cache key' do expected = { - 'key' => /[a-f0-9]{40}/, - 'paths' => ['logs/'], - 'policy' => 'pull-push', - 'when' => 'on_success' + key: /[a-f0-9]{40}/, + paths: ['logs/'], + policy: 'pull-push', + when: 'on_success' } expect(pipeline).to be_persisted @@ -82,10 +82,10 @@ RSpec.describe Ci::CreatePipelineService do it 'uses default cache key' do expected = { - 'key' => /default/, - 'paths' => ['logs/'], - 'policy' => 'pull-push', - 'when' => 'on_success' + key: /default/, + paths: ['logs/'], + policy: 'pull-push', + when: 'on_success' } expect(pipeline).to be_persisted @@ -115,10 +115,10 @@ RSpec.describe Ci::CreatePipelineService do it 'builds a cache key' do expected = { - 'key' => /\$ENV_VAR-[a-f0-9]{40}/, - 'paths' => ['logs/'], - 'policy' => 'pull-push', - 'when' => 'on_success' + key: /\$ENV_VAR-[a-f0-9]{40}/, + paths: ['logs/'], + policy: 'pull-push', + when: 'on_success' } expect(pipeline).to be_persisted @@ -131,10 +131,10 @@ RSpec.describe Ci::CreatePipelineService do it 'uses default cache key' do expected = { - 'key' => /\$ENV_VAR-default/, - 'paths' => ['logs/'], - 'policy' => 'pull-push', - 'when' => 'on_success' + key: /\$ENV_VAR-default/, + paths: ['logs/'], + policy: 'pull-push', + when: 'on_success' } expect(pipeline).to be_persisted 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 7193e5bd7d4..a42770aae20 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 @@ -69,7 +69,7 @@ RSpec.describe Ci::CreatePipelineService do end it 'contains both errors and warnings' do - error_message = 'build job: need test is not defined in prior stages' + error_message = 'build job: need test is not defined in current or prior stages' warning_message = /jobs:test may allow multiple pipelines to run/ expect(pipeline.yaml_errors).to eq(error_message) diff --git a/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb b/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb index 4cf52223e38..5dceb9f57f0 100644 --- a/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb +++ b/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb @@ -39,8 +39,8 @@ RSpec.describe Ci::CreatePipelineService do it 'creates a pipeline' do expect(pipeline).to be_persisted expect(pipeline.builds.first.options).to match(a_hash_including({ - 'before_script' => ['ls'], - 'script' => [ + before_script: ['ls'], + script: [ 'echo doing my first step', 'echo doing step 1 of job 1', 'echo doing my last step' diff --git a/spec/services/ci/create_pipeline_service/dry_run_spec.rb b/spec/services/ci/create_pipeline_service/dry_run_spec.rb index 0fb500f5729..01df7772eef 100644 --- a/spec/services/ci/create_pipeline_service/dry_run_spec.rb +++ b/spec/services/ci/create_pipeline_service/dry_run_spec.rb @@ -84,7 +84,7 @@ RSpec.describe Ci::CreatePipelineService do 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' + error_message = 'build job: need test is not defined in current or prior stages' expect(subject.error_messages.map(&:content)).to eq([error_message]) expect(subject.errors).not_to be_empty @@ -109,7 +109,7 @@ RSpec.describe Ci::CreatePipelineService do it_behaves_like 'returns a non persisted pipeline' it 'returns a pipeline with errors', :aggregate_failures do - error_message = "'test' job needs 'build' job, but it was not added to the pipeline" + error_message = "'test' job needs 'build' job, but 'build' is not in any previous stage" expect(subject.error_messages.map(&:content)).to eq([error_message]) expect(subject.errors).not_to be_empty diff --git a/spec/services/ci/create_pipeline_service/evaluate_runner_tags_spec.rb b/spec/services/ci/create_pipeline_service/evaluate_runner_tags_spec.rb new file mode 100644 index 00000000000..df881c1ac8f --- /dev/null +++ b/spec/services/ci/create_pipeline_service/evaluate_runner_tags_spec.rb @@ -0,0 +1,144 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::CreatePipelineService do + let_it_be(:group) { create(:group, :private) } + let_it_be(:group_variable) { create(:ci_group_variable, group: group, key: 'RUNNER_TAG', value: 'group')} + let_it_be(:project) { create(:project, :repository, group: group) } + let_it_be(:user) { create(:user) } + + let(:service) { described_class.new(project, user, ref: 'master') } + let(:pipeline) { service.execute(:push) } + let(:job) { pipeline.builds.find_by(name: 'job') } + + before do + project.add_developer(user) + stub_ci_pipeline_yaml_file config + end + + context 'when the variable is set' do + let(:config) do + <<~EOS + variables: + KUBERNETES_RUNNER: kubernetes + + job: + tags: + - docker + - $KUBERNETES_RUNNER + script: + - echo "Hello runner selector feature" + EOS + end + + it 'uses the evaluated variable' do + expect(pipeline).to be_created_successfully + expect(job.tags.pluck(:name)).to match_array(%w[docker kubernetes]) + end + end + + context 'when the tag is composed by two variables' do + let(:config) do + <<~EOS + variables: + CLOUD_PROVIDER: aws + KUBERNETES_RUNNER: kubernetes + ENVIRONMENT_NAME: prod + + job: + tags: + - docker + - $CLOUD_PROVIDER-$KUBERNETES_RUNNER-$ENVIRONMENT_NAME + script: + - echo "Hello runner selector feature" + EOS + end + + it 'uses the evaluated variables' do + expect(pipeline).to be_created_successfully + expect(job.tags.pluck(:name)).to match_array(%w[docker aws-kubernetes-prod]) + end + end + + context 'when the variable is not set' do + let(:config) do + <<~EOS + job: + tags: + - docker + - $KUBERNETES_RUNNER + script: + - echo "Hello runner selector feature" + EOS + end + + it 'uses the variable as a regular string' do + expect(pipeline).to be_created_successfully + expect(job.tags.pluck(:name)).to match_array(%w[docker $KUBERNETES_RUNNER]) + end + end + + context 'when the tag uses group variables' do + let(:config) do + <<~EOS + job: + tags: + - docker + - $RUNNER_TAG + script: + - echo "Hello runner selector feature" + EOS + end + + it 'uses the evaluated variables' do + expect(pipeline).to be_created_successfully + expect(job.tags.pluck(:name)).to match_array(%w[docker group]) + end + end + + context 'when the tag has the same variable name defined for both group and project' do + let_it_be(:project_variable) { create(:ci_variable, project: project, key: 'RUNNER_TAG', value: 'project') } + + let(:config) do + <<~EOS + variables: + RUNNER_TAG: pipeline + job: + tags: + - docker + - $RUNNER_TAG + script: + - echo "Hello runner selector feature" + EOS + end + + it 'uses the project variable instead of group due to variable precedence' do + expect(pipeline).to be_created_successfully + expect(job.tags.pluck(:name)).to match_array(%w[docker project]) + end + end + + context 'with parallel:matrix config' do + let(:tags) { pipeline.builds.map(&:tags).flatten.pluck(:name) } + + let(:config) do + <<~EOS + job: + parallel: + matrix: + - PROVIDER: [aws, gcp] + STACK: [monitoring, backup, app] + tags: + - ${PROVIDER}-${STACK} + script: + - echo "Hello runner selector feature" + EOS + end + + it 'uses the evaluated variables' do + expect(pipeline).to be_created_successfully + expect(tags).to match_array(%w[aws-monitoring aws-backup aws-app gcp-monitoring gcp-backup gcp-app]) + end + end +end diff --git a/spec/services/ci/create_pipeline_service/needs_spec.rb b/spec/services/ci/create_pipeline_service/needs_spec.rb index 3b4a6178b8f..d096db10d0b 100644 --- a/spec/services/ci/create_pipeline_service/needs_spec.rb +++ b/spec/services/ci/create_pipeline_service/needs_spec.rb @@ -104,7 +104,7 @@ RSpec.describe Ci::CreatePipelineService do it 'saves dependencies' do expect(test_a_build.options) - .to match(a_hash_including('dependencies' => ['build_a'])) + .to match(a_hash_including(dependencies: ['build_a'])) end it 'artifacts default to true' do @@ -257,7 +257,7 @@ RSpec.describe Ci::CreatePipelineService do it 'returns error' do expect(pipeline.yaml_errors) - .to eq("'test' job needs 'build' job, but it was not added to the pipeline") + .to eq("'test' job needs 'build' job, but 'build' is not in any previous stage") end context 'when need is optional' do diff --git a/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb b/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb index 512cf546e6a..7a6535ed3fa 100644 --- a/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb +++ b/spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb @@ -69,9 +69,9 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do it_behaves_like 'successful creation' do let(:expected_bridge_options) do { - 'trigger' => { - 'include' => [ - { 'local' => 'path/to/child.yml' } + trigger: { + include: [ + { local: 'path/to/child.yml' } ] } } @@ -149,9 +149,9 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do it_behaves_like 'successful creation' do let(:expected_bridge_options) do { - 'trigger' => { - 'include' => [ - { 'local' => 'path/to/child.yml' } + trigger: { + include: [ + { local: 'path/to/child.yml' } ] } } @@ -175,8 +175,8 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do it_behaves_like 'successful creation' do let(:expected_bridge_options) do { - 'trigger' => { - 'include' => 'path/to/child.yml' + trigger: { + include: 'path/to/child.yml' } } end @@ -202,8 +202,8 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do it_behaves_like 'successful creation' do let(:expected_bridge_options) do { - 'trigger' => { - 'include' => ['path/to/child.yml', 'path/to/child2.yml'] + trigger: { + include: ['path/to/child.yml', 'path/to/child2.yml'] } } end @@ -252,7 +252,7 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do end it_behaves_like 'creation failure' do - let(:expected_error) { /test job: dependency generator is not defined in prior stages/ } + let(:expected_error) { /test job: dependency generator is not defined in current or prior stages/ } end end @@ -295,12 +295,12 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do it_behaves_like 'successful creation' do let(:expected_bridge_options) do { - 'trigger' => { - 'include' => [ + trigger: { + include: [ { - 'file' => 'path/to/child.yml', - 'project' => 'my-namespace/my-project', - 'ref' => 'master' + file: 'path/to/child.yml', + project: 'my-namespace/my-project', + ref: 'master' } ] } @@ -353,11 +353,11 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do it_behaves_like 'successful creation' do let(:expected_bridge_options) do { - 'trigger' => { - 'include' => [ + trigger: { + include: [ { - 'file' => ["path/to/child1.yml", "path/to/child2.yml"], - 'project' => 'my-namespace/my-project' + file: ["path/to/child1.yml", "path/to/child2.yml"], + project: 'my-namespace/my-project' } ] } diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index 33ec6aacc44..acdf38bbc13 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -230,22 +230,6 @@ RSpec.describe Ci::CreatePipelineService do [nil, nil, nil, 'job var 4', nil, nil, 'overridden var 7'] ) end - - context 'when FF ci_workflow_rules_variables is disabled' do - before do - stub_feature_flags(ci_workflow_rules_variables: false) - end - - it 'does not affect workflow variables but job variables' do - expect(job1.scoped_variables.to_hash.values_at(*variable_keys)).to eq( - ['overridden var 1', 'job var 2', nil, 'workflow var 4', 'job var 5', nil, 'workflow var 7'] - ) - - expect(job2.scoped_variables.to_hash.values_at(*variable_keys)).to eq( - [nil, nil, nil, 'job var 4', nil, nil, 'overridden var 7'] - ) - end - end end context 'when matching to the second rule' do diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 3316f8c3d9b..64e8c6ac2df 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -1001,7 +1001,7 @@ RSpec.describe Ci::CreatePipelineService do expect(pipeline.yaml_errors).not_to be_present expect(pipeline).to be_persisted expect(build).to be_kind_of(Ci::Build) - expect(build.options).to eq(config[:release].except(:stage, :only).with_indifferent_access) + expect(build.options).to eq(config[:release].except(:stage, :only)) expect(build).to be_persisted end end @@ -1715,7 +1715,7 @@ RSpec.describe Ci::CreatePipelineService do it 'contains the expected errors' do expect(pipeline.builds).to be_empty - error_message = "'test_a' job needs 'build_a' job, but it was not added to the pipeline" + error_message = "'test_a' job needs 'build_a' job, but 'build_a' is not in any previous stage" expect(pipeline.yaml_errors).to eq(error_message) expect(pipeline.error_messages.map(&:content)).to contain_exactly(error_message) expect(pipeline.errors[:base]).to contain_exactly(error_message) diff --git a/spec/services/ci/destroy_pipeline_service_spec.rb b/spec/services/ci/destroy_pipeline_service_spec.rb index 302233cea5a..588ff0b1762 100644 --- a/spec/services/ci/destroy_pipeline_service_spec.rb +++ b/spec/services/ci/destroy_pipeline_service_spec.rb @@ -67,6 +67,30 @@ RSpec.describe ::Ci::DestroyPipelineService do end end end + + context 'when pipeline is in cancelable state' do + before do + allow(pipeline).to receive(:cancelable?).and_return(true) + end + + it 'cancels the pipeline' do + expect(pipeline).to receive(:cancel_running) + + subject + end + + context 'when cancel_pipelines_prior_to_destroy is disabled' do + before do + stub_feature_flags(cancel_pipelines_prior_to_destroy: false) + end + + it "doesn't cancel the pipeline" do + expect(pipeline).not_to receive(:cancel_running) + + subject + end + end + end end context 'user is not owner' do diff --git a/spec/services/ci/job_token_scope/add_project_service_spec.rb b/spec/services/ci/job_token_scope/add_project_service_spec.rb new file mode 100644 index 00000000000..ba889465fac --- /dev/null +++ b/spec/services/ci/job_token_scope/add_project_service_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Ci::JobTokenScope::AddProjectService do + let(:service) { described_class.new(project, current_user) } + + let_it_be(:project) { create(:project, ci_job_token_scope_enabled: true).tap(&:save!) } + let_it_be(:target_project) { create(:project) } + let_it_be(:current_user) { create(:user) } + + describe '#execute' do + subject(:result) { service.execute(target_project) } + + it_behaves_like 'editable job token scope' do + context 'when user has permissions on source and target projects' do + before do + project.add_maintainer(current_user) + target_project.add_developer(current_user) + end + + it 'adds the project to the scope' do + expect do + expect(result).to be_success + end.to change { Ci::JobToken::ProjectScopeLink.count }.by(1) + end + end + + context 'when target project is same as the source project' do + before do + project.add_maintainer(current_user) + end + + let(:target_project) { project } + + it_behaves_like 'returns error', "Validation failed: Target project can't be the same as the source project" + end + end + end +end diff --git a/spec/services/ci/job_token_scope/remove_project_service_spec.rb b/spec/services/ci/job_token_scope/remove_project_service_spec.rb new file mode 100644 index 00000000000..238fc879f54 --- /dev/null +++ b/spec/services/ci/job_token_scope/remove_project_service_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Ci::JobTokenScope::RemoveProjectService do + let(:service) { described_class.new(project, current_user) } + + let_it_be(:project) { create(:project, ci_job_token_scope_enabled: true).tap(&:save!) } + let_it_be(:target_project) { create(:project) } + let_it_be(:current_user) { create(:user) } + + let_it_be(:link) do + create(:ci_job_token_project_scope_link, + source_project: project, + target_project: target_project) + end + + describe '#execute' do + subject(:result) { service.execute(target_project) } + + it_behaves_like 'editable job token scope' do + context 'when user has permissions on source and target project' do + before do + project.add_maintainer(current_user) + target_project.add_developer(current_user) + end + + it 'removes the project from the scope' do + expect do + expect(result).to be_success + end.to change { Ci::JobToken::ProjectScopeLink.count }.by(-1) + end + end + + context 'when target project is same as the source project' do + before do + project.add_maintainer(current_user) + end + + let(:target_project) { project } + + it_behaves_like 'returns error', "Source project cannot be removed from the job token scope" + end + end + 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 13c924a3089..5089f8d5dba 100644 --- a/spec/services/ci/pipeline_processing/shared_processing_service.rb +++ b/spec/services/ci/pipeline_processing/shared_processing_service.rb @@ -842,20 +842,6 @@ RSpec.shared_examples 'Pipeline Processing Service' do expect(all_builds.manual).to contain_exactly(linux_build) expect(all_builds.skipped).to contain_exactly(deploy) end - - context 'when FF ci_fix_pipeline_status_for_dag_needs_manual is disabled' do - before do - stub_feature_flags(ci_fix_pipeline_status_for_dag_needs_manual: false) - end - - it 'makes deploy DAG to be waiting for optional manual to finish' do - expect(process_pipeline).to be_truthy - - expect(stages).to eq(%w(skipped created)) - expect(all_builds.manual).to contain_exactly(linux_build) - expect(all_builds.created).to contain_exactly(deploy) - end - end end context 'when a bridge job has parallel:matrix config', :sidekiq_inline do diff --git a/spec/services/ci/pipeline_processing/test_cases/dag_same_stages.yml b/spec/services/ci/pipeline_processing/test_cases/dag_same_stages.yml new file mode 100644 index 00000000000..2a63daeb561 --- /dev/null +++ b/spec/services/ci/pipeline_processing/test_cases/dag_same_stages.yml @@ -0,0 +1,47 @@ +config: + build: + stage: test + script: exit 0 + + test: + stage: test + script: exit 0 + needs: [build] + + deploy: + stage: test + script: exit 0 + needs: [test] + +init: + expect: + pipeline: pending + stages: + test: pending + jobs: + build: pending + test: created + deploy: created + +transitions: + - event: success + jobs: [build] + expect: + pipeline: running + stages: + test: running + jobs: + build: success + test: pending + deploy: created + + - event: success + jobs: [test] + expect: + pipeline: running + stages: + test: running + jobs: + build: success + test: success + deploy: pending diff --git a/spec/services/ci/pipelines/add_job_service_spec.rb b/spec/services/ci/pipelines/add_job_service_spec.rb new file mode 100644 index 00000000000..a72ffbfdc87 --- /dev/null +++ b/spec/services/ci/pipelines/add_job_service_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::Pipelines::AddJobService do + let_it_be(:pipeline) { create(:ci_pipeline) } + + let(:job) { build(:ci_build) } + + subject(:service) { described_class.new(pipeline) } + + context 'when the pipeline is not persisted' do + let(:pipeline) { build(:ci_pipeline) } + + it 'raises error' do + expect { service }.to raise_error('Pipeline must be persisted for this service to be used') + end + end + + describe '#execute!' do + subject(:execute) do + service.execute!(job) do |job| + job.save! + end + end + + it 'assigns pipeline attributes to the job' do + expect do + execute + end.to change { job.slice(:pipeline, :project, :ref) }.to( + pipeline: pipeline, project: pipeline.project, ref: pipeline.ref + ) + end + + it 'returns a service response with the job as payload' do + expect(execute).to be_success + expect(execute.payload[:job]).to eq(job) + end + + it 'calls update_older_statuses_retried!' do + expect(job).to receive(:update_older_statuses_retried!) + + execute + end + + context 'when the block raises an error' do + subject(:execute) do + service.execute!(job) do |job| + raise "this is an error" + end + end + + it 'returns a service response with the error and the job as payload' do + expect(execute).to be_error + expect(execute.payload[:job]).to eq(job) + expect(execute.message).to eq('this is an error') + end + end + + context 'when the FF ci_fix_commit_status_retried is disabled' do + before do + stub_feature_flags(ci_fix_commit_status_retried: false) + end + + it 'does not call update_older_statuses_retried!' do + expect(job).not_to receive(:update_older_statuses_retried!) + + execute + end + end + end +end diff --git a/spec/services/ci/play_bridge_service_spec.rb b/spec/services/ci/play_bridge_service_spec.rb index d6130325b5a..3f97bfdf5ae 100644 --- a/spec/services/ci/play_bridge_service_spec.rb +++ b/spec/services/ci/play_bridge_service_spec.rb @@ -45,16 +45,6 @@ RSpec.describe Ci::PlayBridgeService, '#execute' do it 'marks the subsequent job as processable' do expect { execute_service }.to change { job.reload.status }.from('skipped').to('created') end - - context 'when the FF ci_fix_pipeline_status_for_dag_needs_manual is disabled' do - before do - stub_feature_flags(ci_fix_pipeline_status_for_dag_needs_manual: false) - end - - it 'does not change the subsequent job' do - expect { execute_service }.not_to change { job.reload.status }.from('skipped') - end - end end context 'when bridge is not playable' do diff --git a/spec/services/ci/play_build_service_spec.rb b/spec/services/ci/play_build_service_spec.rb index 78de91675f9..babd601e0cf 100644 --- a/spec/services/ci/play_build_service_spec.rb +++ b/spec/services/ci/play_build_service_spec.rb @@ -71,16 +71,6 @@ RSpec.describe Ci::PlayBuildService, '#execute' do it 'marks the subsequent job as processable' do expect { service.execute(build) }.to change { job.reload.status }.from('skipped').to('created') end - - context 'when the FF ci_fix_pipeline_status_for_dag_needs_manual is disabled' do - before do - stub_feature_flags(ci_fix_pipeline_status_for_dag_needs_manual: false) - end - - it 'does not change the subsequent job' do - expect { service.execute(build) }.not_to change { job.reload.status }.from('skipped') - end - end end context 'when variables are supplied' do diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index c4b1e2133ed..6e5d7725a7a 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -145,7 +145,7 @@ module Ci context 'when using DEFCON mode that disables fair scheduling' do before do - stub_feature_flags(ci_queueing_disaster_recovery: true) + stub_feature_flags(ci_queueing_disaster_recovery_disable_fair_scheduling: true) end context 'when all builds are pending' do @@ -269,51 +269,31 @@ module Ci let!(:unrelated_group_runner) { create(:ci_runner, :group, groups: [unrelated_group]) } it 'does not consider builds from other group runners' do - expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 6 + queue = ::Ci::Queue::BuildQueueService.new(group_runner) + + expect(queue.builds_for_group_runner.size).to eq 6 execute(group_runner) - expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 5 + expect(queue.builds_for_group_runner.size).to eq 5 execute(group_runner) - expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 4 + expect(queue.builds_for_group_runner.size).to eq 4 execute(group_runner) - expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 3 + expect(queue.builds_for_group_runner.size).to eq 3 execute(group_runner) - expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 2 + expect(queue.builds_for_group_runner.size).to eq 2 execute(group_runner) - expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 1 + expect(queue.builds_for_group_runner.size).to eq 1 execute(group_runner) - expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 0 + expect(queue.builds_for_group_runner.size).to eq 0 expect(execute(group_runner)).to be_nil end end - context 'when the use_distinct_in_register_job_object_hierarchy feature flag is enabled' do - before do - stub_feature_flags(use_distinct_in_register_job_object_hierarchy: true) - stub_feature_flags(use_distinct_for_all_object_hierarchy: true) - end - - it 'calls DISTINCT' do - expect(described_class.new(group_runner).send(:builds_for_group_runner).to_sql).to include("DISTINCT") - end - end - - context 'when the use_distinct_in_register_job_object_hierarchy feature flag is disabled' do - before do - stub_feature_flags(use_distinct_in_register_job_object_hierarchy: false) - stub_feature_flags(use_distinct_for_all_object_hierarchy: false) - end - - it 'does not call DISTINCT' do - expect(described_class.new(group_runner).send(:builds_for_group_runner).to_sql).not_to include("DISTINCT") - end - end - context 'group runner' do let(:build) { execute(group_runner) } @@ -349,8 +329,9 @@ module Ci let!(:other_build) { create(:ci_build, :pending, :queued, pipeline: pipeline) } before do - allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_project_runner) - .and_return(Ci::Build.where(id: [pending_job, other_build])) + allow_any_instance_of(::Ci::Queue::BuildQueueService) + .to receive(:execute) + .and_return(Ci::Build.where(id: [pending_job, other_build]).pluck(:id)) end it "receives second build from the queue" do @@ -361,8 +342,9 @@ module Ci context 'when single build is in queue' do before do - allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_project_runner) - .and_return(Ci::Build.where(id: pending_job)) + allow_any_instance_of(::Ci::Queue::BuildQueueService) + .to receive(:execute) + .and_return(Ci::Build.where(id: pending_job).pluck(:id)) end it "does not receive any valid result" do @@ -372,8 +354,9 @@ module Ci context 'when there is no build in queue' do before do - allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_project_runner) - .and_return(Ci::Build.none) + allow_any_instance_of(::Ci::Queue::BuildQueueService) + .to receive(:execute) + .and_return([]) end it "does not receive builds but result is valid" do @@ -721,17 +704,17 @@ module Ci include_examples 'handles runner assignment' end - context 'when joining with pending builds table' do + context 'when using pending builds table' do before do - stub_feature_flags(ci_pending_builds_queue_join: true) + stub_feature_flags(ci_pending_builds_queue_source: true) end include_examples 'handles runner assignment' end - context 'when not joining with pending builds table' do + context 'when not using pending builds table' do before do - stub_feature_flags(ci_pending_builds_queue_join: false) + stub_feature_flags(ci_pending_builds_queue_source: false) end include_examples 'handles runner assignment' diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index c71bec31984..42d6e66b38b 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -30,7 +30,7 @@ RSpec.describe Ci::RetryBuildService do project.add_reporter(reporter) end - clone_accessors = described_class.clone_accessors + clone_accessors = described_class.clone_accessors.without(described_class.extra_accessors) reject_accessors = %i[id status user token token_encrypted coverage trace runner @@ -39,7 +39,7 @@ RSpec.describe Ci::RetryBuildService do erased_at auto_canceled_by job_artifacts job_artifacts_archive job_artifacts_metadata job_artifacts_trace job_artifacts_junit job_artifacts_sast job_artifacts_secret_detection job_artifacts_dependency_scanning - job_artifacts_container_scanning job_artifacts_dast + job_artifacts_container_scanning job_artifacts_cluster_image_scanning job_artifacts_dast job_artifacts_license_scanning job_artifacts_performance job_artifacts_browser_performance job_artifacts_load_performance job_artifacts_lsif job_artifacts_terraform job_artifacts_cluster_applications @@ -98,7 +98,7 @@ RSpec.describe Ci::RetryBuildService do end clone_accessors.each do |attribute| - it "clones #{attribute} build attribute" do + it "clones #{attribute} build attribute", :aggregate_failures do expect(attribute).not_to be_in(forbidden_associations), "association #{attribute} must be `belongs_to`" expect(build.send(attribute)).not_to be_nil expect(new_build.send(attribute)).not_to be_nil @@ -134,7 +134,7 @@ RSpec.describe Ci::RetryBuildService do end end - it 'has correct number of known attributes' do + it 'has correct number of known attributes', :aggregate_failures do processed_accessors = clone_accessors + reject_accessors known_accessors = processed_accessors + ignore_accessors @@ -146,9 +146,10 @@ RSpec.describe Ci::RetryBuildService do Ci::Build.attribute_names.map(&:to_sym) + Ci::Build.attribute_aliases.keys.map(&:to_sym) + Ci::Build.reflect_on_all_associations.map(&:name) + - [:tag_list, :needs_attributes] - - current_accessors << :secrets if Gitlab.ee? + [:tag_list, :needs_attributes] - + # ee-specific accessors should be tested in ee/spec/services/ci/retry_build_service_spec.rb instead + described_class.extra_accessors - + [:dast_site_profiles_build, :dast_scanner_profiles_build] # join tables current_accessors.uniq! diff --git a/spec/services/ci/update_build_queue_service_spec.rb b/spec/services/ci/update_build_queue_service_spec.rb index 44d7809b85f..2e2ef120f1b 100644 --- a/spec/services/ci/update_build_queue_service_spec.rb +++ b/spec/services/ci/update_build_queue_service_spec.rb @@ -45,7 +45,7 @@ RSpec.describe Ci::UpdateBuildQueueService do context 'when duplicate entry exists' do before do - ::Ci::PendingBuild.create!(build: build, project: project) + create(:ci_pending_build, build: build, project: build.project) end it 'does nothing and returns build id' do @@ -66,7 +66,7 @@ RSpec.describe Ci::UpdateBuildQueueService do context 'when pending build exists' do before do - Ci::PendingBuild.create!(build: build, project: project) + create(:ci_pending_build, build: build, project: build.project) end it 'removes pending build in a transaction' do @@ -146,9 +146,7 @@ RSpec.describe Ci::UpdateBuildQueueService do context 'when duplicate entry exists' do before do - ::Ci::RunningBuild.create!( - build: build, project: project, runner: runner, runner_type: runner.runner_type - ) + create(:ci_running_build, build: build, project: project, runner: runner) end it 'does nothing and returns build id' do @@ -169,9 +167,7 @@ RSpec.describe Ci::UpdateBuildQueueService do context 'when shared runner build tracking entry exists' do before do - Ci::RunningBuild.create!( - build: build, project: project, runner: runner, runner_type: runner.runner_type - ) + create(:ci_running_build, build: build, project: project, runner: runner) end it 'removes shared runner build' do diff --git a/spec/services/clusters/applications/prometheus_health_check_service_spec.rb b/spec/services/clusters/applications/prometheus_health_check_service_spec.rb index ee47d00f700..e6c7b147ab7 100644 --- a/spec/services/clusters/applications/prometheus_health_check_service_spec.rb +++ b/spec/services/clusters/applications/prometheus_health_check_service_spec.rb @@ -42,6 +42,7 @@ RSpec.describe Clusters::Applications::PrometheusHealthCheckService, '#execute' context 'when cluster is project_type' do let_it_be(:project) { create(:project) } let_it_be(:integration) { create(:alert_management_http_integration, project: project) } + let(:applications_prometheus_healthy) { true } let(:prometheus) { create(:clusters_applications_prometheus, status: prometheus_status_value, healthy: applications_prometheus_healthy) } let(:cluster) { create(:cluster, :project, application_prometheus: prometheus, projects: [project]) } diff --git a/spec/services/commits/commit_patch_service_spec.rb b/spec/services/commits/commit_patch_service_spec.rb index 55cbd0e5d66..edd0918e488 100644 --- a/spec/services/commits/commit_patch_service_spec.rb +++ b/spec/services/commits/commit_patch_service_spec.rb @@ -87,7 +87,7 @@ RSpec.describe Commits::CommitPatchService do context 'when specifying a non existent start branch' do let(:start_branch) { 'does-not-exist' } - it_behaves_like 'an error response', 'Invalid reference name' + it_behaves_like 'an error response', 'Failed to create branch' end end end diff --git a/spec/services/container_expiration_policy_service_spec.rb b/spec/services/container_expiration_policy_service_spec.rb index 4294e6b3f06..41dd890dd35 100644 --- a/spec/services/container_expiration_policy_service_spec.rb +++ b/spec/services/container_expiration_policy_service_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe ContainerExpirationPolicyService do let_it_be(:user) { create(:user) } let_it_be(:container_expiration_policy) { create(:container_expiration_policy, :runnable) } + let(:project) { container_expiration_policy.project } let(:container_repository) { create(:container_repository, project: project) } diff --git a/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb b/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb index 40a2f954786..1c8ae860d10 100644 --- a/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb +++ b/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe DependencyProxy::FindOrCreateManifestService do let_it_be(:image) { 'alpine' } let_it_be(:tag) { 'latest' } let_it_be(:dependency_proxy_manifest) { create(:dependency_proxy_manifest, file_name: "#{image}:#{tag}.json") } + let(:manifest) { dependency_proxy_manifest.file.read } let(:group) { dependency_proxy_manifest.group } let(:token) { Digest::SHA256.hexdigest('123') } diff --git a/spec/services/design_management/copy_design_collection/copy_service_spec.rb b/spec/services/design_management/copy_design_collection/copy_service_spec.rb index 186d2481c19..89a78c9bf5f 100644 --- a/spec/services/design_management/copy_design_collection/copy_service_spec.rb +++ b/spec/services/design_management/copy_design_collection/copy_service_spec.rb @@ -191,8 +191,8 @@ RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitla expect(commits_on_master(limit: 99)).to include(*target_issue.design_versions.ordered.pluck(:sha)) end - it 'creates a master branch if none previously existed' do - expect { subject }.to change { target_repository.branch_names }.from([]).to(['master']) + it 'creates a default branch if none previously existed' do + expect { subject }.to change { target_repository.branch_names }.from([]).to([project.design_repository.root_ref]) end it 'does not create default branch when one exists' do @@ -255,7 +255,7 @@ RSpec.describe DesignManagement::CopyDesignCollection::CopyService, :clean_gitla end def commits_on_master(limit: 10) - target_repository.commits('master', limit: limit).map(&:id) + target_repository.commits(target_repository.root_ref, limit: limit).map(&:id) end end end diff --git a/spec/services/design_management/copy_design_collection/queue_service_spec.rb b/spec/services/design_management/copy_design_collection/queue_service_spec.rb index 2d9ea4633a0..05a7b092ccf 100644 --- a/spec/services/design_management/copy_design_collection/queue_service_spec.rb +++ b/spec/services/design_management/copy_design_collection/queue_service_spec.rb @@ -39,7 +39,7 @@ RSpec.describe DesignManagement::CopyDesignCollection::QueueService, :clean_gitl expect { subject }.to change { target_issue.design_collection.copy_state }.from('ready').to('in_progress') end - it 'queues a DesignManagement::CopyDesignCollectionWorker' do + it 'queues a DesignManagement::CopyDesignCollectionWorker', :clean_gitlab_redis_queues do expect { subject }.to change(DesignManagement::CopyDesignCollectionWorker.jobs, :size).by(1) end diff --git a/spec/services/design_management/save_designs_service_spec.rb b/spec/services/design_management/save_designs_service_spec.rb index 5bc763cc95e..b76c91fbac9 100644 --- a/spec/services/design_management/save_designs_service_spec.rb +++ b/spec/services/design_management/save_designs_service_spec.rb @@ -177,6 +177,18 @@ RSpec.describe DesignManagement::SaveDesignsService do end end + context 'when HEAD branch is different from master' do + before do + stub_feature_flags(main_branch_over_master: true) + end + + it 'does not raise an exception during update' do + run_service + + expect { run_service }.not_to raise_error + end + end + context 'when a design is being updated' do before do run_service @@ -343,7 +355,7 @@ RSpec.describe DesignManagement::SaveDesignsService do path = File.join(build(:design, issue: issue, filename: filename).full_path) design_repository.create_if_not_exists design_repository.create_file(user, path, 'something fake', - branch_name: 'master', + branch_name: project.default_branch_or_main, message: 'Somehow created without being tracked in db') end diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb index 24de1d90526..9cc27973bcb 100644 --- a/spec/services/discussions/resolve_service_spec.rb +++ b/spec/services/discussions/resolve_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe Discussions::ResolveService do let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user, developer_projects: [project]) } let_it_be(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds, source_project: project) } + let(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion } let(:service) { described_class.new(project, user, one_or_more_discussions: discussion) } @@ -100,6 +101,7 @@ RSpec.describe Discussions::ResolveService do context 'when discussion is not for a merge request' do let_it_be(:design) { create(:design, :with_file, issue: create(:issue, project: project)) } + let(:discussion) { create(:diff_note_on_design, noteable: design, project: project).to_discussion } it 'does not execute the notification service' do diff --git a/spec/services/discussions/unresolve_service_spec.rb b/spec/services/discussions/unresolve_service_spec.rb index 6298a00a474..0009239232c 100644 --- a/spec/services/discussions/unresolve_service_spec.rb +++ b/spec/services/discussions/unresolve_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe Discussions::UnresolveService do let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user, developer_projects: [project]) } let_it_be(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds, source_project: project) } + let(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion } let(:service) { described_class.new(discussion, user) } diff --git a/spec/services/error_tracking/collect_error_service_spec.rb b/spec/services/error_tracking/collect_error_service_spec.rb new file mode 100644 index 00000000000..14cd588f40b --- /dev/null +++ b/spec/services/error_tracking/collect_error_service_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ErrorTracking::CollectErrorService do + let_it_be(:project) { create(:project) } + let_it_be(:parsed_event) { Gitlab::Json.parse(fixture_file('error_tracking/parsed_event.json')) } + + subject { described_class.new(project, nil, event: parsed_event) } + + describe '#execute' do + it 'creates Error and creates ErrorEvent' do + expect { subject.execute } + .to change { ErrorTracking::Error.count }.by(1) + .and change { ErrorTracking::ErrorEvent.count }.by(1) + end + + it 'updates Error and created ErrorEvent on second hit' do + subject.execute + + expect { subject.execute }.not_to change { ErrorTracking::Error.count } + expect { subject.execute }.to change { ErrorTracking::ErrorEvent.count }.by(1) + end + + it 'has correct values set' do + subject.execute + + event = ErrorTracking::ErrorEvent.last + error = event.error + + expect(error.name).to eq 'ActionView::MissingTemplate' + expect(error.description).to start_with 'Missing template posts/error2' + expect(error.actor).to eq 'PostsController#error2' + expect(error.platform).to eq 'ruby' + expect(error.last_seen_at).to eq '2021-07-08T12:59:16Z' + + expect(event.description).to eq 'ActionView::MissingTemplate' + expect(event.occurred_at).to eq '2021-07-08T12:59:16Z' + expect(event.level).to eq 'error' + expect(event.environment).to eq 'development' + expect(event.payload).to eq parsed_event + end + end +end diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 17b2c7b38e1..611e821f3e5 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe EventCreateService do +RSpec.describe EventCreateService, :clean_gitlab_redis_cache, :clean_gitlab_redis_shared_state do let(:service) { described_class.new } let_it_be(:user, reload: true) { create :user } @@ -50,7 +50,7 @@ RSpec.describe EventCreateService do end end - describe 'Merge Requests', :clean_gitlab_redis_shared_state do + describe 'Merge Requests' do describe '#open_mr' do subject(:open_mr) { service.open_mr(merge_request, merge_request.author) } @@ -194,7 +194,7 @@ RSpec.describe EventCreateService do end end - describe '#wiki_event', :clean_gitlab_redis_shared_state do + describe '#wiki_event' do let_it_be(:user) { create(:user) } let_it_be(:wiki_page) { create(:wiki_page) } let_it_be(:meta) { create(:wiki_page_meta, :for_wiki_page, wiki_page: wiki_page) } @@ -247,7 +247,7 @@ RSpec.describe EventCreateService do end end - describe '#push', :clean_gitlab_redis_shared_state do + describe '#push' do let(:push_data) do { commits: [ @@ -272,7 +272,7 @@ RSpec.describe EventCreateService do end end - describe '#bulk_push', :clean_gitlab_redis_shared_state do + describe '#bulk_push' do let(:push_data) do { action: :created, @@ -306,7 +306,7 @@ RSpec.describe EventCreateService do end end - describe 'design events', :clean_gitlab_redis_shared_state do + describe 'design events' do let_it_be(:design) { create(:design, project: project) } let_it_be(:author) { user } @@ -318,7 +318,8 @@ RSpec.describe EventCreateService do specify { expect { result }.to change { Event.count }.by(8) } - specify { expect { result }.not_to exceed_query_limit(1) } + # An addditional query due to event tracking + specify { expect { result }.not_to exceed_query_limit(2) } it 'creates 3 created design events' do ids = result.pluck('id') @@ -347,7 +348,8 @@ RSpec.describe EventCreateService do specify { expect { result }.to change { Event.count }.by(5) } - specify { expect { result }.not_to exceed_query_limit(1) } + # An addditional query due to event tracking + specify { expect { result }.not_to exceed_query_limit(2) } it 'creates 5 destroyed design events' do ids = result.pluck('id') diff --git a/spec/services/git/base_hooks_service_spec.rb b/spec/services/git/base_hooks_service_spec.rb index 4ab27c7ab05..539c294a2e7 100644 --- a/spec/services/git/base_hooks_service_spec.rb +++ b/spec/services/git/base_hooks_service_spec.rb @@ -59,7 +59,7 @@ RSpec.describe Git::BaseHooksService do end end - describe 'project hooks and services' do + describe 'project hooks and integrations' do context 'hooks' do before do expect(project).to receive(:has_active_hooks?).and_return(active) @@ -88,45 +88,45 @@ RSpec.describe Git::BaseHooksService do end end - context 'services' do + context 'with integrations' do before do - expect(project).to receive(:has_active_services?).and_return(active) + expect(project).to receive(:has_active_integrations?).and_return(active) end - context 'active services' do + context 'with active integrations' do let(:active) { true } it 'executes the services' do expect(subject).to receive(:push_data).at_least(:once).and_call_original - expect(project).to receive(:execute_services) + expect(project).to receive(:execute_integrations) subject.execute end end - context 'inactive services' do + context 'with inactive integrations' do let(:active) { false } it 'does not execute the services' do expect(subject).not_to receive(:push_data) - expect(project).not_to receive(:execute_services) + expect(project).not_to receive(:execute_integrations) subject.execute end end end - context 'execute_project_hooks param set to false' do + context 'when execute_project_hooks param is set to false' do before do params[:execute_project_hooks] = false allow(project).to receive(:has_active_hooks?).and_return(true) - allow(project).to receive(:has_active_services?).and_return(true) + allow(project).to receive(:has_active_integrations?).and_return(true) end - it 'does not execute hooks and services' do + it 'does not execute hooks and integrations' do expect(project).not_to receive(:execute_hooks) - expect(project).not_to receive(:execute_services) + expect(project).not_to receive(:execute_integrations) subject.execute end diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index cc3ba21f002..fc629fe583d 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe Git::BranchPushService, services: true do let_it_be(:user) { create(:user) } let_it_be(:project, reload: true) { create(:project, :repository) } + let(:blankrev) { Gitlab::Git::BLANK_SHA } let(:oldrev) { sample_commit.parent_id } let(:newrev) { sample_commit.id } @@ -411,13 +412,13 @@ RSpec.describe Git::BranchPushService, services: true do context "for jira issue tracker" do include JiraServiceHelper - let(:jira_tracker) { project.create_jira_service if project.jira_service.nil? } + let(:jira_tracker) { project.create_jira_integration if project.jira_integration.nil? } before do - # project.create_jira_service doesn't seem to invalidate the cache here + # project.create_jira_integration doesn't seem to invalidate the cache here project.has_external_issue_tracker = true - stub_jira_service_test - jira_service_settings + stub_jira_integration_test + jira_integration_settings stub_jira_urls("JIRA-1") allow(closing_commit).to receive_messages({ @@ -553,24 +554,13 @@ RSpec.describe Git::BranchPushService, services: true do end end - describe "housekeeping" do + describe "housekeeping", :clean_gitlab_redis_cache, :clean_gitlab_redis_queues, :clean_gitlab_redis_shared_state do let(:housekeeping) { Repositories::HousekeepingService.new(project) } before do - # Flush any raw key-value data stored by the housekeeping code. - Gitlab::Redis::Cache.with { |conn| conn.flushall } - Gitlab::Redis::Queues.with { |conn| conn.flushall } - Gitlab::Redis::SharedState.with { |conn| conn.flushall } - allow(Repositories::HousekeepingService).to receive(:new).and_return(housekeeping) end - after do - Gitlab::Redis::Cache.with { |conn| conn.flushall } - Gitlab::Redis::Queues.with { |conn| conn.flushall } - Gitlab::Redis::SharedState.with { |conn| conn.flushall } - end - it 'does not perform housekeeping when not needed' do expect(housekeeping).not_to receive(:execute) @@ -707,6 +697,7 @@ RSpec.describe Git::BranchPushService, services: true do context 'Jira Connect hooks' do let_it_be(:project) { create(:project, :repository) } + let(:branch_to_sync) { nil } let(:commits_to_sync) { [] } let(:params) do diff --git a/spec/services/git/wiki_push_service_spec.rb b/spec/services/git/wiki_push_service_spec.rb index 151c2a1d014..7e5d7066e89 100644 --- a/spec/services/git/wiki_push_service_spec.rb +++ b/spec/services/git/wiki_push_service_spec.rb @@ -5,11 +5,12 @@ require 'spec_helper' RSpec.describe Git::WikiPushService, services: true do include RepoHelpers + let_it_be(:current_user) { create(:user) } let_it_be(:key_id) { create(:key, user: current_user).shell_id } - let_it_be(:wiki) { create(:project_wiki) } - let_it_be(:current_user) { wiki.container.default_owner } - let_it_be(:git_wiki) { wiki.wiki } - let_it_be(:repository) { wiki.repository } + + let(:wiki) { create(:project_wiki, user: current_user) } + let(:git_wiki) { wiki.wiki } + let(:repository) { wiki.repository } describe '#execute' do it 'executes model-specific callbacks' do @@ -64,6 +65,26 @@ RSpec.describe Git::WikiPushService, services: true do expect(Event.last(count).pluck(:action)).to match_array(Event::WIKI_ACTIONS.map(&:to_s)) end + + context 'when wiki_page slug is not UTF-8 ' do + let(:binary_title) { Gitlab::EncodingHelper.encode_binary('编码') } + + def run_service + wiki_page = create(:wiki_page, wiki: wiki, title: "#{binary_title} 'foo'") + + process_changes do + # Test that new_path is converted to UTF-8 + create(:wiki_page, wiki: wiki, title: binary_title) + + # Test that old_path is also is converted to UTF-8 + update_page(wiki_page.title, 'foo') + end + end + + it 'does not raise an error' do + expect { run_service }.not_to raise_error + end + end end context 'two pages have been created' do @@ -345,9 +366,10 @@ RSpec.describe Git::WikiPushService, services: true do ::Wikis::CreateAttachmentService.new(container: wiki.container, current_user: current_user, params: params).execute end - def update_page(title) + def update_page(title, new_title = nil) + new_title = title unless new_title.present? page = git_wiki.page(title: title) - git_wiki.update_page(page.path, title, 'markdown', 'Hey', commit_details) + git_wiki.update_page(page.path, new_title, 'markdown', 'Hey', commit_details) end def delete_page(page) diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb index b59ee894fe8..bcba39b0eb4 100644 --- a/spec/services/groups/create_service_spec.rb +++ b/spec/services/groups/create_service_spec.rb @@ -161,7 +161,7 @@ RSpec.describe Groups::CreateService, '#execute' do let(:created_group) { service.execute } context 'with an active instance-level integration' do - let!(:instance_integration) { create(:prometheus_service, :instance, api_url: 'https://prometheus.instance.com/') } + let!(:instance_integration) { create(:prometheus_integration, :instance, api_url: 'https://prometheus.instance.com/') } it 'creates a service from the instance-level integration' do expect(created_group.integrations.count).to eq(1) @@ -171,7 +171,7 @@ RSpec.describe Groups::CreateService, '#execute' do context 'with an active group-level integration' do let(:service) { described_class.new(user, group_params.merge(parent_id: group.id)) } - let!(:group_integration) { create(:prometheus_service, group: group, project: nil, api_url: 'https://prometheus.group.com/') } + let!(:group_integration) { create(:prometheus_integration, group: group, project: nil, api_url: 'https://prometheus.group.com/') } let(:group) do create(:group).tap do |group| group.add_owner(user) @@ -186,7 +186,7 @@ RSpec.describe Groups::CreateService, '#execute' do context 'with an active subgroup' do let(:service) { described_class.new(user, group_params.merge(parent_id: subgroup.id)) } - let!(:subgroup_integration) { create(:prometheus_service, group: subgroup, project: nil, api_url: 'https://prometheus.subgroup.com/') } + let!(:subgroup_integration) { create(:prometheus_integration, group: subgroup, project: nil, api_url: 'https://prometheus.subgroup.com/') } let(:subgroup) do create(:group, parent: group).tap do |subgroup| subgroup.add_owner(user) diff --git a/spec/services/groups/group_links/destroy_service_spec.rb b/spec/services/groups/group_links/destroy_service_spec.rb index 97fe23e9147..e63adc07313 100644 --- a/spec/services/groups/group_links/destroy_service_spec.rb +++ b/spec/services/groups/group_links/destroy_service_spec.rb @@ -24,7 +24,7 @@ RSpec.describe Groups::GroupLinks::DestroyService, '#execute' do expect { subject.execute(link) }.to change { shared_group.shared_with_group_links.count }.from(1).to(0) end - it 'revokes project authorization' do + it 'revokes project authorization', :sidekiq_inline do group.add_developer(user) expect { subject.execute(link) }.to( @@ -47,8 +47,8 @@ RSpec.describe Groups::GroupLinks::DestroyService, '#execute' do it 'updates project authorization once per group' do expect(GroupGroupLink).to receive(:delete).and_call_original - expect(group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true).once - expect(another_group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true).once + expect(group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true, blocking: false).once + expect(another_group).to receive(:refresh_members_authorized_projects).with(direct_members_only: true, blocking: false).once subject.execute(links) end diff --git a/spec/services/groups/group_links/update_service_spec.rb b/spec/services/groups/group_links/update_service_spec.rb index 82c4a10f15a..31446c8e4bf 100644 --- a/spec/services/groups/group_links/update_service_spec.rb +++ b/spec/services/groups/group_links/update_service_spec.rb @@ -36,7 +36,7 @@ RSpec.describe Groups::GroupLinks::UpdateService, '#execute' do expect(link.expires_at).to eq(expiry_date) end - it 'updates project permissions' do + it 'updates project permissions', :sidekiq_inline do expect { subject }.to change { group_member_user.can?(:create_release, project) }.from(true).to(false) end diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index 2fbd5eeef5f..889b5551746 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -241,7 +241,7 @@ RSpec.describe Groups::TransferService do context 'when the group is allowed to be transferred' do let_it_be(:new_parent_group, reload: true) { create(:group, :public) } - let_it_be(:new_parent_group_integration) { create(:slack_service, group: new_parent_group, project: nil, webhook: 'http://new-group.slack.com') } + let_it_be(:new_parent_group_integration) { create(:integrations_slack, group: new_parent_group, project: nil, webhook: 'http://new-group.slack.com') } before do allow(PropagateIntegrationWorker).to receive(:perform_async) @@ -277,8 +277,8 @@ RSpec.describe Groups::TransferService do let(:new_created_integration) { Integration.find_by(group: group) } context 'with an inherited integration' do - let_it_be(:instance_integration) { create(:slack_service, :instance, webhook: 'http://project.slack.com') } - let_it_be(:group_integration) { create(:slack_service, group: group, project: nil, webhook: 'http://group.slack.com', inherit_from_id: instance_integration.id) } + let_it_be(:instance_integration) { create(:integrations_slack, :instance, webhook: 'http://project.slack.com') } + let_it_be(:group_integration) { create(:integrations_slack, group: group, project: nil, webhook: 'http://group.slack.com', inherit_from_id: instance_integration.id) } it 'replaces inherited integrations', :aggregate_failures do expect(new_created_integration.webhook).to eq(new_parent_group_integration.webhook) @@ -288,7 +288,7 @@ RSpec.describe Groups::TransferService do end context 'with a custom integration' do - let_it_be(:group_integration) { create(:slack_service, group: group, project: nil, webhook: 'http://group.slack.com') } + let_it_be(:group_integration) { create(:integrations_slack, group: group, project: nil, webhook: 'http://group.slack.com') } it 'does not updates the integrations', :aggregate_failures do expect { transfer_service.execute(new_parent_group) }.not_to change { group_integration.webhook } diff --git a/spec/services/import/bitbucket_server_service_spec.rb b/spec/services/import/bitbucket_server_service_spec.rb index c548e87b040..56d93625b91 100644 --- a/spec/services/import/bitbucket_server_service_spec.rb +++ b/spec/services/import/bitbucket_server_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Import::BitbucketServerService do let_it_be(:user) { create(:user) } + let(:base_uri) { "https://test:7990" } let(:token) { "asdasd12345" } let(:secret) { "sekrettt" } diff --git a/spec/services/incident_management/incidents/create_service_spec.rb b/spec/services/incident_management/incidents/create_service_spec.rb index 4601bd807d0..0f32a4b5425 100644 --- a/spec/services/incident_management/incidents/create_service_spec.rb +++ b/spec/services/incident_management/incidents/create_service_spec.rb @@ -5,6 +5,7 @@ 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 diff --git a/spec/services/incident_management/incidents/update_severity_service_spec.rb b/spec/services/incident_management/incidents/update_severity_service_spec.rb deleted file mode 100644 index bc1abf82cf2..00000000000 --- a/spec/services/incident_management/incidents/update_severity_service_spec.rb +++ /dev/null @@ -1,86 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe IncidentManagement::Incidents::UpdateSeverityService do - let_it_be(:user) { create(:user) } - - describe '#execute' do - let(:severity) { 'low' } - let(:system_note_worker) { ::IncidentManagement::AddSeveritySystemNoteWorker } - - subject(:update_severity) { described_class.new(issuable, user, severity).execute } - - before do - allow(system_note_worker).to receive(:perform_async) - end - - shared_examples 'adds a system note' do - it 'calls AddSeveritySystemNoteWorker' do - update_severity - - expect(system_note_worker).to have_received(:perform_async).with(issuable.id, user.id) - end - end - - context 'when issuable not an incident' do - %i(issue merge_request).each do |issuable_type| - let(:issuable) { build_stubbed(issuable_type) } - - it { is_expected.to be_nil } - - it 'does not set severity' do - expect { update_severity }.not_to change(IssuableSeverity, :count) - end - - it 'does not add a system note' do - update_severity - - expect(system_note_worker).not_to have_received(:perform_async) - end - end - end - - context 'when issuable is an incident' do - let!(:issuable) { create(:incident) } - - context 'when issuable does not have issuable severity yet' do - it 'creates new record' do - expect { update_severity }.to change { IssuableSeverity.where(issue: issuable).count }.to(1) - end - - it 'sets severity to specified value' do - expect { update_severity }.to change { issuable.severity }.to('low') - end - - it_behaves_like 'adds a system note' - end - - context 'when issuable has an issuable severity' do - let!(:issuable_severity) { create(:issuable_severity, issue: issuable, severity: 'medium') } - - it 'does not create new record' do - expect { update_severity }.not_to change(IssuableSeverity, :count) - end - - it 'updates existing issuable severity' do - expect { update_severity }.to change { issuable_severity.severity }.to(severity) - end - - it_behaves_like 'adds a system note' - end - - context 'when severity value is unsupported' do - let(:severity) { 'unsupported-severity' } - - it 'sets the severity to default value' do - update_severity - - expect(issuable.issuable_severity.severity).to eq(IssuableSeverity::DEFAULT) - end - - it_behaves_like 'adds a system note' - 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 73ad0532e07..fb536df5d17 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 @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe IncidentManagement::PagerDuty::CreateIncidentIssueService do let_it_be(:project, reload: true) { create(:project) } let_it_be(:user) { User.alert_bot } + let(:webhook_payload) { Gitlab::Json.parse(fixture_file('pager_duty/webhook_incident_trigger.json')) } let(:parsed_payload) { ::PagerDuty::WebhookPayloadParser.call(webhook_payload) } let(:incident_payload) { parsed_payload.first['incident'] } 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 0caffb16f42..8b6eb21c25d 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 @@ -95,6 +95,7 @@ RSpec.describe IncidentManagement::PagerDuty::ProcessWebhookService do 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 diff --git a/spec/services/integrations/test/project_service_spec.rb b/spec/services/integrations/test/project_service_spec.rb index 052b25b0f10..32f9f632d7a 100644 --- a/spec/services/integrations/test/project_service_spec.rb +++ b/spec/services/integrations/test/project_service_spec.rb @@ -7,7 +7,8 @@ RSpec.describe Integrations::Test::ProjectService do describe '#execute' do let_it_be(:project) { create(:project) } - let(:integration) { create(:slack_service, project: project) } + + let(:integration) { create(:integrations_slack, project: project) } let(:user) { project.owner } let(:event) { nil } let(:sample_data) { { data: 'sample' } } @@ -23,8 +24,8 @@ RSpec.describe Integrations::Test::ProjectService do expect(subject).to eq(success_result) end - context 'PipelinesEmailService' do - let(:integration) { create(:pipelines_email_service, project: project) } + context 'with Integrations::PipelinesEmail' do + let(:integration) { create(:pipelines_email_integration, project: project) } it_behaves_like 'tests for integration with pipeline data' end @@ -32,7 +33,7 @@ RSpec.describe Integrations::Test::ProjectService do context 'with event specified' do context 'event not supported by integration' do - let(:integration) { create(:jira_service, project: project) } + let(:integration) { create(:jira_integration, project: project) } let(:event) { 'push' } it 'returns error message' do @@ -131,6 +132,7 @@ RSpec.describe Integrations::Test::ProjectService do context 'deployment' do let_it_be(:project) { create(:project, :test_repo) } + let(:deployment) { build(:deployment) } let(:event) { 'deployment' } @@ -168,6 +170,7 @@ RSpec.describe Integrations::Test::ProjectService do context 'wiki_page' do let_it_be(:project) { create(:project, :wiki_repo) } + let(:event) { 'wiki_page' } it 'returns error message if wiki disabled' do diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb index dfdfb57111c..55e0e799c19 100644 --- a/spec/services/issuable/bulk_update_service_spec.rb +++ b/spec/services/issuable/bulk_update_service_spec.rb @@ -314,6 +314,7 @@ RSpec.describe Issuable::BulkUpdateService do context 'with issuables at a group level' do let_it_be(:group) { create(:group) } + let(:parent) { group } before do diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 0b315422be8..9a70de80123 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -81,7 +81,7 @@ RSpec.describe Issues::CloseService do describe '#close_issue' do context 'with external issue' do context 'with an active external issue tracker supporting close_issue' do - let!(:external_issue_tracker) { create(:jira_service, project: project) } + let!(:external_issue_tracker) { create(:jira_integration, project: project) } it 'closes the issue on the external issue tracker' do project.reload @@ -92,7 +92,7 @@ RSpec.describe Issues::CloseService do end context 'with inactive external issue tracker supporting close_issue' do - let!(:external_issue_tracker) { create(:jira_service, project: project, active: false) } + let!(:external_issue_tracker) { create(:jira_integration, project: project, active: false) } it 'does not close the issue on the external issue tracker' do project.reload @@ -323,7 +323,7 @@ RSpec.describe Issues::CloseService do context 'when issue is not confidential' do it 'executes issue hooks' do expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) - expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks) + expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :issue_hooks) described_class.new(project: project, current_user: user).close_issue(issue) end @@ -334,7 +334,7 @@ RSpec.describe Issues::CloseService do issue = create(:issue, :confidential, project: project) expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks) - expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks) + expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :confidential_issue_hooks) described_class.new(project: project, current_user: user).close_issue(issue) end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 94810d6134a..b073ffd291f 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -8,11 +8,17 @@ RSpec.describe Issues::CreateService do let_it_be_with_reload(:project) { create(:project) } let_it_be(:user) { create(:user) } + let(:spam_params) { double } + describe '#execute' do let_it_be(:assignee) { create(:user) } let_it_be(:milestone) { create(:milestone, project: project) } - let(:issue) { described_class.new(project: project, current_user: user, params: opts).execute } + let(:issue) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute } + + before do + stub_spam_services + end context 'when params are valid' do let_it_be(:labels) { create_pair(:label, project: project) } @@ -44,7 +50,7 @@ RSpec.describe Issues::CreateService do end context 'when skip_system_notes is true' do - let(:issue) { described_class.new(project: project, current_user: user, params: opts).execute(skip_system_notes: true) } + let(:issue) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute(skip_system_notes: true) } it 'does not call Issuable::CommonSystemNotesService' do expect(Issuable::CommonSystemNotesService).not_to receive(:new) @@ -92,7 +98,7 @@ RSpec.describe Issues::CreateService do let_it_be(:non_member) { create(:user) } it 'filters out params that cannot be set without the :set_issue_metadata permission' do - issue = described_class.new(project: project, current_user: non_member, params: opts).execute + issue = described_class.new(project: project, current_user: non_member, params: opts, spam_params: spam_params).execute expect(issue).to be_persisted expect(issue.title).to eq('Awesome issue') @@ -104,7 +110,7 @@ RSpec.describe Issues::CreateService do end it 'can create confidential issues' do - issue = described_class.new(project: project, current_user: non_member, params: { confidential: true }).execute + issue = described_class.new(project: project, current_user: non_member, params: { confidential: true }, spam_params: spam_params).execute expect(issue.confidential).to be_truthy end @@ -113,7 +119,7 @@ RSpec.describe Issues::CreateService do it 'moves the issue to the end, in an asynchronous worker' do expect(IssuePlacementWorker).to receive(:perform_async).with(be_nil, Integer) - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute end context 'when label belongs to project group' do @@ -200,7 +206,7 @@ RSpec.describe Issues::CreateService do it 'invalidates open issues counter for assignees when issue is assigned' do project.add_maintainer(assignee) - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(assignee.assigned_open_issues_count).to eq 1 end @@ -224,18 +230,18 @@ RSpec.describe Issues::CreateService do opts = { title: 'Title', description: 'Description', confidential: false } expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) - expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks) + expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :issue_hooks) - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute end it 'executes confidential issue hooks when issue is confidential' do opts = { title: 'Title', description: 'Description', confidential: true } expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks) - expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks) + expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :confidential_issue_hooks) - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute end context 'after_save callback to store_mentions' do @@ -279,7 +285,7 @@ RSpec.describe Issues::CreateService do it 'removes assignee when user id is invalid' do opts = { title: 'Title', description: 'Description', assignee_ids: [-1] } - issue = described_class.new(project: project, current_user: user, params: opts).execute + issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(issue.assignees).to be_empty end @@ -287,7 +293,7 @@ RSpec.describe Issues::CreateService do it 'removes assignee when user id is 0' do opts = { title: 'Title', description: 'Description', assignee_ids: [0] } - issue = described_class.new(project: project, current_user: user, params: opts).execute + issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(issue.assignees).to be_empty end @@ -296,7 +302,7 @@ RSpec.describe Issues::CreateService do project.add_maintainer(assignee) opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } - issue = described_class.new(project: project, current_user: user, params: opts).execute + issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(issue.assignees).to eq([assignee]) end @@ -314,7 +320,7 @@ RSpec.describe Issues::CreateService do project.update!(visibility_level: level) opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } - issue = described_class.new(project: project, current_user: user, params: opts).execute + issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(issue.assignees).to be_empty end @@ -324,7 +330,7 @@ RSpec.describe Issues::CreateService do end it_behaves_like 'issuable record that supports quick actions' do - let(:issuable) { described_class.new(project: project, current_user: user, params: params).execute } + let(:issuable) { described_class.new(project: project, current_user: user, params: params, spam_params: spam_params).execute } end context 'Quick actions' do @@ -364,14 +370,14 @@ RSpec.describe Issues::CreateService do let(:opts) { { discussion_to_resolve: discussion.id, merge_request_to_resolve_discussions_of: merge_request.iid } } it 'resolves the discussion' do - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute discussion.first_note.reload expect(discussion.resolved?).to be(true) end it 'added a system note to the discussion' do - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute reloaded_discussion = MergeRequest.find(merge_request.id).discussions.first @@ -379,7 +385,7 @@ RSpec.describe Issues::CreateService do end it 'assigns the title and description for the issue' do - issue = described_class.new(project: project, current_user: user, params: opts).execute + issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(issue.title).not_to be_nil expect(issue.description).not_to be_nil @@ -391,7 +397,8 @@ RSpec.describe Issues::CreateService do merge_request_to_resolve_discussions_of: merge_request, description: nil, title: nil - }).execute + }, + spam_params: spam_params).execute expect(issue.description).to be_nil expect(issue.title).to be_nil @@ -402,14 +409,14 @@ RSpec.describe Issues::CreateService do let(:opts) { { merge_request_to_resolve_discussions_of: merge_request.iid } } it 'resolves the discussion' do - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute discussion.first_note.reload expect(discussion.resolved?).to be(true) end it 'added a system note to the discussion' do - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute reloaded_discussion = MergeRequest.find(merge_request.id).discussions.first @@ -417,7 +424,7 @@ RSpec.describe Issues::CreateService do end it 'assigns the title and description for the issue' do - issue = described_class.new(project: project, current_user: user, params: opts).execute + issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(issue.title).not_to be_nil expect(issue.description).not_to be_nil @@ -429,7 +436,8 @@ RSpec.describe Issues::CreateService do merge_request_to_resolve_discussions_of: merge_request, description: nil, title: nil - }).execute + }, + spam_params: spam_params).execute expect(issue.description).to be_nil expect(issue.title).to be_nil @@ -438,47 +446,27 @@ RSpec.describe Issues::CreateService do end context 'checking spam' do - let(:request) { double(:request, headers: nil) } - let(:api) { true } - let(:captcha_response) { 'abc123' } - let(:spam_log_id) { 1 } - let(:params) do { - title: 'Spam issue', - request: request, - api: api, - captcha_response: captcha_response, - spam_log_id: spam_log_id + title: 'Spam issue' } end subject do - described_class.new(project: project, current_user: user, params: params) - end - - before do - allow_next_instance_of(UserAgentDetailService) do |instance| - allow(instance).to receive(:create) - end + described_class.new(project: project, current_user: user, params: params, spam_params: spam_params) end it 'executes SpamActionService' do - spam_params = Spam::SpamParams.new( - api: api, - captcha_response: captcha_response, - spam_log_id: spam_log_id - ) expect_next_instance_of( Spam::SpamActionService, { - spammable: an_instance_of(Issue), - request: request, - user: user, + spammable: kind_of(Issue), + spam_params: spam_params, + user: an_instance_of(User), action: :create } ) do |instance| - expect(instance).to receive(:execute).with(spam_params: spam_params) + expect(instance).to receive(:execute) end subject.execute diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index 76588860957..36af38aef18 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -38,6 +38,10 @@ RSpec.describe Issues::MoveService do context 'issue movable' do include_context 'user can move issue' + it 'creates resource state event' do + expect { move_service.execute(old_issue, new_project) }.to change(ResourceStateEvent.where(issue_id: old_issue), :count).by(1) + end + context 'generic issue' do include_context 'issue move executed' @@ -87,6 +91,10 @@ RSpec.describe Issues::MoveService do expect(old_issue.moved_to).to eq new_issue end + it 'marks issue as closed' do + expect(old_issue.closed?).to eq true + end + it 'preserves create time' do expect(old_issue.created_at).to eq new_issue.created_at end diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb index 746a9105531..d58c27289c2 100644 --- a/spec/services/issues/reopen_service_spec.rb +++ b/spec/services/issues/reopen_service_spec.rb @@ -65,7 +65,7 @@ RSpec.describe Issues::ReopenService do context 'when issue is not confidential' do it 'executes issue hooks' do expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) - expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks) + expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :issue_hooks) described_class.new(project: project, current_user: user).execute(issue) end @@ -76,7 +76,7 @@ RSpec.describe Issues::ReopenService do issue = create(:issue, :confidential, :closed, project: project) expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks) - expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks) + expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :confidential_issue_hooks) described_class.new(project: project, current_user: user).execute(issue) end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index b95d94e3784..70c3c2a0f5d 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -83,16 +83,16 @@ RSpec.describe Issues::UpdateService, :mailer do end context 'when issue type is not incident' do - it 'returns default severity' do + before do update_issue(opts) - - expect(issue.severity).to eq(IssuableSeverity::DEFAULT) end - it_behaves_like 'not an incident issue' do - before do - update_issue(opts) - end + it_behaves_like 'not an incident issue' + + context 'when confidentiality is changed' do + subject { update_issue(confidential: true) } + + it_behaves_like 'does not track incident management event' end end @@ -105,12 +105,16 @@ RSpec.describe Issues::UpdateService, :mailer do it_behaves_like 'incident issue' - it 'changes updates the severity' do - expect(issue.severity).to eq('low') + it 'does not add an incident label' do + expect(issue.labels).to match_array [label] end - it 'does not apply incident labels' do - expect(issue.labels).to match_array [label] + context 'when confidentiality is changed' do + let(:current_user) { user } + + subject { update_issue(confidential: true) } + + it_behaves_like 'an incident management tracked event', :incident_management_incident_change_confidential end end @@ -140,24 +144,6 @@ RSpec.describe Issues::UpdateService, :mailer do expect(issue.confidential).to be_falsey end - context 'issue in incident type' do - let(:current_user) { user } - - before do - opts.merge!(issue_type: 'incident', confidential: true) - end - - subject { update_issue(opts) } - - it_behaves_like 'an incident management tracked event', :incident_management_incident_change_confidential - - it_behaves_like 'incident issue' do - before do - subject - end - end - end - context 'changing issue_type' do let!(:label_1) { create(:label, project: project, title: 'incident') } let!(:label_2) { create(:label, project: project, title: 'missed-sla') } @@ -167,6 +153,12 @@ RSpec.describe Issues::UpdateService, :mailer do end context 'from issue to incident' do + it_behaves_like 'incident issue' do + before do + update_issue(**opts, issue_type: 'incident') + end + end + it 'adds a `incident` label if one does not exist' do expect { update_issue(issue_type: 'incident') }.to change(issue.labels, :count).by(1) expect(issue.labels.pluck(:title)).to eq(['incident']) @@ -488,6 +480,21 @@ RSpec.describe Issues::UpdateService, :mailer do end end end + + it 'verifies the number of queries' do + update_issue(description: "- [ ] Task 1 #{user.to_reference}") + + baseline = ActiveRecord::QueryRecorder.new do + update_issue(description: "- [x] Task 1 #{user.to_reference}") + end + + recorded = ActiveRecord::QueryRecorder.new do + update_issue(description: "- [x] Task 1 #{user.to_reference}\n- [ ] Task 2 #{user.to_reference}") + end + + expect(recorded.count).to eq(baseline.count - 1) + expect(recorded.cached_count).to eq(0) + end end context 'when description changed' do @@ -522,7 +529,7 @@ RSpec.describe Issues::UpdateService, :mailer do it 'executes confidential issue hooks' do expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks) - expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks) + expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :confidential_issue_hooks) update_issue(confidential: true) end @@ -1005,6 +1012,101 @@ RSpec.describe Issues::UpdateService, :mailer do include_examples 'updating mentions', described_class end + context 'updating severity' do + let(:opts) { { severity: 'low' } } + + shared_examples 'updates the severity' do |expected_severity| + it 'has correct value' do + update_issue(opts) + + expect(issue.severity).to eq(expected_severity) + end + + it 'creates a system note' do + expect(::IncidentManagement::AddSeveritySystemNoteWorker).to receive(:perform_async).with(issue.id, user.id) + + update_issue(opts) + end + + it 'triggers webhooks' do + expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) + expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :issue_hooks) + + update_issue(opts) + end + end + + shared_examples 'does not change the severity' do + it 'retains the original value' do + expected_severity = issue.severity + + update_issue(opts) + + expect(issue.severity).to eq(expected_severity) + end + + it 'does not trigger side-effects' do + expect(::IncidentManagement::AddSeveritySystemNoteWorker).not_to receive(:perform_async) + expect(project).not_to receive(:execute_hooks) + expect(project).not_to receive(:execute_integrations) + + expect { update_issue(opts) }.not_to change(IssuableSeverity, :count) + end + end + + context 'on incidents' do + let(:issue) { create(:incident, project: project) } + + context 'when severity has not been set previously' do + it_behaves_like 'updates the severity', 'low' + + it 'creates a new record' do + expect { update_issue(opts) }.to change(IssuableSeverity, :count).by(1) + end + + context 'with unsupported severity value' do + let(:opts) { { severity: 'unsupported-severity' } } + + it_behaves_like 'does not change the severity' + end + + context 'with severity value defined but unchanged' do + let(:opts) { { severity: IssuableSeverity::DEFAULT } } + + it_behaves_like 'does not change the severity' + end + end + + context 'when severity has been set before' do + before do + create(:issuable_severity, issue: issue, severity: 'high') + end + + it_behaves_like 'updates the severity', 'low' + + it 'does not create a new record' do + expect { update_issue(opts) }.not_to change(IssuableSeverity, :count) + end + + context 'with unsupported severity value' do + let(:opts) { { severity: 'unsupported-severity' } } + + it_behaves_like 'updates the severity', IssuableSeverity::DEFAULT + end + + context 'with severity value defined but unchanged' do + let(:opts) { { severity: issue.severity } } + + it_behaves_like 'does not change the severity' + end + end + end + + context 'when issue type is not incident' do + it_behaves_like 'does not change the severity' + end + end + context 'duplicate issue' do let(:canonical_issue) { create(:issue, project: project) } diff --git a/spec/services/jira/requests/projects/list_service_spec.rb b/spec/services/jira/requests/projects/list_service_spec.rb index 0fff51b1226..ab15254d948 100644 --- a/spec/services/jira/requests/projects/list_service_spec.rb +++ b/spec/services/jira/requests/projects/list_service_spec.rb @@ -5,17 +5,17 @@ require 'spec_helper' RSpec.describe Jira::Requests::Projects::ListService do include AfterNextHelpers - let(:jira_service) { create(:jira_service) } + let(:jira_integration) { create(:jira_integration) } let(:params) { {} } describe '#execute' do - let(:service) { described_class.new(jira_service, params) } + let(:service) { described_class.new(jira_integration, params) } subject { service.execute } - context 'without jira_service' do + context 'without jira_integration' do before do - jira_service.update!(active: false) + jira_integration.update!(active: false) end it 'returns an error response' do @@ -24,8 +24,8 @@ RSpec.describe Jira::Requests::Projects::ListService do end end - context 'when jira_service is nil' do - let(:jira_service) { nil } + context 'when jira_integration is nil' do + let(:jira_integration) { nil } it 'returns an error response' do expect(subject.error?).to be_truthy @@ -33,11 +33,11 @@ RSpec.describe Jira::Requests::Projects::ListService do end end - context 'with jira_service' do + context 'with jira_integration' do context 'when validations and params are ok' do let(:response_headers) { { 'content-type' => 'application/json' } } let(:response_body) { [].to_json } - let(:expected_url_pattern) { /.*jira.example.com\/rest\/api\/2\/project/ } + let(:expected_url_pattern) { %r{.*jira.example.com/rest/api/2/project} } before do stub_request(:get, expected_url_pattern).to_return(status: 200, body: response_body, headers: response_headers) @@ -59,8 +59,8 @@ RSpec.describe Jira::Requests::Projects::ListService do end context 'when jira runs on a subpath' do - let(:jira_service) { create(:jira_service, url: 'http://jira.example.com/jira') } - let(:expected_url_pattern) { /.*jira.example.com\/jira\/rest\/api\/2\/project/ } + let(:jira_integration) { create(:jira_integration, url: 'http://jira.example.com/jira') } + let(:expected_url_pattern) { %r{.*jira.example.com/jira/rest/api/2/project} } it 'takes the subpath into account' do expect(subject.success?).to be_truthy diff --git a/spec/services/jira_connect/sync_service_spec.rb b/spec/services/jira_connect/sync_service_spec.rb index edd0bad70f5..c20aecaaef0 100644 --- a/spec/services/jira_connect/sync_service_spec.rb +++ b/spec/services/jira_connect/sync_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe JiraConnect::SyncService do describe '#execute' do let_it_be(:project) { create(:project, :repository) } + let(:client) { Atlassian::JiraConnect::Client } let(:info) { { a: 'Some', b: 'Info' } } diff --git a/spec/services/jira_connect_installations/destroy_service_spec.rb b/spec/services/jira_connect_installations/destroy_service_spec.rb new file mode 100644 index 00000000000..bb5bab53ccb --- /dev/null +++ b/spec/services/jira_connect_installations/destroy_service_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe JiraConnectInstallations::DestroyService do + describe '.execute' do + it 'creates an instance and calls execute' do + expect_next_instance_of(described_class, 'param1', 'param2', 'param3') do |destroy_service| + expect(destroy_service).to receive(:execute) + end + + described_class.execute('param1', 'param2', 'param3') + end + end + + describe '#execute' do + let!(:installation) { create(:jira_connect_installation) } + let(:jira_base_path) { '/-/jira_connect' } + let(:jira_event_path) { '/-/jira_connect/events/uninstalled' } + + subject { described_class.new(installation, jira_base_path, jira_event_path).execute } + + it { is_expected.to be_truthy } + + it 'deletes the installation' do + expect { subject }.to change(JiraConnectInstallation, :count).by(-1) + end + + context 'and the installation has an instance_url set' do + let!(:installation) { create(:jira_connect_installation, instance_url: 'http://example.com') } + + it { is_expected.to be_truthy } + + it 'schedules a ForwardEventWorker background job and keeps the installation' do + expect(JiraConnect::ForwardEventWorker).to receive(:perform_async).with(installation.id, jira_base_path, jira_event_path) + + expect { subject }.not_to change(JiraConnectInstallation, :count) + end + end + end +end diff --git a/spec/services/jira_import/start_import_service_spec.rb b/spec/services/jira_import/start_import_service_spec.rb index a10928355ef..e04e3314158 100644 --- a/spec/services/jira_import/start_import_service_spec.rb +++ b/spec/services/jira_import/start_import_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe JiraImport::StartImportService do let_it_be(:user) { create(:user) } let_it_be(:project, reload: true) { create(:project) } + let(:key) { 'KEY' } let(:mapping) do [ @@ -28,10 +29,10 @@ RSpec.describe JiraImport::StartImportService do end context 'when project validation is ok' do - let!(:jira_service) { create(:jira_service, project: project, active: true) } + let!(:jira_integration) { create(:jira_integration, project: project, active: true) } before do - stub_jira_service_test + stub_jira_integration_test allow(Gitlab::JiraImport).to receive(:validate_project_settings!) end diff --git a/spec/services/jira_import/users_importer_spec.rb b/spec/services/jira_import/users_importer_spec.rb index 2e8c556d62c..af408847260 100644 --- a/spec/services/jira_import/users_importer_spec.rb +++ b/spec/services/jira_import/users_importer_spec.rb @@ -33,7 +33,7 @@ RSpec.describe JiraImport::UsersImporter do end before do - stub_jira_service_test + stub_jira_integration_test project.add_maintainer(user) end @@ -45,7 +45,7 @@ RSpec.describe JiraImport::UsersImporter do RSpec.shared_examples 'maps Jira users to GitLab users' do |users_mapper_service:| context 'when Jira import is configured correctly' do - let_it_be(:jira_service) { create(:jira_service, project: project, active: true, url: "http://jira.example.net") } + let_it_be(:jira_integration) { create(:jira_integration, project: project, active: true, url: "http://jira.example.net") } context 'when users mapper service raises an error' do let(:error) { Timeout::Error.new } @@ -98,9 +98,9 @@ RSpec.describe JiraImport::UsersImporter do context 'when Jira instance is of Server deployment type' do before do - allow(project).to receive(:jira_service).and_return(jira_service) + allow(project).to receive(:jira_integration).and_return(jira_integration) - jira_service.data_fields.deployment_server! + jira_integration.data_fields.deployment_server! end it_behaves_like 'maps Jira users to GitLab users', users_mapper_service: JiraImport::ServerUsersMapperService @@ -108,9 +108,9 @@ RSpec.describe JiraImport::UsersImporter do context 'when Jira instance is of Cloud deployment type' do before do - allow(project).to receive(:jira_service).and_return(jira_service) + allow(project).to receive(:jira_integration).and_return(jira_integration) - jira_service.data_fields.deployment_cloud! + jira_integration.data_fields.deployment_cloud! end it_behaves_like 'maps Jira users to GitLab users', users_mapper_service: JiraImport::CloudUsersMapperService diff --git a/spec/services/keys/destroy_service_spec.rb b/spec/services/keys/destroy_service_spec.rb index 59ce4a941c7..dd40f9d73fd 100644 --- a/spec/services/keys/destroy_service_spec.rb +++ b/spec/services/keys/destroy_service_spec.rb @@ -8,7 +8,7 @@ RSpec.describe Keys::DestroyService do subject { described_class.new(user) } it 'destroys a key' do - key = create(:key) + key = create(:personal_key) expect { subject.execute(key) }.to change(Key, :count).by(-1) end diff --git a/spec/services/markdown_content_rewriter_service_spec.rb b/spec/services/markdown_content_rewriter_service_spec.rb index 47332bec319..37c8a210ba5 100644 --- a/spec/services/markdown_content_rewriter_service_spec.rb +++ b/spec/services/markdown_content_rewriter_service_spec.rb @@ -6,6 +6,7 @@ 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 @@ -34,6 +35,7 @@ RSpec.describe MarkdownContentRewriterService do # 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 diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index ffe63a8a94b..ee5250b5b3d 100644 --- a/spec/services/members/create_service_spec.rb +++ b/spec/services/members/create_service_spec.rb @@ -2,12 +2,13 @@ require 'spec_helper' -RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_shared_state, :sidekiq_inline do +RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_cache, :clean_gitlab_redis_shared_state, :sidekiq_inline do let_it_be(:source) { create(:project) } let_it_be(:user) { create(:user) } let_it_be(:member) { create(:user) } let_it_be(:user_ids) { member.id.to_s } let_it_be(:access_level) { Gitlab::Access::GUEST } + let(:additional_params) { { invite_source: '_invite_source_' } } let(:params) { { user_ids: user_ids, access_level: access_level }.merge(additional_params) } diff --git a/spec/services/members/groups/creator_service_spec.rb b/spec/services/members/groups/creator_service_spec.rb new file mode 100644 index 00000000000..4427c4e7d9f --- /dev/null +++ b/spec/services/members/groups/creator_service_spec.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::Groups::CreatorService do + it_behaves_like 'member creation' do + let_it_be(:source, reload: true) { create(:group, :public) } + let_it_be(:member_type) { GroupMember } + end + + describe '.access_levels' do + it 'returns Gitlab::Access.options_with_owner' do + expect(described_class.access_levels).to eq(Gitlab::Access.sym_options_with_owner) + end + end +end diff --git a/spec/services/members/invite_service_spec.rb b/spec/services/members/invite_service_spec.rb index c530e3d0c53..dd82facaf14 100644 --- a/spec/services/members/invite_service_spec.rb +++ b/spec/services/members/invite_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ let_it_be(:user) { project.owner } let_it_be(:project_user) { create(:user) } let_it_be(:namespace) { project.namespace } + let(:params) { {} } let(:base_params) { { access_level: Gitlab::Access::GUEST, source: project, invite_source: '_invite_source_' } } diff --git a/spec/services/members/projects/creator_service_spec.rb b/spec/services/members/projects/creator_service_spec.rb new file mode 100644 index 00000000000..c6917a21bcd --- /dev/null +++ b/spec/services/members/projects/creator_service_spec.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Members::Projects::CreatorService do + it_behaves_like 'member creation' do + let_it_be(:source, reload: true) { create(:project, :public) } + let_it_be(:member_type) { ProjectMember } + end + + describe '.access_levels' do + it 'returns Gitlab::Access.sym_options' do + expect(described_class.access_levels).to eq(Gitlab::Access.sym_options) + end + end +end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index d10f82289bd..0f282384661 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -252,8 +252,8 @@ RSpec.describe MergeRequests::BuildService do context 'when the source branch matches an issue' do where(:factory, :source_branch, :closing_message) do - :jira_service | 'FOO-123-fix-issue' | 'Closes FOO-123' - :jira_service | 'fix-issue' | nil + :jira_integration | 'FOO-123-fix-issue' | 'Closes FOO-123' + :jira_integration | 'fix-issue' | nil :custom_issue_tracker_integration | '123-fix-issue' | 'Closes #123' :custom_issue_tracker_integration | 'fix-issue' | nil nil | '123-fix-issue' | 'Closes #123' @@ -351,8 +351,8 @@ RSpec.describe MergeRequests::BuildService do context 'when the source branch matches an issue' do where(:factory, :source_branch, :title, :closing_message) do - :jira_service | 'FOO-123-fix-issue' | 'Resolve FOO-123 "Fix issue"' | 'Closes FOO-123' - :jira_service | 'fix-issue' | 'Fix issue' | nil + :jira_integration | 'FOO-123-fix-issue' | 'Resolve FOO-123 "Fix issue"' | 'Closes FOO-123' + :jira_integration | 'fix-issue' | 'Fix issue' | nil :custom_issue_tracker_integration | '123-fix-issue' | 'Resolve #123 "Fix issue"' | 'Closes #123' :custom_issue_tracker_integration | 'fix-issue' | 'Fix issue' | nil nil | '123-fix-issue' | 'Resolve "A bug"' | 'Closes #123' @@ -400,8 +400,8 @@ RSpec.describe MergeRequests::BuildService do context 'when the source branch matches an issue' do where(:factory, :source_branch, :title, :closing_message) do - :jira_service | 'FOO-123-fix-issue' | 'Resolve FOO-123 "Fix issue"' | 'Closes FOO-123' - :jira_service | 'fix-issue' | 'Fix issue' | nil + :jira_integration | 'FOO-123-fix-issue' | 'Resolve FOO-123 "Fix issue"' | 'Closes FOO-123' + :jira_integration | 'fix-issue' | 'Fix issue' | nil :custom_issue_tracker_integration | '123-fix-issue' | 'Resolve #123 "Fix issue"' | 'Closes #123' :custom_issue_tracker_integration | 'fix-issue' | 'Fix issue' | nil nil | '123-fix-issue' | 'Resolve "A bug"' | 'Closes #123' diff --git a/spec/services/merge_requests/handle_assignees_change_service_spec.rb b/spec/services/merge_requests/handle_assignees_change_service_spec.rb index f9eed6eea2d..c43f5db6059 100644 --- a/spec/services/merge_requests/handle_assignees_change_service_spec.rb +++ b/spec/services/merge_requests/handle_assignees_change_service_spec.rb @@ -104,9 +104,9 @@ RSpec.describe MergeRequests::HandleAssigneesChangeService do context 'when execute_hooks option is set to true' do let(:options) { { execute_hooks: true } } - it 'execute hooks and services' do + it 'executes hooks and integrations' do expect(merge_request.project).to receive(:execute_hooks).with(anything, :merge_request_hooks) - expect(merge_request.project).to receive(:execute_services).with(anything, :merge_request_hooks) + expect(merge_request.project).to receive(:execute_integrations).with(anything, :merge_request_hooks) expect(service).to receive(:enqueue_jira_connect_messages_for).with(merge_request) execute diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 503c0282bd6..b3af4d67896 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -163,14 +163,14 @@ RSpec.describe MergeRequests::MergeService do context 'with Jira integration' do include JiraServiceHelper - let(:jira_tracker) { project.create_jira_service } + let(:jira_tracker) { project.create_jira_integration } let(:jira_issue) { ExternalIssue.new('JIRA-123', project) } let(:commit) { double('commit', safe_message: "Fixes #{jira_issue.to_reference}") } before do - stub_jira_service_test + stub_jira_integration_test project.update!(has_external_issue_tracker: true) - jira_service_settings + jira_integration_settings stub_jira_urls(jira_issue.id) allow(merge_request).to receive(:commits).and_return([commit]) end diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index 87c3fc6a2d8..5f76f6f5c44 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -5,11 +5,16 @@ require 'spec_helper' RSpec.describe MergeRequests::PushOptionsHandlerService do include ProjectForksHelper - let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:parent_group) { create(:group, :public) } + let_it_be(:child_group) { create(:group, :public, parent: parent_group) } + let_it_be(:project) { create(:project, :public, :repository, group: child_group) } let_it_be(:user1) { create(:user, developer_projects: [project]) } let_it_be(:user2) { create(:user, developer_projects: [project]) } let_it_be(:user3) { create(:user, developer_projects: [project]) } let_it_be(:forked_project) { fork_project(project, user1, repository: true) } + let_it_be(:parent_group_milestone) { create(:milestone, group: parent_group, title: 'ParentGroupMilestone1.0') } + let_it_be(:child_group_milestone) { create(:milestone, group: child_group, title: 'ChildGroupMilestone1.0') } + let_it_be(:project_milestone) { create(:milestone, project: project, title: 'ProjectMilestone1.0') } let(:service) { described_class.new(project: project, current_user: user1, changes: changes, push_options: push_options) } let(:source_branch) { 'fix' } @@ -59,6 +64,16 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do end end + shared_examples_for 'a service that can set the milestone of a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'sets the milestone' do + service.execute + + expect(last_mr.milestone&.title).to eq(expected_milestone) + end + end + shared_examples_for 'a service that can set the merge request to merge when pipeline succeeds' do subject(:last_mr) { MergeRequest.last } @@ -514,6 +529,90 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'with the project default branch' end + describe '`milestone` push option' do + context 'with a valid milestone' do + let(:expected_milestone) { project_milestone.title } + let(:push_options) { { milestone: project_milestone.title } } + + context 'with a new branch' do + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + service.execute + + expect(service.errors).to include(error_mr_required) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, milestone: project_milestone.title } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the milestone of a merge request' + end + end + + context 'with an existing branch but no open MR' do + let(:changes) { existing_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + service.execute + + expect(service.errors).to include(error_mr_required) + end + + context 'when coupled with the `create` push option' do + let(:push_options) { { create: true, milestone: project_milestone.title } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the milestone of a merge request' + end + end + + context 'with an existing branch that has a merge request open' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can set the milestone of a merge request' + end + + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' + end + + context 'with invalid milestone' do + let(:expected_milestone) { nil } + let(:changes) { new_branch_changes } + let(:push_options) { { create: true, milestone: 'invalid_milestone' } } + + it_behaves_like 'a service that can set the milestone of a merge request' + end + + context 'with an ancestor milestone' do + let(:changes) { existing_branch_changes } + + context 'with immediate parent milestone' do + let(:push_options) { { create: true, milestone: child_group_milestone.title } } + let(:expected_milestone) { child_group_milestone.title } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the milestone of a merge request' + end + + context 'with multi-level ancestor milestone' do + let(:push_options) { { create: true, milestone: parent_group_milestone.title } } + let(:expected_milestone) { parent_group_milestone.title } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can set the milestone of a merge request' + end + end + end + shared_examples 'with an existing branch that has a merge request open in foss' do let(:changes) { existing_branch_changes } let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} diff --git a/spec/services/merge_requests/rebase_service_spec.rb b/spec/services/merge_requests/rebase_service_spec.rb index a46f3cf6148..ca561376581 100644 --- a/spec/services/merge_requests/rebase_service_spec.rb +++ b/spec/services/merge_requests/rebase_service_spec.rb @@ -25,30 +25,6 @@ RSpec.describe MergeRequests::RebaseService do end describe '#execute' do - context 'when another rebase is already in progress' do - before do - allow(repository).to receive(:rebase_in_progress?).with(merge_request.id).and_return(true) - end - - it 'saves the error message' do - service.execute(merge_request) - - expect(merge_request.reload.merge_error).to eq 'Rebase task canceled: Another rebase is already in progress' - end - - it 'returns an error' do - expect(service.execute(merge_request)).to match(status: :error, - message: described_class::REBASE_ERROR) - end - - it 'clears rebase_jid' do - expect { service.execute(merge_request) } - .to change { merge_request.rebase_jid } - .from(rebase_jid) - .to(nil) - end - end - shared_examples 'sequence of failure and success' do it 'properly clears the error message' do allow(repository).to receive(:gitaly_operation_client).and_raise('Something went wrong') @@ -150,6 +126,13 @@ RSpec.describe MergeRequests::RebaseService do it_behaves_like 'a service that can execute a successful rebase' + it 'clears rebase_jid' do + expect { service.execute(merge_request) } + .to change(merge_request, :rebase_jid) + .from(rebase_jid) + .to(nil) + end + context 'when skip_ci flag is set' do let(:skip_ci) { true } diff --git a/spec/services/metrics/dashboard/annotations/create_service_spec.rb b/spec/services/metrics/dashboard/annotations/create_service_spec.rb index c3fe7238047..8f5484fcabe 100644 --- a/spec/services/metrics/dashboard/annotations/create_service_spec.rb +++ b/spec/services/metrics/dashboard/annotations/create_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Metrics::Dashboard::Annotations::CreateService do let_it_be(:user) { create(:user) } + let(:description) { 'test annotation' } let(:dashboard_path) { 'config/prometheus/common_metrics.yml' } let(:starting_at) { 15.minutes.ago } 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 d5928b1b5af..2905e4599f3 100644 --- a/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/gitlab_alert_embed_service_spec.rb @@ -8,6 +8,7 @@ RSpec.describe Metrics::Dashboard::GitlabAlertEmbedService do let_it_be(:alert) { create(:prometheus_alert) } let_it_be(:project) { alert.project } let_it_be(:user) { create(:user) } + let(:alert_id) { alert.id } before_all do diff --git a/spec/services/metrics/users_starred_dashboards/create_service_spec.rb b/spec/services/metrics/users_starred_dashboards/create_service_spec.rb index 910b556b8dd..1435e39e458 100644 --- a/spec/services/metrics/users_starred_dashboards/create_service_spec.rb +++ b/spec/services/metrics/users_starred_dashboards/create_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Metrics::UsersStarredDashboards::CreateService do let_it_be(:user) { create(:user) } + let(:dashboard_path) { 'config/prometheus/common_metrics.yml' } let(:service_instance) { described_class.new(user, project, dashboard_path) } let(:project) { create(:project) } diff --git a/spec/services/namespace_settings/update_service_spec.rb b/spec/services/namespace_settings/update_service_spec.rb index 8e176dbc6cd..e0f32cb3821 100644 --- a/spec/services/namespace_settings/update_service_spec.rb +++ b/spec/services/namespace_settings/update_service_spec.rb @@ -76,34 +76,61 @@ RSpec.describe NamespaceSettings::UpdateService do end end - context "updating :prevent_sharing_groups_outside_hierarchy" do - let(:settings) { { prevent_sharing_groups_outside_hierarchy: true } } + describe 'validating settings param for root group' do + using RSpec::Parameterized::TableSyntax - context 'when user is a group owner' do - before do - group.add_owner(user) - end + where(:setting_key, :setting_changes_from, :setting_changes_to) do + :prevent_sharing_groups_outside_hierarchy | false | true + :new_user_signups_cap | nil | 100 + end - it 'changes settings' do - expect { service.execute } - .to change { group.namespace_settings.prevent_sharing_groups_outside_hierarchy } - .from(false).to(true) + with_them do + let(:settings) do + { setting_key => setting_changes_to } end - end - context 'when user is not a group owner' do - before do - group.add_maintainer(user) + context 'when user is not a group owner' do + before do + group.add_maintainer(user) + end + + it 'does not change settings' do + expect { service.execute }.not_to change { group.namespace_settings.public_send(setting_key) } + end + + it 'returns the group owner error' do + service.execute + + expect(group.namespace_settings.errors.messages[setting_key]).to include('can only be changed by a group admin.') + end end - it 'does not change settings' do - expect { service.execute }.not_to change { group.namespace_settings.prevent_sharing_groups_outside_hierarchy } + context 'with a subgroup' do + let(:subgroup) { create(:group, parent: group) } + + before do + group.add_owner(user) + end + + it 'does not change settings' do + service = described_class.new(user, subgroup, settings) + + expect { service.execute }.not_to change { group.namespace_settings.public_send(setting_key) } + + expect(subgroup.namespace_settings.errors.messages[setting_key]).to include('only available on top-level groups.') + end end - it 'returns the group owner error' do - service.execute + context 'when user is a group owner' do + before do + group.add_owner(user) + end - expect(group.namespace_settings.errors.messages[:prevent_sharing_groups_outside_hierarchy]).to include('can only be changed by a group admin.') + it 'changes settings' do + expect { service.execute } + .to change { group.namespace_settings.public_send(setting_key) } + .from(setting_changes_from).to(setting_changes_to) + end end end end diff --git a/spec/services/namespaces/in_product_marketing_emails_service_spec.rb b/spec/services/namespaces/in_product_marketing_emails_service_spec.rb index 2bf02e541f9..9d4fcf9ca64 100644 --- a/spec/services/namespaces/in_product_marketing_emails_service_spec.rb +++ b/spec/services/namespaces/in_product_marketing_emails_service_spec.rb @@ -11,7 +11,6 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do let(:frozen_time) { Time.zone.parse('23 Mar 2021 10:14:40 UTC') } let(:previous_action_completed_at) { frozen_time - 2.days } let(:current_action_completed_at) { nil } - let(:experiment_enabled) { true } let(:user_can_perform_current_track_action) { true } let(:actions_completed) { { created_at: previous_action_completed_at, git_write_at: current_action_completed_at } } @@ -22,7 +21,6 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do travel_to(frozen_time) create(:onboarding_progress, namespace: group, **actions_completed) group.add_developer(user) - stub_experiment_for_subject(in_product_marketing_emails: experiment_enabled) allow(Ability).to receive(:allowed?).with(user, anything, anything).and_return(user_can_perform_current_track_action) allow(Notify).to receive(:in_product_marketing_email).and_return(double(deliver_later: nil)) end @@ -85,50 +83,6 @@ RSpec.describe Namespaces::InProductMarketingEmailsService, '#execute' do end end - describe 'experimentation' do - context 'when on dotcom' do - before do - allow(::Gitlab).to receive(:com?).and_return(true) - end - - context 'when the experiment is enabled' do - it 'adds the group as an experiment subject in the experimental group' do - expect(Experiment).to receive(:add_group) - .with(:in_product_marketing_emails, variant: :experimental, group: group) - - execute_service - end - end - - context 'when the experiment is disabled' do - let(:experiment_enabled) { false } - - it 'adds the group as an experiment subject in the control group' do - expect(Experiment).to receive(:add_group) - .with(:in_product_marketing_emails, variant: :control, group: group) - - execute_service - end - - it { is_expected.not_to send_in_product_marketing_email } - end - - context 'when not on dotcom' do - before do - allow(::Gitlab).to receive(:com?).and_return(false) - end - - it 'does not add the group as an experiment subject' do - expect(Experiment).not_to receive(:add_group) - - execute_service - end - - it { is_expected.to send_in_product_marketing_email(user.id, group.id, :create, 0) } - end - end - end - context 'when the previous track action is not yet completed' do let(:previous_action_completed_at) { nil } diff --git a/spec/services/notes/copy_service_spec.rb b/spec/services/notes/copy_service_spec.rb index d9b6bafd7ff..dd11fa15ea8 100644 --- a/spec/services/notes/copy_service_spec.rb +++ b/spec/services/notes/copy_service_spec.rb @@ -16,6 +16,7 @@ RSpec.describe Notes::CopyService do 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) } diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 5b4d6188b66..6621ad1f294 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Notes::CreateService do let_it_be(:project) { create(:project, :repository) } let_it_be(:issue) { create(:issue, project: project) } let_it_be(:user) { create(:user) } + let(:opts) do { note: 'Awesome comment', noteable_type: 'Issue', noteable_id: issue.id, confidential: true } end @@ -295,6 +296,7 @@ RSpec.describe Notes::CreateService do context 'for merge requests' do let_it_be(:merge_request) { create(:merge_request, source_project: project, labels: [bug_label]) } + let(:issuable) { merge_request } let(:note_params) { opts.merge(noteable_type: 'MergeRequest', noteable_id: merge_request.id) } let(:merge_request_quick_actions) do diff --git a/spec/services/notes/destroy_service_spec.rb b/spec/services/notes/destroy_service_spec.rb index eebbdcc33b8..55acdabef82 100644 --- a/spec/services/notes/destroy_service_spec.rb +++ b/spec/services/notes/destroy_service_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe Notes::DestroyService do let_it_be(:project) { create(:project, :public) } let_it_be(:issue) { create(:issue, project: project) } + let(:user) { issue.author } describe '#execute' do diff --git a/spec/services/notes/post_process_service_spec.rb b/spec/services/notes/post_process_service_spec.rb index 07ef08d36c4..17001733c5b 100644 --- a/spec/services/notes/post_process_service_spec.rb +++ b/spec/services/notes/post_process_service_spec.rb @@ -21,7 +21,7 @@ RSpec.describe Notes::PostProcessService do it do expect(project).to receive(:execute_hooks) - expect(project).to receive(:execute_services) + expect(project).to receive(:execute_integrations) described_class.new(@note).execute end @@ -29,16 +29,16 @@ RSpec.describe Notes::PostProcessService do context 'with a confidential issue' do let(:issue) { create(:issue, :confidential, project: project) } - it "doesn't call note hooks/services" do + it "doesn't call note hooks/integrations" do expect(project).not_to receive(:execute_hooks).with(anything, :note_hooks) - expect(project).not_to receive(:execute_services).with(anything, :note_hooks) + expect(project).not_to receive(:execute_integrations).with(anything, :note_hooks) described_class.new(@note).execute end - it "calls confidential-note hooks/services" do + it "calls confidential-note hooks/integrations" do expect(project).to receive(:execute_hooks).with(anything, :confidential_note_hooks) - expect(project).to receive(:execute_services).with(anything, :confidential_note_hooks) + expect(project).to receive(:execute_integrations).with(anything, :confidential_note_hooks) described_class.new(@note).execute end diff --git a/spec/services/notes/quick_actions_service_spec.rb b/spec/services/notes/quick_actions_service_spec.rb index cb7d0163cac..0a56f01ebba 100644 --- a/spec/services/notes/quick_actions_service_spec.rb +++ b/spec/services/notes/quick_actions_service_spec.rb @@ -43,6 +43,7 @@ RSpec.describe Notes::QuickActionsService do context '/relate' do let_it_be(:issue) { create(:issue, project: project) } let_it_be(:other_issue) { create(:issue, project: project) } + let(:note_text) { "/relate #{other_issue.to_reference}" } let(:note) { create(:note_on_issue, noteable: issue, project: project, note: note_text) } diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index 000f3d26efa..71ac1641ca5 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -273,6 +273,7 @@ RSpec.describe Notes::UpdateService do context 'for a personal snippet' do let_it_be(:snippet) { create(:personal_snippet, :public) } + let(:note) { create(:note, project: nil, noteable: snippet, author: user, note: "Note on a snippet with reference #{issue.to_reference}" ) } it 'does not create todos' do diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index c3a0766cb17..ac82e4c025f 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -361,6 +361,7 @@ RSpec.describe NotificationService, :mailer do let_it_be_with_reload(:issue) { create(:issue, project: project, assignees: [assignee]) } let_it_be(:mentioned_issue) { create(:issue, assignees: issue.assignees) } let_it_be_with_reload(:author) { create(:user) } + let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@mention referenced, @unsubscribed_mentioned and @outsider also') } subject { notification.new_note(note) } @@ -376,41 +377,31 @@ RSpec.describe NotificationService, :mailer do let(:subject) { NotificationService.new } let(:mailer) { double(deliver_later: true) } + let(:issue) { create(:issue, author: User.support_bot) } + let(:project) { issue.project } + let(:note) { create(:note, noteable: issue, project: project) } - def should_email! - expect(Notify).to receive(:service_desk_new_note_email) - .with(issue.id, note.id, issue.external_author) - end + shared_examples 'notification with exact metric events' do |number_of_events| + it 'adds metric event' do + metric_transaction = double('Gitlab::Metrics::WebTransaction', increment: true, observe: true) + allow(::Gitlab::Metrics::BackgroundTransaction).to receive(:current).and_return(metric_transaction) + expect(metric_transaction).to receive(:add_event).with(:service_desk_new_note_email).exactly(number_of_events).times - def should_not_email! - expect(Notify).not_to receive(:service_desk_new_note_email) + subject.new_note(note) + end end - def execute! - subject.new_note(note) - end + shared_examples 'no participants are notified' do + it 'does not send the email' do + expect(Notify).not_to receive(:service_desk_new_note_email) - def self.it_should_email! - it 'sends the email' do - should_email! - execute! + subject.new_note(note) end - end - def self.it_should_not_email! - it 'doesn\'t send the email' do - should_not_email! - execute! - end + it_behaves_like 'notification with exact metric events', 0 end - let(:issue) { create(:issue, author: User.support_bot) } - let(:project) { issue.project } - let(:note) { create(:note, noteable: issue, project: project) } - - context 'do not exist' do - it_should_not_email! - end + it_behaves_like 'no participants are notified' context 'do exist and note not confidential' do let!(:issue_email_participant) { issue.issue_email_participants.create!(email: 'service.desk@example.com') } @@ -420,7 +411,14 @@ RSpec.describe NotificationService, :mailer do project.update!(service_desk_enabled: true) end - it_should_email! + it 'sends the email' do + expect(Notify).to receive(:service_desk_new_note_email) + .with(issue.id, note.id, issue.external_author) + + subject.new_note(note) + end + + it_behaves_like 'notification with exact metric events', 1 end context 'do exist and note is confidential' do @@ -432,7 +430,7 @@ RSpec.describe NotificationService, :mailer do project.update!(service_desk_enabled: true) end - it_should_not_email! + it_behaves_like 'no participants are notified' end end @@ -644,6 +642,7 @@ RSpec.describe NotificationService, :mailer do let_it_be(:issue) { create(:issue, project: project, assignees: [assignee]) } let_it_be(:mentioned_issue) { create(:issue, assignees: issue.assignees) } let_it_be(:author) { create(:user) } + let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@all mentioned') } before_all do @@ -930,6 +929,10 @@ RSpec.describe NotificationService, :mailer do end context 'design management is disabled' do + before do + enable_design_management(false) + end + it 'does not notify anyone' do notification.new_note(note) @@ -2616,6 +2619,16 @@ RSpec.describe NotificationService, :mailer do end end + describe '#user_deactivated', :deliver_mails_inline do + let_it_be(:user) { create(:user) } + + it 'sends the user an email' do + notification.user_deactivated(user.name, user.notification_email) + + should_only_email(user) + end + end + describe 'GroupMember', :deliver_mails_inline do let(:added_user) { create(:user) } diff --git a/spec/services/packages/composer/create_package_service_spec.rb b/spec/services/packages/composer/create_package_service_spec.rb index 526c7b4929b..553d58fdd86 100644 --- a/spec/services/packages/composer/create_package_service_spec.rb +++ b/spec/services/packages/composer/create_package_service_spec.rb @@ -8,6 +8,7 @@ RSpec.describe Packages::Composer::CreatePackageService do let_it_be(:json) { { name: package_name }.to_json } let_it_be(:project) { create(:project, :custom_repo, files: { 'composer.json' => json } ) } let_it_be(:user) { create(:user) } + let(:params) do { branch: branch, diff --git a/spec/services/packages/conan/search_service_spec.rb b/spec/services/packages/conan/search_service_spec.rb index 39d284ee088..55dcdfe646d 100644 --- a/spec/services/packages/conan/search_service_spec.rb +++ b/spec/services/packages/conan/search_service_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe Packages::Conan::SearchService do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project, :public) } + let!(:conan_package) { create(:conan_package, project: project) } let!(:conan_package2) { create(:conan_package, project: project) } diff --git a/spec/services/packages/create_package_file_service_spec.rb b/spec/services/packages/create_package_file_service_spec.rb index e4b4b15ebf9..2ff00ea8568 100644 --- a/spec/services/packages/create_package_file_service_spec.rb +++ b/spec/services/packages/create_package_file_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Packages::CreatePackageFileService do let_it_be(:package) { create(:maven_package) } let_it_be(:user) { create(:user) } + let(:service) { described_class.new(package, params) } describe '#execute' do diff --git a/spec/services/packages/debian/find_or_create_package_service_spec.rb b/spec/services/packages/debian/find_or_create_package_service_spec.rb index 3582b1f1dc3..f06f86b0146 100644 --- a/spec/services/packages/debian/find_or_create_package_service_spec.rb +++ b/spec/services/packages/debian/find_or_create_package_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Packages::Debian::FindOrCreatePackageService do let_it_be(:distribution) { create(:debian_project_distribution) } let_it_be(:project) { distribution.project } let_it_be(:user) { create(:user) } + let(:params) { { name: 'foo', version: '1.0+debian', distribution_name: distribution.codename } } subject(:service) { described_class.new(project, user, params) } diff --git a/spec/services/packages/destroy_package_service_spec.rb b/spec/services/packages/destroy_package_service_spec.rb new file mode 100644 index 00000000000..92db8da968c --- /dev/null +++ b/spec/services/packages/destroy_package_service_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::DestroyPackageService do + let_it_be(:user) { create(:user) } + + let!(:package) { create(:npm_package) } + + describe '#execute' do + subject(:service) { described_class.new(container: package, current_user: user) } + + context 'when the user is authorized' do + before do + package.project.add_maintainer(user) + end + + context 'when the destroy is successfull' do + it 'destroy the package' do + expect(package).to receive(:sync_maven_metadata).and_call_original + expect { service.execute }.to change { Packages::Package.count }.by(-1) + end + + it 'returns a success ServiceResponse' do + response = service.execute + + expect(response).to be_a(ServiceResponse) + expect(response).to be_success + expect(response.message).to eq("Package was successfully deleted") + end + end + + context 'when the destroy is not successful' do + before do + allow(package).to receive(:destroy!).and_raise(StandardError, "test") + end + + it 'returns an error ServiceResponse' do + response = service.execute + + expect(package).not_to receive(:sync_maven_metadata) + expect(response).to be_a(ServiceResponse) + expect(response).to be_error + expect(response.message).to eq("Failed to remove the package") + expect(response.status).to eq(:error) + end + end + end + + context 'when the user is not authorized' do + it 'returns an error ServiceResponse' do + response = service.execute + + expect(response).to be_a(ServiceResponse) + expect(response).to be_error + expect(response.message).to eq("You don't have access to this package") + expect(response.status).to eq(:error) + end + end + end +end 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 803371af4bf..d8b48af0121 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 @@ -91,6 +91,7 @@ RSpec.describe Packages::Maven::FindOrCreatePackageService do context 'with a build' do let_it_be(:pipeline) { create(:ci_pipeline, user: user) } + let(:build) { double('build', pipeline: pipeline) } let(:params) { { path: param_path, file_name: file_name, build: build } } @@ -103,6 +104,7 @@ RSpec.describe Packages::Maven::FindOrCreatePackageService do let_it_be_with_refind(:package_settings) { create(:namespace_package_setting, :group, maven_duplicates_allowed: false) } let_it_be_with_refind(:group) { package_settings.namespace } let_it_be_with_refind(:project) { create(:project, group: group) } + let!(:existing_package) { create(:maven_package, name: path, version: version, project: project) } it { expect { subject }.not_to change { project.package_files.count } } diff --git a/spec/services/packages/nuget/metadata_extraction_service_spec.rb b/spec/services/packages/nuget/metadata_extraction_service_spec.rb index 79428b58bd9..8eddd27f8a2 100644 --- a/spec/services/packages/nuget/metadata_extraction_service_spec.rb +++ b/spec/services/packages/nuget/metadata_extraction_service_spec.rb @@ -21,7 +21,8 @@ RSpec.describe Packages::Nuget::MetadataExtractionService do version: '12.0.3' } ], - package_tags: [] + package_tags: [], + package_types: [] } it { is_expected.to eq(expected_metadata) } @@ -47,6 +48,16 @@ RSpec.describe Packages::Nuget::MetadataExtractionService do end end + context 'with package types' do + let(:nuspec_filepath) { 'packages/nuget/with_package_types.nuspec' } + + it { is_expected.to have_key(:package_types) } + + it 'extracts package types' do + expect(subject[:package_types]).to include('SymbolsPackage') + end + end + context 'with a nuspec file with metadata' do let(:nuspec_filepath) { 'packages/nuget/with_metadata.nuspec' } diff --git a/spec/services/packages/nuget/search_service_spec.rb b/spec/services/packages/nuget/search_service_spec.rb index 1838065c5be..66c91487a8f 100644 --- a/spec/services/packages/nuget/search_service_spec.rb +++ b/spec/services/packages/nuget/search_service_spec.rb @@ -13,6 +13,7 @@ RSpec.describe Packages::Nuget::SearchService do let_it_be(:package_d) { create(:nuget_package, project: project, name: 'FooBarD') } let_it_be(:other_package_a) { create(:nuget_package, name: 'DummyPackageA') } let_it_be(:other_package_a) { create(:nuget_package, name: 'DummyPackageB') } + let(:search_term) { 'ummy' } let(:per_page) { 5 } let(:padding) { 0 } diff --git a/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb b/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb index ffe1a5b7646..328484c3e5a 100644 --- a/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb +++ b/spec/services/packages/nuget/update_package_from_metadata_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_redis_shared_state do include ExclusiveLeaseHelpers - let(:package) { create(:nuget_package, :processing) } + let(:package) { create(:nuget_package, :processing, :with_symbol_package) } let(:package_file) { package.package_files.first } let(:service) { described_class.new(package_file) } let(:package_name) { 'DummyProject.DummyPackage' } @@ -201,6 +201,41 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, :clean_gitlab_ it_behaves_like 'raising an', ::Packages::Nuget::MetadataExtractionService::ExtractionError end + context 'with a symbol package' do + let(:package_file) { package.package_files.last } + let(:package_file_name) { 'dummyproject.dummypackage.1.0.0.snupkg' } + + context 'with no existing package' do + let(:package_id) { package.id } + + it_behaves_like 'raising an', ::Packages::Nuget::UpdatePackageFromMetadataService::InvalidMetadataError + end + + context 'with existing package' do + let!(:existing_package) { create(:nuget_package, project: package.project, name: package_name, version: package_version) } + let(:package_id) { existing_package.id } + + it 'link existing package and updates package file', :aggregate_failures do + expect(service).to receive(:try_obtain_lease).and_call_original + expect(::Packages::Nuget::SyncMetadatumService).not_to receive(:new) + expect(::Packages::UpdateTagsService).not_to receive(:new) + + expect { subject } + .to change { ::Packages::Package.count }.by(-1) + .and change { Packages::Dependency.count }.by(0) + .and change { Packages::DependencyLink.count }.by(0) + .and change { Packages::Nuget::DependencyLinkMetadatum.count }.by(0) + .and change { ::Packages::Nuget::Metadatum.count }.by(0) + expect(package_file.reload.file_name).to eq(package_file_name) + expect(package_file.package).to eq(existing_package) + end + + it_behaves_like 'taking the lease' + + it_behaves_like 'not updating the package if the lease is taken' + end + end + context 'with an invalid package name' do invalid_names = [ '', diff --git a/spec/services/packages/rubygems/dependency_resolver_service_spec.rb b/spec/services/packages/rubygems/dependency_resolver_service_spec.rb index 78abfc96ed5..f23ed0e5fbc 100644 --- a/spec/services/packages/rubygems/dependency_resolver_service_spec.rb +++ b/spec/services/packages/rubygems/dependency_resolver_service_spec.rb @@ -5,6 +5,7 @@ RSpec.describe Packages::Rubygems::DependencyResolverService do let_it_be(:project) { create(:project, :private) } let_it_be(:package) { create(:package, project: project) } let_it_be(:user) { create(:user) } + let(:gem_name) { package.name } let(:service) { described_class.new(project, user, gem_name: gem_name) } diff --git a/spec/services/pod_logs/base_service_spec.rb b/spec/services/pod_logs/base_service_spec.rb index 6f7731fda3a..d2f6300ab65 100644 --- a/spec/services/pod_logs/base_service_spec.rb +++ b/spec/services/pod_logs/base_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe ::PodLogs::BaseService do include KubernetesHelpers let_it_be(:cluster) { create(:cluster, :provided_by_gcp, environment_scope: '*') } + let(:namespace) { 'autodevops-deploy-9-production' } let(:pod_name) { 'pod-1' } diff --git a/spec/services/pod_logs/elasticsearch_service_spec.rb b/spec/services/pod_logs/elasticsearch_service_spec.rb index 598b162aee4..1111d9b9307 100644 --- a/spec/services/pod_logs/elasticsearch_service_spec.rb +++ b/spec/services/pod_logs/elasticsearch_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe ::PodLogs::ElasticsearchService do let_it_be(:cluster) { create(:cluster, :provided_by_gcp, environment_scope: '*') } + let(:namespace) { 'autodevops-deploy-9-production' } let(:pod_name) { 'pod-1' } diff --git a/spec/services/pod_logs/kubernetes_service_spec.rb b/spec/services/pod_logs/kubernetes_service_spec.rb index 3e31ff15c1b..c06a87830ca 100644 --- a/spec/services/pod_logs/kubernetes_service_spec.rb +++ b/spec/services/pod_logs/kubernetes_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe ::PodLogs::KubernetesService do include KubernetesHelpers let_it_be(:cluster) { create(:cluster, :provided_by_gcp, environment_scope: '*') } + let(:namespace) { 'autodevops-deploy-9-production' } let(:pod_name) { 'pod-1' } diff --git a/spec/services/post_receive_service_spec.rb b/spec/services/post_receive_service_spec.rb index 2a78dc454c7..871ed95bf28 100644 --- a/spec/services/post_receive_service_spec.rb +++ b/spec/services/post_receive_service_spec.rb @@ -283,7 +283,7 @@ RSpec.describe PostReceiveService do context 'with a redirected data' do it 'returns redirected message on the response' do - project_moved = Gitlab::Checks::ProjectMoved.new(project.repository, user, 'http', 'foo/baz') + project_moved = Gitlab::Checks::ContainerMoved.new(project.repository, user, 'http', 'foo/baz') project_moved.add_message expect(subject).to include(build_basic_message(project_moved.message)) diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index ac0b6cc8ef1..defeadb479a 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -190,6 +190,7 @@ RSpec.describe Projects::CreateService, '#execute' do let_it_be(:group) { create(:group) } let_it_be(:shared_group) { create(:group) } let_it_be(:shared_group_user) { create(:user) } + let(:opts) do { name: 'GitLab', @@ -221,6 +222,7 @@ RSpec.describe Projects::CreateService, '#execute' do let_it_be(:subgroup_for_projects) { create(:group, :private, parent: group) } let_it_be(:subgroup_for_access) { create(:group, :private, parent: group) } let_it_be(:group_maintainer) { create(:user) } + let(:group_access_level) { Gitlab::Access::REPORTER } let(:subgroup_access_level) { Gitlab::Access::DEVELOPER } let(:share_max_access_level) { Gitlab::Access::MAINTAINER } @@ -582,32 +584,49 @@ RSpec.describe Projects::CreateService, '#execute' do expect(branches.size).to eq(1) expect(branches.collect(&:name)).to contain_exactly('example_branch') end + + describe 'advanced readme content', experiment: :new_project_readme_content do + before do + stub_experiments(new_project_readme_content: :advanced) + end + + it_behaves_like 'creates README.md' + + it 'includes advanced content in the README.md' do + content = project.repository.readme.data + expect(content).to include <<~MARKDOWN + git remote add origin #{project.http_url_to_repo} + git branch -M example_branch + git push -uf origin example_branch + MARKDOWN + end + end end end - describe 'create service for the project' do + describe 'create integration for the project' do subject(:project) { create_project(user, opts) } - context 'with an active service template' do - let!(:template_integration) { create(:prometheus_service, :template, api_url: 'https://prometheus.template.com/') } + context 'with an active integration template' do + let!(:template_integration) { create(:prometheus_integration, :template, api_url: 'https://prometheus.template.com/') } - it 'creates a service from the template' do + it 'creates an integration from the template' do expect(project.integrations.count).to eq(1) expect(project.integrations.first.api_url).to eq(template_integration.api_url) expect(project.integrations.first.inherit_from_id).to be_nil end context 'with an active instance-level integration' do - let!(:instance_integration) { create(:prometheus_service, :instance, api_url: 'https://prometheus.instance.com/') } + let!(:instance_integration) { create(:prometheus_integration, :instance, api_url: 'https://prometheus.instance.com/') } - it 'creates a service from the instance-level integration' do + it 'creates an integration from the instance-level integration' do expect(project.integrations.count).to eq(1) expect(project.integrations.first.api_url).to eq(instance_integration.api_url) expect(project.integrations.first.inherit_from_id).to eq(instance_integration.id) end context 'with an active group-level integration' do - let!(:group_integration) { create(:prometheus_service, group: group, project: nil, api_url: 'https://prometheus.group.com/') } + let!(:group_integration) { create(:prometheus_integration, group: group, project: nil, api_url: 'https://prometheus.group.com/') } let!(:group) do create(:group).tap do |group| group.add_owner(user) @@ -621,14 +640,14 @@ RSpec.describe Projects::CreateService, '#execute' do } end - it 'creates a service from the group-level integration' do + it 'creates an integration from the group-level integration' do expect(project.integrations.count).to eq(1) expect(project.integrations.first.api_url).to eq(group_integration.api_url) expect(project.integrations.first.inherit_from_id).to eq(group_integration.id) end context 'with an active subgroup' do - let!(:subgroup_integration) { create(:prometheus_service, group: subgroup, project: nil, api_url: 'https://prometheus.subgroup.com/') } + let!(:subgroup_integration) { create(:prometheus_integration, group: subgroup, project: nil, api_url: 'https://prometheus.subgroup.com/') } let!(:subgroup) do create(:group, parent: group).tap do |subgroup| subgroup.add_owner(user) @@ -642,7 +661,7 @@ RSpec.describe Projects::CreateService, '#execute' do } end - it 'creates a service from the subgroup-level integration' do + it 'creates an integration from the subgroup-level integration' do expect(project.integrations.count).to eq(1) expect(project.integrations.first.api_url).to eq(subgroup_integration.api_url) expect(project.integrations.first.inherit_from_id).to eq(subgroup_integration.id) @@ -686,69 +705,6 @@ RSpec.describe Projects::CreateService, '#execute' do create_project(user, opts) end - context 'when project has access to shared service' do - before do - stub_feature_flags(projects_post_creation_worker: false) - end - - context 'Prometheus integration is shared via group cluster' do - let(:cluster) { create(:cluster, :group, groups: [group]) } - let(:group) do - create(:group).tap do |group| - group.add_owner(user) - end - end - - before do - create(:clusters_integrations_prometheus, cluster: cluster) - end - - it 'creates PrometheusService record', :aggregate_failures do - project = create_project(user, opts.merge!(namespace_id: group.id)) - service = project.prometheus_service - - expect(service.active).to be true - expect(service.manual_configuration?).to be false - expect(service.persisted?).to be true - end - end - - context 'Prometheus integration is shared via instance cluster' do - let(:cluster) { create(:cluster, :instance) } - - before do - create(:clusters_integrations_prometheus, cluster: cluster) - end - - it 'creates PrometheusService record', :aggregate_failures do - project = create_project(user, opts) - service = project.prometheus_service - - expect(service.active).to be true - expect(service.manual_configuration?).to be false - expect(service.persisted?).to be true - end - - it 'cleans invalid record and logs warning', :aggregate_failures do - invalid_service_record = build(:prometheus_service, properties: { api_url: nil, manual_configuration: true }.to_json) - allow(PrometheusService).to receive(:new).and_return(invalid_service_record) - - expect(Gitlab::ErrorTracking).to receive(:track_exception).with(an_instance_of(ActiveRecord::RecordInvalid), include(extra: { project_id: a_kind_of(Integer) })) - project = create_project(user, opts) - - expect(project.prometheus_service).to be_nil - end - end - - context 'shared Prometheus integration is not available' do - it 'does not persist PrometheusService record', :aggregate_failures do - project = create_project(user, opts) - - expect(project.prometheus_service).to be_nil - end - end - end - context 'with external authorization enabled' do before do enable_external_authorization_service_check diff --git a/spec/services/projects/destroy_rollback_service_spec.rb b/spec/services/projects/destroy_rollback_service_spec.rb index f63939337b8..3eaacc8c1e7 100644 --- a/spec/services/projects/destroy_rollback_service_spec.rb +++ b/spec/services/projects/destroy_rollback_service_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe Projects::DestroyRollbackService do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project, :repository, namespace: user.namespace) } + let(:repository) { project.repository } let(:repository_storage) { project.repository_storage } diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index c6b2b1e2b21..4a76347ea45 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do include ProjectForksHelper let_it_be(:user) { create(:user) } + let!(:project) { create(:project, :repository, namespace: user.namespace) } let(:path) { project.repository.disk_path } let(:remove_path) { removal_path(path) } diff --git a/spec/services/projects/gitlab_projects_import_service_spec.rb b/spec/services/projects/gitlab_projects_import_service_spec.rb index 09d093a9916..d32e720a49f 100644 --- a/spec/services/projects/gitlab_projects_import_service_spec.rb +++ b/spec/services/projects/gitlab_projects_import_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Projects::GitlabProjectsImportService do let_it_be(:namespace) { create(:namespace) } + let(:path) { 'test-path' } let(:file) { fixture_file_upload('spec/fixtures/project_export.tar.gz') } let(:overwrite) { false } diff --git a/spec/services/projects/group_links/create_service_spec.rb b/spec/services/projects/group_links/create_service_spec.rb index 9bc780fe177..4ea5f2b3a53 100644 --- a/spec/services/projects/group_links/create_service_spec.rb +++ b/spec/services/projects/group_links/create_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Projects::GroupLinks::CreateService, '#execute' do let_it_be(:user) { create :user } let_it_be(:group) { create :group } let_it_be(:project) { create :project } + let(:group_access) { Gitlab::Access::DEVELOPER } let(:opts) do { @@ -49,9 +50,9 @@ RSpec.describe Projects::GroupLinks::CreateService, '#execute' do expect(AuthorizedProjectsWorker).not_to( receive(:bulk_perform_async) ) - expect(AuthorizedProjectUpdate::ProjectGroupLinkCreateWorker).to( + expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).to( receive(:perform_async) - .with(project.id, group.id, group_access) + .with(project.id) .and_call_original ) expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( diff --git a/spec/services/projects/group_links/destroy_service_spec.rb b/spec/services/projects/group_links/destroy_service_spec.rb index d60e9a01e54..d65afb68bfe 100644 --- a/spec/services/projects/group_links/destroy_service_spec.rb +++ b/spec/services/projects/group_links/destroy_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Projects::GroupLinks::DestroyService, '#execute' do let_it_be(:user) { create :user } let_it_be(:project) { create(:project, :private) } let_it_be(:group) { create(:group) } + let!(:group_link) { create(:project_group_link, project: project, group: group) } subject { described_class.new(project, user) } diff --git a/spec/services/projects/group_links/update_service_spec.rb b/spec/services/projects/group_links/update_service_spec.rb index 053c5eb611e..4a38fb0c7d9 100644 --- a/spec/services/projects/group_links/update_service_spec.rb +++ b/spec/services/projects/group_links/update_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Projects::GroupLinks::UpdateService, '#execute' do let_it_be(:user) { create :user } let_it_be(:group) { create :group } let_it_be(:project) { create :project } + let!(:link) { create(:project_group_link, project: project, group: group) } let(:expiry_date) { 1.month.from_now.to_date } @@ -32,25 +33,87 @@ RSpec.describe Projects::GroupLinks::UpdateService, '#execute' do expect(link.expires_at).to eq(expiry_date) end - it 'updates project permissions' do - expect { subject }.to change { user.can?(:create_release, project) }.from(true).to(false) - end + context 'project authorizations update' do + context 'when the feature flag `specialized_worker_for_project_share_update_auth_recalculation` is enabled' do + before do + stub_feature_flags(specialized_worker_for_project_share_update_auth_recalculation: true) + end + + it 'calls AuthorizedProjectUpdate::ProjectRecalculateWorker to update project authorizations' do + expect(AuthorizedProjectUpdate::ProjectRecalculateWorker) + .to receive(:perform_async).with(link.project.id) + + subject + end + + it 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations' do + expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( + receive(:bulk_perform_in) + .with(1.hour, + [[user.id]], + batch_delay: 30.seconds, batch_size: 100) + ) - it 'executes UserProjectAccessChangedService' do - expect_next_instance_of(UserProjectAccessChangedService) do |service| - expect(service).to receive(:execute) + subject + end + + it 'updates project authorizations of users who had access to the project via the group share', :sidekiq_inline do + group.add_maintainer(user) + + expect { subject }.to( + change { Ability.allowed?(user, :create_release, project) } + .from(true).to(false)) + end end - subject + context 'when the feature flag `specialized_worker_for_project_share_update_auth_recalculation` is disabled' do + before do + stub_feature_flags(specialized_worker_for_project_share_update_auth_recalculation: false) + end + + it 'calls UserProjectAccessChangedService to update project authorizations' do + expect_next_instance_of(UserProjectAccessChangedService, [user.id]) do |service| + expect(service).to receive(:execute) + end + + subject + end + + it 'updates project authorizations of users who had access to the project via the group share' do + group.add_maintainer(user) + + expect { subject }.to( + change { Ability.allowed?(user, :create_release, project) } + .from(true).to(false)) + end + end end context 'with only param not requiring authorization refresh' do let(:group_link_params) { { expires_at: Date.tomorrow } } - it 'does not execute UserProjectAccessChangedService' do - expect(UserProjectAccessChangedService).not_to receive(:new) + context 'when the feature flag `specialized_worker_for_project_share_update_auth_recalculation` is enabled' do + before do + stub_feature_flags(specialized_worker_for_project_share_update_auth_recalculation: true) + end + + it 'does not perform any project authorizations update using `AuthorizedProjectUpdate::ProjectRecalculateWorker`' do + expect(AuthorizedProjectUpdate::ProjectRecalculateWorker).not_to receive(:perform_async) + + subject + end + end + + context 'when the feature flag `specialized_worker_for_project_share_update_auth_recalculation` is disabled' do + before do + stub_feature_flags(specialized_worker_for_project_share_update_auth_recalculation: false) + end + + it 'does not perform any project authorizations update using `UserProjectAccessChangedService`' do + expect(UserProjectAccessChangedService).not_to receive(:new) - subject + subject + end end end end diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 1fb6dae0c08..f27ebb2e19e 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -106,6 +106,26 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do end end + context 'when file download returns a redirect' do + let(:redirect_link) { 'http://external-link' } + + before do + stub_full_request(download_link).to_return(status: 301, body: 'You are being redirected', headers: { 'Location' => redirect_link } ) + stub_full_request(redirect_link).to_return(body: lfs_content) + end + + it_behaves_like 'lfs object is created' + + it 'correctly stores lfs object' do + subject.execute + + new_lfs_object = LfsObject.first + + expect(new_lfs_object).to have_attributes(oid: oid, size: size) + expect(File.binread(new_lfs_object.file.file.file)).to eq lfs_content + end + end + context 'when downloaded lfs file has a different size' do let(:size) { 1 } @@ -252,6 +272,18 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do context 'and first fragments are the same' do let(:lfs_content) { existing_lfs_object.file.read } + context 'when lfs_link_existing_object feature flag disabled' do + before do + stub_feature_flags(lfs_link_existing_object: false) + end + + it 'does not call link_existing_lfs_object!' do + expect(subject).not_to receive(:link_existing_lfs_object!) + + subject.execute + end + end + it 'returns success' do expect(subject.execute).to eq({ status: :success }) end diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index 018bfa8ef61..f91f879b772 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -378,8 +378,8 @@ RSpec.describe Projects::Operations::UpdateService do context 'prometheus integration' do context 'prometheus params were passed into service' do - let(:prometheus_service) do - build_stubbed(:prometheus_service, project: project, properties: { + let(:prometheus_integration) do + build_stubbed(:prometheus_integration, project: project, properties: { api_url: "http://example.prometheus.com", manual_configuration: "0" }) @@ -394,18 +394,18 @@ RSpec.describe Projects::Operations::UpdateService do } end - it 'uses Project#find_or_initialize_service to include instance defined defaults and pass them to Projects::UpdateService', :aggregate_failures do + it 'uses Project#find_or_initialize_integration to include instance defined defaults and pass them to Projects::UpdateService', :aggregate_failures do project_update_service = double(Projects::UpdateService) expect(project) - .to receive(:find_or_initialize_service) + .to receive(:find_or_initialize_integration) .with('prometheus') - .and_return(prometheus_service) + .and_return(prometheus_integration) expect(Projects::UpdateService).to receive(:new) do |project_arg, user_arg, update_params_hash| expect(project_arg).to eq project expect(user_arg).to eq user - expect(update_params_hash[:prometheus_service_attributes]).to include('properties' => { 'api_url' => 'http://new.prometheus.com', 'manual_configuration' => '1' }) - expect(update_params_hash[:prometheus_service_attributes]).not_to include(*%w(id project_id created_at updated_at)) + expect(update_params_hash[:prometheus_integration_attributes]).to include('properties' => { 'api_url' => 'http://new.prometheus.com', 'manual_configuration' => '1' }) + expect(update_params_hash[:prometheus_integration_attributes]).not_to include(*%w(id project_id created_at updated_at)) end.and_return(project_update_service) expect(project_update_service).to receive(:execute) @@ -413,13 +413,13 @@ RSpec.describe Projects::Operations::UpdateService do end end - context 'prometheus params were not passed into service' do + context 'when prometheus params are not passed into service' do let(:params) { { something: :else } } it 'does not pass any prometheus params into Projects::UpdateService', :aggregate_failures do project_update_service = double(Projects::UpdateService) - expect(project).not_to receive(:find_or_initialize_service) + expect(project).not_to receive(:find_or_initialize_integration) expect(Projects::UpdateService) .to receive(:new) .with(project, user, {}) diff --git a/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 5235c64d451..25cf588dedf 100644 --- a/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -115,7 +115,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do let(:alert_manager_token) { token_input } before do - create(:prometheus_service, project: project) + create(:prometheus_integration, project: project) if alerting_setting create(:project_alerting_setting, @@ -165,7 +165,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do context 'incident settings' do before do - create(:prometheus_service, project: project) + create(:prometheus_integration, project: project) create(:project_alerting_setting, project: project, token: token) end @@ -204,7 +204,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do let(:process_service) { instance_double(AlertManagement::ProcessPrometheusAlertService) } before do - create(:prometheus_service, project: project) + create(:prometheus_integration, project: project) create(:project_alerting_setting, project: project, token: token) end diff --git a/spec/services/projects/protect_default_branch_service_spec.rb b/spec/services/projects/protect_default_branch_service_spec.rb index a485a64ca35..c8aa421cdd4 100644 --- a/spec/services/projects/protect_default_branch_service_spec.rb +++ b/spec/services/projects/protect_default_branch_service_spec.rb @@ -99,6 +99,53 @@ RSpec.describe Projects::ProtectDefaultBranchService do .not_to have_received(:create_protected_branch) end end + + context 'when protected branch does not exist' do + before do + allow(service) + .to receive(:protected_branch_exists?) + .and_return(false) + allow(service) + .to receive(:protect_branch?) + .and_return(true) + end + + it 'changes the HEAD of the project' do + service.protect_default_branch + + expect(project) + .to have_received(:change_head) + end + + it 'protects the default branch' do + service.protect_default_branch + + expect(service) + .to have_received(:create_protected_branch) + end + end + + context 'when protected branch already exists' do + before do + allow(service) + .to receive(:protected_branch_exists?) + .and_return(true) + end + + it 'changes the HEAD of the project' do + service.protect_default_branch + + expect(project) + .to have_received(:change_head) + end + + it 'does not protect the default branch' do + service.protect_default_branch + + expect(service) + .not_to have_received(:create_protected_branch) + end + end end describe '#create_protected_branch' do diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 3171abfb36f..b71677a5e8f 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -7,7 +7,8 @@ RSpec.describe Projects::TransferService do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } - let_it_be(:group_integration) { create(:slack_service, group: group, project: nil, webhook: 'http://group.slack.com') } + let_it_be(:group_integration) { create(:integrations_slack, group: group, project: nil, webhook: 'http://group.slack.com') } + let(:project) { create(:project, :repository, :legacy_storage, namespace: user.namespace) } subject(:execute_transfer) { described_class.new(project, user).execute(group).tap { project.reload } } @@ -121,24 +122,24 @@ RSpec.describe Projects::TransferService do context 'with a project integration' do let_it_be_with_reload(:project) { create(:project, namespace: user.namespace) } - let_it_be(:instance_integration) { create(:slack_service, :instance, webhook: 'http://project.slack.com') } + let_it_be(:instance_integration) { create(:integrations_slack, :instance, webhook: 'http://project.slack.com') } context 'with an inherited integration' do - let_it_be(:project_integration) { create(:slack_service, project: project, webhook: 'http://project.slack.com', inherit_from_id: instance_integration.id) } + let_it_be(:project_integration) { create(:integrations_slack, project: project, webhook: 'http://project.slack.com', inherit_from_id: instance_integration.id) } it 'replaces inherited integrations', :aggregate_failures do execute_transfer - expect(project.slack_service.webhook).to eq(group_integration.webhook) + expect(project.slack_integration.webhook).to eq(group_integration.webhook) expect(Integration.count).to eq(3) end end context 'with a custom integration' do - let_it_be(:project_integration) { create(:slack_service, project: project, webhook: 'http://project.slack.com') } + let_it_be(:project_integration) { create(:integrations_slack, project: project, webhook: 'http://project.slack.com') } it 'does not updates the integrations' do - expect { execute_transfer }.not_to change { project.slack_service.webhook } + expect { execute_transfer }.not_to change { project.slack_integration.webhook } end end end @@ -434,28 +435,74 @@ RSpec.describe Projects::TransferService do end describe 'refreshing project authorizations' do + let(:old_group) { create(:group) } + let!(:project) { create(:project, namespace: old_group) } + let(:member_of_old_group) { create(:user) } let(:group) { create(:group) } - let(:owner) { project.namespace.owner } - let(:group_member) { create(:user) } + let(:member_of_new_group) { create(:user) } before do - group.add_user(owner, GroupMember::MAINTAINER) - group.add_user(group_member, GroupMember::DEVELOPER) + old_group.add_developer(member_of_old_group) + group.add_maintainer(member_of_new_group) + + # Add the executing user as owner in both groups, so that + # transfer can be executed. + old_group.add_owner(user) + group.add_owner(user) end - it 'refreshes the permissions of the old and new namespace' do - execute_transfer + context 'when the feature flag `specialized_worker_for_project_transfer_auth_recalculation` is enabled' do + before do + stub_feature_flags(specialized_worker_for_project_transfer_auth_recalculation: true) + end + + it 'calls AuthorizedProjectUpdate::ProjectRecalculateWorker to update project authorizations' do + expect(AuthorizedProjectUpdate::ProjectRecalculateWorker) + .to receive(:perform_async).with(project.id) + + execute_transfer + end + + it 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations' do + user_ids = [user.id, member_of_old_group.id, member_of_new_group.id].map { |id| [id] } + + expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( + receive(:bulk_perform_in) + .with(1.hour, + user_ids, + batch_delay: 30.seconds, batch_size: 100) + ) - expect(group_member.authorized_projects).to include(project) - expect(owner.authorized_projects).to include(project) + subject + end + + it 'refreshes the permissions of the members of the old and new namespace', :sidekiq_inline do + expect { execute_transfer } + .to change { member_of_old_group.authorized_projects.include?(project) }.from(true).to(false) + .and change { member_of_new_group.authorized_projects.include?(project) }.from(false).to(true) + end end - it 'only schedules a single job for every user' do - expect_next_instance_of(UserProjectAccessChangedService, [owner.id, group_member.id]) do |service| - expect(service).to receive(:execute).once.and_call_original + context 'when the feature flag `specialized_worker_for_project_transfer_auth_recalculation` is disabled' do + before do + stub_feature_flags(specialized_worker_for_project_transfer_auth_recalculation: false) end - execute_transfer + it 'calls UserProjectAccessChangedService to update project authorizations' do + user_ids = [user.id, member_of_old_group.id, member_of_new_group.id] + + expect_next_instance_of(UserProjectAccessChangedService, user_ids) do |service| + expect(service).to receive(:execute) + end + + execute_transfer + end + + it 'refreshes the permissions of the members of the old and new namespace' do + expect { execute_transfer } + .to change { member_of_old_group.authorized_projects.include?(project) }.from(true).to(false) + .and change { member_of_new_group.authorized_projects.include?(project) }.from(false).to(true) + end end end diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index b11607bc213..5898504b463 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -5,6 +5,7 @@ require "spec_helper" RSpec.describe Projects::UpdatePagesService do let_it_be(:project, refind: true) { create(:project, :repository) } let_it_be(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) } + let(:build) { create(:ci_build, pipeline: pipeline, ref: 'HEAD') } let(:invalid_file) { fixture_file_upload('spec/fixtures/dk.png') } diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index 5b15b7d5f34..17d01a57221 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -83,9 +83,10 @@ RSpec.describe Projects::UpdateRepositoryStorageService do .with(project.repository.raw) .and_raise(Gitlab::Git::CommandError) - result = subject.execute + expect do + subject.execute + end.to raise_error(Gitlab::Git::CommandError) - 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 @@ -101,9 +102,10 @@ RSpec.describe Projects::UpdateRepositoryStorageService do expect(original_project_repository_double).to receive(:remove) .and_raise(Gitlab::Git::CommandError) - result = subject.execute + expect do + subject.execute + end.to raise_error(Gitlab::Git::CommandError) - expect(result).to be_error expect(repository_storage_move).to be_cleanup_failed end end @@ -118,9 +120,10 @@ RSpec.describe Projects::UpdateRepositoryStorageService do expect(project_repository_double).to receive(:checksum) .and_return('not matching checksum') - result = subject.execute + expect do + subject.execute + end.to raise_error(UpdateRepositoryStorageMethods::Error, /Failed to verify project repository checksum/) - expect(result).to be_error expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('default') end diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index e1b22da2e61..c74a8295d0a 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -200,17 +200,32 @@ RSpec.describe Projects::UpdateService do context 'when updating a default branch' do let(:project) { create(:project, :repository) } - it 'changes a default branch' do + it 'changes default branch, tracking the previous branch' do + previous_default_branch = project.default_branch + update_project(project, admin, default_branch: 'feature') - expect(Project.find(project.id).default_branch).to eq 'feature' + project.reload + + expect(project.default_branch).to eq('feature') + expect(project.previous_default_branch).to eq(previous_default_branch) + + update_project(project, admin, default_branch: previous_default_branch) + + project.reload + + expect(project.default_branch).to eq(previous_default_branch) + expect(project.previous_default_branch).to eq('feature') end it 'does not change a default branch' do # The branch 'unexisted-branch' does not exist. update_project(project, admin, default_branch: 'unexisted-branch') - expect(Project.find(project.id).default_branch).to eq 'master' + project.reload + + expect(project.default_branch).to eq 'master' + expect(project.previous_default_branch).to be_nil end end @@ -468,58 +483,58 @@ RSpec.describe Projects::UpdateService do end end - context 'when updating nested attributes for prometheus service' do - context 'prometheus service exists' do - let(:prometheus_service_attributes) do - attributes_for(:prometheus_service, + context 'when updating nested attributes for prometheus integration' do + context 'prometheus integration exists' do + let(:prometheus_integration_attributes) do + attributes_for(:prometheus_integration, project: project, properties: { api_url: "http://new.prometheus.com", manual_configuration: "0" } ) end - let!(:prometheus_service) do - create(:prometheus_service, + let!(:prometheus_integration) do + create(:prometheus_integration, project: project, properties: { api_url: "http://old.prometheus.com", manual_configuration: "0" } ) end it 'updates existing record' do - expect { update_project(project, user, prometheus_service_attributes: prometheus_service_attributes) } - .to change { prometheus_service.reload.api_url } + expect { update_project(project, user, prometheus_integration_attributes: prometheus_integration_attributes) } + .to change { prometheus_integration.reload.api_url } .from("http://old.prometheus.com") .to("http://new.prometheus.com") end end - context 'prometheus service does not exist' do + context 'prometheus integration does not exist' do context 'valid parameters' do - let(:prometheus_service_attributes) do - attributes_for(:prometheus_service, + let(:prometheus_integration_attributes) do + attributes_for(:prometheus_integration, project: project, properties: { api_url: "http://example.prometheus.com", manual_configuration: "0" } ) end it 'creates new record' do - expect { update_project(project, user, prometheus_service_attributes: prometheus_service_attributes) } - .to change { ::PrometheusService.where(project: project).count } + expect { update_project(project, user, prometheus_integration_attributes: prometheus_integration_attributes) } + .to change { ::Integrations::Prometheus.where(project: project).count } .from(0) .to(1) end end context 'invalid parameters' do - let(:prometheus_service_attributes) do - attributes_for(:prometheus_service, + let(:prometheus_integration_attributes) do + attributes_for(:prometheus_integration, project: project, properties: { api_url: nil, manual_configuration: "1" } ) end it 'does not create new record' do - expect { update_project(project, user, prometheus_service_attributes: prometheus_service_attributes) } - .not_to change { ::PrometheusService.where(project: project).count } + expect { update_project(project, user, prometheus_integration_attributes: prometheus_integration_attributes) } + .not_to change { ::Integrations::Prometheus.where(project: project).count } end end end diff --git a/spec/services/prometheus/create_default_alerts_service_spec.rb b/spec/services/prometheus/create_default_alerts_service_spec.rb index e149161d881..0880799b589 100644 --- a/spec/services/prometheus/create_default_alerts_service_spec.rb +++ b/spec/services/prometheus/create_default_alerts_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Prometheus::CreateDefaultAlertsService do let_it_be(:project) { create(:project, :repository) } + let(:instance) { described_class.new(project: project) } let(:expected_alerts) { described_class::DEFAULT_ALERTS } diff --git a/spec/services/prometheus/proxy_service_spec.rb b/spec/services/prometheus/proxy_service_spec.rb index f22ea361fde..b78683cace7 100644 --- a/spec/services/prometheus/proxy_service_spec.rb +++ b/spec/services/prometheus/proxy_service_spec.rb @@ -65,7 +65,7 @@ RSpec.describe Prometheus::ProxyService do end describe '#execute' do - let(:prometheus_adapter) { instance_double(PrometheusService) } + let(:prometheus_adapter) { instance_double(::Integrations::Prometheus) } let(:params) { ActionController::Parameters.new(query: '1').permit! } subject { described_class.new(environment, 'GET', 'query', params) } diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 4af76bc65ab..d7f5c39e457 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -15,6 +15,7 @@ RSpec.describe QuickActions::InterpretService do let_it_be(:inprogress) { create(:label, project: project, title: 'In Progress') } let_it_be(:helmchart) { create(:label, project: project, title: 'Helm Chart Registry') } let_it_be(:bug) { create(:label, project: project, title: 'Bug') } + let(:service) { described_class.new(project, developer) } before_all do diff --git a/spec/services/releases/create_evidence_service_spec.rb b/spec/services/releases/create_evidence_service_spec.rb index 818d20f0468..0ac15a7291d 100644 --- a/spec/services/releases/create_evidence_service_spec.rb +++ b/spec/services/releases/create_evidence_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Releases::CreateEvidenceService do let_it_be(:project) { create(:project) } + let(:release) { create(:release, project: project) } let(:service) { described_class.new(release) } diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb index 7287825a0be..bf28fde3d90 100644 --- a/spec/services/releases/create_service_spec.rb +++ b/spec/services/releases/create_service_spec.rb @@ -44,6 +44,21 @@ RSpec.describe Releases::CreateService do it_behaves_like 'a successful release creation' + context 'when tag is protected and user does not have access to it' do + let!(:protected_tag) { create(:protected_tag, :no_one_can_create, name: '*', project: project) } + + it 'track the error event' do + stub_feature_flags(evalute_protected_tag_for_release_permissions: false) + + expect(Gitlab::ErrorTracking).to receive(:log_exception).with( + kind_of(described_class::ReleaseProtectedTagAccessError), + project_id: project.id, + user_id: user.id) + + service.execute + end + end + context 'when the tag does not exist' do let(:tag_name) { 'non-exist-tag' } diff --git a/spec/services/releases/destroy_service_spec.rb b/spec/services/releases/destroy_service_spec.rb index bc5bff0b31d..38cdcef3825 100644 --- a/spec/services/releases/destroy_service_spec.rb +++ b/spec/services/releases/destroy_service_spec.rb @@ -28,6 +28,21 @@ RSpec.describe Releases::DestroyService do it 'returns the destroyed object' do is_expected.to include(status: :success, release: release) end + + context 'when tag is protected and user does not have access to it' do + let!(:protected_tag) { create(:protected_tag, :no_one_can_create, name: '*', project: project) } + + it 'track the error event' do + stub_feature_flags(evalute_protected_tag_for_release_permissions: false) + + expect(Gitlab::ErrorTracking).to receive(:log_exception).with( + kind_of(described_class::ReleaseProtectedTagAccessError), + project_id: project.id, + user_id: user.id) + + service.execute + end + end end context 'when tag does not exist in the repository' do diff --git a/spec/services/releases/update_service_spec.rb b/spec/services/releases/update_service_spec.rb index 932a7fab5ec..96b562a8071 100644 --- a/spec/services/releases/update_service_spec.rb +++ b/spec/services/releases/update_service_spec.rb @@ -38,6 +38,21 @@ RSpec.describe Releases::UpdateService do service.execute end + context 'when tag is protected and user does not have access to it' do + let!(:protected_tag) { create(:protected_tag, :no_one_can_create, name: '*', project: project) } + + it 'track the error event' do + stub_feature_flags(evalute_protected_tag_for_release_permissions: false) + + expect(Gitlab::ErrorTracking).to receive(:log_exception).with( + kind_of(described_class::ReleaseProtectedTagAccessError), + project_id: project.id, + user_id: user.id) + + service.execute + end + end + context 'when the tag does not exists' do let(:tag_name) { 'foobar' } diff --git a/spec/services/repositories/changelog_service_spec.rb b/spec/services/repositories/changelog_service_spec.rb index 9a5b0f33fbb..02d60f076ca 100644 --- a/spec/services/repositories/changelog_service_spec.rb +++ b/spec/services/repositories/changelog_service_spec.rb @@ -76,7 +76,7 @@ RSpec.describe Repositories::ChangelogService do recorder = ActiveRecord::QueryRecorder.new { service.execute } changelog = project.repository.blob_at('master', 'CHANGELOG.md')&.data - expect(recorder.count).to eq(12) + expect(recorder.count).to eq(11) expect(changelog).to include('Title 1', 'Title 2') end diff --git a/spec/services/repositories/destroy_rollback_service_spec.rb b/spec/services/repositories/destroy_rollback_service_spec.rb index 9cc41a4c7f8..717e52f0e40 100644 --- a/spec/services/repositories/destroy_rollback_service_spec.rb +++ b/spec/services/repositories/destroy_rollback_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Repositories::DestroyRollbackService do let_it_be(:user) { create(:user) } + let!(:project) { create(:project, :repository, namespace: user.namespace) } let(:repository) { project.repository } let(:path) { repository.disk_path } diff --git a/spec/services/repositories/destroy_service_spec.rb b/spec/services/repositories/destroy_service_spec.rb index 81bda2130a6..240f837e973 100644 --- a/spec/services/repositories/destroy_service_spec.rb +++ b/spec/services/repositories/destroy_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Repositories::DestroyService do let_it_be(:user) { create(:user) } + let!(:project) { create(:project, :repository, namespace: user.namespace) } let(:repository) { project.repository } let(:path) { repository.disk_path } diff --git a/spec/services/repositories/shell_destroy_service_spec.rb b/spec/services/repositories/shell_destroy_service_spec.rb index 9020ef7b209..65168a1784a 100644 --- a/spec/services/repositories/shell_destroy_service_spec.rb +++ b/spec/services/repositories/shell_destroy_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Repositories::ShellDestroyService do let_it_be(:user) { create(:user) } + let!(:project) { create(:project, :repository, namespace: user.namespace) } let(:path) { project.repository.disk_path } let(:remove_path) { "#{path}+#{project.id}#{described_class::DELETED_FLAG}" } diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index 517ed086713..11069dc1bb8 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -88,12 +88,28 @@ RSpec.describe ResourceAccessTokens::CreateService do end end - it 'adds the bot user as a maintainer in the resource' do - response = subject - access_token = response.payload[:access_token] - bot_user = access_token.user + context 'access level' do + context 'when user does not specify an access level' do + it 'adds the bot user as a maintainer in the resource' do + response = subject + access_token = response.payload[:access_token] + bot_user = access_token.user + + expect(resource.members.maintainers.map(&:user_id)).to include(bot_user.id) + end + end - expect(resource.members.maintainers.map(&:user_id)).to include(bot_user.id) + context 'when user specifies an access level' do + let_it_be(:params) { { access_level: Gitlab::Access::DEVELOPER } } + + it 'adds the bot user with the specified access level in the resource' do + response = subject + access_token = response.payload[:access_token] + bot_user = access_token.user + + expect(resource.members.developers.map(&:user_id)).to include(bot_user.id) + end + end end context 'personal access token' do @@ -176,6 +192,7 @@ RSpec.describe ResourceAccessTokens::CreateService do context "when access provisioning fails" do let_it_be(:bot_user) { create(:user, :project_bot) } + let(:unpersisted_member) { build(:project_member, source: resource, user: bot_user) } before do diff --git a/spec/services/resource_access_tokens/revoke_service_spec.rb b/spec/services/resource_access_tokens/revoke_service_spec.rb index 99adb4bb7a0..4f4e2ab0c99 100644 --- a/spec/services/resource_access_tokens/revoke_service_spec.rb +++ b/spec/services/resource_access_tokens/revoke_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe ResourceAccessTokens::RevokeService do subject { described_class.new(user, resource, access_token).execute } let_it_be(:user) { create(:user) } + let(:access_token) { create(:personal_access_token, user: resource_bot) } describe '#execute', :sidekiq_inline do @@ -80,6 +81,7 @@ RSpec.describe ResourceAccessTokens::RevokeService do context 'when resource is a project' do let_it_be(:resource) { create(:project, :private) } + let(:resource_bot) { create(:user, :project_bot) } before do diff --git a/spec/services/resource_events/change_labels_service_spec.rb b/spec/services/resource_events/change_labels_service_spec.rb index 8eac6ae0b49..012168ef719 100644 --- a/spec/services/resource_events/change_labels_service_spec.rb +++ b/spec/services/resource_events/change_labels_service_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe ResourceEvents::ChangeLabelsService do let_it_be(:project) { create(:project) } let_it_be(:author) { create(:user) } + let(:resource) { create(:issue, project: project) } describe '.change_labels' do diff --git a/spec/services/resource_events/merge_into_notes_service_spec.rb b/spec/services/resource_events/merge_into_notes_service_spec.rb index 6209294f4ce..abe00e72f20 100644 --- a/spec/services/resource_events/merge_into_notes_service_spec.rb +++ b/spec/services/resource_events/merge_into_notes_service_spec.rb @@ -21,6 +21,7 @@ RSpec.describe ResourceEvents::MergeIntoNotesService do let_it_be(:resource) { create(:issue, project: project) } let_it_be(:label) { create(:label, project: project) } let_it_be(:label2) { create(:label, project: project) } + let(:time) { Time.current } describe '#execute' do diff --git a/spec/services/security/ci_configuration/sast_parser_service_spec.rb b/spec/services/security/ci_configuration/sast_parser_service_spec.rb index 4fe99f20879..4346d0a9e07 100644 --- a/spec/services/security/ci_configuration/sast_parser_service_spec.rb +++ b/spec/services/security/ci_configuration/sast_parser_service_spec.rb @@ -3,11 +3,13 @@ require 'spec_helper' RSpec.describe Security::CiConfiguration::SastParserService do + include Ci::TemplateHelpers + describe '#configuration' do include_context 'read ci configuration for sast enabled project' let(:configuration) { described_class.new(project).configuration } - let(:secure_analyzers_prefix) { configuration['global'][0] } + let(:secure_analyzers) { configuration['global'][0] } let(:sast_excluded_paths) { configuration['global'][1] } let(:sast_pipeline_stage) { configuration['pipeline'][0] } let(:sast_search_max_depth) { configuration['pipeline'][1] } @@ -16,7 +18,7 @@ RSpec.describe Security::CiConfiguration::SastParserService do let(:sast_brakeman_level) { brakeman['variables'][0] } it 'parses the configuration for SAST' do - expect(secure_analyzers_prefix['default_value']).to eql('registry.gitlab.com/gitlab-org/security-products/analyzers') + expect(secure_analyzers['default_value']).to eql(secure_analyzers_prefix) expect(sast_excluded_paths['default_value']).to eql('spec, test, tests, tmp') expect(sast_pipeline_stage['default_value']).to eql('test') expect(sast_search_max_depth['default_value']).to eql('4') @@ -28,7 +30,7 @@ RSpec.describe Security::CiConfiguration::SastParserService do context 'when .gitlab-ci.yml is present' do it 'populates the current values from the file' do allow(project.repository).to receive(:blob_data_at).and_return(gitlab_ci_yml_content) - expect(secure_analyzers_prefix['value']).to eql('registry.gitlab.com/gitlab-org/security-products/analyzers2') + expect(secure_analyzers['value']).to eql("registry.gitlab.com/gitlab-org/security-products/analyzers2") expect(sast_excluded_paths['value']).to eql('spec, executables') expect(sast_pipeline_stage['value']).to eql('our_custom_security_stage') expect(sast_search_max_depth['value']).to eql('8') @@ -50,7 +52,7 @@ RSpec.describe Security::CiConfiguration::SastParserService do context 'when .gitlab-ci.yml is absent' do it 'populates the current values with the default values' do allow(project.repository).to receive(:blob_data_at).and_return(nil) - expect(secure_analyzers_prefix['value']).to eql('registry.gitlab.com/gitlab-org/security-products/analyzers') + expect(secure_analyzers['value']).to eql(secure_analyzers_prefix) expect(sast_excluded_paths['value']).to eql('spec, test, tests, tmp') expect(sast_pipeline_stage['value']).to eql('test') expect(sast_search_max_depth['value']).to eql('4') @@ -67,7 +69,7 @@ RSpec.describe Security::CiConfiguration::SastParserService do end it 'populates the current values with the default values' do - expect(secure_analyzers_prefix['value']).to eql('registry.gitlab.com/gitlab-org/security-products/analyzers') + expect(secure_analyzers['value']).to eql(secure_analyzers_prefix) expect(sast_excluded_paths['value']).to eql('spec, test, tests, tmp') expect(sast_pipeline_stage['value']).to eql('test') expect(sast_search_max_depth['value']).to eql('4') diff --git a/spec/services/service_ping/build_payload_service_spec.rb b/spec/services/service_ping/build_payload_service_spec.rb new file mode 100644 index 00000000000..cd2685069c9 --- /dev/null +++ b/spec/services/service_ping/build_payload_service_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ServicePing::BuildPayloadService do + describe '#execute', :without_license do + subject(:service_ping_payload) { described_class.new.execute } + + include_context 'stubbed service ping metrics definitions' do + let(:subscription_metrics) do + [ + metric_attributes('active_user_count', "Subscription") + ] + end + end + + context 'when usage_ping_enabled setting is false' do + before do + # Gitlab::CurrentSettings.usage_ping_enabled? == false + stub_config_setting(usage_ping_enabled: false) + end + + it 'returns empty service ping payload' do + expect(service_ping_payload).to eq({}) + end + end + + context 'when usage_ping_enabled setting is true' do + before do + # Gitlab::CurrentSettings.usage_ping_enabled? == true + stub_config_setting(usage_ping_enabled: true) + end + + it_behaves_like 'complete service ping payload' + + context 'with require stats consent enabled' do + before do + allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: true)) + end + + it 'returns empty service ping payload' do + expect(service_ping_payload).to eq({}) + end + end + end + end +end diff --git a/spec/services/service_ping/permit_data_categories_service_spec.rb b/spec/services/service_ping/permit_data_categories_service_spec.rb new file mode 100644 index 00000000000..4fd5c6f9ccb --- /dev/null +++ b/spec/services/service_ping/permit_data_categories_service_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ServicePing::PermitDataCategoriesService do + using RSpec::Parameterized::TableSyntax + + describe '#execute', :without_license do + subject(:permitted_categories) { described_class.new.execute } + + context 'when usage ping setting is set to true' do + before do + allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: false)) + stub_config_setting(usage_ping_enabled: true) + end + + it 'returns all categories' do + expect(permitted_categories).to match_array(%w[Standard Subscription Operational Optional]) + end + end + + context 'when usage ping setting is set to false' do + before do + allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: false)) + stub_config_setting(usage_ping_enabled: false) + end + + it 'returns no categories' do + expect(permitted_categories).to match_array([]) + end + end + + context 'when User.single_user&.requires_usage_stats_consent? is required' do + before do + allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: true)) + stub_config_setting(usage_ping_enabled: true) + end + + it 'returns no categories' do + expect(permitted_categories).to match_array([]) + end + end + end + + describe '#product_intelligence_enabled?' do + where(:usage_ping_enabled, :requires_usage_stats_consent, :expected_product_intelligence_enabled) do + # Usage ping enabled + true | false | true + true | true | false + + # Usage ping disabled + false | false | false + false | true | false + end + + with_them do + before do + allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: requires_usage_stats_consent)) + stub_config_setting(usage_ping_enabled: usage_ping_enabled) + end + + it 'has the correct product_intelligence_enabled?' do + expect(described_class.new.product_intelligence_enabled?).to eq(expected_product_intelligence_enabled) + end + end + end +end diff --git a/spec/services/submit_usage_ping_service_spec.rb b/spec/services/service_ping/submit_service_ping_service_spec.rb index 7133dc35fc3..8a3065e6bc6 100644 --- a/spec/services/submit_usage_ping_service_spec.rb +++ b/spec/services/service_ping/submit_service_ping_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe SubmitUsagePingService do +RSpec.describe ServicePing::SubmitService do include StubRequests include UsageDataHelpers @@ -98,6 +98,34 @@ RSpec.describe SubmitUsagePingService do it_behaves_like 'does not run' end + context 'when product_intelligence_enabled is false' do + before do + allow_next_instance_of(ServicePing::PermitDataCategoriesService) do |service| + allow(service).to receive(:product_intelligence_enabled?).and_return(false) + end + end + + it_behaves_like 'does not run' + end + + context 'when product_intelligence_enabled is true' do + before do + stub_usage_data_connections + + allow_next_instance_of(ServicePing::PermitDataCategoriesService) do |service| + allow(service).to receive(:product_intelligence_enabled?).and_return(true) + end + end + + it 'generates service ping' do + stub_response(body: with_dev_ops_score_params) + + expect(Gitlab::UsageData).to receive(:data).with(force_refresh: true).and_call_original + + subject.execute + end + end + context 'when usage ping is enabled' do before do stub_usage_data_connections @@ -217,11 +245,63 @@ RSpec.describe SubmitUsagePingService do context 'and usage data is nil' do before do + allow(ServicePing::BuildPayloadService).to receive(:execute).and_return(nil) allow(Gitlab::UsageData).to receive(:data).and_return(nil) end it_behaves_like 'does not send a blank usage ping payload' end + + context 'if payload service fails' do + before do + stub_response(body: with_dev_ops_score_params) + allow(ServicePing::BuildPayloadService).to receive(:execute).and_raise(described_class::SubmissionError, 'SubmissionError') + end + + it 'calls UsageData .data method' do + usage_data = build_usage_data + + expect(Gitlab::UsageData).to receive(:data).and_return(usage_data) + + subject.execute + end + end + + context 'calls BuildPayloadService first' do + before do + stub_response(body: with_dev_ops_score_params) + end + + it 'returns usage data' do + usage_data = build_usage_data + + expect_next_instance_of(ServicePing::BuildPayloadService) do |service| + expect(service).to receive(:execute).and_return(usage_data) + end + + subject.execute + end + end + + context 'if version app response fails' do + before do + stub_response(body: with_dev_ops_score_params, status: 404) + + usage_data = build_usage_data + allow_next_instance_of(ServicePing::BuildPayloadService) do |service| + allow(service).to receive(:execute).and_return(usage_data) + end + end + + it 'calls UsageData .data method' do + usage_data = build_usage_data + + expect(Gitlab::UsageData).to receive(:data).and_return(usage_data) + + # SubmissionError is raised as a result of 404 in response from HTTP Request + expect { subject.execute }.to raise_error(described_class::SubmissionError) + end + end end def stub_response(body:, status: 201) @@ -232,4 +312,8 @@ RSpec.describe SubmitUsagePingService do status: status ) end + + def build_usage_data + { uuid: 'uuid', recorded_at: Time.current } + end end diff --git a/spec/services/snippets/bulk_destroy_service_spec.rb b/spec/services/snippets/bulk_destroy_service_spec.rb index 8a6250a8b45..2f399d10188 100644 --- a/spec/services/snippets/bulk_destroy_service_spec.rb +++ b/spec/services/snippets/bulk_destroy_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Snippets::BulkDestroyService do let_it_be(:project) { create(:project) } + let(:user) { create(:user) } let!(:personal_snippet) { create(:personal_snippet, :repository, author: user) } let!(:project_snippet) { create(:project_snippet, :repository, project: project, author: user) } diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index eb6e85eb408..0eb73c8edd2 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Snippets::CreateService do describe '#execute' do let_it_be(:user) { create(:user) } let_it_be(:admin) { create(:user, :admin) } + let(:action) { :create } let(:opts) { base_opts.merge(extra_opts) } let(:base_opts) do @@ -19,8 +20,9 @@ RSpec.describe Snippets::CreateService do let(:extra_opts) { {} } let(:creator) { admin } + let(:spam_params) { double } - subject { described_class.new(project: project, current_user: creator, params: opts).execute } + subject { described_class.new(project: project, current_user: creator, params: opts, spam_params: spam_params).execute } let(:snippet) { subject.payload[:snippet] } @@ -301,6 +303,10 @@ RSpec.describe Snippets::CreateService do end end + before do + stub_spam_services + end + context 'when ProjectSnippet' do let_it_be(:project) { create(:project) } diff --git a/spec/services/snippets/update_repository_storage_service_spec.rb b/spec/services/snippets/update_repository_storage_service_spec.rb index 50b28a5a125..fdea3615fb1 100644 --- a/spec/services/snippets/update_repository_storage_service_spec.rb +++ b/spec/services/snippets/update_repository_storage_service_spec.rb @@ -75,9 +75,10 @@ RSpec.describe Snippets::UpdateRepositoryStorageService do .with(snippet.repository.raw) .and_raise(Gitlab::Git::CommandError) - result = subject.execute + expect do + subject.execute + end.to raise_error(Gitlab::Git::CommandError) - expect(result).to be_error expect(snippet).not_to be_repository_read_only expect(snippet.repository_storage).to eq('default') expect(repository_storage_move).to be_failed @@ -93,9 +94,10 @@ RSpec.describe Snippets::UpdateRepositoryStorageService do expect(original_snippet_repository_double).to receive(:remove) .and_raise(Gitlab::Git::CommandError) - result = subject.execute + expect do + subject.execute + end.to raise_error(Gitlab::Git::CommandError) - expect(result).to be_error expect(repository_storage_move).to be_cleanup_failed end end @@ -107,9 +109,10 @@ RSpec.describe Snippets::UpdateRepositoryStorageService do expect(snippet_repository_double).to receive(:checksum) .and_return('not matching checksum') - result = subject.execute + expect do + subject.execute + end.to raise_error(UpdateRepositoryStorageMethods::Error, /Failed to verify snippet repository checksum from \w+ to not matching checksum/) - expect(result).to be_error expect(snippet).not_to be_repository_read_only expect(snippet.repository_storage).to eq('default') end diff --git a/spec/services/snippets/update_service_spec.rb b/spec/services/snippets/update_service_spec.rb index 46bc62e11ef..f61d33e2436 100644 --- a/spec/services/snippets/update_service_spec.rb +++ b/spec/services/snippets/update_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Snippets::UpdateService do describe '#execute', :aggregate_failures do let_it_be(:user) { create(:user) } let_it_be(:admin) { create :user, admin: true } + let(:action) { :update } let(:visibility_level) { Gitlab::VisibilityLevel::PRIVATE } let(:base_opts) do @@ -20,7 +21,9 @@ RSpec.describe Snippets::UpdateService do let(:extra_opts) { {} } let(:options) { base_opts.merge(extra_opts) } let(:updater) { user } - let(:service) { Snippets::UpdateService.new(project: project, current_user: updater, params: options) } + let(:spam_params) { double } + + let(:service) { Snippets::UpdateService.new(project: project, current_user: updater, params: options, spam_params: spam_params) } subject { service.execute(snippet) } @@ -721,8 +724,13 @@ RSpec.describe Snippets::UpdateService do end end + before do + stub_spam_services + end + context 'when Project Snippet' do let_it_be(:project) { create(:project) } + let!(:snippet) { create(:project_snippet, :repository, author: user, project: project) } before do diff --git a/spec/services/spam/akismet_service_spec.rb b/spec/services/spam/akismet_service_spec.rb index 1cd049da592..d9f62258a53 100644 --- a/spec/services/spam/akismet_service_spec.rb +++ b/spec/services/spam/akismet_service_spec.rb @@ -59,7 +59,7 @@ RSpec.describe Spam::AkismetService do it_behaves_like 'no activity if Akismet is not enabled', :spam?, :check context 'if Akismet is enabled' do - it 'correctly transforms options for the akismet client' do + it 'correctly transforms options for the akismet client, including spelling of referrer key' do expected_check_params = { type: 'comment', text: text, diff --git a/spec/services/spam/ham_service_spec.rb b/spec/services/spam/ham_service_spec.rb index c947de6cf92..0101a8e7704 100644 --- a/spec/services/spam/ham_service_spec.rb +++ b/spec/services/spam/ham_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Spam::HamService do let_it_be(:user) { create(:user) } + let!(:spam_log) { create(:spam_log, user: user, submitted_as_ham: false) } let(:fake_akismet_service) { double(:akismet_service) } diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb index 9ca52b92267..3a92e5acb5a 100644 --- a/spec/services/spam/spam_action_service_spec.rb +++ b/spec/services/spam/spam_action_service_spec.rb @@ -5,15 +5,20 @@ require 'spec_helper' RSpec.describe Spam::SpamActionService do include_context 'includes Spam constants' - let(:request) { double(:request, env: env, headers: {}) } let(:issue) { create(:issue, project: project, author: user) } let(:fake_ip) { '1.2.3.4' } let(:fake_user_agent) { 'fake-user-agent' } let(:fake_referer) { 'fake-http-referer' } - let(:env) do - { 'action_dispatch.remote_ip' => fake_ip, - 'HTTP_USER_AGENT' => fake_user_agent, - 'HTTP_REFERER' => fake_referer } + let(:captcha_response) { 'abc123' } + let(:spam_log_id) { existing_spam_log.id } + let(:spam_params) do + ::Spam::SpamParams.new( + captcha_response: captcha_response, + spam_log_id: spam_log_id, + ip_address: fake_ip, + user_agent: fake_user_agent, + referer: fake_referer + ) end let_it_be(:project) { create(:project, :public) } @@ -23,32 +28,33 @@ RSpec.describe Spam::SpamActionService do issue.spam = false end - shared_examples 'only checks for spam if a request is provided' do - context 'when request is missing' do - let(:request) { nil } + describe 'constructor argument validation' do + subject do + described_service = described_class.new(spammable: issue, spam_params: spam_params, user: user, action: :create) + described_service.execute + end - it "doesn't check as spam" do - expect(fake_verdict_service).not_to receive(:execute) + context 'when spam_params is nil' do + let(:spam_params) { nil } + let(:expected_service_params_not_present_message) do + /Skipped spam check because spam_params was not present/ + end + it "returns success with a messaage" do response = subject - expect(response.message).to match(/request was not present/) + expect(response.message).to match(expected_service_params_not_present_message) expect(issue).not_to be_spam end end - - context 'when request exists' do - it 'creates a spam log' do - expect { subject } - .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue') - end - end end shared_examples 'creates a spam log' do it do - expect { subject }.to change(SpamLog, :count).by(1) + expect { subject } + .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue') + # TODO: These checks should be incorporated into the `log_spam` RSpec matcher above new_spam_log = SpamLog.last expect(new_spam_log.user_id).to eq(user.id) expect(new_spam_log.title).to eq(issue.title) @@ -56,25 +62,14 @@ RSpec.describe Spam::SpamActionService do expect(new_spam_log.source_ip).to eq(fake_ip) expect(new_spam_log.user_agent).to eq(fake_user_agent) expect(new_spam_log.noteable_type).to eq('Issue') - expect(new_spam_log.via_api).to eq(false) + expect(new_spam_log.via_api).to eq(true) end end describe '#execute' do - let(:request) { double(:request, env: env, headers: nil) } let(:fake_captcha_verification_service) { double(:captcha_verification_service) } let(:fake_verdict_service) { double(:spam_verdict_service) } let(:allowlisted) { false } - let(:api) { nil } - let(:captcha_response) { 'abc123' } - let(:spam_log_id) { existing_spam_log.id } - let(:spam_params) do - ::Spam::SpamParams.new( - api: api, - captcha_response: captcha_response, - spam_log_id: spam_log_id - ) - end let(:verdict_service_opts) do { @@ -88,7 +83,6 @@ RSpec.describe Spam::SpamActionService do { target: issue, user: user, - request: request, options: verdict_service_opts, context: { action: :create, @@ -100,40 +94,20 @@ RSpec.describe Spam::SpamActionService do let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false) } subject do - described_service = described_class.new(spammable: issue, request: request, user: user, action: :create) + described_service = described_class.new(spammable: issue, spam_params: spam_params, user: user, action: :create) allow(described_service).to receive(:allowlisted?).and_return(allowlisted) - described_service.execute(spam_params: spam_params) + described_service.execute end before do - allow(Captcha::CaptchaVerificationService).to receive(:new) { fake_captcha_verification_service } + allow(Captcha::CaptchaVerificationService).to receive(:new).with(spam_params: spam_params) { fake_captcha_verification_service } allow(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args).and_return(fake_verdict_service) end - context 'when the captcha params are passed in the headers' do - let(:request) { double(:request, env: env, headers: headers) } - let(:spam_params) { Spam::SpamActionService.filter_spam_params!({ api: api }, request) } - let(:headers) do - { - 'X-GitLab-Captcha-Response' => captcha_response, - 'X-GitLab-Spam-Log-Id' => spam_log_id - } - end - - it 'extracts the headers correctly' do - expect(fake_captcha_verification_service) - .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(true) - expect(SpamLog) - .to receive(:verify_recaptcha!).with(user_id: user.id, id: spam_log_id) - - subject - end - end - context 'when captcha response verification returns true' do before do allow(fake_captcha_verification_service) - .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(true) + .to receive(:execute).and_return(true) end it "doesn't check with the SpamVerdictService" do @@ -156,7 +130,7 @@ RSpec.describe Spam::SpamActionService do context 'when captcha response verification returns false' do before do allow(fake_captcha_verification_service) - .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(false) + .to receive(:execute).and_return(false) end context 'when spammable attributes have not changed' do @@ -200,8 +174,6 @@ RSpec.describe Spam::SpamActionService do stub_feature_flags(allow_possible_spam: false) end - it_behaves_like 'only checks for spam if a request is provided' - it 'marks as spam' do response = subject @@ -211,8 +183,6 @@ RSpec.describe Spam::SpamActionService do end context 'when allow_possible_spam feature flag is true' do - it_behaves_like 'only checks for spam if a request is provided' - it 'does not mark as spam' do response = subject @@ -232,8 +202,6 @@ RSpec.describe Spam::SpamActionService do stub_feature_flags(allow_possible_spam: false) end - it_behaves_like 'only checks for spam if a request is provided' - it 'marks as spam' do response = subject @@ -243,8 +211,6 @@ RSpec.describe Spam::SpamActionService do end context 'when allow_possible_spam feature flag is true' do - it_behaves_like 'only checks for spam if a request is provided' - it 'does not mark as spam' do response = subject @@ -264,8 +230,6 @@ RSpec.describe Spam::SpamActionService do stub_feature_flags(allow_possible_spam: false) end - it_behaves_like 'only checks for spam if a request is provided' - it_behaves_like 'creates a spam log' it 'does not mark as spam' do @@ -284,8 +248,6 @@ RSpec.describe Spam::SpamActionService do end context 'when allow_possible_spam feature flag is true' do - it_behaves_like 'only checks for spam if a request is provided' - it_behaves_like 'creates a spam log' it 'does not mark as needing reCAPTCHA' do @@ -334,37 +296,10 @@ RSpec.describe Spam::SpamActionService do allow(fake_verdict_service).to receive(:execute).and_return(ALLOW) end - context 'when the request is nil' do - let(:request) { nil } - let(:issue_ip_address) { '1.2.3.4' } - let(:issue_user_agent) { 'lynx' } - let(:verdict_service_opts) do - { - ip_address: issue_ip_address, - user_agent: issue_user_agent - } - end - - before do - allow(issue).to receive(:ip_address) { issue_ip_address } - allow(issue).to receive(:user_agent) { issue_user_agent } - end - - it 'assembles the options with information from the spammable' do - # TODO: This code untestable, because we do not perform a verification if there is not a - # request. See corresponding comment in code - # expect(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args) - - subject - end - end - - context 'when the request is present' do - it 'assembles the options with information from the request' do - expect(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args) + it 'assembles the options with information from the request' do + expect(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args) - subject - end + subject end end end diff --git a/spec/services/spam/spam_params_spec.rb b/spec/services/spam/spam_params_spec.rb new file mode 100644 index 00000000000..e7e8b468adb --- /dev/null +++ b/spec/services/spam/spam_params_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Spam::SpamParams do + describe '.new_from_request' do + let(:captcha_response) { 'abc123' } + let(:spam_log_id) { 42 } + let(:ip_address) { '0.0.0.0' } + let(:user_agent) { 'Lynx' } + let(:referer) { 'http://localhost' } + let(:headers) do + { + 'X-GitLab-Captcha-Response' => captcha_response, + 'X-GitLab-Spam-Log-Id' => spam_log_id + } + end + + let(:env) do + { + 'action_dispatch.remote_ip' => ip_address, + 'HTTP_USER_AGENT' => user_agent, + 'HTTP_REFERER' => referer + } + end + + let(:request) {double(:request, headers: headers, env: env)} + + it 'constructs from a request' do + expected = ::Spam::SpamParams.new( + captcha_response: captcha_response, + spam_log_id: spam_log_id, + ip_address: ip_address, + user_agent: user_agent, + referer: referer + ) + expect(described_class.new_from_request(request: request)).to eq(expected) + end + end +end diff --git a/spec/services/spam/spam_verdict_service_spec.rb b/spec/services/spam/spam_verdict_service_spec.rb index 215df81de63..659c21b7d4f 100644 --- a/spec/services/spam/spam_verdict_service_spec.rb +++ b/spec/services/spam/spam_verdict_service_spec.rb @@ -14,13 +14,12 @@ RSpec.describe Spam::SpamVerdictService do 'HTTP_REFERER' => fake_referer } end - let(:request) { double(:request, env: env) } - let(:check_for_spam) { true } let_it_be(:user) { create(:user) } let_it_be(:issue) { create(:issue, author: user) } + let(:service) do - described_class.new(user: user, target: issue, request: request, options: {}) + described_class.new(user: user, target: issue, options: {}) end let(:attribs) do diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 54cef164f1c..e9bd40b058b 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -11,6 +11,7 @@ RSpec.describe SystemNoteService do let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, :repository, group: group) } let_it_be(:author) { create(:user) } + let(:noteable) { create(:issue, project: project) } let(:issue) { noteable } @@ -355,15 +356,15 @@ RSpec.describe SystemNoteService do let(:issue) { create(:issue, project: project) } let(:merge_request) { create(:merge_request, :simple, target_project: project, source_project: project) } let(:jira_issue) { ExternalIssue.new("JIRA-1", project)} - let(:jira_tracker) { project.jira_service } + let(:jira_tracker) { project.jira_integration } let(:commit) { project.commit } let(:comment_url) { jira_api_comment_url(jira_issue.id) } let(:success_message) { "SUCCESS: Successfully posted to http://jira.example.net." } before do - stub_jira_service_test + stub_jira_integration_test stub_jira_urls(jira_issue.id) - jira_service_settings + jira_integration_settings end def cross_reference(type, link_exists = false) diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index 0eb327ea7f1..1ea3c241d27 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -8,6 +8,7 @@ RSpec.describe ::SystemNotes::IssuablesService do let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, :repository, group: group) } let_it_be(:author) { create(:user) } + let(:noteable) { create(:issue, project: project) } let(:issue) { noteable } @@ -728,7 +729,7 @@ RSpec.describe ::SystemNotes::IssuablesService do let(:noteable) { ExternalIssue.new('EXT-1234', project) } it 'is false with issue tracker supporting referencing' do - create(:jira_service, project: project) + create(:jira_integration, project: project) project.reload expect(service.cross_reference_disallowed?(noteable)).to be_falsey diff --git a/spec/services/test_hooks/project_service_spec.rb b/spec/services/test_hooks/project_service_spec.rb index a87e612e378..cd6284b4a87 100644 --- a/spec/services/test_hooks/project_service_spec.rb +++ b/spec/services/test_hooks/project_service_spec.rb @@ -9,6 +9,7 @@ RSpec.describe TestHooks::ProjectService do describe '#execute' do let_it_be(:project) { create(:project, :repository) } + let(:hook) { create(:project_hook, project: project) } let(:trigger) { 'not_implemented_events' } let(:service) { described_class.new(hook, current_user, trigger) } @@ -163,6 +164,7 @@ RSpec.describe TestHooks::ProjectService do context 'wiki_page_events' do let_it_be(:project) { create(:project, :wiki_repo) } + let(:trigger) { 'wiki_page_events' } let(:trigger_key) { :wiki_page_hooks } diff --git a/spec/services/test_hooks/system_service_spec.rb b/spec/services/test_hooks/system_service_spec.rb index e500a1057ab..a13ae471b4b 100644 --- a/spec/services/test_hooks/system_service_spec.rb +++ b/spec/services/test_hooks/system_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe TestHooks::SystemService do describe '#execute' do let_it_be(:project) { create(:project, :repository) } + let(:hook) { create(:system_hook) } let(:service) { described_class.new(hook, project.owner, trigger) } let(:success_result) { { status: :success, http_status: 200, message: 'ok' } } diff --git a/spec/services/user_project_access_changed_service_spec.rb b/spec/services/user_project_access_changed_service_spec.rb index 4723619afd2..f8835fefc84 100644 --- a/spec/services/user_project_access_changed_service_spec.rb +++ b/spec/services/user_project_access_changed_service_spec.rb @@ -30,6 +30,17 @@ RSpec.describe UserProjectAccessChangedService do described_class.new([1, 2]).execute(blocking: false, priority: described_class::LOW_PRIORITY) end + + it 'sets the current caller_id as related_class in the context of all the enqueued jobs' do + Gitlab::ApplicationContext.with_context(caller_id: 'Foo') do + described_class.new([1, 2]).execute(blocking: false, + priority: described_class::LOW_PRIORITY) + end + + expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker.jobs).to all( + include(Labkit::Context.log_key(:related_class) => 'Foo') + ) + end end context 'with load balancing enabled' do diff --git a/spec/services/users/approve_service_spec.rb b/spec/services/users/approve_service_spec.rb index 9999e674c7d..078dde546c9 100644 --- a/spec/services/users/approve_service_spec.rb +++ b/spec/services/users/approve_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Users::ApproveService do let_it_be(:current_user) { create(:admin) } + let(:user) { create(:user, :blocked_pending_approval) } subject(:execute) { described_class.new(current_user).execute(user) } diff --git a/spec/services/users/reject_service_spec.rb b/spec/services/users/reject_service_spec.rb index b9aaff5cde5..b0094a7c47e 100644 --- a/spec/services/users/reject_service_spec.rb +++ b/spec/services/users/reject_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Users::RejectService do let_it_be(:current_user) { create(:admin) } + let(:user) { create(:user, :blocked_pending_approval) } subject(:execute) { described_class.new(current_user).execute(user) } diff --git a/spec/services/users/validate_otp_service_spec.rb b/spec/services/users/validate_otp_service_spec.rb index 42f0c10488c..46b80b2149f 100644 --- a/spec/services/users/validate_otp_service_spec.rb +++ b/spec/services/users/validate_otp_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Users::ValidateOtpService do let_it_be(:user) { create(:user) } + let(:otp_code) { 42 } subject(:validate) { described_class.new(user).execute(otp_code) } diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 5f53d6f34d8..f9fa46a4fc8 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -418,19 +418,6 @@ RSpec.describe WebHookService do described_class.new(other_hook, data, :push_hooks).async_execute end end - - context 'when the feature flag is disabled' do - before do - stub_feature_flags(web_hooks_rate_limit: false) - end - - it 'queues a worker without tracking the call' do - expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?) - expect_to_perform_worker(project_hook) - - service_instance.async_execute - end - end end context 'when hook has custom context attributes' do diff --git a/spec/services/wiki_pages/create_service_spec.rb b/spec/services/wiki_pages/create_service_spec.rb index 44b57088319..fd3776f4207 100644 --- a/spec/services/wiki_pages/create_service_spec.rb +++ b/spec/services/wiki_pages/create_service_spec.rb @@ -4,4 +4,24 @@ require 'spec_helper' RSpec.describe WikiPages::CreateService do it_behaves_like 'WikiPages::CreateService#execute', :project + + describe '#execute' do + let_it_be(:project) { create(:project) } + + subject(:service) { described_class.new(container: project) } + + context 'when wiki create fails due to git error' do + let(:wiki_git_error) { 'Could not create wiki page' } + + it 'catches the thrown error and returns a ServiceResponse error' do + allow_next_instance_of(WikiPage) do |instance| + allow(instance).to receive(:create).and_raise(Gitlab::Git::CommandError.new(wiki_git_error)) + end + + result = service.execute + expect(result).to be_error + expect(result.message).to eq(wiki_git_error) + end + end + end end diff --git a/spec/services/wiki_pages/event_create_service_spec.rb b/spec/services/wiki_pages/event_create_service_spec.rb index 974f2591763..6bc6a678189 100644 --- a/spec/services/wiki_pages/event_create_service_spec.rb +++ b/spec/services/wiki_pages/event_create_service_spec.rb @@ -10,6 +10,7 @@ RSpec.describe WikiPages::EventCreateService do describe '#execute' do let_it_be(:page) { create(:wiki_page, project: project) } + let(:slug) { generate(:sluggified_title) } let(:action) { :created } let(:fingerprint) { page.sha } diff --git a/spec/services/wiki_pages/update_service_spec.rb b/spec/services/wiki_pages/update_service_spec.rb index 33ac98e764d..62881817e32 100644 --- a/spec/services/wiki_pages/update_service_spec.rb +++ b/spec/services/wiki_pages/update_service_spec.rb @@ -4,4 +4,26 @@ require 'spec_helper' RSpec.describe WikiPages::UpdateService do it_behaves_like 'WikiPages::UpdateService#execute', :project + + describe '#execute' do + let_it_be(:project) { create(:project) } + + let(:page) { create(:wiki_page, project: project) } + + subject(:service) { described_class.new(container: project) } + + context 'when wiki create fails due to git error' do + let(:wiki_git_error) { 'Could not update wiki page' } + + it 'catches the thrown error and returns a ServiceResponse error' do + allow_next_instance_of(WikiPage) do |instance| + allow(instance).to receive(:update).and_raise(Gitlab::Git::CommandError.new(wiki_git_error)) + end + + result = service.execute(page) + expect(result).to be_error + expect(result.message).to eq(wiki_git_error) + end + end + end end |