From 3cad04cd51fb2453e33044a0eb2923dd389c2d20 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 12 Dec 2017 17:21:00 +0100 Subject: Add rubocops to ensure Sidekiq workers include ApplicationWorker and don't manually set their queue --- app/workers/concerns/application_worker.rb | 4 ++-- rubocop/cop/include_sidekiq_worker.rb | 29 +++++++++++++++++++++++ rubocop/cop/sidekiq_options_queue.rb | 27 +++++++++++++++++++++ rubocop/rubocop.rb | 2 ++ spec/rubocop/cop/include_sidekiq_worker_spec.rb | 31 +++++++++++++++++++++++++ spec/rubocop/cop/sidekiq_options_queue_spec.rb | 26 +++++++++++++++++++++ spec/workers/every_sidekiq_worker_spec.rb | 4 ---- 7 files changed, 117 insertions(+), 6 deletions(-) create mode 100644 rubocop/cop/include_sidekiq_worker.rb create mode 100644 rubocop/cop/sidekiq_options_queue.rb create mode 100644 spec/rubocop/cop/include_sidekiq_worker_spec.rb create mode 100644 spec/rubocop/cop/sidekiq_options_queue_spec.rb diff --git a/app/workers/concerns/application_worker.rb b/app/workers/concerns/application_worker.rb index 285b5bada7d..37586e161c9 100644 --- a/app/workers/concerns/application_worker.rb +++ b/app/workers/concerns/application_worker.rb @@ -3,7 +3,7 @@ Sidekiq::Worker.extend ActiveSupport::Concern module ApplicationWorker extend ActiveSupport::Concern - include Sidekiq::Worker + include Sidekiq::Worker # rubocop:disable Cop/IncludeSidekiqWorker included do set_queue @@ -17,7 +17,7 @@ module ApplicationWorker def set_queue queue_name = [queue_namespace, base_queue_name].compact.join(':') - sidekiq_options queue: queue_name + sidekiq_options queue: queue_name # rubocop:disable Cop/SidekiqOptionsQueue end def base_queue_name diff --git a/rubocop/cop/include_sidekiq_worker.rb b/rubocop/cop/include_sidekiq_worker.rb new file mode 100644 index 00000000000..4a6332286a2 --- /dev/null +++ b/rubocop/cop/include_sidekiq_worker.rb @@ -0,0 +1,29 @@ +require_relative '../spec_helpers' + +module RuboCop + module Cop + # Cop that makes sure workers include `ApplicationWorker`, not `Sidekiq::Worker`. + class IncludeSidekiqWorker < RuboCop::Cop::Cop + include SpecHelpers + + MSG = 'Include `ApplicationWorker`, not `Sidekiq::Worker`.'.freeze + + def_node_matcher :includes_sidekiq_worker?, <<~PATTERN + (send nil :include (const (const nil :Sidekiq) :Worker)) + PATTERN + + def on_send(node) + return if in_spec?(node) + return unless includes_sidekiq_worker?(node) + + add_offense(node.arguments.first, :expression) + end + + def autocorrect(node) + lambda do |corrector| + corrector.replace(node.source_range, 'ApplicationWorker') + end + end + end + end +end diff --git a/rubocop/cop/sidekiq_options_queue.rb b/rubocop/cop/sidekiq_options_queue.rb new file mode 100644 index 00000000000..43b35ba0214 --- /dev/null +++ b/rubocop/cop/sidekiq_options_queue.rb @@ -0,0 +1,27 @@ +require_relative '../spec_helpers' + +module RuboCop + module Cop + # Cop that prevents manually setting a queue in Sidekiq workers. + class SidekiqOptionsQueue < RuboCop::Cop::Cop + include SpecHelpers + + MSG = 'Do not manually set a queue; `ApplicationWorker` sets one automatically.'.freeze + + def_node_matcher :sidekiq_options?, <<~PATTERN + (send nil :sidekiq_options $...) + PATTERN + + def on_send(node) + return if in_spec?(node) + return unless sidekiq_options?(node) + + node.arguments.first.each_node(:pair) do |pair| + key_name = pair.key.children[0] + + add_offense(pair, :expression) if key_name == :queue + end + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index eb52be3d731..3e3b4c8349a 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -3,10 +3,12 @@ require_relative 'cop/active_record_serialize' require_relative 'cop/custom_error_class' require_relative 'cop/gem_fetcher' require_relative 'cop/in_batches' +require_relative 'cop/include_sidekiq_worker' require_relative 'cop/line_break_after_guard_clauses' require_relative 'cop/polymorphic_associations' require_relative 'cop/project_path_helper' require_relative 'cop/redirect_with_status' +require_relative 'cop/sidekiq_options_queue' require_relative 'cop/migration/add_column' require_relative 'cop/migration/add_concurrent_foreign_key' require_relative 'cop/migration/add_concurrent_index' diff --git a/spec/rubocop/cop/include_sidekiq_worker_spec.rb b/spec/rubocop/cop/include_sidekiq_worker_spec.rb new file mode 100644 index 00000000000..7f406535dda --- /dev/null +++ b/spec/rubocop/cop/include_sidekiq_worker_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../rubocop/cop/include_sidekiq_worker' + +describe RuboCop::Cop::IncludeSidekiqWorker do + include CopHelper + + subject(:cop) { described_class.new } + + context 'when `Sidekiq::Worker` is included' do + let(:source) { 'include Sidekiq::Worker' } + let(:correct_source) { 'include ApplicationWorker' } + + it 'registers an offense ' do + inspect_source(cop, source) + + aggregate_failures do + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([1]) + expect(cop.highlights).to eq(['Sidekiq::Worker']) + end + end + + it 'autocorrects to the right version' do + autocorrected = autocorrect_source(cop, source) + + expect(autocorrected).to eq(correct_source) + end + end +end diff --git a/spec/rubocop/cop/sidekiq_options_queue_spec.rb b/spec/rubocop/cop/sidekiq_options_queue_spec.rb new file mode 100644 index 00000000000..a31de381631 --- /dev/null +++ b/spec/rubocop/cop/sidekiq_options_queue_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../rubocop/cop/sidekiq_options_queue' + +describe RuboCop::Cop::SidekiqOptionsQueue do + include CopHelper + + subject(:cop) { described_class.new } + + it 'registers an offense when `sidekiq_options` is used with the `queue` option' do + inspect_source(cop, 'sidekiq_options queue: "some_queue"') + + aggregate_failures do + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([1]) + expect(cop.highlights).to eq(['queue: "some_queue"']) + end + end + + it 'does not register an offense when `sidekiq_options` is used with another option' do + inspect_source(cop, 'sidekiq_options retry: false') + + expect(cop.offenses).to be_empty + end +end diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index bf99c24e73c..9e3b99b3502 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -1,10 +1,6 @@ require 'spec_helper' describe 'Every Sidekiq worker' do - it 'includes ApplicationWorker' do - expect(Gitlab::SidekiqConfig.workers).to all(include(ApplicationWorker)) - end - it 'does not use the default queue' do expect(Gitlab::SidekiqConfig.workers.map(&:queue)).not_to include('default') end -- cgit v1.2.3