From 8521d9d2dd9ed822c8661018309302fe38e3fce7 Mon Sep 17 00:00:00 2001 From: Maxim Krizhanovski Date: Thu, 21 Jan 2021 00:44:44 +0200 Subject: [PATCH] [Fix #647] Add OrderedHooks cop --- CHANGELOG.md | 1 + config/default.yml | 6 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_rspec.adoc | 41 ++++ lib/rubocop/cop/rspec/ordered_hooks.rb | 86 ++++++++ lib/rubocop/cop/rspec_cops.rb | 1 + spec/rubocop/cop/rspec/ordered_hooks_spec.rb | 213 +++++++++++++++++++ 7 files changed, 349 insertions(+) create mode 100644 lib/rubocop/cop/rspec/ordered_hooks.rb create mode 100644 spec/rubocop/cop/rspec/ordered_hooks_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index d61b76753..3a1c40aab 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/config/default.yml b/config/default.yml index 85f73e4ee..a014966b2 100644 --- a/config/default.yml +++ b/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 diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 4330d141f..a3f236dad 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/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] diff --git a/docs/modules/ROOT/pages/cops_rspec.adoc b/docs/modules/ROOT/pages/cops_rspec.adoc index 079b74e33..065379777 100644 --- a/docs/modules/ROOT/pages/cops_rspec.adoc +++ b/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 |=== diff --git a/lib/rubocop/cop/rspec/ordered_hooks.rb b/lib/rubocop/cop/rspec/ordered_hooks.rb new file mode 100644 index 000000000..279a777ff --- /dev/null +++ b/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 = '`%s` is supposed to appear before `%s`' \ + ' at 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 diff --git a/lib/rubocop/cop/rspec_cops.rb b/lib/rubocop/cop/rspec_cops.rb index 1de7e025d..4c4531674 100644 --- a/lib/rubocop/cop/rspec_cops.rb +++ b/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' diff --git a/spec/rubocop/cop/rspec/ordered_hooks_spec.rb b/spec/rubocop/cop/rspec/ordered_hooks_spec.rb new file mode 100644 index 000000000..352424010 --- /dev/null +++ b/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