Merge branch 'bvl-run-sidekiq-server-middleware-in-specs' into 'master'

Run sidekiq-server middleware in all specs

See merge request gitlab-org/gitlab!29509

rubocop/cop/rspec/modify_sidekiq_middleware.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module RSpec
+      # This cop checks for `Sidekiq::Testing.server_middleware`
+      # usage in specs.
+      #
+      # @example
+      #
+      #   # bad
+      #   Sidekiq::Testing.server_middleware do |chain|
+      #     chain.add(MyMiddlewareUnderTest)
+      #   end
+      #
+      #
+      #   # good
+      #   with_custom_sidekiq_middleware do |chain|
+      #     chain.add(MyMiddlewareUnderTest)
+      #   end
+      #
+      #
+      class ModifySidekiqMiddleware < RuboCop::Cop::Cop
+        MSG = <<~MSG
+          Don't modify global sidekiq middleware, use the `#with_sidekiq_server_middleware`
+          helper instead
+        MSG
+
+        def_node_search :modifies_sidekiq_middleware?, <<~PATTERN
+          (send
+            (const
+              (const nil? :Sidekiq) :Testing) :server_middleware)
+        PATTERN
+
+        def on_send(node)
+          return unless modifies_sidekiq_middleware?(node)
+
+          add_offense(node, location: :expression)
+        end
+
+        def autocorrect(node)
+          -> (corrector) do
+            corrector.replace(node.loc.expression,
+                              'with_sidekiq_server_middleware')
+          end
+        end
+      end
+    end
+  end
+end

spec/features/admin/admin_mode/workers_spec.rb

@@ -8,8 +8,6 @@ describe 'Admin mode for workers', :do_not_mock_admin_mode, :request_store, :cle
   let(:user_to_delete) { create(:user) }
 
   before do
-    add_sidekiq_middleware
-
     sign_in(user)
   end
 
@@ -60,12 +58,6 @@ describe 'Admin mode for workers', :do_not_mock_admin_mode, :request_store, :cle
     end
   end
 
-  def add_sidekiq_middleware
-    Sidekiq::Testing.server_middleware do |chain|
-      chain.add Gitlab::SidekiqMiddleware::AdminMode::Server
-    end
-  end
-
   def execute_jobs_signed_out(user)
     gitlab_sign_out
 

spec/lib/gitlab/import_export/project/import_task_spec.rb

@@ -2,7 +2,7 @@
 
 require 'rake_helper'
 
-describe Gitlab::ImportExport::Project::ImportTask do
+describe Gitlab::ImportExport::Project::ImportTask, :request_store do
   let(:username) { 'root' }
   let(:namespace_path) { username }
   let!(:user) { create(:user, username: username) }

spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/server_spec.rb

@@ -21,18 +21,9 @@ describe Gitlab::SidekiqMiddleware::DuplicateJobs::Server, :clean_gitlab_redis_q
   end
 
   around do |example|
-    Sidekiq::Testing.inline! { example.run }
-  end
-
-  before(:context) do
-    Sidekiq::Testing.server_middleware do |chain|
+    with_sidekiq_server_middleware do |chain|
       chain.add described_class
-    end
-  end
-
-  after(:context) do
-    Sidekiq::Testing.server_middleware do |chain|
-      chain.remove described_class
+      Sidekiq::Testing.inline! { example.run }
     end
   end
 

spec/lib/gitlab/sidekiq_middleware/worker_context/server_spec.rb

@@ -41,18 +41,9 @@ describe Gitlab::SidekiqMiddleware::WorkerContext::Server do
   end
 
   around do |example|
-    Sidekiq::Testing.inline! { example.run }
-  end
-
-  before(:context) do
-    Sidekiq::Testing.server_middleware do |chain|
+    with_sidekiq_server_middleware do |chain|
       chain.add described_class
-    end
-  end
-
-  after(:context) do
-    Sidekiq::Testing.server_middleware do |chain|
-      chain.remove described_class
+      Sidekiq::Testing.inline! { example.run }
     end
   end
 

