diff --git a/.rubocop.yml b/.rubocop.yml index e8625243b..4362ff878 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -293,3 +293,4 @@ Performance/ZipWithoutBlock: {Enabled: true} # Enable our own pending cops. RSpec/IncludeExamples: {Enabled: true} +RSpec/LeakyLocalVariable: {Enabled: true} diff --git a/CHANGELOG.md b/CHANGELOG.md index cc0f4bb5e..d1c4dc56f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Fix a false positive for `RSpec/LeakyConstantDeclaration` when defining constants in explicit namespaces. ([@naveg]) - Add support for error matchers (`raise_exception` and `raise_error`) to `RSpec/Dialect`. ([@lovro-bikic]) - Don't register offenses for `RSpec/DescribedClass` within `Data.define` blocks. ([@lovro-bikic]) +- Add new cop `RSpec/LeakyLocalVariable`. ([@lovro-bikic]) ## 3.6.0 (2025-04-18) diff --git a/config/default.yml b/config/default.yml index a8412c062..7b37cdddf 100644 --- a/config/default.yml +++ b/config/default.yml @@ -606,6 +606,12 @@ RSpec/LeakyConstantDeclaration: StyleGuide: https://rspec.rubystyle.guide/#declare-constants Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/LeakyConstantDeclaration +RSpec/LeakyLocalVariable: + Description: Checks for outside local variables used in examples. + Enabled: pending + VersionAdded: "<>" + Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/LeakyLocalVariable + RSpec/LetBeforeExamples: Description: Checks for `let` definitions that come after an example. Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 08329092b..fb43db294 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -59,6 +59,7 @@ * xref:cops_rspec.adoc#rspeciteratedexpectation[RSpec/IteratedExpectation] * xref:cops_rspec.adoc#rspecleadingsubject[RSpec/LeadingSubject] * xref:cops_rspec.adoc#rspecleakyconstantdeclaration[RSpec/LeakyConstantDeclaration] +* xref:cops_rspec.adoc#rspecleakylocalvariable[RSpec/LeakyLocalVariable] * xref:cops_rspec.adoc#rspecletbeforeexamples[RSpec/LetBeforeExamples] * xref:cops_rspec.adoc#rspecletsetup[RSpec/LetSetup] * xref:cops_rspec.adoc#rspecmatcharray[RSpec/MatchArray] diff --git a/docs/modules/ROOT/pages/cops_rspec.adoc b/docs/modules/ROOT/pages/cops_rspec.adoc index 04a639841..9b8eedb5c 100644 --- a/docs/modules/ROOT/pages/cops_rspec.adoc +++ b/docs/modules/ROOT/pages/cops_rspec.adoc @@ -3391,6 +3391,88 @@ end * https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/LeakyConstantDeclaration * https://rspec.info/features/3-12/rspec-mocks/mutating-constants +[#rspecleakylocalvariable] +== RSpec/LeakyLocalVariable + +|=== +| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed + +| Pending +| Yes +| No +| <> +| - +|=== + +Checks for outside local variables used in examples. + +Local variables assigned outside an example but used within it act +as shared state, which can make tests non-deterministic. + +[#examples-rspecleakylocalvariable] +=== Examples + +[source,ruby] +---- +# bad - outside variable used in a hook +user = create(:user) + +before { user.update(admin: true) } + +# good +let(:user) { create(:user) } + +before { user.update(admin: true) } + +# bad - outside variable used in an example +user = create(:user) + +it 'is persisted' do + expect(user).to be_persisted +end + +# good +let(:user) { create(:user) } + +it 'is persisted' do + expect(user).to be_persisted +end + +# also good - assigning the variable within the example +it 'is persisted' do + user = create(:user) + + expect(user).to be_persisted +end + +# bad - outside variable passed to included examples +attrs = ['foo', 'bar'] + +it_behaves_like 'some examples', attrs + +# good +it_behaves_like 'some examples' do + let(:attrs) { ['foo', 'bar'] } +end + +# good - when variable is used only as example description +attribute = 'foo' + +it "#{attribute} is persisted" do + expectations +end + +# good - when variable is used only to include other examples +examples = foo ? 'some examples' : 'other examples' + +it_behaves_like examples, another_argument +---- + +[#references-rspecleakylocalvariable] +=== References + +* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/LeakyLocalVariable + [#rspecletbeforeexamples] == RSpec/LetBeforeExamples diff --git a/lib/rubocop/cop/rspec/leaky_local_variable.rb b/lib/rubocop/cop/rspec/leaky_local_variable.rb new file mode 100644 index 000000000..e77285d54 --- /dev/null +++ b/lib/rubocop/cop/rspec/leaky_local_variable.rb @@ -0,0 +1,133 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + # Checks for outside local variables used in examples. + # + # Local variables assigned outside an example but used within it act + # as shared state, which can make tests non-deterministic. + # + # @example + # # bad - outside variable used in a hook + # user = create(:user) + # + # before { user.update(admin: true) } + # + # # good + # let(:user) { create(:user) } + # + # before { user.update(admin: true) } + # + # # bad - outside variable used in an example + # user = create(:user) + # + # it 'is persisted' do + # expect(user).to be_persisted + # end + # + # # good + # let(:user) { create(:user) } + # + # it 'is persisted' do + # expect(user).to be_persisted + # end + # + # # also good - assigning the variable within the example + # it 'is persisted' do + # user = create(:user) + # + # expect(user).to be_persisted + # end + # + # # bad - outside variable passed to included examples + # attrs = ['foo', 'bar'] + # + # it_behaves_like 'some examples', attrs + # + # # good + # it_behaves_like 'some examples' do + # let(:attrs) { ['foo', 'bar'] } + # end + # + # # good - when variable is used only as example description + # attribute = 'foo' + # + # it "#{attribute} is persisted" do + # expectations + # end + # + # # good - when variable is used only to include other examples + # examples = foo ? 'some examples' : 'other examples' + # + # it_behaves_like examples, another_argument + # + class LeakyLocalVariable < Base + MSG = 'Use `let` instead of a local variable which can leak ' \ + 'between examples.' + + # @!method example_method?(node) + def_node_matcher :example_method?, <<~PATTERN + (send nil? #Examples.all _) + PATTERN + + # @!method includes_method?(node) + def_node_matcher :includes_method?, <<~PATTERN + (send nil? #Includes.all ...) + PATTERN + + def self.joining_forces + VariableForce + end + + def after_leaving_scope(scope, _variable_table) + scope.variables.each_value { |variable| check_references(variable) } + end + + private + + def check_references(variable) + variable.assignments.each do |assignment| + next if part_of_example_scope?(assignment.node) + + assignment.references.each do |reference| + next unless part_of_example_scope?(reference) + next if allowed_reference?(reference) + + add_offense(assignment.node) + end + end + end + + def allowed_reference?(node) + node.each_ancestor.any? do |ancestor| + next true if example_method?(ancestor) + if includes_method?(ancestor) + next allowed_includes_arguments?(ancestor, node) + end + + false + end + end + + def allowed_includes_arguments?(node, argument) + node.arguments[1..].all? do |argument_node| + next true if argument_node.type?(:dstr, :dsym) + + argument_node != argument && + argument_node.each_descendant.none?(argument) + end + end + + def part_of_example_scope?(node) + node.each_ancestor.any? { |ancestor| example_scope?(ancestor) } + end + + def example_scope?(node) + subject?(node) || let?(node) || hook?(node) || example?(node) || + include?(node) + end + end + end + end +end diff --git a/lib/rubocop/cop/rspec_cops.rb b/lib/rubocop/cop/rspec_cops.rb index cf2879e11..d64ca9e51 100644 --- a/lib/rubocop/cop/rspec_cops.rb +++ b/lib/rubocop/cop/rspec_cops.rb @@ -57,6 +57,7 @@ require_relative 'rspec/iterated_expectation' require_relative 'rspec/leading_subject' require_relative 'rspec/leaky_constant_declaration' +require_relative 'rspec/leaky_local_variable' require_relative 'rspec/let_before_examples' require_relative 'rspec/let_setup' require_relative 'rspec/match_array' diff --git a/spec/rubocop/cop/rspec/leaky_local_variable_spec.rb b/spec/rubocop/cop/rspec/leaky_local_variable_spec.rb new file mode 100644 index 000000000..8f146660a --- /dev/null +++ b/spec/rubocop/cop/rspec/leaky_local_variable_spec.rb @@ -0,0 +1,257 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::RSpec::LeakyLocalVariable, :config do + it 'registers an offense when outside variable is used in `before`' do + expect_offense(<<~RUBY) + user = create(:user) + ^^^^^^^^^^^^^^^^^^^^ Use `let` instead of a local variable which can leak between examples. + + before { user.update(admin: true) } + RUBY + end + + it 'registers an offense when outside variable is used in `it`' do + expect_offense(<<~RUBY) + user = create(:user) + ^^^^^^^^^^^^^^^^^^^^ Use `let` instead of a local variable which can leak between examples. + + it 'updates the user' do + expect { user.update(admin: true) }.to change(user, :updated_at) + end + RUBY + end + + it 'registers an offense when outside variable is used as ' \ + '`it_behaves_like` argument' do + expect_offense(<<~RUBY) + user = create(:user) + ^^^^^^^^^^^^^^^^^^^^ Use `let` instead of a local variable which can leak between examples. + + it_behaves_like 'some example', user + RUBY + end + + it 'registers an offense when outside variable is used as part of the ' \ + '`it_behaves_like` argument' do + expect_offense(<<~RUBY) + user = create(:user) + ^^^^^^^^^^^^^^^^^^^^ Use `let` instead of a local variable which can leak between examples. + + it_behaves_like 'some example', [user, user2] + RUBY + end + + it 'registers an offense when outside variable is used in `let`' do + expect_offense(<<~RUBY) + user = create(:user) + ^^^^^^^^^^^^^^^^^^^^ Use `let` instead of a local variable which can leak between examples. + + let(:my_user) { user } + RUBY + end + + it 'registers an offense when outside variable is used in `subject`' do + expect_offense(<<~RUBY) + user = create(:user) + ^^^^^^^^^^^^^^^^^^^^ Use `let` instead of a local variable which can leak between examples. + + subject { user } + RUBY + end + + it 'registers an offense when using outside block argument which is also ' \ + 'reassigned outside' do + expect_offense(<<~RUBY) + shared_examples 'some examples' do |subject| + subject = SecureRandom.uuid + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `let` instead of a local variable which can leak between examples. + + it 'renders the subject' do + expect(mail.subject).to eq(subject) + end + end + RUBY + end + + it 'registers an offense when variable is used in interpolation inside an ' \ + 'example' do + expect_offense(<<~RUBY) + user = create(:user) + ^^^^^^^^^^^^^^^^^^^^ Use `let` instead of a local variable which can leak between examples. + + it 'does something' do + expect("foo_\#{user.name}").to eq("foo_bar") + end + RUBY + end + + it 'registers an offense when outside variable is used as part of `it` ' \ + 'argument and in `it` block' do + expect_offense(<<~RUBY) + article = foo ? 'a' : 'the' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `let` instead of a local variable which can leak between examples. + + it "updates \#{article} user" do + user.update(preferred_article: article) + end + RUBY + end + + it 'registers an offense when outside variable is used in `it` and ' \ + 'reassigned within `it` after referencing' do + expect_offense(<<~RUBY) + user = create(:user) + ^^^^^^^^^^^^^^^^^^^^ Use `let` instead of a local variable which can leak between examples. + + it 'updates the user' do + expect { user.update(admin: true) }.to change(user, :updated_at) + user = create(:user) + end + RUBY + end + + it 'does not register an offense when outside variable is used in `it` and ' \ + 'reassigned within `it` before referencing' do + expect_no_offenses(<<~RUBY) + user = create(:user) + + it 'updates the user' do + user = create(:user) + expect { user.update(admin: true) }.to change(user, :updated_at) + end + RUBY + end + + it 'does not register an offense when using outside block argument which ' \ + 'is reassigned inside' do + expect_no_offenses(<<~RUBY) + shared_examples 'some examples' do |subject| + it 'renders the subject' do + subject = 'hello' + expect(mail.subject).to eq(subject) + end + end + RUBY + end + + it 'does not register an offense when using outside block argument' do + expect_no_offenses(<<~RUBY) + shared_examples 'some examples' do |subject| + it 'renders the subject' do + expect(mail.subject).to eq(subject) + end + end + RUBY + end + + it 'does not register an offense when using outside block keyword argument' do + expect_no_offenses(<<~RUBY) + shared_examples 'some examples' do |subject:| + it 'renders the subject' do + expect(mail.subject).to eq(subject) + end + end + RUBY + end + + it 'does not register an offense when variable is assigned in the example ' \ + 'scope' do + expect_no_offenses(<<~RUBY) + it 'updates the user' do + user = create(:user) + + expect { user.update(admin: true) }.to change(user, :updated_at) + end + RUBY + end + + it 'does not register an offense for two variables of same name in ' \ + 'different scopes' do + expect_no_offenses(<<~RUBY) + let(:my_user) do + user = create(:user) + user.flag! + user + end + + it 'updates the user' do + user = create(:user) + + expect { user.update(admin: true) }.to change(user, :updated_at) + end + RUBY + end + + it 'does not register an offense when variable is used as `it` argument' do + expect_no_offenses(<<~RUBY) + description = "updates the user" + + it description do + expect { user.update(admin: true) }.to change(user, :updated_at) + end + RUBY + end + + it 'does not register an offense when variable is used as part of `it` ' \ + 'argument' do + expect_no_offenses(<<~RUBY) + article = foo ? 'a' : 'the' + + it "updates \#{article} user" do + expect { user.update(admin: true) }.to change(user, :updated_at) + end + RUBY + end + + it 'does not register an offense when variable is used in string ' \ + 'interpolation for `it_behaves_like` argument' do + expect_no_offenses(<<~RUBY) + article = foo ? 'a' : 'the' + + it_behaves_like 'some example', "\#{article} user" + RUBY + end + + it 'does not register an offense when variable is used in symbol ' \ + 'interpolation for `it_behaves_like` argument' do + expect_no_offenses(<<~RUBY) + article = foo ? 'a' : 'the' + + it_behaves_like 'some example', :"\#{article}_user" + RUBY + end + + it 'does not register an offense when variable is used as first ' \ + '`it_behaves_like` argument' do + expect_no_offenses(<<~RUBY) + examples = foo ? 'definite article' : 'indefinite article' + + it_behaves_like examples + RUBY + end + + it 'does not register an offense when block argument is shadowed by local ' \ + 'variable' do + expect_no_offenses(<<~RUBY) + %i[user user2].each do |user| + let(user) do + user = create(:user) + user.flag! + user + end + end + RUBY + end + + it 'does not register an offense when outside variable is not referenced ' \ + 'in an example' do + expect_no_offenses(<<~RUBY) + user = create(:user) + user.flag! + + it 'does something' do + expect(foo).to eq(bar) + end + RUBY + end +end