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

Use rspec-sidekiq for better expectations

See merge request gitlab-org/gitlab!62601

Gemfile

@@ -432,6 +432,7 @@ 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,6 +1090,9 @@ 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)
@@ -1605,6 +1608,7 @@ 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
-  expect(BackgroundJob).to receive(:perform_async)
-
   subject.execute
+
+  expect(BackgroundJob).to have_enqueued_sidekiq_job
 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,6 +738,28 @@ 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,10 +146,9 @@ 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,17 +328,16 @@ RSpec.describe Projects::MergeRequestsController do
     end
 
     def expect_rebase_worker_for(user)
-      expect(RebaseWorker).to receive(:perform_async).with(merge_request.id, user.id, false)
+      expect(RebaseWorker).to have_enqueued_sidekiq_job(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,10 +55,14 @@ RSpec.describe Mutations::RequirementsManagement::ExportRequirements do
         end
 
         it 'exports requirements' do
-          expect(IssuableExportCsvWorker).to receive(:perform_async)
-            .with(:requirement, user.id, project.id, args.except(:project_path))
-
           subject
+
+          expect(IssuableExportCsvWorker).to have_enqueued_sidekiq_job(
+            :requirement,
+            user.id,
+            project.id,
+            args.except(:project_path)
+          )
         end
       end
 

ee/spec/lib/analytics/merge_request_metrics_refresh_spec.rb

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

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

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

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
-    expect(GroupSamlGroupSyncWorker).not_to receive(:perform_async)
-
     update_membership
+
+    expect(GroupSamlGroupSyncWorker).not_to have_enqueued_sidekiq_job
   end
 
   context 'when SAML group links exist' do
@@ -73,30 +73,34 @@ RSpec.describe Gitlab::Auth::GroupSaml::MembershipUpdater do
       end
 
       it 'does not enqueue group sync' do
-        expect(GroupSamlGroupSyncWorker).not_to receive(:perform_async)
+        expect(GroupSamlGroupSyncWorker).not_to have_enqueued_sidekiq_job
       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
-        expect(GroupSamlGroupSyncWorker).to receive(:perform_async).with(user.id, group.id, match_array([group_link.id, subgroup_link.id]))
-
         update_membership
+
+        expect(GroupSamlGroupSyncWorker).to have_enqueued_sidekiq_job(user.id, group.id, match_array([group_link.id, subgroup_link.id]))
       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
-          expect(GroupSamlGroupSyncWorker).to receive(:perform_async).with(user.id, group.id, match_array([group_link.id, subgroup_link.id]))
-
           update_membership
+
+          expect(GroupSamlGroupSyncWorker).to have_enqueued_sidekiq_job(user.id, group.id, match_array([group_link.id, subgroup_link.id]))
         end
       end
     end

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

@@ -233,10 +233,9 @@ 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)
 
-          expect(LdapGroupSyncWorker).to receive(:perform_async)
-            .with(a_collection_containing_exactly(*group_ids), provider)
-
           access.update_user
+
+          expect(LdapGroupSyncWorker).to have_enqueued_sidekiq_job(a_collection_containing_exactly(*group_ids), provider)
         end
 
         it "doesn't trigger a sync when in a read-only GitLab instance" do
@@ -244,9 +243,9 @@ RSpec.describe Gitlab::Auth::Ldap::Access do
           create(:ldap_group_link, cn: 'Group1', provider: provider)
           create(:ldap_group_link, cn: 'Group2', provider: provider)
 
-          expect(LdapGroupSyncWorker).not_to receive(:perform_async)
-
           access.update_user
+
+          expect(LdapGroupSyncWorker).not_to have_enqueued_sidekiq_job
         end
 
         it "doesn't trigger a sync when there are no links for the provider" do
@@ -254,9 +253,9 @@ RSpec.describe Gitlab::Auth::Ldap::Access do
                                      cn: 'Group1',
                                      provider: 'not-this-ldap')
 
-          expect(LdapGroupSyncWorker).not_to receive(:perform_async)
-
           access.update_user
+
+          expect(LdapGroupSyncWorker).not_to have_enqueued_sidekiq_job
         end
 
         it 'does not performs the membership update for existing users' do
@@ -265,9 +264,10 @@ 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,9 +275,10 @@ 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])
 
-        expect(Geo::ProjectSyncWorker).not_to receive(:perform_async).with(project.id, anything)
-
         daemon.find_and_handle_events!
+
+        expect(Geo::ProjectSyncWorker).not_to have_enqueued_sidekiq_job(project.id, anything)
       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'])
 
-        expect(Geo::ProjectSyncWorker).not_to receive(:perform_async).with(project.id, anything)
-
         daemon.find_and_handle_events!
+
+        expect(Geo::ProjectSyncWorker).not_to have_enqueued_sidekiq_job(project.id, anything)
       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
