Merge branch 'revert-946fc181' into 'master'

Revert "Merge branch '331835-use-rspec-sidekiq' into 'master'"

See merge request gitlab-org/gitlab!63894

Gemfile

@@ -432,7 +432,6 @@ group :test do
   gem 'concurrent-ruby', '~> 1.1'
   gem 'test-prof', '~> 0.12.0'
   gem 'rspec_junit_formatter'
-  gem 'rspec-sidekiq'
   gem 'guard-rspec'
 
   # Moved in `test` because https://gitlab.com/gitlab-org/gitlab/-/issues/217527

Gemfile.lock

@@ -1090,9 +1090,6 @@ GEM
       rspec-support (~> 3.10)
     rspec-retry (0.6.1)
       rspec-core (> 3.3)
-    rspec-sidekiq (3.1.0)
-      rspec-core (~> 3.0, >= 3.0.0)
-      sidekiq (>= 2.4.0)
     rspec-support (3.10.2)
     rspec_junit_formatter (0.4.1)
       rspec-core (>= 2, < 4, != 2.12.0)
@@ -1608,7 +1605,6 @@ DEPENDENCIES
   rspec-parameterized
   rspec-rails (~> 5.0.1)
   rspec-retry (~> 0.6.1)
-  rspec-sidekiq
   rspec_junit_formatter
   rspec_profiling (~> 0.0.6)
   ruby-fogbugz (~> 0.2.1)

doc/development/testing_guide/best_practices.md

@@ -259,9 +259,9 @@ it 'sets the frobulance' do
 end
 
 it 'schedules a background job' do
-  subject.execute
+  expect(BackgroundJob).to receive(:perform_async)
 
-  expect(BackgroundJob).to have_enqueued_sidekiq_job
+  subject.execute
 end
 ```
 
@@ -271,11 +271,11 @@ combining the examples:
 
 ```ruby
 it 'performs the expected side-effects' do
+  expect(BackgroundJob).to receive(:perform_async)
+
   expect { subject.execute }
     .to change(Event, :count).by(1)
     .and change { arg_0.frobulance }.to('wibble')
