RSpec/Dialect causes a false negative when using below setting mentioned in the documentation

[sanfrecce-osaka]

RSpec/Dialect causes a false negative when using below setting mentioned in the documentation.

RSpec/Dialect:
  PreferredMethods:
    background: :before
    scenario:   :it
    xscenario:  :xit
    given:      :let
    given!:     :let!
    feature:    :describe

cf. https://docs.rubocop.org/rubocop-rspec/cops_rspec.html#rspecdialect

Expected behavior

The following code should not be passed:

background do # 👮‍♂️ C: [Correctable] RSpec/Dialect: Prefer before over background.
  # some setup codes
end

Actual behavior

The following code is passed:

background do
  # some setup codes
end

In contrast, the following codes doesn't pass.

RSpec.feature do # 👮‍♂️ C: [Correctable] RSpec/Dialect: Prefer describe over feature.
  # some codes
end

scenario do # 👮‍♂️ C: [Correctable] RSpec/Dialect: Prefer it over scenario.
  # some codes
end

xscenario do # 👮‍♂️ C: [Correctable] RSpec/Dialect: Prefer xit over xscenario.
  # some codes
end

I look like rspec_method? returns nil when node.method_name is background.

https://github.com/rubocop/rubocop-rspec/blob/e02576f1a8c39d9dc5b3ca37fea4596f7e75f493/lib/rubocop/cop/rspec/dialect.rb#L67-L80

RuboCop version

$ bundle exec rubocop -V
1.65.1 (using Parser 3.3.4.2, rubocop-ast 1.32.0, running on ruby 3.2.3) +server [arm64-darwin21]
  - rubocop-capybara 2.21.0
  - rubocop-factory_bot 2.26.1
  - rubocop-performance 1.21.1
  - rubocop-rails 2.25.1
  - rubocop-rspec 3.0.4
  - rubocop-rspec_rails 2.30.0

[pirj]

Thanks for reporting!

The unless rspec_method?(node) seems incorrect. The spec didn’t catch that because it only checked an RSpec basic method.

Would you like to send a PR to fix this?

[sanfrecce-osaka]

Thank you for your response.

Will you send me a PR to resolve this issue?

Yes, I would. I was reading the code, and found that it is due to the lack of background on https://github.com/rubocop/rubocop-rspec/blob/v3.0.4/config/default.yml#L71-L81.

[pirj]

This cop should handle any source DEL without needing to configure it in the Language section. You just tell the cop to change background to describe, and that’s it.

So for the PR, I suggest just removing the line that checks if it’s a rspec_method?. This has a side effect that it will detect methods outside of specs, so I suggest adding a inside_example_group? check, we should have similar helpers somewhere. I can lend a hand if you get stuck.

[sanfrecce-osaka]

So for the PR, I suggest just removing the line that checks if it’s a rspec_method?. This has a side effect that it will detect methods outside of specs, so I suggest adding a inside_example_group? check, we should have similar helpers somewhere.

Thanks for the suggestion. The check with inside_example_group? alone will give false positives in the following cases.

RSpec.describe 'context' do
  it 'tests common context invocations' do
    expect(request.context).to be_empty? # 👮‍♂️ C: [Correctable] RSpec/Dialect: Prefer describe over context.
  end
end

So how about changing it to the following?

        class Dialect < Base
          extend AutoCorrector
          include MethodPreference
+         include InsideExampleGroup

          MSG = 'Prefer `%<prefer>s` over `%<current>s`.'

          # @!method rspec_method?(node)
-         def_node_matcher :rspec_method?, '(send #rspec? #ALL.all ...)'
+         def_node_matcher :rspec_method?, '(send #rspec? ...)'

          def on_send(node)
+           return unless inside_example_group?(node)
            return unless rspec_method?(node)
            return unless preferred_methods[node.method_name]

            msg = format(MSG, prefer: preferred_method(node.method_name),
                              current: node.method_name)

            add_offense(node, message: msg) do |corrector|
              current = node.loc.selector
              preferred = preferred_method(current.source)

              corrector.replace(current, preferred)
            end
          end
        end

Also, inside_example_group? seems to return true if methods matches #SharedGroups.all or #ExampleGroups.all, even outside of specs.

describe 'display name presence' do # `inside_example_group?` returns true
end

before do # `inside_example_group?` returns false
end

RSpec.describe 'context' do
  before do # `inside_example_group?` returns true
  end

  describe 'display name presence' do # `inside_example_group?` returns true
  end
end

Does this behaviour need fixing?

[pirj]

send #rspec? ... is nearly a match-any pattern, as it would match any method call without a receiver. Since we already in an “on send” hook, this is mostly redundant. The only case that it would filter out are calls with an explicit receiver where the receiver is not RSpec, eg foo.describe.

[pirj]

I think the inside_example_group? is fine because it returns true for the top-level example group like yours, and this will trigger for the top-level background, exactly what we need! Not everyone have turned on the non-monkey-patching mode yet, so a too-level background is a valid syntax (though legacy and deprecated, but it’s not an area where Dialect should care). In the future, if we detect RSpec 4, we would not detect top-level example groups without an explicit RSpec as the receiver.

[sanfrecce-osaka]

Thank you for your feedback. What about https://github.com/rubocop/rubocop-rspec/pull/1952?