-      expect(::Geo::EventWorker).to receive(:perform_async).with(
+      subject.process
+
+      expect(::Geo::EventWorker).to have_enqueued_sidekiq_job(
         "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,20 +13,19 @@ 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
-      expect(::Geo::HashedStorageAttachmentsMigrationWorker).to receive(:perform_async)
-        .with(project.id, old_attachments_path, new_attachments_path)
-
       subject.process
+
+      expect(::Geo::HashedStorageAttachmentsMigrationWorker).to have_enqueued_sidekiq_job(
+        project.id,
+        old_attachments_path,
+        new_attachments_path
+      )
     end
 
     it_behaves_like 'logs event source info'

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

@@ -14,10 +14,6 @@ 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
@@ -25,20 +21,23 @@ RSpec.describe Gitlab::Geo::LogCursor::Events::HashedStorageMigratedEvent, :clea
       end
 
       it 'does not schedule a Geo::HashedStorageMigrationWorker' do
-        expect(::Geo::HashedStorageMigrationWorker).not_to receive(:perform_async)
-          .with(project.id, old_disk_path, new_disk_path, old_storage_version)
-
         subject.process
+
+        expect(::Geo::HashedStorageMigrationWorker).not_to have_enqueued_sidekiq_job
       end
     end
 
     it 'schedules a Geo::HashedStorageMigrationWorker' do
       create(:geo_project_registry, project: project)
 
-      expect(::Geo::HashedStorageMigrationWorker).to receive(:perform_async)
-        .with(project.id, old_disk_path, new_disk_path, old_storage_version)
-
       subject.process
+
+      expect(::Geo::HashedStorageMigrationWorker).to have_enqueued_sidekiq_job(
+        project.id,
+        old_disk_path,
+        new_disk_path,
+        old_storage_version
+      )
     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
-        expect(Geo::ProjectSyncWorker).not_to receive(:perform_async).with(project.id, anything)
-
         subject.process
+
+        expect(Geo::ProjectSyncWorker).not_to have_enqueued_sidekiq_job
       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).to be_delayed
+        expect(entry.delayed?).to be_truthy
       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).not_to be_delayed
+        expect(entry.delayed?).to be_falsey
       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')).to be_unique
+      expect(find('public.bar_key').unique?).to be_truthy
     end
 
     it 'returns false for a regular, non-unique index' do
-      expect(find('public.foo_idx')).not_to be_unique
+      expect(find('public.foo_idx').unique?).to be_falsey
     end
 
     it 'returns true for a primary key index' do
-      expect(find('public.example_table_pkey')).to be_unique
+      expect(find('public.example_table_pkey').unique?).to be_truthy
     end
   end
 

spec/models/ci/build_spec.rb

@@ -1842,7 +1842,7 @@ RSpec.describe Ci::Build do
     end
 
     describe '#retryable?' do
-      subject { build }
+      subject { build.retryable? }
 
       context 'when build is retryable' do
         context 'when build is successful' do
@@ -1850,7 +1850,7 @@ RSpec.describe Ci::Build do
             build.success!
           end
 
-          it { is_expected.to be_retryable }
+          it { is_expected.to be_truthy }
         end
 
         context 'when build is failed' do
@@ -1858,7 +1858,7 @@ RSpec.describe Ci::Build do
             build.drop!
           end
 
-          it { is_expected.to be_retryable }
+          it { is_expected.to be_truthy }
         end
 
         context 'when build is canceled' do
@@ -1866,7 +1866,7 @@ RSpec.describe Ci::Build do
             build.cancel!
           end
 
-          it { is_expected.to be_retryable }
+          it { is_expected.to be_truthy }
         end
       end
 
@@ -1876,7 +1876,7 @@ RSpec.describe Ci::Build do
             build.run!
           end
 
-          it { is_expected.not_to be_retryable }
+          it { is_expected.to be_falsey }
         end
 
         context 'when build is skipped' do
@@ -1884,7 +1884,7 @@ RSpec.describe Ci::Build do
             build.skip!
           end
 
-          it { is_expected.not_to be_retryable }
+          it { is_expected.to be_falsey }
         end
 
         context 'when build is degenerated' do
@@ -1892,7 +1892,7 @@ RSpec.describe Ci::Build do
             build.degenerate!
           end
 
-          it { is_expected.not_to be_retryable }
+          it { is_expected.to be_falsey }
         end
 
         context 'when a canceled build has been retried already' do
@@ -1903,7 +1903,7 @@ RSpec.describe Ci::Build do
           end
 
           context 'when prevent_retry_of_retried_jobs feature flag is enabled' do
-            it { is_expected.not_to be_retryable }
+            it { is_expected.to be_falsey }
           end
 
           context 'when prevent_retry_of_retried_jobs feature flag is disabled' do
@@ -1911,7 +1911,7 @@ RSpec.describe Ci::Build do
               stub_feature_flags(prevent_retry_of_retried_jobs: false)
             end
 
-            it { is_expected.to be_retryable }
+            it { is_expected.to be_truthy }
           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