-
-  expect(BackgroundJob).to have_enqueued_sidekiq_job
 end
 ```
 
@@ -738,28 +738,6 @@ The usage of `perform_enqueued_jobs` is useful only for testing delayed mail
 deliveries, because our Sidekiq workers aren't inheriting from `ApplicationJob`
 / `ActiveJob::Base`.
 
-GitLab uses the [RSpec-Sidekiq](https://github.com/philostler/rspec-sidekiq) gem for expectations.
-We prefer you check that a job has been enqueued, rather than checking the worker has
-`received` the `perform_async` method:
-
-```ruby
-# bad
-expect(Worker).to receive(:perform_async).with(1, 'string')
-# Good
-expect(Worker).to have_enqueued_sidekiq_job(1, 'string')
-```
-
-The only exception to this rule: if the spec actually needs to make sure the job is
-enqueued only once, or a specific number of times:
-
-```ruby
-# good
-expect(Worker).to receive(:perform_async).with(1, 'string').once
-```
-
-If you need test that the job is only enqueued a specific number of times, you will have to disable the cop
-that enforces usage of `have_enqueued_sidekiq_job` (`RSpec/HaveEnqueuedSidekiqJob`) in that test.
-
 #### DNS
 
 DNS requests are stubbed universally in the test suite

ee/spec/controllers/ee/registrations_controller_spec.rb

@@ -146,9 +146,10 @@ RSpec.describe RegistrationsController do
       end
 
       it 'redirects without deleting the account' do
+        expect(DeleteUserWorker).not_to receive(:perform_async)
+
         post :destroy, params: { username: user.username }
 
-        expect(DeleteUserWorker).not_to have_enqueued_sidekiq_job
         expect(flash[:alert]).to eq 'Account could not be deleted. GitLab was unable to verify your identity.'
         expect(response).to have_gitlab_http_status(:see_other)
         expect(response).to redirect_to profile_account_path

ee/spec/controllers/projects/merge_requests_controller_spec.rb

@@ -328,16 +328,17 @@ RSpec.describe Projects::MergeRequestsController do
     end
 
     def expect_rebase_worker_for(user)
-      expect(RebaseWorker).to have_enqueued_sidekiq_job(merge_request.id, user.id, false)
+      expect(RebaseWorker).to receive(:perform_async).with(merge_request.id, user.id, false)
     end
 
     context 'approvals pending' do
       let(:project) { create(:project, :repository, approvals_before_merge: 1) }
 
       it 'returns 200' do
+        expect_rebase_worker_for(viewer)
+
         post_rebase
 
-        expect_rebase_worker_for(viewer)
         expect(response).to have_gitlab_http_status(:ok)
       end
     end

ee/spec/graphql/mutations/requirements_management/export_requirements_spec.rb

@@ -55,14 +55,10 @@ RSpec.describe Mutations::RequirementsManagement::ExportRequirements do
         end
 
         it 'exports requirements' do
-          subject
+          expect(IssuableExportCsvWorker).to receive(:perform_async)
+            .with(:requirement, user.id, project.id, args.except(:project_path))
 
-          expect(IssuableExportCsvWorker).to have_enqueued_sidekiq_job(
-            :requirement,
-            user.id,
-            project.id,
-            args.except(:project_path)
-          )
+          subject
         end
       end
 

ee/spec/lib/analytics/merge_request_metrics_refresh_spec.rb

@@ -55,10 +55,11 @@ RSpec.describe Analytics::MergeRequestMetricsRefresh do
 
   describe '#execute_async' do
     it 'schedules CodeReviewMetricsWorker with params' do
-      subject.execute_async(force: true)
-
       expect(Analytics::CodeReviewMetricsWorker)
-        .to have_enqueued_sidekiq_job('MyTestClass', merge_request.id, force: true)
+        .to receive(:perform_async)
+              .with('MyTestClass', merge_request.id, force: true)
+
+      subject.execute_async(force: true)
     end
   end
 end

ee/spec/lib/ee/gitlab/background_migration/populate_namespace_statistics_spec.rb

@@ -39,10 +39,10 @@ RSpec.describe Gitlab::BackgroundMigration::PopulateNamespaceStatistics do
 
     expect(namespace_statistics.count).to eq 1
 
-    subject
+    expect(Namespaces::ScheduleAggregationWorker).to receive(:perform_async).with(group1.id)
+    expect(Namespaces::ScheduleAggregationWorker).to receive(:perform_async).with(group2.id)
 
-    expect(Namespaces::ScheduleAggregationWorker).to have_enqueued_sidekiq_job(group1.id)
-    expect(Namespaces::ScheduleAggregationWorker).to have_enqueued_sidekiq_job(group2.id)
+    subject
 
     expect(namespace_statistics.count).to eq 2
 

ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb

@@ -54,9 +54,9 @@ RSpec.describe Gitlab::Auth::GroupSaml::MembershipUpdater do
   end
 
   it 'does not enqueue group sync' do
-    update_membership
+    expect(GroupSamlGroupSyncWorker).not_to receive(:perform_async)
 
-    expect(GroupSamlGroupSyncWorker).not_to have_enqueued_sidekiq_job
+    update_membership
   end
 
   context 'when SAML group links exist' do
@@ -73,34 +73,30 @@ RSpec.describe Gitlab::Auth::GroupSaml::MembershipUpdater do
       end
 
       it 'does not enqueue group sync' do
-        expect(GroupSamlGroupSyncWorker).not_to have_enqueued_sidekiq_job
+        expect(GroupSamlGroupSyncWorker).not_to receive(:perform_async)
       end
     end
 
     context 'when group sync is available' do
       before do
         stub_saml_group_sync_available(true)
-        group_link
-        subgroup_link
       end
 
       it 'enqueues group sync' do
-        update_membership
+        expect(GroupSamlGroupSyncWorker).to receive(:perform_async).with(user.id, group.id, match_array([group_link.id, subgroup_link.id]))
 
-        expect(GroupSamlGroupSyncWorker).to have_enqueued_sidekiq_job(user.id, group.id, match_array([group_link.id, subgroup_link.id]))
+        update_membership
       end
 
       context 'with a group link outside the top-level group' do
         before do
           create(:saml_group_link, saml_group_name: 'Developers', group: create(:group))
-          group_link
-          subgroup_link
         end
 
         it 'enqueues group sync without the outside group' do
-          update_membership
+          expect(GroupSamlGroupSyncWorker).to receive(:perform_async).with(user.id, group.id, match_array([group_link.id, subgroup_link.id]))
 
-          expect(GroupSamlGroupSyncWorker).to have_enqueued_sidekiq_job(user.id, group.id, match_array([group_link.id, subgroup_link.id]))
+          update_membership
         end
       end
     end

ee/spec/lib/gitlab/auth/ldap/access_spec.rb

@@ -233,9 +233,10 @@ RSpec.describe Gitlab::Auth::Ldap::Access do
           group_link_2 = create(:ldap_group_link, cn: 'Group2', provider: provider)
           group_ids = [group_link_1, group_link_2].map(&:group_id)
 
-          access.update_user
+          expect(LdapGroupSyncWorker).to receive(:perform_async)
+            .with(a_collection_containing_exactly(*group_ids), provider)
 
-          expect(LdapGroupSyncWorker).to have_enqueued_sidekiq_job(a_collection_containing_exactly(*group_ids), provider)
+          access.update_user
         end
 
         it "doesn't trigger a sync when in a read-only GitLab instance" do
@@ -243,9 +244,9 @@ RSpec.describe Gitlab::Auth::Ldap::Access do
           create(:ldap_group_link, cn: 'Group1', provider: provider)
           create(:ldap_group_link, cn: 'Group2', provider: provider)
 
-          access.update_user
+          expect(LdapGroupSyncWorker).not_to receive(:perform_async)
 
-          expect(LdapGroupSyncWorker).not_to have_enqueued_sidekiq_job
+          access.update_user
         end
 
         it "doesn't trigger a sync when there are no links for the provider" do
@@ -253,9 +254,9 @@ RSpec.describe Gitlab::Auth::Ldap::Access do
                                      cn: 'Group1',
                                      provider: 'not-this-ldap')
 
-          access.update_user
+          expect(LdapGroupSyncWorker).not_to receive(:perform_async)
 
-          expect(LdapGroupSyncWorker).not_to have_enqueued_sidekiq_job
+          access.update_user
         end
 
         it 'does not performs the membership update for existing users' do
@@ -264,10 +265,9 @@ RSpec.describe Gitlab::Auth::Ldap::Access do
           user.save
 
           expect(LdapGroupLink).not_to receive(:where)
+          expect(LdapGroupSyncWorker).not_to receive(:perform_async)
 
           access.update_user
-
-          expect(LdapGroupSyncWorker).not_to have_enqueued_sidekiq_job
         end
       end
 
@@ -275,10 +275,9 @@ RSpec.describe Gitlab::Auth::Ldap::Access do
         stub_ldap_person_find_by_dn(entry, provider)
 
         expect(LdapGroupLink).not_to receive(:where)
+        expect(LdapGroupSyncWorker).not_to receive(:perform_async)
 
         access.update_user
-
-        expect(LdapGroupSyncWorker).not_to have_enqueued_sidekiq_job
       end
     end
 

ee/spec/lib/gitlab/geo/log_cursor/daemon_spec.rb

@@ -173,9 +173,9 @@ RSpec.describe Gitlab::Geo::LogCursor::Daemon, :clean_gitlab_redis_shared_state
       it 'does not replay events for projects that do not belong to selected namespaces to replicate' do
         secondary.update!(selective_sync_type: 'namespaces', namespaces: [group_2])
 
-        daemon.find_and_handle_events!
+        expect(Geo::ProjectSyncWorker).not_to receive(:perform_async).with(project.id, anything)
 
-        expect(Geo::ProjectSyncWorker).not_to have_enqueued_sidekiq_job(project.id, anything)
+        daemon.find_and_handle_events!
       end
 
       it 'detects when an event was skipped' do
@@ -240,9 +240,9 @@ RSpec.describe Gitlab::Geo::LogCursor::Daemon, :clean_gitlab_redis_shared_state
       it 'does not replay events for projects that do not belong to selected shards to replicate' do
         secondary.update!(selective_sync_type: 'shards', selective_sync_shards: ['broken'])
 
-        daemon.find_and_handle_events!
+        expect(Geo::ProjectSyncWorker).not_to receive(:perform_async).with(project.id, anything)
 
-        expect(Geo::ProjectSyncWorker).not_to have_enqueued_sidekiq_job(project.id, anything)
+        daemon.find_and_handle_events!
       end
     end
   end

ee/spec/lib/gitlab/geo/log_cursor/events/event_spec.rb

@@ -13,13 +13,13 @@ RSpec.describe Gitlab::Geo::LogCursor::Events::Event, :clean_gitlab_redis_shared
 
   describe "#process" do
     it "enqueues Geo::EventWorker" do
-      subject.process
-
-      expect(::Geo::EventWorker).to have_enqueued_sidekiq_job(
+      expect(::Geo::EventWorker).to receive(:perform_async).with(
         "package_file",
         "created",
         { "model_record_id" => replicable.id }
       )
+
+      subject.process
     end
 
     it "eventually calls Replicator#consume", :sidekiq_inline do

ee/spec/lib/gitlab/geo/log_cursor/events/hashed_storage_attachments_event_spec.rb

@@ -13,19 +13,20 @@ RSpec.describe Gitlab::Geo::LogCursor::Events::HashedStorageAttachmentsEvent, :c
 
   subject { described_class.new(hashed_storage_attachments_event, Time.now, logger) }
 
+  around do |example|
+    Sidekiq::Testing.fake! { example.run }
+  end
+
   describe '#process' do
     it 'does not create a new project registry' do
       expect { subject.process }.not_to change(Geo::ProjectRegistry, :count)
     end
 
     it 'schedules a Geo::HashedStorageAttachmentsMigrationWorker' do
-      subject.process
+      expect(::Geo::HashedStorageAttachmentsMigrationWorker).to receive(:perform_async)
+        .with(project.id, old_attachments_path, new_attachments_path)
 
-      expect(::Geo::HashedStorageAttachmentsMigrationWorker).to have_enqueued_sidekiq_job(
-        project.id,
-        old_attachments_path,
-        new_attachments_path
-      )
+      subject.process
     end
 
     it_behaves_like 'logs event source info'

ee/spec/lib/gitlab/geo/log_cursor/events/hashed_storage_migrated_event_spec.rb

@@ -14,6 +14,10 @@ RSpec.describe Gitlab::Geo::LogCursor::Events::HashedStorageMigratedEvent, :clea
 
   subject { described_class.new(hashed_storage_migrated_event, Time.now, logger) }
 
+  around do |example|
+    Sidekiq::Testing.fake! { example.run }
+  end
+
   describe '#process' do
     context 'when a tracking entry does not exist' do
       it 'does not create a tracking entry' do
@@ -21,23 +25,20 @@ RSpec.describe Gitlab::Geo::LogCursor::Events::HashedStorageMigratedEvent, :clea
       end
 
       it 'does not schedule a Geo::HashedStorageMigrationWorker' do
-        subject.process
+        expect(::Geo::HashedStorageMigrationWorker).not_to receive(:perform_async)
+          .with(project.id, old_disk_path, new_disk_path, old_storage_version)
 
-        expect(::Geo::HashedStorageMigrationWorker).not_to have_enqueued_sidekiq_job
+        subject.process
       end
     end
 
     it 'schedules a Geo::HashedStorageMigrationWorker' do
       create(:geo_project_registry, project: project)
 
-      subject.process
+      expect(::Geo::HashedStorageMigrationWorker).to receive(:perform_async)
+        .with(project.id, old_disk_path, new_disk_path, old_storage_version)
 
-      expect(::Geo::HashedStorageMigrationWorker).to have_enqueued_sidekiq_job(
-        project.id,
-        old_disk_path,
-        new_disk_path,
-        old_storage_version
-      )
+      subject.process
     end
 
     it_behaves_like 'logs event source info'

ee/spec/lib/gitlab/geo/log_cursor/events/repository_created_event_spec.rb

@@ -82,9 +82,9 @@ RSpec.describe Gitlab::Geo::LogCursor::Events::RepositoryCreatedEvent, :clean_gi
       it_behaves_like 'RepositoryCreatedEvent'
 
       it 'does not schedule a Geo::ProjectSyncWorker job' do
-        subject.process
+        expect(Geo::ProjectSyncWorker).not_to receive(:perform_async).with(project.id, anything)
 
-        expect(Geo::ProjectSyncWorker).not_to have_enqueued_sidekiq_job
+        subject.process
       end
     end
   end

ee/spec/models/application_setting_spec.rb

@@ -270,11 +270,11 @@ RSpec.describe ApplicationSetting do
     end
 
     it 'resumes indexing' do
+      expect(ElasticIndexingControlWorker).to receive(:perform_async)
+
       setting.save!
       setting.elasticsearch_pause_indexing = false
       setting.save!
-
-      expect(ElasticIndexingControlWorker).to have_enqueued_sidekiq_job
     end
   end
 

rubocop/cop/rspec/have_enqueued_sidekiq_job.rb

-# frozen_string_literal: true
-
-module RuboCop
-  module Cop
-    module RSpec
-      # This cop checks for `expect(worker).to receive(:perform_async)` usage in specs
-      #
-      # @example
-      #   # bad
-      #   it "enqueues a worker" do
-      #     expect(MyWorker).to receive(:perform_async)
-      #   end
-      #
-      #   # good
-      #   it "enqueues a worker" do
-      #     expect(MyWorker).to have_enqueued_sidekiq_job
-      #   end
-      #
-      #   # bad
-      #   it "enqueues a worker" do
-      #     expect(MyWorker).to receive(:perform_async).with(1, 2)
-      #   end
-      #
-      #   # good
-      #   it "enqueues a worker" do
-      #     expect(MyWorker).to have_enqueued_sidekiq_job(1, 2)
-      #   end
-      #
-      class HaveEnqueuedSidekiqJob < RuboCop::Cop::Cop
-        MESSAGE = 'Do not use `receive(:perform_async)` to check a job has been enqueued, use `have_enqueued_sidekiq_job` instead.'
-
-        def_node_search :expect_to_receive_perform_async?, <<~PATTERN
-          (send
-            (send nil? :expect ...)
-            {:to :not_to :to_not}
-            {
-              (send nil? :receive (sym :perform_async))
-
-              (send
-                (send nil? :receive (sym :perform_async)) ...
-              )
-
-              (send
-                (send
-                  (send nil? :receive (sym :perform_async)) ...
-                ) ...
-              )
-            }
-          )
-        PATTERN
-
-        def on_send(node)
-          return unless expect_to_receive_perform_async?(node)
-
-          add_offense(node, location: :expression, message: MESSAGE)
-        end
-      end
-    end
-  end
-end

spec/lib/gitlab/ci/config/entry/job_spec.rb

@@ -658,7 +658,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
       let(:config) { { script: 'deploy', when: 'delayed' } }
 
       it 'is a delayed' do
-        expect(entry.delayed?).to be_truthy
+        expect(entry).to be_delayed
       end
     end
 
@@ -666,7 +666,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Job do
       let(:config) { { script: 'deploy' } }
 
       it 'is not a delayed' do
-        expect(entry.delayed?).to be_falsey
+        expect(entry).not_to be_delayed
       end
     end
   end

spec/lib/gitlab/database/postgres_index_spec.rb

@@ -69,15 +69,15 @@ RSpec.describe Gitlab::Database::PostgresIndex do
 
   describe '#unique?' do
     it 'returns true for a unique index' do
-      expect(find('public.bar_key').unique?).to be_truthy
+      expect(find('public.bar_key')).to be_unique
     end
 
     it 'returns false for a regular, non-unique index' do
-      expect(find('public.foo_idx').unique?).to be_falsey
+      expect(find('public.foo_idx')).not_to be_unique
     end
 
     it 'returns true for a primary key index' do
-      expect(find('public.example_table_pkey').unique?).to be_truthy
+      expect(find('public.example_table_pkey')).to be_unique
     end
   end
 

spec/models/ci/build_spec.rb

@@ -1870,7 +1870,7 @@ RSpec.describe Ci::Build do
     end
 
     describe '#retryable?' do
-      subject { build.retryable? }
+      subject { build }
 
       context 'when build is retryable' do
         context 'when build is successful' do
@@ -1878,7 +1878,7 @@ RSpec.describe Ci::Build do
             build.success!
           end
 
-          it { is_expected.to be_truthy }
+          it { is_expected.to be_retryable }
         end
 
         context 'when build is failed' do
@@ -1886,7 +1886,7 @@ RSpec.describe Ci::Build do
             build.drop!
           end
 
-          it { is_expected.to be_truthy }
+          it { is_expected.to be_retryable }
         end
 
         context 'when build is canceled' do
@@ -1894,7 +1894,7 @@ RSpec.describe Ci::Build do
             build.cancel!
           end
 
-          it { is_expected.to be_truthy }
+          it { is_expected.to be_retryable }
         end
       end
 
@@ -1904,7 +1904,7 @@ RSpec.describe Ci::Build do
             build.run!
           end
 
-          it { is_expected.to be_falsey }
+          it { is_expected.not_to be_retryable }
         end
 
         context 'when build is skipped' do
@@ -1912,7 +1912,7 @@ RSpec.describe Ci::Build do
             build.skip!
           end
 
-          it { is_expected.to be_falsey }
+          it { is_expected.not_to be_retryable }
         end
 
         context 'when build is degenerated' do
@@ -1920,7 +1920,7 @@ RSpec.describe Ci::Build do
             build.degenerate!
           end
 
-          it { is_expected.to be_falsey }
+          it { is_expected.not_to be_retryable }
         end
 
         context 'when a canceled build has been retried already' do
@@ -1931,7 +1931,7 @@ RSpec.describe Ci::Build do
           end
 
           context 'when prevent_retry_of_retried_jobs feature flag is enabled' do
-            it { is_expected.to be_falsey }
+            it { is_expected.not_to be_retryable }
           end
 
           context 'when prevent_retry_of_retried_jobs feature flag is disabled' do
@@ -1939,7 +1939,7 @@ RSpec.describe Ci::Build do
               stub_feature_flags(prevent_retry_of_retried_jobs: false)
             end
 
-            it { is_expected.to be_truthy }
+            it { is_expected.to be_retryable }
           end
         end
       end

spec/rubocop/cop/rspec/have_enqueued_sidekiq_job_spec.rb

-# frozen_string_literal: true
-
-require 'fast_spec_helper'
-require_relative '../../../../rubocop/cop/rspec/have_enqueued_sidekiq_job'
-
-RSpec.describe RuboCop::Cop::RSpec::HaveEnqueuedSidekiqJob do
-  let(:source_file) { 'spec/foo_spec.rb' }
-
-  subject(:cop) { described_class.new }
-
-  shared_examples 'cop' do |good:, bad:|
-    context "using #{bad} call" do
-      it 'registers an offense', :aggregate_failures do
-        expect_offense(<<~CODE, node: bad)
-          %{node}
-          ^{node} Do not use `receive(:perform_async)` to check a job has been enqueued, use `have_enqueued_sidekiq_job` instead.
-        CODE
-      end
-    end
-
-    context "using #{good} call" do
-      it 'does not register an offense' do
-        expect_no_offenses(good)
-      end
-    end
-  end
-
-  it_behaves_like 'cop',
-    bad: 'expect(Worker).to receive(:perform_async)',
-    good: 'expect(Worker).to have_enqueued_sidekiq_job'
-
-  include_examples 'cop',
-    bad: 'expect(Worker).not_to receive(:perform_async)',
-    good: 'expect(Worker).not_to have_enqueued_sidekiq_job'
-
-  include_examples 'cop',
-    bad: 'expect(Worker).to_not receive(:perform_async)',
-    good: 'expect(Worker).to_not have_enqueued_sidekiq_job'
-
-  include_examples 'cop',
-    bad: 'expect(Worker).to receive(:perform_async).with(1)',
-    good: 'expect(Worker).to have_enqueued_sidekiq_job(1)'
-
-  include_examples 'cop',
-    bad: 'expect(Worker).to receive(:perform_async).with(1).once',
-    good: 'expect(Worker).to have_enqueued_sidekiq_job(1)'
-
-  include_examples 'cop',
-    bad: 'expect(any_variable_or_method).to receive(:perform_async).with(1)',
-    good: 'expect(any_variable_or_method).to have_enqueued_sidekiq_job(1)'
-end