spec/lib/gitlab/sidekiq_middleware_spec.rb

@@ -28,11 +28,16 @@ describe Gitlab::SidekiqMiddleware do
   # 2) yielding exactly once
   describe '.server_configurator' do
     around do |example|
-      original = Sidekiq::Testing.server_middleware.dup
-
-      example.run
-
-      Sidekiq::Testing.instance_variable_set :@server_chain, original
+      with_sidekiq_server_middleware do |chain|
+        described_class.server_configurator(
+          metrics: metrics,
+          arguments_logger: arguments_logger,
+          memory_killer: memory_killer,
+          request_store: request_store
+        ).call(chain)
+
+        example.run
+      end
     end
 
     let(:middleware_expected_args) { [a_kind_of(worker_class), hash_including({ 'args' => job_args }), anything] }
@@ -54,21 +59,17 @@ describe Gitlab::SidekiqMiddleware do
     end
     let(:enabled_sidekiq_middlewares) { all_sidekiq_middlewares - disabled_sidekiq_middlewares }
 
-    before do
-      Sidekiq::Testing.server_middleware.clear
-      Sidekiq::Testing.server_middleware(&described_class.server_configurator(
-        metrics: metrics,
-        arguments_logger: arguments_logger,
-        memory_killer: memory_killer,
-        request_store: request_store
-      ))
-
-      enabled_sidekiq_middlewares.each do |middleware|
-        expect_any_instance_of(middleware).to receive(:call).with(*middleware_expected_args).once.and_call_original
-      end
+    shared_examples "a server middleware chain" do
+      it "passes through the right server middlewares" do
+        enabled_sidekiq_middlewares.each do |middleware|
+          expect_any_instance_of(middleware).to receive(:call).with(*middleware_expected_args).once.and_call_original
+        end
 
-      disabled_sidekiq_middlewares.each do |middleware|
-        expect_any_instance_of(Gitlab::SidekiqMiddleware::ArgumentsLogger).not_to receive(:call)
+        disabled_sidekiq_middlewares.each do |middleware|
+          expect_any_instance_of(middleware).not_to receive(:call)
+        end
+
+        worker_class.perform_async(*job_args)
       end
     end
 
@@ -86,9 +87,7 @@ describe Gitlab::SidekiqMiddleware do
         ]
       end
 
-      it "passes through server middlewares" do
-        worker_class.perform_async(*job_args)
-      end
+      it_behaves_like "a server middleware chain"
     end
 
     context "all optional middlewares on" do
@@ -98,9 +97,7 @@ describe Gitlab::SidekiqMiddleware do
       let(:request_store) { true }
       let(:disabled_sidekiq_middlewares) { [] }
 
-      it "passes through server middlewares" do
-        worker_class.perform_async(*job_args)
-      end
+      it_behaves_like "a server middleware chain"
 
       context "server metrics" do
         let(:gitaly_histogram) { double(:gitaly_histogram) }

spec/lib/marginalia_spec.rb

@@ -24,20 +24,6 @@ describe 'Marginalia spec' do
     end
   end
 
-  def add_sidekiq_middleware
-    # Reference: https://github.com/mperham/sidekiq/wiki/Testing#testing-server-middlewaresidekiq
-    # Sidekiq test harness fakes worker without its server middlewares, so include instrumentation to 'Sidekiq::Testing' server middleware.
-    Sidekiq::Testing.server_middleware do |chain|
-      chain.add Marginalia::SidekiqInstrumentation::Middleware
-    end
-  end
-
-  def remove_sidekiq_middleware
-    Sidekiq::Testing.server_middleware do |chain|
-      chain.remove Marginalia::SidekiqInstrumentation::Middleware
-    end
-  end
-
   def stub_feature(value)
     allow(Gitlab::Marginalia).to receive(:cached_feature_enabled?).and_return(value)
   end
@@ -88,20 +74,16 @@ describe 'Marginalia spec' do
   end
 
   describe 'for Sidekiq worker jobs' do
