[Fix #647] Add OrderedHooks cop

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 * Fix `HooksBeforeExamples`, `LeadingSubject`, `LetBeforeExamples` and `ScatteredLet` autocorrection to take into account inline comments and comments immediately before the moved node. ([@Darhazer][])
 * Improve rubocop-rspec performance. ([@Darhazer][])
+* Add `RSpec/OrderdHooks` cop. ([@Darhazer][])
 
 ## 2.1.0 (2020-12-17)
 

config/default.yml

@@ -518,6 +518,12 @@ RSpec/NotToNot:
   VersionAdded: '1.4'
   StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/NotToNot
 
+RSpec/OrderedHooks:
+  Description: Checks that before/around/after hooks are defined in the correct order.
+  Enabled: pending
+  VersionAdded: '2.2'
+  StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/OrderedHooks
+
 RSpec/OverwritingSetup:
   Description: Checks if there is a let/subject that overwrites an existing one.
   Enabled: true

docs/modules/ROOT/pages/cops.adoc

@@ -57,6 +57,7 @@
 * xref:cops_rspec.adoc#rspecnamedsubject[RSpec/NamedSubject]
 * xref:cops_rspec.adoc#rspecnestedgroups[RSpec/NestedGroups]
 * xref:cops_rspec.adoc#rspecnottonot[RSpec/NotToNot]
+* xref:cops_rspec.adoc#rspecorderedhooks[RSpec/OrderedHooks]
 * xref:cops_rspec.adoc#rspecoverwritingsetup[RSpec/OverwritingSetup]
 * xref:cops_rspec.adoc#rspecpending[RSpec/Pending]
 * xref:cops_rspec.adoc#rspecpredicatematcher[RSpec/PredicateMatcher]

docs/modules/ROOT/pages/cops_rspec.adoc

@@ -2973,6 +2973,47 @@ end
 
 * https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/NotToNot
 
+== RSpec/OrderedHooks
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Pending
+| Yes
+| Yes
+| 2.2
+| -
+|===
+
+Checks that before/around/after hooks are defined in the correct order.
+
+If multiple hooks are defined in example group, they should appear in
+the following order:
+before :suite
+before :context
+before :example
+around :example
+after :example
+after :context
+after :suite
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+after { run_cleanup }
+before { run_setup }
+
+# good
+before { run_setup }
+after { run_cleanup }
+----
+
+=== References
+
+* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/OrderedHooks
+
 == RSpec/OverwritingSetup
 
 |===

lib/rubocop/cop/rspec/ordered_hooks.rb

@@ -0,0 +1,86 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module RSpec
+      # Checks that before/around/after hooks are defined in the correct order.
+      #
+      # If multiple hooks are defined in example group, they should appear in
+      # the following order:
+      # before :suite
+      # before :context
+      # before :example
+      # around :example
+      # after :example
+      # after :context
+      # after :suite
+      #
+      # @example
+      #  # bad
+      #  after { run_cleanup }
+      #  before { run_setup }
+      #
+      #  # good
+      #  before { run_setup }
+      #  after { run_cleanup }
+      #
+      class OrderedHooks < Base
+        extend AutoCorrector
+
+        MSG = '`%<hook>s` is supposed to appear before `%<previous>s`' \
+          ' at line %<line>d.'
+
+        EXPECTED_HOOK_ORDER = %i[before around after].freeze
+        EXPECTED_SCOPE_ORDER = %i[suite context each].freeze
+
+        def on_block(node)
+          return unless example_group_with_body?(node)
+
+          RuboCop::RSpec::ExampleGroup.new(node)
+            .hooks
+            .each_cons(2) { |previous, current| check_order(previous, current) }
+        end
+
+        private
+
+        def check_order(previous, current)
+          previous_idx = EXPECTED_HOOK_ORDER.index(previous.name)
+          current_idx = EXPECTED_HOOK_ORDER.index(current.name)
+
+          if previous_idx == current_idx
+            check_scope_order(previous, current)
+          elsif previous_idx > current_idx
+            order_violation(previous, current)
+          end
+        end
+
+        def check_scope_order(previous, current)
+          previous_idx = EXPECTED_SCOPE_ORDER.index(previous.scope)
+          current_idx = EXPECTED_SCOPE_ORDER.index(current.scope)
+
+          if current.name == :after # for after we expect reversed order
+            order_violation(previous, current) if previous_idx < current_idx
+          elsif previous_idx > current_idx
+            order_violation(previous, current)
+          end
+        end
+
+        def order_violation(previous, current)
+          message = format(MSG,
+                           hook: format_hook(current),
+                           previous: format_hook(previous),
+                           line: previous.to_node.loc.line)
+
+          add_offense(current.to_node.send_node, message: message)
+        end
+
+        def format_hook(hook)
+          msg = hook.name.to_s
+          raw_scope_name = hook.send(:scope_name)
+          msg += "(:#{raw_scope_name})" if raw_scope_name
+          msg
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rspec_cops.rb

@@ -69,6 +69,7 @@
 require_relative 'rspec/named_subject'
 require_relative 'rspec/nested_groups'
 require_relative 'rspec/not_to_not'
+require_relative 'rspec/ordered_hooks'
 require_relative 'rspec/overwriting_setup'
 require_relative 'rspec/pending'
 require_relative 'rspec/predicate_matcher'

spec/rubocop/cop/rspec/ordered_hooks_spec.rb

@@ -0,0 +1,213 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::RSpec::OrderedHooks do
+  it 'detects `before` after `around`' do
+    expect_offense(<<~RUBY)
+      describe "hooks order" do
+        around do |example|
+          example.call
+        end
+
+        before do
+        ^^^^^^ `before` is supposed to appear before `around` at line 2.
+          run_setup
+        end
+      end
+    RUBY
+  end
+
+  it 'detects `before` after `after`' do
+    expect_offense(<<~RUBY)
+      describe "hooks order" do
+        after do
+          run_teardown
+        end
+
+        before do
+        ^^^^^^ `before` is supposed to appear before `after` at line 2.
+          run_setup
+        end
+      end
+    RUBY
+  end
+
+  it 'detects `around` after `after`' do
+    expect_offense(<<~RUBY)
+      describe "hooks order" do
+        after do
+          run_teardown
+        end
+
+        around do |example|
+        ^^^^^^ `around` is supposed to appear before `after` at line 2.
+          example.run
+        end
+      end
+    RUBY
+  end
+
+  context 'with multiple hooks' do
+    it 'reports each violation independently' do
+      expect_offense(<<~RUBY)
+        describe "hooks order" do
+          after { cleanup }
+
+          around do |example|
+          ^^^^^^ `around` is supposed to appear before `after` at line 2.
+            example.call
+          end
+
+          before { some_preparation }
+          ^^^^^^ `before` is supposed to appear before `around` at line 4.
+
+          before { some_other_preparation }
+
+          after { more_cleanup }
+
+          before { more_preparation }
+          ^^^^^^ `before` is supposed to appear before `after` at line 12.
+        end
+      RUBY
+    end
+  end
+
+  context 'with scoped hooks' do
+    context 'when hook is before' do
+      it 'detects `suite` after `context`' do
+        expect_offense(<<~RUBY)
+          describe "hooks order" do
+            before(:context) { init_factories }
+            before(:suite) { global_setup }
+            ^^^^^^^^^^^^^^ `before(:suite)` is supposed to appear before `before(:context)` at line 2.
+          end
+        RUBY
+      end
+
+      it 'detects `suite` after `each`' do
+        expect_offense(<<~RUBY)
+          describe "hooks order" do
+            before(:each) { init_data }
+            before(:suite) { global_setup }
+            ^^^^^^^^^^^^^^ `before(:suite)` is supposed to appear before `before(:each)` at line 2.
+          end
+        RUBY
+      end
+
+      it 'detects `context` after `each`' do
+        expect_offense(<<~RUBY)
+          describe "hooks order" do
+            before(:each) { init_data }
+            before(:context) { setup }
+            ^^^^^^^^^^^^^^^^ `before(:context)` is supposed to appear before `before(:each)` at line 2.
+          end
+        RUBY
+      end
+
+      it 'accepts `example` and `each`' do
+        expect_no_offenses(<<~RUBY)
+          describe "hooks order" do
+            before { setup1 }
+            before(:each) { setup2 }
+            before(:example) { setup3 }
+          end
+        RUBY
+      end
+
+      it 'detects `context` after `example`' do
+        expect_offense(<<~RUBY)
+          describe "hooks order" do
+            before(:example) { init_data }
+            before(:context) { setup }
+            ^^^^^^^^^^^^^^^^ `before(:context)` is supposed to appear before `before(:example)` at line 2.
+          end
+        RUBY
+      end
+    end
+
+    context 'when hook is after' do
+      it 'detects `context` after `suite`' do
+        expect_offense(<<~RUBY)
+          describe "hooks order" do
+            after(:suite) { global_teardown }
+            after(:context) { teardown }
+            ^^^^^^^^^^^^^^^ `after(:context)` is supposed to appear before `after(:suite)` at line 2.
+          end
+        RUBY
+      end
+
+      it 'detects `each` after `suite`' do
+        expect_offense(<<~RUBY)
+          describe "hooks order" do
+            after(:suite) { global_teardown }
+            after(:each) { teardown }
+            ^^^^^^^^^^^^ `after(:each)` is supposed to appear before `after(:suite)` at line 2.
+          end
+        RUBY
+      end
+
+      it 'detects `each` after `context`' do
+        expect_offense(<<~RUBY)
+          describe "hooks order" do
+            after(:context) { teardown }
+            after(:each) { cleanup }
+            ^^^^^^^^^^^^ `after(:each)` is supposed to appear before `after(:context)` at line 2.
+          end
+        RUBY
+      end
+
+      it 'accepts `example` and `each`' do
+        expect_no_offenses(<<~RUBY)
+          describe "hooks order" do
+            after { setup1 }
+            after(:each) { setup2 }
+            after(:example) { setup3 }
+          end
+        RUBY
+      end
+
+      it 'detects `example` after `context`' do
+        expect_offense(<<~RUBY)
+          describe "hooks order" do
+            after(:context) { cleanup }
+            after(:example) { teardown }
+            ^^^^^^^^^^^^^^^ `after(:example)` is supposed to appear before `after(:context)` at line 2.
+          end
+        RUBY
+      end
+    end
+  end
+
+  it 'accepts hooks in order' do
+    expect_no_offenses(<<~RUBY)
+      desribe "correctly ordered hooks" do
+        before(:suite) do
+          run_global_setup
+        end
+
+        before(:context) do
+          run_context_setup
+        end
+
+        before(:example) do
+          run_setup
+        end
+
+        around(:example) do |example|
+          example.run
+        end
+
+        after(:example) do
+          run_teardown
+        end
+
+        after(:context) do
+          run_context_teardown
+        end
+
+        after(:suite) do
+          run_global_teardown
+        end
+      end
+    RUBY
+  end
+end