diff options
Diffstat (limited to 'spec/workers')
32 files changed, 1240 insertions, 641 deletions
diff --git a/spec/workers/authorized_project_update/project_create_worker_spec.rb b/spec/workers/authorized_project_update/project_create_worker_spec.rb deleted file mode 100644 index 5226ab30de7..00000000000 --- a/spec/workers/authorized_project_update/project_create_worker_spec.rb +++ /dev/null @@ -1,50 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe AuthorizedProjectUpdate::ProjectCreateWorker do - let_it_be(:group) { create(:group, :private) } - let_it_be(:group_project) { create(:project, group: group) } - let_it_be(:group_user) { create(:user) } - - let(:access_level) { Gitlab::Access::MAINTAINER } - - subject(:worker) { described_class.new } - - it 'calls AuthorizedProjectUpdate::ProjectCreateService' do - expect_next_instance_of(AuthorizedProjectUpdate::ProjectCreateService) do |service| - expect(service).to(receive(:execute)) - end - - worker.perform(group_project.id) - end - - it 'returns ServiceResponse.success' do - result = worker.perform(group_project.id) - - expect(result.success?).to be_truthy - end - - context 'idempotence' do - before do - create(:group_member, access_level: access_level, group: group, user: group_user) - ProjectAuthorization.delete_all - end - - include_examples 'an idempotent worker' do - let(:job_args) { group_project.id } - - it 'creates project authorization' do - subject - - project_authorization = ProjectAuthorization.where( - project_id: group_project.id, - user_id: group_user.id, - access_level: access_level) - - expect(project_authorization).to exist - expect(ProjectAuthorization.count).to eq(1) - end - end - end -end diff --git a/spec/workers/authorized_project_update/project_group_link_create_worker_spec.rb b/spec/workers/authorized_project_update/project_group_link_create_worker_spec.rb deleted file mode 100644 index 7c4ad4ce641..00000000000 --- a/spec/workers/authorized_project_update/project_group_link_create_worker_spec.rb +++ /dev/null @@ -1,52 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe AuthorizedProjectUpdate::ProjectGroupLinkCreateWorker do - let_it_be(:group) { create(:group, :private) } - let_it_be(:group_project) { create(:project, group: group) } - let_it_be(:shared_with_group) { create(:group, :private) } - let_it_be(:user) { create(:user) } - - let(:access_level) { Gitlab::Access::MAINTAINER } - - subject(:worker) { described_class.new } - - it 'calls AuthorizedProjectUpdate::ProjectCreateService' do - expect_next_instance_of(AuthorizedProjectUpdate::ProjectGroupLinkCreateService) do |service| - expect(service).to(receive(:execute)) - end - - worker.perform(group_project.id, shared_with_group.id) - end - - it 'returns ServiceResponse.success' do - result = worker.perform(group_project.id, shared_with_group.id) - - expect(result.success?).to be_truthy - end - - context 'idempotence' do - before do - create(:group_member, group: shared_with_group, user: user, access_level: access_level) - create(:project_group_link, project: group_project, group: shared_with_group) - ProjectAuthorization.delete_all - end - - include_examples 'an idempotent worker' do - let(:job_args) { [group_project.id, shared_with_group.id] } - - it 'creates project authorization' do - subject - - project_authorization = ProjectAuthorization.where( - project_id: group_project.id, - user_id: user.id, - access_level: access_level) - - expect(project_authorization).to exist - expect(ProjectAuthorization.count).to eq(1) - end - end - end -end diff --git a/spec/workers/build_finished_worker_spec.rb b/spec/workers/build_finished_worker_spec.rb index 2ca7837066b..b4b986662d2 100644 --- a/spec/workers/build_finished_worker_spec.rb +++ b/spec/workers/build_finished_worker_spec.rb @@ -13,7 +13,7 @@ RSpec.describe BuildFinishedWorker do before do stub_feature_flags(ci_build_finished_worker_namespace_changed: build.project) - expect(Ci::Build).to receive(:find_by).with(id: build.id).and_return(build) + expect(Ci::Build).to receive(:find_by).with({ id: build.id }).and_return(build) end it 'calculates coverage and calls hooks', :aggregate_failures do diff --git a/spec/workers/bulk_imports/pipeline_worker_spec.rb b/spec/workers/bulk_imports/pipeline_worker_spec.rb index 3578fec5bc0..209ae8862b6 100644 --- a/spec/workers/bulk_imports/pipeline_worker_spec.rb +++ b/spec/workers/bulk_imports/pipeline_worker_spec.rb @@ -9,7 +9,7 @@ RSpec.describe BulkImports::PipelineWorker do def run; end - def self.ndjson_pipeline? + def self.file_extraction_pipeline? false end end @@ -222,14 +222,14 @@ RSpec.describe BulkImports::PipelineWorker do end end - context 'when ndjson pipeline' do - let(:ndjson_pipeline) do + context 'when file extraction pipeline' do + let(:file_extraction_pipeline) do Class.new do def initialize(_); end def run; end - def self.ndjson_pipeline? + def self.file_extraction_pipeline? true end @@ -249,11 +249,11 @@ RSpec.describe BulkImports::PipelineWorker do end before do - stub_const('NdjsonPipeline', ndjson_pipeline) + stub_const('NdjsonPipeline', file_extraction_pipeline) allow_next_instance_of(BulkImports::Groups::Stage) do |instance| allow(instance).to receive(:pipelines) - .and_return([[0, ndjson_pipeline]]) + .and_return([[0, file_extraction_pipeline]]) end end @@ -278,7 +278,7 @@ RSpec.describe BulkImports::PipelineWorker do expect(described_class) .to receive(:perform_in) .with( - described_class::NDJSON_PIPELINE_PERFORM_DELAY, + described_class::FILE_EXTRACTION_PIPELINE_PERFORM_DELAY, pipeline_tracker.id, pipeline_tracker.stage, entity.id diff --git a/spec/workers/ci/build_finished_worker_spec.rb b/spec/workers/ci/build_finished_worker_spec.rb index 839723ac2fc..e9e7a057f98 100644 --- a/spec/workers/ci/build_finished_worker_spec.rb +++ b/spec/workers/ci/build_finished_worker_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Ci::BuildFinishedWorker do before do stub_feature_flags(ci_build_finished_worker_namespace_changed: build.project) - expect(Ci::Build).to receive(:find_by).with(id: build.id).and_return(build) + expect(Ci::Build).to receive(:find_by).with({ id: build.id }).and_return(build) end it 'calculates coverage and calls hooks', :aggregate_failures do diff --git a/spec/workers/cleanup_container_repository_worker_spec.rb b/spec/workers/cleanup_container_repository_worker_spec.rb index 6723ea2049d..edb815f426d 100644 --- a/spec/workers/cleanup_container_repository_worker_spec.rb +++ b/spec/workers/cleanup_container_repository_worker_spec.rb @@ -13,11 +13,11 @@ RSpec.describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_stat let(:service) { instance_double(Projects::ContainerRepository::CleanupTagsService) } context 'bulk delete api' do - let(:params) { { key: 'value', 'container_expiration_policy' => false } } + let(:params) { { key: 'value' } } it 'executes the destroy service' do expect(Projects::ContainerRepository::CleanupTagsService).to receive(:new) - .with(repository, user, params.merge('container_expiration_policy' => false)) + .with(repository, user, params) .and_return(service) expect(service).to receive(:execute) @@ -36,40 +36,5 @@ RSpec.describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_stat end.not_to raise_error end end - - context 'container expiration policy' do - let(:params) { { key: 'value', 'container_expiration_policy' => true } } - - before do - allow(ContainerRepository) - .to receive(:find_by_id).with(repository.id).and_return(repository) - end - - it 'executes the destroy service' do - expect(repository).to receive(:start_expiration_policy!).and_call_original - expect(repository).to receive(:reset_expiration_policy_started_at!).and_call_original - expect(Projects::ContainerRepository::CleanupTagsService).to receive(:new) - .with(repository, nil, params.merge('container_expiration_policy' => true)) - .and_return(service) - - expect(service).to receive(:execute).and_return(status: :success) - - subject.perform(nil, repository.id, params) - expect(repository.reload.expiration_policy_started_at).to be_nil - end - - it "doesn't reset the expiration policy started at if the destroy service returns an error" do - expect(repository).to receive(:start_expiration_policy!).and_call_original - expect(repository).not_to receive(:reset_expiration_policy_started_at!) - expect(Projects::ContainerRepository::CleanupTagsService).to receive(:new) - .with(repository, nil, params.merge('container_expiration_policy' => true)) - .and_return(service) - - expect(service).to receive(:execute).and_return(status: :error, message: 'timeout while deleting tags') - - subject.perform(nil, repository.id, params) - expect(repository.reload.expiration_policy_started_at).not_to be_nil - end - end end end diff --git a/spec/workers/clusters/applications/activate_service_worker_spec.rb b/spec/workers/clusters/applications/activate_service_worker_spec.rb index 019bfe7a750..d13ff76613c 100644 --- a/spec/workers/clusters/applications/activate_service_worker_spec.rb +++ b/spec/workers/clusters/applications/activate_service_worker_spec.rb @@ -46,7 +46,7 @@ RSpec.describe Clusters::Applications::ActivateServiceWorker, '#perform' do context 'cluster does not exist' do it 'does not raise Record Not Found error' do - expect { described_class.new.perform(0, 'ignored in this context') }.not_to raise_error(ActiveRecord::RecordNotFound) + expect { described_class.new.perform(0, 'ignored in this context') }.not_to raise_error end end end diff --git a/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb b/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb index b5252294b27..3cd82b8bf4d 100644 --- a/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb +++ b/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb @@ -83,19 +83,23 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter, :aggregate_failures do expect(Gitlab::GithubImport::Logger) .to receive(:info) .with( - github_identifiers: github_identifiers, - message: 'starting importer', - project_id: project.id, - importer: 'klass_name' + { + github_identifiers: github_identifiers, + message: 'starting importer', + project_id: project.id, + importer: 'klass_name' + } ) expect(Gitlab::GithubImport::Logger) .to receive(:info) .with( - github_identifiers: github_identifiers, - message: 'importer finished', - project_id: project.id, - importer: 'klass_name' + { + github_identifiers: github_identifiers, + message: 'importer finished', + project_id: project.id, + importer: 'klass_name' + } ) worker.import(project, client, { 'number' => 10, 'github_id' => 1 }) @@ -120,10 +124,12 @@ RSpec.describe Gitlab::GithubImport::ObjectImporter, :aggregate_failures do expect(Gitlab::GithubImport::Logger) .to receive(:info) .with( - github_identifiers: github_identifiers, - message: 'starting importer', - project_id: project.id, - importer: 'klass_name' + { + github_identifiers: github_identifiers, + message: 'starting importer', + project_id: project.id, + importer: 'klass_name' + } ) expect(Gitlab::Import::ImportFailureService) diff --git a/spec/workers/concerns/gitlab/github_import/stage_methods_spec.rb b/spec/workers/concerns/gitlab/github_import/stage_methods_spec.rb index aeb86f5aa8c..1e088929f66 100644 --- a/spec/workers/concerns/gitlab/github_import/stage_methods_spec.rb +++ b/spec/workers/concerns/gitlab/github_import/stage_methods_spec.rb @@ -38,17 +38,21 @@ RSpec.describe Gitlab::GithubImport::StageMethods do expect(Gitlab::GithubImport::Logger) .to receive(:info) .with( - message: 'starting stage', - project_id: project.id, - import_stage: 'DummyStage' + { + message: 'starting stage', + project_id: project.id, + import_stage: 'DummyStage' + } ) expect(Gitlab::GithubImport::Logger) .to receive(:info) .with( - message: 'stage finished', - project_id: project.id, - import_stage: 'DummyStage' + { + message: 'stage finished', + project_id: project.id, + import_stage: 'DummyStage' + } ) worker.perform(project.id) @@ -70,18 +74,22 @@ RSpec.describe Gitlab::GithubImport::StageMethods do expect(Gitlab::GithubImport::Logger) .to receive(:info) .with( - message: 'starting stage', - project_id: project.id, - import_stage: 'DummyStage' + { + message: 'starting stage', + project_id: project.id, + import_stage: 'DummyStage' + } ) expect(Gitlab::Import::ImportFailureService) .to receive(:track) .with( - project_id: project.id, - exception: exception, - error_source: 'DummyStage', - fail_import: false + { + project_id: project.id, + exception: exception, + error_source: 'DummyStage', + fail_import: false + } ).and_call_original expect { worker.perform(project.id) } @@ -125,9 +133,11 @@ RSpec.describe Gitlab::GithubImport::StageMethods do expect(Gitlab::GithubImport::Logger) .to receive(:info) .with( - message: 'starting stage', - project_id: project.id, - import_stage: 'DummyStage' + { + message: 'starting stage', + project_id: project.id, + import_stage: 'DummyStage' + } ) expect(Gitlab::Import::ImportFailureService) diff --git a/spec/workers/container_expiration_policies/cleanup_container_repository_worker_spec.rb b/spec/workers/container_expiration_policies/cleanup_container_repository_worker_spec.rb index cbffb8f3870..3cb83a7a5d7 100644 --- a/spec/workers/container_expiration_policies/cleanup_container_repository_worker_spec.rb +++ b/spec/workers/container_expiration_policies/cleanup_container_repository_worker_spec.rb @@ -524,13 +524,5 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do end it { is_expected.to eq(capacity) } - - context 'with feature flag disabled' do - before do - stub_feature_flags(container_registry_expiration_policies_throttling: false) - end - - it { is_expected.to eq(0) } - end end end diff --git a/spec/workers/container_expiration_policy_worker_spec.rb b/spec/workers/container_expiration_policy_worker_spec.rb index 2cfb613865d..ef6266aeba3 100644 --- a/spec/workers/container_expiration_policy_worker_spec.rb +++ b/spec/workers/container_expiration_policy_worker_spec.rb @@ -11,15 +11,13 @@ RSpec.describe ContainerExpirationPolicyWorker do describe '#perform' do subject { worker.perform } - shared_examples 'not executing any policy' do - it 'does not run any policy' do - expect(ContainerExpirationPolicyService).not_to receive(:new) + context 'process cleanups' do + it 'calls the limited capacity worker' do + expect(ContainerExpirationPolicies::CleanupContainerRepositoryWorker).to receive(:perform_with_capacity) subject end - end - shared_examples 'handling a taken exclusive lease' do context 'with exclusive lease taken' do before do stub_exclusive_lease_taken(worker.lease_key, timeout: 5.hours) @@ -34,82 +32,6 @@ RSpec.describe ContainerExpirationPolicyWorker do end end - context 'with throttling enabled' do - before do - stub_feature_flags(container_registry_expiration_policies_throttling: true) - end - - it 'calls the limited capacity worker' do - expect(ContainerExpirationPolicies::CleanupContainerRepositoryWorker).to receive(:perform_with_capacity) - - subject - end - - it_behaves_like 'handling a taken exclusive lease' - end - - context 'with throttling disabled' do - before do - stub_feature_flags(container_registry_expiration_policies_throttling: false) - end - - context 'with no container expiration policies' do - it_behaves_like 'not executing any policy' - end - - context 'with container expiration policies' do - let_it_be(:container_expiration_policy, reload: true) { create(:container_expiration_policy, :runnable) } - let_it_be(:container_repository) { create(:container_repository, project: container_expiration_policy.project) } - - context 'a valid policy' do - it 'runs the policy' do - expect(ContainerExpirationPolicyService) - .to receive(:new).with(container_expiration_policy.project, nil).and_call_original - expect(CleanupContainerRepositoryWorker).to receive(:perform_async).once.and_call_original - - expect { subject }.not_to raise_error - end - end - - context 'a disabled policy' do - before do - container_expiration_policy.disable! - end - - it_behaves_like 'not executing any policy' - end - - context 'a policy that is not due for a run' do - before do - container_expiration_policy.update_column(:next_run_at, 2.minutes.from_now) - end - - it_behaves_like 'not executing any policy' - end - - context 'a policy linked to no container repository' do - before do - container_expiration_policy.container_repositories.delete_all - end - - it_behaves_like 'not executing any policy' - end - - context 'an invalid policy' do - before do - container_expiration_policy.update_column(:name_regex, '*production') - end - - it 'disables the policy and tracks an error' do - expect(ContainerExpirationPolicyService).not_to receive(:new).with(container_expiration_policy, nil) - expect(Gitlab::ErrorTracking).to receive(:log_exception).with(instance_of(described_class::InvalidPolicyError), container_expiration_policy_id: container_expiration_policy.id) - - expect { subject }.to change { container_expiration_policy.reload.enabled }.from(true).to(false) - end - end - end - end - context 'process stale ongoing cleanups' do let_it_be(:stuck_cleanup) { create(:container_repository, :cleanup_ongoing, expiration_policy_started_at: 1.day.ago) } let_it_be(:container_repository1) { create(:container_repository, :cleanup_scheduled) } diff --git a/spec/workers/container_registry/migration/enqueuer_worker_spec.rb b/spec/workers/container_registry/migration/enqueuer_worker_spec.rb index 81fa28dc603..a57a9e3b2e8 100644 --- a/spec/workers/container_registry/migration/enqueuer_worker_spec.rb +++ b/spec/workers/container_registry/migration/enqueuer_worker_spec.rb @@ -23,273 +23,669 @@ RSpec.describe ContainerRegistry::Migration::EnqueuerWorker, :aggregate_failures shared_examples 'no action' do it 'does not queue or change any repositories' do + expect(worker).not_to receive(:handle_next_migration) + expect(worker).not_to receive(:handle_aborted_migration) + subject expect(container_repository.reload).to be_default end end - shared_examples 're-enqueuing based on capacity' do |capacity_limit: 4| - context 'below capacity' do + context 'with container_registry_migration_phase2_enqueuer_loop disabled' do + before do + stub_feature_flags(container_registry_migration_phase2_enqueuer_loop: false) + end + + shared_examples 're-enqueuing based on capacity' do |capacity_limit: 4| + context 'below capacity' do + before do + allow(ContainerRegistry::Migration).to receive(:capacity).and_return(capacity_limit) + end + + it 're-enqueues the worker' do + expect(described_class).to receive(:perform_async) + expect(described_class).to receive(:perform_in).with(7.seconds) + + subject + end + + context 'enqueue_twice feature flag disabled' do + before do + stub_feature_flags(container_registry_migration_phase2_enqueue_twice: false) + end + + it 'only enqueues the worker once' do + expect(described_class).to receive(:perform_async) + expect(described_class).not_to receive(:perform_in) + + subject + end + end + end + + context 'above capacity' do + before do + allow(ContainerRegistry::Migration).to receive(:capacity).and_return(-1) + end + + it 'does not re-enqueue the worker' do + expect(described_class).not_to receive(:perform_async) + expect(described_class).not_to receive(:perform_in).with(7.seconds) + + subject + end + end + end + + context 'with qualified repository' do before do - allow(ContainerRegistry::Migration).to receive(:capacity).and_return(capacity_limit) + allow_worker(on: :next_repository) do |repository| + allow(repository).to receive(:migration_pre_import).and_return(:ok) + end end - it 're-enqueues the worker' do - expect(described_class).to receive(:perform_async) + shared_examples 'starting the next import' do + it 'starts the pre-import for the next qualified repository' do + expect_log_extra_metadata( + import_type: 'next', + container_repository_id: container_repository.id, + container_repository_path: container_repository.path, + container_repository_migration_state: 'pre_importing' + ) - subject + expect { subject }.to make_queries_matching(/LIMIT 2/) + + expect(container_repository.reload).to be_pre_importing + end + end + + it_behaves_like 'starting the next import' + + context 'when the new pre-import maxes out the capacity' do + before do + # set capacity to 10 + stub_feature_flags( + container_registry_migration_phase2_capacity_25: false + ) + + # Plus 2 created above gives 9 importing repositories + create_list(:container_repository, 7, :importing) + end + + it 'does not re-enqueue the worker' do + expect(described_class).not_to receive(:perform_async) + expect(described_class).not_to receive(:perform_in) + + subject + end + end + + it_behaves_like 're-enqueuing based on capacity' + + context 'max tag count is 0' do + before do + stub_application_setting(container_registry_import_max_tags_count: 0) + # Add 8 tags to the next repository + stub_container_registry_tags( + repository: container_repository.path, tags: %w(a b c d e f g h), with_manifest: true + ) + end + + it_behaves_like 'starting the next import' + end + end + + context 'migrations are disabled' do + before do + allow(ContainerRegistry::Migration).to receive(:enabled?).and_return(false) + end + + it_behaves_like 'no action' do + before do + expect_log_extra_metadata(migration_enabled: false) + end end end context 'above capacity' do before do - allow(ContainerRegistry::Migration).to receive(:capacity).and_return(-1) + create(:container_repository, :importing) + create(:container_repository, :importing) + allow(ContainerRegistry::Migration).to receive(:capacity).and_return(1) + end + + it_behaves_like 'no action' do + before do + expect_log_extra_metadata(below_capacity: false, max_capacity_setting: 1) + end end it 'does not re-enqueue the worker' do - expect(described_class).not_to receive(:perform_async) + expect(ContainerRegistry::Migration::EnqueuerWorker).not_to receive(:perform_async) + expect(ContainerRegistry::Migration::EnqueuerWorker).not_to receive(:perform_in) subject end end - end - context 'with qualified repository' do - before do - method = worker.method(:next_repository) - allow(worker).to receive(:next_repository) do - next_qualified_repository = method.call - allow(next_qualified_repository).to receive(:migration_pre_import).and_return(:ok) - next_qualified_repository + context 'too soon before previous completed import step' do + where(:state, :timestamp) do + :import_done | :migration_import_done_at + :pre_import_done | :migration_pre_import_done_at + :import_aborted | :migration_aborted_at + :import_skipped | :migration_skipped_at end - end - it 'starts the pre-import for the next qualified repository' do - expect_log_extra_metadata( - import_type: 'next', - container_repository_id: container_repository.id, - container_repository_path: container_repository.path, - container_repository_migration_state: 'pre_importing' - ) + with_them do + before do + allow(ContainerRegistry::Migration).to receive(:enqueue_waiting_time).and_return(45.minutes) + create(:container_repository, state, timestamp => 1.minute.ago) + end - subject + it_behaves_like 'no action' do + before do + expect_log_extra_metadata(waiting_time_passed: false, current_waiting_time_setting: 45.minutes) + end + end + end - expect(container_repository.reload).to be_pre_importing + context 'when last completed repository has nil timestamps' do + before do + allow(ContainerRegistry::Migration).to receive(:enqueue_waiting_time).and_return(45.minutes) + create(:container_repository, migration_state: 'import_done') + end + + it 'continues to try the next import' do + expect { subject }.to change { container_repository.reload.migration_state } + end + end end - context 'when the new pre-import maxes out the capacity' do - before do - # set capacity to 10 - stub_feature_flags( - container_registry_migration_phase2_capacity_25: false - ) + context 'when an aborted import is available' do + let_it_be(:aborted_repository) { create(:container_repository, :import_aborted) } - # Plus 2 created above gives 9 importing repositories - create_list(:container_repository, 7, :importing) + context 'with a successful registry request' do + before do + allow_worker(on: :next_aborted_repository) do |repository| + allow(repository).to receive(:migration_import).and_return(:ok) + allow(repository.gitlab_api_client).to receive(:import_status).and_return('import_failed') + end + end + + it 'retries the import for the aborted repository' do + expect_log_extra_metadata( + import_type: 'retry', + container_repository_id: aborted_repository.id, + container_repository_path: aborted_repository.path, + container_repository_migration_state: 'importing' + ) + + subject + + expect(aborted_repository.reload).to be_importing + expect(container_repository.reload).to be_default + end + + it_behaves_like 're-enqueuing based on capacity' end - it 'does not re-enqueue the worker' do - expect(described_class).not_to receive(:perform_async) + context 'when an error occurs' do + it 'does not abort that migration' do + allow_worker(on: :next_aborted_repository) do |repository| + allow(repository).to receive(:retry_aborted_migration).and_raise(StandardError) + end - subject + expect_log_extra_metadata( + import_type: 'retry', + container_repository_id: aborted_repository.id, + container_repository_path: aborted_repository.path, + container_repository_migration_state: 'import_aborted' + ) + + subject + + expect(aborted_repository.reload).to be_import_aborted + expect(container_repository.reload).to be_default + end end end - it_behaves_like 're-enqueuing based on capacity' - end + context 'when no repository qualifies' do + include_examples 'an idempotent worker' do + before do + allow(ContainerRepository).to receive(:ready_for_import).and_return(ContainerRepository.none) + end - context 'migrations are disabled' do - before do - allow(ContainerRegistry::Migration).to receive(:enabled?).and_return(false) + it_behaves_like 'no action' + end end - it_behaves_like 'no action' do + context 'over max tag count' do before do - expect_log_extra_metadata(migration_enabled: false) + stub_application_setting(container_registry_import_max_tags_count: 2) end - end - end - context 'above capacity' do - before do - create(:container_repository, :importing) - create(:container_repository, :importing) - allow(ContainerRegistry::Migration).to receive(:capacity).and_return(1) + it 'skips the repository' do + expect_log_extra_metadata( + import_type: 'next', + container_repository_id: container_repository.id, + container_repository_path: container_repository.path, + container_repository_migration_state: 'import_skipped', + tags_count_too_high: true, + max_tags_count_setting: 2 + ) + + subject + + expect(container_repository.reload).to be_import_skipped + expect(container_repository.migration_skipped_reason).to eq('too_many_tags') + expect(container_repository.migration_skipped_at).not_to be_nil + end + + context 're-enqueuing' do + before do + # skipping will also re-enqueue, so we isolate the capacity behavior here + allow_worker(on: :next_repository) do |repository| + allow(repository).to receive(:skip_import).and_return(true) + end + end + + it_behaves_like 're-enqueuing based on capacity', capacity_limit: 3 + end end - it_behaves_like 'no action' do + context 'when an error occurs' do before do - expect_log_extra_metadata(below_capacity: false, max_capacity_setting: 1) + allow(ContainerRegistry::Migration).to receive(:max_tags_count).and_raise(StandardError) end - end - it 'does not re-enqueue the worker' do - expect(ContainerRegistry::Migration::EnqueuerWorker).not_to receive(:perform_async) + it 'aborts the import' do + expect_log_extra_metadata( + import_type: 'next', + container_repository_id: container_repository.id, + container_repository_path: container_repository.path, + container_repository_migration_state: 'import_aborted' + ) + + expect(Gitlab::ErrorTracking).to receive(:log_exception).with( + instance_of(StandardError), + next_repository_id: container_repository.id + ) - subject + subject + + expect(container_repository.reload).to be_import_aborted + end end - end - context 'too soon before previous completed import step' do - where(:state, :timestamp) do - :import_done | :migration_import_done_at - :pre_import_done | :migration_pre_import_done_at - :import_aborted | :migration_aborted_at - :import_skipped | :migration_skipped_at + context 'with the exclusive lease taken' do + let(:lease_key) { worker.send(:lease_key) } + + before do + stub_exclusive_lease_taken(lease_key, timeout: 30.minutes) + end + + it 'does not perform' do + expect(worker).not_to receive(:runnable?) + expect(worker).not_to receive(:re_enqueue_if_capacity) + + subject + end end + end - with_them do + context 'with container_registry_migration_phase2_enqueuer_loop enabled' do + context 'migrations are disabled' do before do - allow(ContainerRegistry::Migration).to receive(:enqueue_waiting_time).and_return(45.minutes) - create(:container_repository, state, timestamp => 1.minute.ago) + allow(ContainerRegistry::Migration).to receive(:enabled?).and_return(false) end it_behaves_like 'no action' do before do - expect_log_extra_metadata(waiting_time_passed: false, current_waiting_time_setting: 45.minutes) + expect_log_extra_metadata(migration_enabled: false) end end end - context 'when last completed repository has nil timestamps' do + context 'with no repository qualifies' do + include_examples 'an idempotent worker' do + before do + allow(ContainerRepository).to receive(:ready_for_import).and_return(ContainerRepository.none) + end + + it_behaves_like 'no action' + end + end + + context 'when multiple aborted imports are available' do + let_it_be(:aborted_repository1) { create(:container_repository, :import_aborted) } + let_it_be(:aborted_repository2) { create(:container_repository, :import_aborted) } + before do - allow(ContainerRegistry::Migration).to receive(:enqueue_waiting_time).and_return(45.minutes) - create(:container_repository, migration_state: 'import_done') + container_repository.update!(created_at: 30.seconds.ago) end - it 'continues to try the next import' do - expect { subject }.to change { container_repository.reload.migration_state } + context 'with successful registry requests' do + before do + allow_worker(on: :next_aborted_repository) do |repository| + allow(repository).to receive(:migration_import).and_return(:ok) + allow(repository.gitlab_api_client).to receive(:import_status).and_return('import_failed') + end + end + + it 'retries the import for the aborted repository' do + expect_log_info( + [ + { + import_type: 'retry', + container_repository_id: aborted_repository1.id, + container_repository_path: aborted_repository1.path, + container_repository_migration_state: 'importing' + }, + { + import_type: 'retry', + container_repository_id: aborted_repository2.id, + container_repository_path: aborted_repository2.path, + container_repository_migration_state: 'importing' + } + ] + ) + + expect(worker).to receive(:handle_next_migration).and_call_original + + subject + + expect(aborted_repository1.reload).to be_importing + expect(aborted_repository2.reload).to be_importing + end + end + + context 'when an error occurs' do + it 'does abort that migration' do + allow_worker(on: :next_aborted_repository) do |repository| + allow(repository).to receive(:retry_aborted_migration).and_raise(StandardError) + end + + expect_log_info( + [ + { + import_type: 'retry', + container_repository_id: aborted_repository1.id, + container_repository_path: aborted_repository1.path, + container_repository_migration_state: 'import_aborted' + } + ] + ) + + subject + + expect(aborted_repository1.reload).to be_import_aborted + expect(aborted_repository2.reload).to be_import_aborted + end end end - end - context 'when an aborted import is available' do - let_it_be(:aborted_repository) { create(:container_repository, :import_aborted) } + context 'when multiple qualified repositories are available' do + let_it_be(:container_repository2) { create(:container_repository, created_at: 2.days.ago) } - context 'with a successful registry request' do before do - method = worker.method(:next_aborted_repository) - allow(worker).to receive(:next_aborted_repository) do - next_aborted_repository = method.call - allow(next_aborted_repository).to receive(:migration_import).and_return(:ok) - allow(next_aborted_repository.gitlab_api_client).to receive(:import_status).and_return('import_failed') - next_aborted_repository + allow_worker(on: :next_repository) do |repository| + allow(repository).to receive(:migration_pre_import).and_return(:ok) end - end - it 'retries the import for the aborted repository' do - expect_log_extra_metadata( - import_type: 'retry', - container_repository_id: aborted_repository.id, - container_repository_path: aborted_repository.path, - container_repository_migration_state: 'importing' + stub_container_registry_tags( + repository: container_repository2.path, + tags: %w(tag4 tag5 tag6), + with_manifest: true ) + end - subject + shared_examples 'starting all the next imports' do + it 'starts the pre-import for the next qualified repositories' do + expect_log_info( + [ + { + import_type: 'next', + container_repository_id: container_repository.id, + container_repository_path: container_repository.path, + container_repository_migration_state: 'pre_importing' + }, + { + import_type: 'next', + container_repository_id: container_repository2.id, + container_repository_path: container_repository2.path, + container_repository_migration_state: 'pre_importing' + } + ] + ) + + expect(worker).to receive(:handle_next_migration).exactly(3).times.and_call_original + + expect { subject }.to make_queries_matching(/LIMIT 2/) + + expect(container_repository.reload).to be_pre_importing + expect(container_repository2.reload).to be_pre_importing + end + end - expect(aborted_repository.reload).to be_importing - expect(container_repository.reload).to be_default + it_behaves_like 'starting all the next imports' + + context 'when the new pre-import maxes out the capacity' do + before do + # set capacity to 10 + stub_feature_flags( + container_registry_migration_phase2_capacity_25: false + ) + + # Plus 2 created above gives 9 importing repositories + create_list(:container_repository, 7, :importing) + end + + it 'starts the pre-import only for one qualified repository' do + expect_log_info( + [ + { + import_type: 'next', + container_repository_id: container_repository.id, + container_repository_path: container_repository.path, + container_repository_migration_state: 'pre_importing' + } + ] + ) + + subject + + expect(container_repository.reload).to be_pre_importing + expect(container_repository2.reload).to be_default + end end - it_behaves_like 're-enqueuing based on capacity' + context 'max tag count is 0' do + before do + stub_application_setting(container_registry_import_max_tags_count: 0) + # Add 8 tags to the next repository + stub_container_registry_tags( + repository: container_repository.path, tags: %w(a b c d e f g h), with_manifest: true + ) + end + + it_behaves_like 'starting all the next imports' + end + + context 'when the deadline is hit' do + it 'does not handle the second qualified repository' do + expect(worker).to receive(:loop_deadline).and_return(5.seconds.from_now, 2.seconds.ago) + expect(worker).to receive(:handle_next_migration).once.and_call_original + + subject + + expect(container_repository.reload).to be_pre_importing + expect(container_repository2.reload).to be_default + end + end end - context 'when an error occurs' do - it 'does not abort that migration' do - method = worker.method(:next_aborted_repository) - allow(worker).to receive(:next_aborted_repository) do - next_aborted_repository = method.call - allow(next_aborted_repository).to receive(:retry_aborted_migration).and_raise(StandardError) - next_aborted_repository + context 'when a mix of aborted imports and qualified repositories are available' do + let_it_be(:aborted_repository) { create(:container_repository, :import_aborted) } + + before do + allow_worker(on: :next_aborted_repository) do |repository| + allow(repository).to receive(:migration_import).and_return(:ok) + allow(repository.gitlab_api_client).to receive(:import_status).and_return('import_failed') end - expect_log_extra_metadata( - import_type: 'retry', - container_repository_id: aborted_repository.id, - container_repository_path: aborted_repository.path, - container_repository_migration_state: 'import_aborted' + allow_worker(on: :next_repository) do |repository| + allow(repository).to receive(:migration_pre_import).and_return(:ok) + end + end + + it 'retries the aborted repository and start the migration on the qualified repository' do + expect_log_info( + [ + { + import_type: 'retry', + container_repository_id: aborted_repository.id, + container_repository_path: aborted_repository.path, + container_repository_migration_state: 'importing' + }, + { + import_type: 'next', + container_repository_id: container_repository.id, + container_repository_path: container_repository.path, + container_repository_migration_state: 'pre_importing' + } + ] ) subject - expect(aborted_repository.reload).to be_import_aborted - expect(container_repository.reload).to be_default + expect(aborted_repository.reload).to be_importing + expect(container_repository.reload).to be_pre_importing end end - end - context 'when no repository qualifies' do - include_examples 'an idempotent worker' do + context 'above capacity' do before do - allow(ContainerRepository).to receive(:ready_for_import).and_return(ContainerRepository.none) + create(:container_repository, :importing) + create(:container_repository, :importing) + allow(ContainerRegistry::Migration).to receive(:capacity).and_return(1) end - it_behaves_like 'no action' + it_behaves_like 'no action' do + before do + expect_log_extra_metadata(below_capacity: false, max_capacity_setting: 1) + end + end end - end - context 'over max tag count' do - before do - stub_application_setting(container_registry_import_max_tags_count: 2) - end + context 'too soon before previous completed import step' do + where(:state, :timestamp) do + :import_done | :migration_import_done_at + :pre_import_done | :migration_pre_import_done_at + :import_aborted | :migration_aborted_at + :import_skipped | :migration_skipped_at + end - it 'skips the repository' do - expect_log_extra_metadata( - import_type: 'next', - container_repository_id: container_repository.id, - container_repository_path: container_repository.path, - container_repository_migration_state: 'import_skipped', - tags_count_too_high: true, - max_tags_count_setting: 2 - ) + with_them do + before do + allow(ContainerRegistry::Migration).to receive(:enqueue_waiting_time).and_return(45.minutes) + create(:container_repository, state, timestamp => 1.minute.ago) + end - subject + it_behaves_like 'no action' do + before do + expect_log_extra_metadata(waiting_time_passed: false, current_waiting_time_setting: 45.minutes) + end + end + end - expect(container_repository.reload).to be_import_skipped - expect(container_repository.migration_skipped_reason).to eq('too_many_tags') - expect(container_repository.migration_skipped_at).not_to be_nil + context 'when last completed repository has nil timestamps' do + before do + allow(ContainerRegistry::Migration).to receive(:enqueue_waiting_time).and_return(45.minutes) + create(:container_repository, migration_state: 'import_done') + end + + it 'continues to try the next import' do + expect { subject }.to change { container_repository.reload.migration_state } + end + end end - it_behaves_like 're-enqueuing based on capacity', capacity_limit: 3 - end + context 'over max tag count' do + before do + stub_application_setting(container_registry_import_max_tags_count: 2) + end - context 'when an error occurs' do - before do - allow(ContainerRegistry::Migration).to receive(:max_tags_count).and_raise(StandardError) + it 'skips the repository' do + expect_log_info( + [ + { + import_type: 'next', + container_repository_id: container_repository.id, + container_repository_path: container_repository.path, + container_repository_migration_state: 'import_skipped', + container_repository_migration_skipped_reason: 'too_many_tags' + } + ] + ) + + expect(worker).to receive(:handle_next_migration).twice.and_call_original + # skipping the migration will re_enqueue the job + expect(described_class).to receive(:enqueue_a_job) + + subject + + expect(container_repository.reload).to be_import_skipped + expect(container_repository.migration_skipped_reason).to eq('too_many_tags') + expect(container_repository.migration_skipped_at).not_to be_nil + end end - it 'aborts the import' do - expect_log_extra_metadata( - import_type: 'next', - container_repository_id: container_repository.id, - container_repository_path: container_repository.path, - container_repository_migration_state: 'import_aborted' - ) + context 'when an error occurs' do + before do + allow(ContainerRegistry::Migration).to receive(:max_tags_count).and_raise(StandardError) + end - expect(Gitlab::ErrorTracking).to receive(:log_exception).with( - instance_of(StandardError), - next_repository_id: container_repository.id - ) + it 'aborts the import' do + expect_log_info( + [ + { + import_type: 'next', + container_repository_id: container_repository.id, + container_repository_path: container_repository.path, + container_repository_migration_state: 'import_aborted' + } + ] + ) - subject + expect(Gitlab::ErrorTracking).to receive(:log_exception).with( + instance_of(StandardError), + next_repository_id: container_repository.id + ) - expect(container_repository.reload).to be_import_aborted - end - end + # aborting the migration will re_enqueue the job + expect(described_class).to receive(:enqueue_a_job) - context 'with the exclusive lease taken' do - let(:lease_key) { worker.send(:lease_key) } + subject - before do - stub_exclusive_lease_taken(lease_key, timeout: 30.minutes) + expect(container_repository.reload).to be_import_aborted + end end - it 'does not perform' do - expect(worker).not_to receive(:runnable?) - expect(worker).not_to receive(:re_enqueue_if_capacity) + context 'with the exclusive lease taken' do + let(:lease_key) { worker.send(:lease_key) } - subject + before do + stub_exclusive_lease_taken(lease_key, timeout: 30.minutes) + end + + it 'does not perform' do + expect(worker).not_to receive(:handle_aborted_migration) + expect(worker).not_to receive(:handle_next_migration) + + subject + end end end @@ -298,5 +694,29 @@ RSpec.describe ContainerRegistry::Migration::EnqueuerWorker, :aggregate_failures expect(worker).to receive(:log_extra_metadata_on_done).with(key, value) end end + + def expect_log_info(expected_multiple_arguments) + expected_multiple_arguments.each do |extras| + expect(worker.logger).to receive(:info).with(worker.structured_payload(extras)) + end + end + + def allow_worker(on:) + method_repository = worker.method(on) + allow(worker).to receive(on) do + repository = method_repository.call + + yield repository if repository + + repository + end + end + end + + describe 'worker attributes' do + it 'has deduplication set' do + expect(described_class.get_deduplicate_strategy).to eq(:until_executing) + expect(described_class.get_deduplication_options).to include(ttl: 30.minutes) + end end end diff --git a/spec/workers/container_registry/migration/guard_worker_spec.rb b/spec/workers/container_registry/migration/guard_worker_spec.rb index 299d1204af3..c52a3fc5d54 100644 --- a/spec/workers/container_registry/migration/guard_worker_spec.rb +++ b/spec/workers/container_registry/migration/guard_worker_spec.rb @@ -25,57 +25,94 @@ RSpec.describe ContainerRegistry::Migration::GuardWorker, :aggregate_failures do allow(::Gitlab).to receive(:com?).and_return(true) end - shared_examples 'handling long running migrations' do + shared_examples 'handling long running migrations' do |timeout:| before do allow_next_found_instance_of(ContainerRepository) do |repository| allow(repository).to receive(:migration_cancel).and_return(migration_cancel_response) end end - context 'migration is canceled' do - let(:migration_cancel_response) { { status: :ok } } - - it 'will not abort the migration' do + shared_examples 'aborting the migration' do + it 'will abort the migration' do expect(worker).to receive(:log_extra_metadata_on_done).with(:stale_migrations_count, 1) expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_stale_migrations_count, 1) expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_long_running_migration_ids, [stale_migration.id]) + expect(ContainerRegistry::Migration).to receive(timeout).and_call_original expect { subject } - .to change(import_skipped_migrations, :count) + .to change(import_aborted_migrations, :count).by(1) + .and change { stale_migration.reload.migration_state }.to('import_aborted') + .and not_change { ongoing_migration.migration_state } + end + + context 'registry_migration_guard_thresholds feature flag disabled' do + before do + stub_feature_flags(registry_migration_guard_thresholds: false) + end + + it 'falls back on the hardcoded value' do + expect(ContainerRegistry::Migration).not_to receive(:pre_import_timeout) - expect(stale_migration.reload.migration_state).to eq('import_skipped') - expect(stale_migration.reload.migration_skipped_reason).to eq('migration_canceled') + expect { subject } + .to change { stale_migration.reload.migration_state }.to('import_aborted') + end end end - context 'migration cancelation fails with an error' do - let(:migration_cancel_response) { { status: :error } } + context 'migration is canceled' do + let(:migration_cancel_response) { { status: :ok } } - it 'will abort the migration' do - expect(worker).to receive(:log_extra_metadata_on_done).with(:stale_migrations_count, 1) - expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_stale_migrations_count, 1) - expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_long_running_migration_ids, [stale_migration.id]) + before do + stub_application_setting(container_registry_import_max_retries: 3) + end - expect { subject } - .to change(import_aborted_migrations, :count).by(1) - .and change { stale_migration.reload.migration_state }.to('import_aborted') - .and not_change { ongoing_migration.migration_state } + context 'when the retry limit has been reached' do + before do + stale_migration.update_column(:migration_retries_count, 2) + end + + it 'will not abort the migration' do + expect(worker).to receive(:log_extra_metadata_on_done).with(:stale_migrations_count, 1) + expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_stale_migrations_count, 1) + expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_long_running_migration_ids, [stale_migration.id]) + expect(ContainerRegistry::Migration).to receive(timeout).and_call_original + + expect { subject } + .to change(import_skipped_migrations, :count) + + expect(stale_migration.reload.migration_state).to eq('import_skipped') + expect(stale_migration.reload.migration_skipped_reason).to eq('migration_canceled') + end + + context 'registry_migration_guard_thresholds feature flag disabled' do + before do + stub_feature_flags(registry_migration_guard_thresholds: false) + end + + it 'falls back on the hardcoded value' do + expect(ContainerRegistry::Migration).not_to receive(timeout) + + expect { subject } + .to change { stale_migration.reload.migration_state }.to('import_skipped') + end + end + end + + context 'when the retry limit has not been reached' do + it_behaves_like 'aborting the migration' end end + context 'migration cancelation fails with an error' do + let(:migration_cancel_response) { { status: :error } } + + it_behaves_like 'aborting the migration' + end + context 'migration receives bad request with a new status' do let(:migration_cancel_response) { { status: :bad_request, migration_state: :import_done } } - it 'will abort the migration' do - expect(worker).to receive(:log_extra_metadata_on_done).with(:stale_migrations_count, 1) - expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_stale_migrations_count, 1) - expect(worker).to receive(:log_extra_metadata_on_done).with(:aborted_long_running_migration_ids, [stale_migration.id]) - - expect { subject } - .to change(import_aborted_migrations, :count).by(1) - .and change { stale_migration.reload.migration_state }.to('import_aborted') - .and not_change { ongoing_migration.migration_state } - end + it_behaves_like 'aborting the migration' end end @@ -96,13 +133,15 @@ RSpec.describe ContainerRegistry::Migration::GuardWorker, :aggregate_failures do context 'with pre_importing stale migrations' do let(:ongoing_migration) { create(:container_repository, :pre_importing) } - let(:stale_migration) { create(:container_repository, :pre_importing, migration_pre_import_started_at: 35.minutes.ago) } + let(:stale_migration) { create(:container_repository, :pre_importing, migration_pre_import_started_at: 11.minutes.ago) } let(:import_status) { 'test' } before do allow_next_instance_of(ContainerRegistry::GitlabApiClient) do |client| allow(client).to receive(:import_status).and_return(import_status) end + + stub_application_setting(container_registry_pre_import_timeout: 10.minutes.to_i) end it 'will abort the migration' do @@ -122,13 +161,13 @@ RSpec.describe ContainerRegistry::Migration::GuardWorker, :aggregate_failures do context 'the client returns pre_import_in_progress' do let(:import_status) { 'pre_import_in_progress' } - it_behaves_like 'handling long running migrations' + it_behaves_like 'handling long running migrations', timeout: :pre_import_timeout end end context 'with pre_import_done stale migrations' do let(:ongoing_migration) { create(:container_repository, :pre_import_done) } - let(:stale_migration) { create(:container_repository, :pre_import_done, migration_pre_import_done_at: 35.minutes.ago) } + let(:stale_migration) { create(:container_repository, :pre_import_done, migration_pre_import_done_at: 11.minutes.ago) } before do allow(::ContainerRegistry::Migration).to receive(:max_step_duration).and_return(5.minutes) @@ -151,13 +190,15 @@ RSpec.describe ContainerRegistry::Migration::GuardWorker, :aggregate_failures do context 'with importing stale migrations' do let(:ongoing_migration) { create(:container_repository, :importing) } - let(:stale_migration) { create(:container_repository, :importing, migration_import_started_at: 35.minutes.ago) } + let(:stale_migration) { create(:container_repository, :importing, migration_import_started_at: 11.minutes.ago) } let(:import_status) { 'test' } before do allow_next_instance_of(ContainerRegistry::GitlabApiClient) do |client| allow(client).to receive(:import_status).and_return(import_status) end + + stub_application_setting(container_registry_import_timeout: 10.minutes.to_i) end it 'will abort the migration' do @@ -177,7 +218,7 @@ RSpec.describe ContainerRegistry::Migration::GuardWorker, :aggregate_failures do context 'the client returns import_in_progress' do let(:import_status) { 'import_in_progress' } - it_behaves_like 'handling long running migrations' + it_behaves_like 'handling long running migrations', timeout: :import_timeout end end end @@ -195,4 +236,11 @@ RSpec.describe ContainerRegistry::Migration::GuardWorker, :aggregate_failures do end end end + + describe 'worker attributes' do + it 'has deduplication set' do + expect(described_class.get_deduplicate_strategy).to eq(:until_executed) + expect(described_class.get_deduplication_options).to include(ttl: 5.minutes) + end + end end diff --git a/spec/workers/create_commit_signature_worker_spec.rb b/spec/workers/create_commit_signature_worker_spec.rb index 0e31faf47af..9d3c63efc8a 100644 --- a/spec/workers/create_commit_signature_worker_spec.rb +++ b/spec/workers/create_commit_signature_worker_spec.rb @@ -10,8 +10,8 @@ RSpec.describe CreateCommitSignatureWorker do let(:x509_commit) { instance_double(Gitlab::X509::Commit) } before do - allow(Project).to receive(:find_by).with(id: project.id).and_return(project) - allow(project).to receive(:commits_by).with(oids: commit_shas).and_return(commits) + allow(Project).to receive(:find_by).with({ id: project.id }).and_return(project) + allow(project).to receive(:commits_by).with({ oids: commit_shas }).and_return(commits) end subject { described_class.new.perform(commit_shas, project.id) } diff --git a/spec/workers/database/ci_namespace_mirrors_consistency_check_worker_spec.rb b/spec/workers/database/ci_namespace_mirrors_consistency_check_worker_spec.rb index 116026ea8f7..e5024c568cb 100644 --- a/spec/workers/database/ci_namespace_mirrors_consistency_check_worker_spec.rb +++ b/spec/workers/database/ci_namespace_mirrors_consistency_check_worker_spec.rb @@ -62,6 +62,15 @@ RSpec.describe Database::CiNamespaceMirrorsConsistencyCheckWorker do expect(worker).to receive(:log_extra_metadata_on_done).with(:results, expected_result) worker.perform end + + it 'calls the consistency_fix_service to fix the inconsistencies' do + allow_next_instance_of(Database::ConsistencyFixService) do |instance| + expect(instance).to receive(:execute).with( + ids: [missing_namespace.id] + ).and_call_original + end + worker.perform + end end end end diff --git a/spec/workers/database/ci_project_mirrors_consistency_check_worker_spec.rb b/spec/workers/database/ci_project_mirrors_consistency_check_worker_spec.rb index b6bd825ffcd..f8e950d8917 100644 --- a/spec/workers/database/ci_project_mirrors_consistency_check_worker_spec.rb +++ b/spec/workers/database/ci_project_mirrors_consistency_check_worker_spec.rb @@ -38,7 +38,7 @@ RSpec.describe Database::CiProjectMirrorsConsistencyCheckWorker do before do redis_shared_state_cleanup! stub_feature_flags(ci_project_mirrors_consistency_check: true) - create_list(:project, 10) # This will also create Ci::NameSpaceMirror objects + create_list(:project, 10) # This will also create Ci::ProjectMirror objects missing_project.delete allow_next_instance_of(Database::ConsistencyCheckService) do |instance| @@ -62,6 +62,15 @@ RSpec.describe Database::CiProjectMirrorsConsistencyCheckWorker do expect(worker).to receive(:log_extra_metadata_on_done).with(:results, expected_result) worker.perform end + + it 'calls the consistency_fix_service to fix the inconsistencies' do + expect_next_instance_of(Database::ConsistencyFixService) do |instance| + expect(instance).to receive(:execute).with( + ids: [missing_project.id] + ).and_call_original + end + worker.perform + end end end end diff --git a/spec/workers/delete_diff_files_worker_spec.rb b/spec/workers/delete_diff_files_worker_spec.rb index cf26dbabb97..c124847ca45 100644 --- a/spec/workers/delete_diff_files_worker_spec.rb +++ b/spec/workers/delete_diff_files_worker_spec.rb @@ -34,11 +34,13 @@ RSpec.describe DeleteDiffFilesWorker do end it 'rollsback if something goes wrong' do + error = RuntimeError.new('something went wrong') + expect(MergeRequestDiffFile).to receive_message_chain(:where, :delete_all) - .and_raise + .and_raise(error) expect { described_class.new.perform(merge_request_diff.id) } - .to raise_error + .to raise_error(error) merge_request_diff.reload diff --git a/spec/workers/delete_user_worker_spec.rb b/spec/workers/delete_user_worker_spec.rb index 52f2c692b8c..4046b670640 100644 --- a/spec/workers/delete_user_worker_spec.rb +++ b/spec/workers/delete_user_worker_spec.rb @@ -16,9 +16,9 @@ RSpec.describe DeleteUserWorker do it "uses symbolized keys" do expect_next_instance_of(Users::DestroyService) do |service| - expect(service).to receive(:execute).with(user, test: "test") + expect(service).to receive(:execute).with(user, { test: "test" }) end - described_class.new.perform(current_user.id, user.id, "test" => "test") + described_class.new.perform(current_user.id, user.id, { "test" => "test" }) end end diff --git a/spec/workers/deployments/hooks_worker_spec.rb b/spec/workers/deployments/hooks_worker_spec.rb index 29b3e8d3ee4..a9240b45360 100644 --- a/spec/workers/deployments/hooks_worker_spec.rb +++ b/spec/workers/deployments/hooks_worker_spec.rb @@ -10,6 +10,16 @@ RSpec.describe Deployments::HooksWorker do allow(ProjectServiceWorker).to receive(:perform_async) end + it 'logs deployment and project IDs as metadata' do + deployment = create(:deployment, :running) + project = deployment.project + + expect(worker).to receive(:log_extra_metadata_on_done).with(:deployment_project_id, project.id) + expect(worker).to receive(:log_extra_metadata_on_done).with(:deployment_id, deployment.id) + + worker.perform(deployment_id: deployment.id, status_changed_at: Time.current) + end + it 'executes project services for deployment_hooks' do deployment = create(:deployment, :running) project = deployment.project diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index 0351b500747..0c83a692ca8 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -126,8 +126,6 @@ RSpec.describe 'Every Sidekiq worker' do 'ApproveBlockedPendingApprovalUsersWorker' => 3, 'ArchiveTraceWorker' => 3, 'AuthorizedKeysWorker' => 3, - 'AuthorizedProjectUpdate::ProjectCreateWorker' => 3, - 'AuthorizedProjectUpdate::ProjectGroupLinkCreateWorker' => 3, 'AuthorizedProjectUpdate::UserRefreshOverUserRangeWorker' => 3, 'AuthorizedProjectUpdate::UserRefreshWithLowUrgencyWorker' => 3, 'AuthorizedProjectUpdate::UserRefreshFromReplicaWorker' => 3, @@ -229,7 +227,6 @@ RSpec.describe 'Every Sidekiq worker' do 'Epics::UpdateEpicsDatesWorker' => 3, 'ErrorTrackingIssueLinkWorker' => 3, 'Experiments::RecordConversionEventWorker' => 3, - 'ExpireBuildInstanceArtifactsWorker' => 3, 'ExpireJobCacheWorker' => 3, 'ExpirePipelineCacheWorker' => 3, 'ExportCsvWorker' => 3, @@ -243,7 +240,6 @@ RSpec.describe 'Every Sidekiq worker' do 'Geo::DesignRepositorySyncWorker' => 1, 'Geo::DestroyWorker' => 3, 'Geo::EventWorker' => 3, - 'Geo::FileDownloadWorker' => 3, 'Geo::FileRegistryRemovalWorker' => 3, 'Geo::FileRemovalWorker' => 3, 'Geo::ProjectSyncWorker' => 1, @@ -352,7 +348,6 @@ RSpec.describe 'Every Sidekiq worker' do 'Namespaces::RefreshRootStatisticsWorker' => 3, 'Namespaces::RootStatisticsWorker' => 3, 'Namespaces::ScheduleAggregationWorker' => 3, - 'NetworkPolicyMetricsWorker' => 3, 'NewEpicWorker' => 3, 'NewIssueWorker' => 3, 'NewMergeRequestWorker' => 3, @@ -386,12 +381,13 @@ RSpec.describe 'Every Sidekiq worker' do 'ProjectDailyStatisticsWorker' => 3, 'ProjectDestroyWorker' => 3, 'ProjectExportWorker' => false, - 'ProjectImportScheduleWorker' => false, + 'ProjectImportScheduleWorker' => 1, 'ProjectScheduleBulkRepositoryShardMovesWorker' => 3, 'ProjectServiceWorker' => 3, 'ProjectTemplateExportWorker' => false, 'ProjectUpdateRepositoryStorageWorker' => 3, 'Projects::GitGarbageCollectWorker' => false, + 'Projects::InactiveProjectsDeletionNotificationWorker' => 3, 'Projects::PostCreationWorker' => 3, 'Projects::ScheduleBulkRepositoryShardMovesWorker' => 3, 'Projects::UpdateRepositoryStorageWorker' => 3, @@ -414,9 +410,9 @@ RSpec.describe 'Every Sidekiq worker' do 'RepositoryCleanupWorker' => 3, 'RepositoryForkWorker' => 5, 'RepositoryImportWorker' => false, - 'RepositoryPushAuditEventWorker' => 3, 'RepositoryRemoveRemoteWorker' => 3, 'RepositoryUpdateMirrorWorker' => false, + 'RepositoryPushAuditEventWorker' => 3, 'RepositoryUpdateRemoteMirrorWorker' => 3, 'RequirementsManagement::ImportRequirementsCsvWorker' => 3, 'RequirementsManagement::ProcessRequirementsReportsWorker' => 3, diff --git a/spec/workers/expire_build_instance_artifacts_worker_spec.rb b/spec/workers/expire_build_instance_artifacts_worker_spec.rb deleted file mode 100644 index 38318447b5f..00000000000 --- a/spec/workers/expire_build_instance_artifacts_worker_spec.rb +++ /dev/null @@ -1,75 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ExpireBuildInstanceArtifactsWorker do - include RepoHelpers - - let(:worker) { described_class.new } - - describe '#perform' do - before do - worker.perform(build.id) - end - - context 'with expired artifacts' do - context 'when associated project is valid' do - let(:build) { create(:ci_build, :artifacts, :expired) } - - it 'does expire' do - expect(build.reload.artifacts_expired?).to be_truthy - end - - it 'does remove files' do - expect(build.reload.artifacts_file.present?).to be_falsey - end - - it 'does remove the job artifact record' do - expect(build.reload.job_artifacts_archive).to be_nil - end - end - end - - context 'with not yet expired artifacts' do - let_it_be(:build) do - create(:ci_build, :artifacts, artifacts_expire_at: Time.current + 7.days) - end - - it 'does not expire' do - expect(build.reload.artifacts_expired?).to be_falsey - end - - it 'does not remove files' do - expect(build.reload.artifacts_file.present?).to be_truthy - end - - it 'does not remove the job artifact record' do - expect(build.reload.job_artifacts_archive).not_to be_nil - end - end - - context 'without expire date' do - let(:build) { create(:ci_build, :artifacts) } - - it 'does not expire' do - expect(build.reload.artifacts_expired?).to be_falsey - end - - it 'does not remove files' do - expect(build.reload.artifacts_file.present?).to be_truthy - end - - it 'does not remove the job artifact record' do - expect(build.reload.job_artifacts_archive).not_to be_nil - end - end - - context 'for expired artifacts' do - let(:build) { create(:ci_build, :expired) } - - it 'is still expired' do - expect(build.reload.artifacts_expired?).to be_truthy - end - end - end -end diff --git a/spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb b/spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb index dd976eef28b..5f60dfc8ca1 100644 --- a/spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb @@ -17,14 +17,16 @@ RSpec.describe Gitlab::GithubImport::Stage::FinishImportWorker do expect(Gitlab::GithubImport::Logger) .to receive(:info) .with( - message: 'GitHub project import finished', - import_stage: 'Gitlab::GithubImport::Stage::FinishImportWorker', - object_counts: { - 'fetched' => {}, - 'imported' => {} - }, - project_id: project.id, - duration_s: 3.01 + { + message: 'GitHub project import finished', + import_stage: 'Gitlab::GithubImport::Stage::FinishImportWorker', + object_counts: { + 'fetched' => {}, + 'imported' => {} + }, + project_id: project.id, + duration_s: 3.01 + } ) worker.import(double(:client), project) diff --git a/spec/workers/merge_requests/close_issue_worker_spec.rb b/spec/workers/merge_requests/close_issue_worker_spec.rb new file mode 100644 index 00000000000..5e6bdc2a43e --- /dev/null +++ b/spec/workers/merge_requests/close_issue_worker_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::CloseIssueWorker do + subject(:worker) { described_class.new } + + describe '#perform' do + let!(:user) { create(:user) } + let!(:project) { create(:project) } + let!(:issue) { create(:issue, project: project) } + let!(:merge_request) { create(:merge_request, source_project: project) } + + it 'calls the close issue service' do + expect_next_instance_of(Issues::CloseService, project: project, current_user: user) do |service| + expect(service).to receive(:execute).with(issue, commit: merge_request) + end + + subject.perform(project.id, user.id, issue.id, merge_request.id) + end + + shared_examples 'when object does not exist' do + it 'does not call the close issue service' do + expect(Issues::CloseService).not_to receive(:new) + + expect { subject.perform(project.id, user.id, issue.id, merge_request.id) } + .not_to raise_exception + end + end + + context 'when the project does not exist' do + before do + project.destroy! + end + + it_behaves_like 'when object does not exist' + end + + context 'when the user does not exist' do + before do + user.destroy! + end + + it_behaves_like 'when object does not exist' + end + + context 'when the issue does not exist' do + before do + issue.destroy! + end + + it_behaves_like 'when object does not exist' + end + + context 'when the merge request does not exist' do + before do + merge_request.destroy! + end + + it_behaves_like 'when object does not exist' + end + end +end diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index 9b33e559c71..3951c20c048 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -354,7 +354,7 @@ RSpec.describe PostReceive do context 'webhook' do it 'fetches the correct project' do - expect(Project).to receive(:find_by).with(id: project.id) + expect(Project).to receive(:find_by).with({ id: project.id }) perform end diff --git a/spec/workers/project_service_worker_spec.rb b/spec/workers/project_service_worker_spec.rb index 7813d011274..55ec07ff79c 100644 --- a/spec/workers/project_service_worker_spec.rb +++ b/spec/workers/project_service_worker_spec.rb @@ -2,26 +2,38 @@ require 'spec_helper' RSpec.describe ProjectServiceWorker, '#perform' do - let(:worker) { described_class.new } - let(:integration) { Integrations::Jira.new } + let_it_be(:integration) { create(:jira_integration) } - before do - allow(Integration).to receive(:find).and_return(integration) - end + let(:worker) { described_class.new } it 'executes integration with given data' do data = { test: 'test' } - expect(integration).to receive(:execute).with(data) - worker.perform(1, data) + expect_next_found_instance_of(integration.class) do |integration| + expect(integration).to receive(:execute).with(data) + end + + worker.perform(integration.id, data) end it 'logs error messages' do error = StandardError.new('invalid URL') - allow(integration).to receive(:execute).and_raise(error) - expect(Gitlab::ErrorTracking).to receive(:log_exception).with(error, integration_class: 'Integrations::Jira') + expect_next_found_instance_of(integration.class) do |integration| + expect(integration).to receive(:execute).and_raise(error) + expect(integration).to receive(:log_exception).with(error) + end + + worker.perform(integration.id, {}) + end + + context 'when integration cannot be found' do + it 'completes silently and does not log an error' do + expect(Gitlab::IntegrationsLogger).not_to receive(:error) - worker.perform(1, {}) + expect do + worker.perform(non_existing_record_id, {}) + end.not_to raise_error + end end end diff --git a/spec/workers/projects/after_import_worker_spec.rb b/spec/workers/projects/after_import_worker_spec.rb new file mode 100644 index 00000000000..332b547bb66 --- /dev/null +++ b/spec/workers/projects/after_import_worker_spec.rb @@ -0,0 +1,131 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::AfterImportWorker do + include GitHelpers + + subject { worker.perform(project.id) } + + let(:worker) { described_class.new } + let(:project) { create(:project, :repository) } + let(:repository) { project.repository } + let(:sha) { project.commit.sha } + let(:housekeeping_service) { double(:housekeeping_service) } + + describe '#execute' do + before do + allow(Repositories::HousekeepingService) + .to receive(:new).with(project).and_return(housekeeping_service) + + allow(housekeeping_service) + .to receive(:execute).and_yield + + allow(housekeeping_service).to receive(:increment!) + end + + it 'performs housekeeping' do + subject + + expect(housekeeping_service).to have_received(:execute) + end + + context 'with some refs in refs/pull/**/*' do + before do + repository.write_ref('refs/pull/1/head', sha) + repository.write_ref('refs/pull/1/merge', sha) + + subject + end + + it 'removes refs/pull/**/*' do + expect(rugged.references.map(&:name)) + .not_to include(%r{\Arefs/pull/}) + end + end + + Repository::RESERVED_REFS_NAMES.each do |name| + context "with a ref in refs/#{name}/tmp" do + before do + repository.write_ref("refs/#{name}/tmp", sha) + + subject + end + + it "does not remove refs/#{name}/tmp" do + expect(rugged.references.map(&:name)) + .to include("refs/#{name}/tmp") + end + end + end + + context 'when after import action throw non-retriable exception' do + let(:exception) { StandardError.new('after import error') } + + before do + allow_next_instance_of(Repository) do |repository| + allow(repository).to receive(:delete_all_refs_except) + .and_raise(exception) + end + end + + it 'throws after import error' do + expect { subject }.to raise_exception('after import error') + end + end + + context 'when housekeeping service lease is taken' do + let(:exception) { Repositories::HousekeepingService::LeaseTaken.new } + + it 'logs the error message' do + allow_next_instance_of(Repositories::HousekeepingService) do |instance| + expect(instance).to receive(:execute).and_raise(exception) + end + + expect(Gitlab::Import::Logger).to receive(:info).with( + { + message: 'Project housekeeping failed', + project_full_path: project.full_path, + project_id: project.id, + 'error.message' => exception.to_s + }).and_call_original + + subject + end + end + + context 'when after import action throw retriable exception one time' do + let(:exception) { GRPC::DeadlineExceeded.new } + + it 'removes refs/pull/**/*' do + subject + + expect(rugged.references.map(&:name)) + .not_to include(%r{\Arefs/pull/}) + end + + it 'records the failures in the database', :aggregate_failures do + expect_next_instance_of(Repository) do |repository| + expect(repository).to receive(:delete_all_refs_except).and_raise(exception) + expect(repository).to receive(:delete_all_refs_except).and_call_original + end + + subject + + import_failure = ImportFailure.last + + expect(import_failure.source).to eq('delete_all_refs') + expect(import_failure.project_id).to eq(project.id) + expect(import_failure.relation_key).to be_nil + expect(import_failure.relation_index).to be_nil + expect(import_failure.exception_class).to eq('GRPC::DeadlineExceeded') + expect(import_failure.exception_message).to be_present + expect(import_failure.correlation_id_value).not_to be_empty + end + end + + def rugged + rugged_repo(repository) + end + end +end diff --git a/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb b/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb new file mode 100644 index 00000000000..0e7b4ea504c --- /dev/null +++ b/spec/workers/projects/inactive_projects_deletion_cron_worker_spec.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::InactiveProjectsDeletionCronWorker do + include ProjectHelpers + + describe "#perform" do + subject(:worker) { described_class.new } + + let_it_be(:admin_user) { create(:user, :admin) } + let_it_be(:non_admin_user) { create(:user) } + let_it_be(:new_blank_project) do + create_project_with_statistics.tap do |project| + project.update!(last_activity_at: Time.current) + end + end + + let_it_be(:inactive_blank_project) do + create_project_with_statistics.tap do |project| + project.update!(last_activity_at: 13.months.ago) + end + end + + let_it_be(:inactive_large_project) do + create_project_with_statistics(with_data: true, size_multiplier: 2.gigabytes) + .tap { |project| project.update!(last_activity_at: 2.years.ago) } + end + + let_it_be(:active_large_project) do + create_project_with_statistics(with_data: true, size_multiplier: 2.gigabytes) + .tap { |project| project.update!(last_activity_at: 1.month.ago) } + end + + before do + stub_application_setting(inactive_projects_min_size_mb: 5) + stub_application_setting(inactive_projects_send_warning_email_after_months: 12) + stub_application_setting(inactive_projects_delete_after_months: 14) + end + + context 'when delete inactive projects feature is disabled' do + before do + stub_application_setting(delete_inactive_projects: false) + end + + it 'does not invoke Projects::InactiveProjectsDeletionNotificationWorker' do + expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) + expect(::Projects::DestroyService).not_to receive(:new) + + worker.perform + end + + it 'does not delete the inactive projects' do + worker.perform + + expect(inactive_large_project.reload.pending_delete).to eq(false) + end + end + + context 'when delete inactive projects feature is enabled' do + before do + stub_application_setting(delete_inactive_projects: true) + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(inactive_projects_deletion: false) + end + + it 'does not invoke Projects::InactiveProjectsDeletionNotificationWorker' do + expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) + expect(::Projects::DestroyService).not_to receive(:new) + + worker.perform + end + + it 'does not delete the inactive projects' do + worker.perform + + expect(inactive_large_project.reload.pending_delete).to eq(false) + end + end + + context 'when feature flag is enabled', :clean_gitlab_redis_shared_state, :sidekiq_inline do + let_it_be(:delay) { anything } + + before do + stub_feature_flags(inactive_projects_deletion: true) + end + + it 'invokes Projects::InactiveProjectsDeletionNotificationWorker for inactive projects' do + Gitlab::Redis::SharedState.with do |redis| + expect(redis).to receive(:hset).with('inactive_projects_deletion_warning_email_notified', + "project:#{inactive_large_project.id}", Date.current) + end + expect(::Projects::InactiveProjectsDeletionNotificationWorker).to receive(:perform_in).with( + delay, inactive_large_project.id, deletion_date).and_call_original + expect(::Projects::DestroyService).not_to receive(:new) + + worker.perform + end + + it 'does not invoke InactiveProjectsDeletionNotificationWorker for already notified inactive projects' do + Gitlab::Redis::SharedState.with do |redis| + redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", + Date.current.to_s) + end + + expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) + expect(::Projects::DestroyService).not_to receive(:new) + + worker.perform + end + + it 'invokes Projects::DestroyService for projects that are inactive even after being notified' do + Gitlab::Redis::SharedState.with do |redis| + redis.hset('inactive_projects_deletion_warning_email_notified', "project:#{inactive_large_project.id}", + 15.months.ago.to_date.to_s) + end + + expect(::Projects::InactiveProjectsDeletionNotificationWorker).not_to receive(:perform_in) + expect(::Projects::DestroyService).to receive(:new).with(inactive_large_project, admin_user, {}) + .at_least(:once).and_call_original + + worker.perform + + expect(inactive_large_project.reload.pending_delete).to eq(true) + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.hget('inactive_projects_deletion_warning_email_notified', + "project:#{inactive_large_project.id}")).to be_nil + end + end + end + + it_behaves_like 'an idempotent worker' + end + end +end diff --git a/spec/workers/projects/inactive_projects_deletion_notification_worker_spec.rb b/spec/workers/projects/inactive_projects_deletion_notification_worker_spec.rb new file mode 100644 index 00000000000..3ddfec0d346 --- /dev/null +++ b/spec/workers/projects/inactive_projects_deletion_notification_worker_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::InactiveProjectsDeletionNotificationWorker do + describe "#perform" do + subject(:worker) { described_class.new } + + let_it_be(:deletion_date) { Date.current } + let_it_be(:non_existing_project_id) { non_existing_record_id } + let_it_be(:project) { create(:project) } + + it 'invokes NotificationService and calls inactive_project_deletion_warning' do + expect_next_instance_of(NotificationService) do |notification| + expect(notification).to receive(:inactive_project_deletion_warning).with(project, deletion_date) + end + + worker.perform(project.id, deletion_date) + end + + it 'adds the project_id to redis key that tracks the deletion warning emails' do + worker.perform(project.id, deletion_date) + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.hget('inactive_projects_deletion_warning_email_notified', + "project:#{project.id}")).to eq(Date.current.to_s) + end + end + + it 'rescues and logs the exception if project does not exist' do + expect(Gitlab::ErrorTracking).to receive(:log_exception).with(instance_of(ActiveRecord::RecordNotFound), + { project_id: non_existing_project_id }) + + worker.perform(non_existing_project_id, deletion_date) + end + + it_behaves_like 'an idempotent worker' do + let(:job_args) { [project.id, deletion_date] } + end + end +end diff --git a/spec/workers/projects/record_target_platforms_worker_spec.rb b/spec/workers/projects/record_target_platforms_worker_spec.rb index eb53e3f8608..01852f252b7 100644 --- a/spec/workers/projects/record_target_platforms_worker_spec.rb +++ b/spec/workers/projects/record_target_platforms_worker_spec.rb @@ -7,11 +7,11 @@ RSpec.describe Projects::RecordTargetPlatformsWorker do let_it_be(:swift) { create(:programming_language, name: 'Swift') } let_it_be(:objective_c) { create(:programming_language, name: 'Objective-C') } + let_it_be(:java) { create(:programming_language, name: 'Java') } + let_it_be(:kotlin) { create(:programming_language, name: 'Kotlin') } let_it_be(:project) { create(:project, :repository, detected_repository_languages: true) } let(:worker) { described_class.new } - let(:service_result) { %w(ios osx watchos) } - let(:service_double) { instance_double(Projects::RecordTargetPlatformsService, execute: service_result) } let(:lease_key) { "#{described_class.name.underscore}:#{project.id}" } let(:lease_timeout) { described_class::LEASE_TIMEOUT } @@ -21,16 +21,20 @@ RSpec.describe Projects::RecordTargetPlatformsWorker do stub_exclusive_lease(lease_key, timeout: lease_timeout) end - shared_examples 'performs detection' do - it 'creates and executes a Projects::RecordTargetPlatformService instance for the project', :aggregate_failures do - expect(Projects::RecordTargetPlatformsService).to receive(:new).with(project) { service_double } + shared_examples 'performs detection' do |detector_service_class| + let(:service_double) { instance_double(detector_service_class, execute: service_result) } + + it "creates and executes a #{detector_service_class} instance for the project", :aggregate_failures do + expect(Projects::RecordTargetPlatformsService).to receive(:new) + .with(project, detector_service_class) { service_double } expect(service_double).to receive(:execute) perform end it 'logs extra metadata on done', :aggregate_failures do - expect(Projects::RecordTargetPlatformsService).to receive(:new).with(project) { service_double } + expect(Projects::RecordTargetPlatformsService).to receive(:new) + .with(project, detector_service_class) { service_double } expect(worker).to receive(:log_extra_metadata_on_done).with(:target_platforms, service_result) perform @@ -45,19 +49,68 @@ RSpec.describe Projects::RecordTargetPlatformsWorker do end end - context 'when project uses Swift programming language' do - let!(:repository_language) { create(:repository_language, project: project, programming_language: swift) } + def create_language(language) + create(:repository_language, project: project, programming_language: language) + end + + context 'when project uses programming language for Apple platform' do + let(:service_result) { %w(ios osx watchos) } + + context 'when project uses Swift programming language' do + before do + create_language(swift) + end + + it_behaves_like 'performs detection', Projects::AppleTargetPlatformDetectorService + end + + context 'when project uses Objective-C programming language' do + before do + create_language(objective_c) + end - include_examples 'performs detection' + it_behaves_like 'performs detection', Projects::AppleTargetPlatformDetectorService + end end - context 'when project uses Objective-C programming language' do - let!(:repository_language) { create(:repository_language, project: project, programming_language: objective_c) } + context 'when project uses programming language for Android platform' do + let(:feature_enabled) { true } + let(:service_result) { %w(android) } + + before do + stub_feature_flags(detect_android_projects: feature_enabled) + end + + context 'when project uses Java' do + before do + create_language(java) + end + + it_behaves_like 'performs detection', Projects::AndroidTargetPlatformDetectorService + + context 'when feature flag is disabled' do + let(:feature_enabled) { false } + + it_behaves_like 'does nothing' + end + end + + context 'when project uses Kotlin' do + before do + create_language(kotlin) + end + + it_behaves_like 'performs detection', Projects::AndroidTargetPlatformDetectorService - include_examples 'performs detection' + context 'when feature flag is disabled' do + let(:feature_enabled) { false } + + it_behaves_like 'does nothing' + end + end end - context 'when the project does not contain programming languages for Apple platforms' do + context 'when the project does not use programming languages for Apple or Android platforms' do it_behaves_like 'does nothing' end diff --git a/spec/workers/prometheus/create_default_alerts_worker_spec.rb b/spec/workers/prometheus/create_default_alerts_worker_spec.rb index 887d677c95f..d935bb20a29 100644 --- a/spec/workers/prometheus/create_default_alerts_worker_spec.rb +++ b/spec/workers/prometheus/create_default_alerts_worker_spec.rb @@ -5,63 +5,9 @@ require 'spec_helper' RSpec.describe Prometheus::CreateDefaultAlertsWorker do let_it_be(:project) { create(:project) } - let(:worker) { described_class.new } - let(:logger) { worker.send(:logger) } - let(:service) { instance_double(Prometheus::CreateDefaultAlertsService) } - let(:service_result) { ServiceResponse.success } - subject { described_class.new.perform(project.id) } - before do - allow(Prometheus::CreateDefaultAlertsService) - .to receive(:new).with(project: project) - .and_return(service) - allow(service).to receive(:execute) - .and_return(service_result) - end - - it_behaves_like 'an idempotent worker' do - let(:job_args) { [project.id] } - - it 'calls the service' do - expect(service).to receive(:execute) - - subject - end - - context 'project is nil' do - let(:job_args) { [nil] } - - it 'does not call the service' do - expect(service).not_to receive(:execute) - - subject - end - end - - context 'when service returns an error' do - let(:error_message) { 'some message' } - let(:service_result) { ServiceResponse.error(message: error_message) } - - it 'succeeds and logs the error' do - expect(logger) - .to receive(:info) - .with(a_hash_including('message' => error_message)) - .exactly(worker_exec_times).times - - subject - end - end - end - - context 'when service raises an exception' do - let(:error_message) { 'some exception' } - let(:exception) { StandardError.new(error_message) } - - it 're-raises exception' do - allow(service).to receive(:execute).and_raise(exception) - - expect { subject }.to raise_error(exception) - end + it 'does nothing' do + expect { subject }.not_to change { PrometheusAlert.count } end end diff --git a/spec/workers/ssh_keys/expired_notification_worker_spec.rb b/spec/workers/ssh_keys/expired_notification_worker_spec.rb index be38391ff8c..26d9460d73e 100644 --- a/spec/workers/ssh_keys/expired_notification_worker_spec.rb +++ b/spec/workers/ssh_keys/expired_notification_worker_spec.rb @@ -16,12 +16,12 @@ RSpec.describe SshKeys::ExpiredNotificationWorker, type: :worker do let_it_be(:user) { create(:user) } context 'with a large batch' do + let_it_be_with_reload(:keys) { create_list(:key, 20, :expired_today, user: user) } + before do stub_const("SshKeys::ExpiredNotificationWorker::BATCH_SIZE", 5) end - let_it_be_with_reload(:keys) { create_list(:key, 20, expires_at: Time.current, user: user) } - it 'updates all keys regardless of batch size' do worker.perform @@ -30,7 +30,7 @@ RSpec.describe SshKeys::ExpiredNotificationWorker, type: :worker do end context 'with expiring key today' do - let_it_be_with_reload(:expired_today) { create(:key, expires_at: Time.current, user: user) } + let_it_be_with_reload(:expired_today) { create(:key, :expired_today, user: user) } it 'invoke the notification service' do expect_next_instance_of(Keys::ExpiryNotificationService) do |expiry_service| @@ -52,7 +52,7 @@ RSpec.describe SshKeys::ExpiredNotificationWorker, type: :worker do end context 'when key has expired in the past' do - let_it_be(:expired_past) { create(:key, expires_at: 1.day.ago, user: user) } + let_it_be(:expired_past) { create(:key, :expired, user: user) } it 'does not update notified column' do expect { worker.perform }.not_to change { expired_past.reload.expiry_notification_delivered_at } @@ -60,7 +60,7 @@ RSpec.describe SshKeys::ExpiredNotificationWorker, type: :worker do context 'when key has already been notified of expiration' do before do - expired_past.update!(expiry_notification_delivered_at: 1.day.ago) + expired_past.update_attribute(:expiry_notification_delivered_at, 1.day.ago) end it 'does not update notified column' do diff --git a/spec/workers/ssh_keys/expiring_soon_notification_worker_spec.rb b/spec/workers/ssh_keys/expiring_soon_notification_worker_spec.rb index 0a1d4a14ad0..e907d035020 100644 --- a/spec/workers/ssh_keys/expiring_soon_notification_worker_spec.rb +++ b/spec/workers/ssh_keys/expiring_soon_notification_worker_spec.rb @@ -38,7 +38,7 @@ RSpec.describe SshKeys::ExpiringSoonNotificationWorker, type: :worker do end context 'when key has expired in the past' do - let_it_be(:expired_past) { create(:key, expires_at: 1.day.ago, user: user) } + let_it_be(:expired_past) { create(:key, :expired, user: user) } it 'does not update notified column' do expect { worker.perform }.not_to change { expired_past.reload.before_expiry_notification_delivered_at } |