-    before(:all) do
-      add_sidekiq_middleware
-
-      # Because of faking, 'Sidekiq.server?' does not work so implicitly set application name which is done in config/initializers/0_marginalia.rb
-      Marginalia.application_name = "sidekiq"
+    around do |example|
+      with_sidekiq_server_middleware do |chain|
+        chain.add Marginalia::SidekiqInstrumentation::Middleware
+        Marginalia.application_name = "sidekiq"
+        example.run
+      end
     end
 
     after(:all) do
       MarginaliaTestJob.clear
-      remove_sidekiq_middleware
-    end
-
-    around do |example|
-      Sidekiq::Testing.fake! { example.run }
     end
 
     before do

spec/rubocop/cop/rspec/modify_sidekiq_middleware_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rubocop'
+require_relative '../../../support/helpers/expect_offense'
+require_relative '../../../../rubocop/cop/rspec/modify_sidekiq_middleware'
+
+describe RuboCop::Cop::RSpec::ModifySidekiqMiddleware do
+  include CopHelper
+  include ExpectOffense
+
+  subject(:cop) { described_class.new }
+
+  let(:source) do
+    <<~SRC
+    Sidekiq::Testing.server_middleware do |chain|
+      chain.add(MyCustomMiddleware)
+    end
+    SRC
+  end
+
+  let(:corrected) do
+    <<~SRC
+    with_sidekiq_server_middleware do |chain|
+      chain.add(MyCustomMiddleware)
+    end
+    SRC
+  end
+
+  it 'registers an offence' do
+    inspect_source(source)
+
+    expect(cop.offenses.size).to eq(1)
+  end
+
+  it 'can autocorrect the source' do
+    expect(autocorrect_source(source)).to eq(corrected)
+  end
+end

spec/spec_helper.rb

@@ -136,6 +136,7 @@ RSpec.configure do |config|
   config.include ExpectRequestWithStatus, type: :request
   config.include IdempotentWorkerHelper, type: :worker
   config.include RailsHelpers
+  config.include SidekiqMiddleware
 
   if ENV['CI'] || ENV['RETRIES']
     # This includes the first try, i.e. tests will be run 4 times before failing.
@@ -299,6 +300,22 @@ RSpec.configure do |config|
     Labkit::Context.with_context { example.run }
   end
 
+  config.around do |example|
+    with_sidekiq_server_middleware do |chain|
+      Gitlab::SidekiqMiddleware.server_configurator(
+        metrics: false, # The metrics don't go anywhere in tests
+        arguments_logger: false, # We're not logging the regular messages for inline jobs
+        memory_killer: false, # This is not a thing we want to do inline in tests
+        # Don't enable this if the request store is active in the spec itself
+        # This needs to run within the `request_store` around block defined above
+        request_store: !RequestStore.active?
+      ).call(chain)
+      chain.add DisableQueryLimit
+
+      example.run
+    end
+  end
+
   config.after do
     Fog.unmock! if Fog.mock?
     Gitlab::CurrentSettings.clear_in_memory_application_settings!

spec/support/sidekiq_middleware.rb

@@ -2,6 +2,17 @@
 
 require 'sidekiq/testing'
 
+# rubocop:disable RSpec/ModifySidekiqMiddleware
+module SidekiqMiddleware
+  def with_sidekiq_server_middleware(&block)
+    Sidekiq::Testing.server_middleware.clear
+    Sidekiq::Testing.server_middleware(&block)
+  ensure
+    Sidekiq::Testing.server_middleware.clear
+  end
+end
+# rubocop:enable RSpec/ModifySidekiqMiddleware
+
 # If Sidekiq::Testing.inline! is used, SQL transactions done inside
 # Sidekiq worker are included in the SQL query limit (in a real
 # deployment sidekiq worker is executed separately). To avoid
@@ -20,8 +31,3 @@ class DisableQueryLimit
     end
   end
 end
-
-Sidekiq::Testing.server_middleware do |chain|
-  chain.add Gitlab::SidekiqStatus::ServerMiddleware
-  chain.add DisableQueryLimit
-end