Capybara/FeatureMethods identifies given calls from other DSLs.

[ccutrer]

I have a spec that is exercising a DSL of its own:

    let(:actor_class) do
      Class.new do
        extend AdheresToPolicy::ClassMethods

        set_policy do
          given { |actor| actor == "allowed actor" || actor.is_a?(user_class) }
          can :read

          given { |actor| actor == "allowed actor" }
          can :read
        end
      end
    end

and yet I'm getting RSpec/Capybara/FeatureMethods: Use let instead of given.

It seems like this cop should only look for given calls immediately inside of describe, context, etc. blocks, and not in arbitrary code. At the least not inside of a let itself.

rubocop -V:

1.22.0 (using Parser 3.0.2.0, rubocop-ast 1.12.0, running on ruby 2.7.2 x86_64-darwin20)
  - rubocop-performance 1.11.5
  - rubocop-rails 2.12.4
  - rubocop-rake 0.6.0
  - rubocop-rspec 2.5.0

[pirj]

Would it make sense to configure this cop to only include feature/system spec paths. What do you think?

this cop should only look for given calls immediately inside of describe, context, etc. blocks

Not really. This would limit its inspection capabilities to the very basic spec structures.
If I'm not mistaken, I've seen lets defined inside lets. Usually, it's done mistakenly, but who knows. I encourage you to try it out to see if RSpec allows for that.

[ccutrer]

While syntactically you can nest lets, at runtime, you get an error:

describe "something" do
    let(:abc) do
      let(:de) do
        1
      end
      2
    end

    it "works" do
      puts abc
      puts de
  end
end

produces:

     Failure/Error:
       let(:de) do
         1
       end
     
       `let` is not available from within an example (e.g. an `it` block) or from constructs that run in the scope of an example (e.g. `before`, `let`, etc). It is only available on an example group (e.g. a `describe` or `context` block).

So only "the very basic spec structures" are where let or given are allowed.

[pirj]

Please accept my apologies. That must have been before inside before, not let.

Alright, it's an edge case, but worth and possible to fix.
I suggest adding a check that Capybara speak is not inside a let/hook somewhere here

rubocop-rspec/lib/rubocop/cop/rspec/capybara/feature_methods.rb

Line 81 in 8821470

next if enabled?(match)

or here

rubocop-rspec/lib/rubocop/cop/rspec/capybara/feature_methods.rb

Line 99 in 8821470

root = node.ancestors.find { |parent| root_node?(parent) }

.

Would you like to send a PR?

[luke-hill]

Whilst this is a bit of a minefield. I'm also finding huge amounts of offenses in .... (wait for it), the internal testing suites for cucumber which doesn't lean too heavily on capybara.

In it we have helpers that write things like feature 'abc' which is actual generating a cucumber internal doc.

A real world example is here: https://github.com/cucumber/cucumber-ruby-core/blob/main/spec/cucumber/core/gherkin/parser_spec.rb#L139-L153

context 'when there are multiple rules and scenarios or examples' do
          source do
            feature do
              rule description: 'First rule' do
                scenario name: 'Do not talk about the fight club' do
                  step 'text'
                end
              end
              rule description: 'Second rule' do
                example name: 'Do not talk about the fight club' do
                  step 'text'
                end
              end
            end
          end

Not sure if it's relevant to be called out, or we should just prefix all our internal tests with maybe an underscore or something similar, so it's cucumber_feature 'abc' instead of feature 'abc'. However this makes the rubocop warden rather "ban happy" and flagging lots of items.

[pirj]

Quite an interesting case, @luke-hill
Wondering how ‘source do’ works?
In RuboCop’s own specs we use heredocs. But I have no certainty that Cucumber actually parses those, probably just evals the block with some sandboxed binding?

Are they even interested in linting their inner code under test? The spec looks like a regular RSpec DSL with its describe/context/it structure.
Does it make sense to just disable Capybara cops there?
I see why they would deliberately omit eg docstrings from example group descriptions, for brevity and not to steal focus from the very topic of a spec.

[pirj]

Sorry for the noise from transferring back and forth, it slipped my mind some Capybara cops remain here.

Speaking of the issue itself.
We have a check ‘return unless inside_example_group?(node)’, but it doesn’t seem to work when e.g ‘given’ is inside a ‘let’.

I would suggest changing this to ‘directly_under_example_group?’. To my best memory, we did this for some other cops already.

A PR is welcome.

[luke-hill]

If it's something that will be fixed in the short-medium term I'm happy to wait. I have 1000 other things to fix up in cucumber.

As for the why, the scenario/feature e.t.c. are generating the pickles for just that. a scenario or feature.

[luke-hill]

Apologies for coming in with more bad news / incorrections. But it seems RSpec/NestedGroups also flags this incorrectly.

Tidying up more things today and saw this:

spec/cucumber/core_spec.rb: RSpec/NestedGroups: Maximum example group nesting exceeded [4/3]. (https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/NestedGroups)
          feature do
          ^^^^^^^

Wider context

context 'without hooks' do
      let(:gherkin_document) do
        gherkin do
          feature 'Feature name' do
            scenario 'The one that passes' do
              step 'passing'
            end

            scenario 'The one that fails' do
              step 'passing'
              step 'failing'
              step 'passing'
              step 'undefined'
            end
          end
        end
      end