Add new RSpec/ExpectInHook cop

[pocke]

This cop adds an offense for usage of expect in hooks such as before.

# bad
before  do
  expect(something).to eq 1
end

expect should be in it, not in before. But RSpec says nothing about this code.

[Darhazer]

This is a good cop to have. I've seen many times people writing expectations in a before or after hook

[Darhazer]

Not connected to the cop itself, but since rubocop-rspec discourages the usage of expect_any_instance_of, I would not add it in the good examples

[Darhazer]

In most cops the node_matchers are at the top

[Darhazer]

You should also detect expects with block. Maybe this can be moved to language, as many cops would be interested in detecting an expectation. @backus what do you think?

[backus]

Yeah it would be nice to centralize as much RSpec DSL traversal as possible but I would only ask @pocke to do so if it is in this PR if is an easy change.

[pocke]

I updated this pull-request.

Maybe this can be moved to language, as many cops would be interested in detecting an expectation.

done, but the language helper is used only by ExpectInHook cop.

[Darhazer]

Actually I've mislead you - you can check only for the send pattern and get directly the send node.

[backus]

Very nice! Will merge once the build is green 😄

[backus]

@pocke I invited you to my custom CI setup on buildkite. You should be able to view the builds there now. They are slower for the first build of the day but if you push frequently like me then you will benefit from the ~20 second build times

[RKushnir]

Could you provide some reasoning behind this statement?

expect should be in it, not in before

[bquorning]

The documentation says “Use before and after hooks to execute arbitrary code before and/or after the body of an example is run”.

I can’t remember having seen any documentation or guides recommending having expectations in the before/after hooks.

[Darhazer]

Moreover, if the expectation in the before hook fails, all examples will fail.

This also breaks the setup/run/verify arrangement, adding a verification in the setup phase

[RKushnir]

In my understanding of the word "arbitrary", the phrase "arbitrary code" means "expectations and anything else" 🙂

As for the reason why you might want to use an expectation in a before block — to make sure the test actually makes sense, to check some preconditions, that the test fixtures were built correctly. So it's kind of a meta-test, testing the test itself.

Also, current implementation produces a false positive for this code:

before do
  allow(User).to receive(:where) do |opts|
    expect(opts[:signed_up_at]).to be_a(Date)
    users
  end
end

[Darhazer]

IMO it's not a false positive, you have an expectation in the hook, although a conditional one
The above stub should be written as
allow(User).to receive(:where).with(hash_including(signed_up_at: an_instance_of(Date)).and_return(users)

[RKushnir]

I don't agree that it's "in the hook". The declaration is in the hook, but the execution is going to happen somewhere else.
I know it's a simple rule and can be rewritten like you have shown, but more complex rules can not.

[seanlinsley]

There are plenty of scenarios where you want to make expectations about code in a before block, and have that apply for each test. I'm disappointed to see this rule included, as it makes writing idiomatic tests for complex systems much more difficult.

[aspiers]

I think I agree with @seanlinsley. This PR description provides no justification for introducing the cop, it just says "expect should be in it, not in before" without explaining why that should be the case.

[pirj]

@aspiers Just like our parent project, RuboCop, rubocop-rspec provides tools to make your project's codebase consistent.

is extremely flexible and most aspects of its behavior can be tweaked via various configuration options.

We provide different styles, and could even potentially provide a style that would make the cop insist on putting all expectations to hooks.

We strive to align the default configuration with the Community RSpec style guide that says:

Be careful not to focus on being 'DRY' by moving repeated expectations into a shared environment too early, as this can lead to brittle tests that rely too much on one another.

In general, it is best to start with doing everything directly in your it blocks even if it is duplication and then refactor your tests after you have them working to be a little more DRY. However, keep in mind that duplication in test suites is NOT frowned upon, in fact it is preferred if it provides easier understanding and reading of a test.

No doubt there are different approaches to covering the same code with tests. It's up to you to adjust the configuration to meet your established code style.

If you feel that the default is off, please feel free to perform an analysis of specs of popular open-source projects that use RSpec. We're open to adjust the default if it turns out that expectations are quite often used in hooks. So does the community RSpec style guide.

I hope this provides you with an answer to "why".

[aspiers]

@pirj Many thanks for taking the time to share this excellent explanation. It would be great if that was more easily discoverable for other users who walk into this particular cop 🙏

[pirj]

It's a long-term misconception that our default configuration is a de-facto standard for writing specs:
#919
#887
#863

The Ruby Style guide says:

When we had to choose between a very established practice and a subjectively better alternative we’ve opted to recommend the established practice.

Unfortunately, the Community RSpec Style guide is missing such a disclaimer.

RSpec Core team is even more cautious in introducing an official style guide:

an official style guide could be used to silence discussion ... I want it to foster discussion due to discussing the pros/cons and various things

The gist is that you can write the code the way you see fit, the way that's best for you. While guidelines are a common ground for the majority of Ruby programmers, and tell the way of writing things in a certain way that doesn't come as surprise to anyone. It does not mean other ways are strictly prohibited if you know what you're doing and if you are comfortable to document the style guide amendments, train your colleagues to understand such style deviations, and be consistent about it.

@aspiers Would you like to send a pull request with an update to README.md that would clarify it for everyone how rubocop-rspec should be used?
The main points would be:

• we strive to harmonize our default configuration with the community style guide
• it's not set in stone
• we look at the commonly established style to pick defaults
• given proper proof, we're open to change the defaults
• it's up to the users to configure rubocop-rspec to meet their project's established style