Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to `receive` message expectation syntax #7067

Merged

Conversation

@pirj
Copy link
Member

commented May 21, 2019

I started off improving RSpec/SubjectStub cop that had a special case of dealing with expect(subject).to all receive(...) that was originally present in RuboCop, and I've removed this special case in this commit, since according to an experiment, it's not raising any single offence for repositories from real-world-ruby-apps (11k files) and real-world-rails (79k files).

Original line when a special case was introduced:

expect(formatter_set).to all(receive(:started).with(files))

later reworked to:

expect(formatter_set).to all(have_received(:started).with(files))

I've replaced this with:

expect(formatter_set[0]).to receive(:started).with(files)
expect(formatter_set[1]).to receive(:started).with(files)

that is quite consistent with the style used in the rest of that spec file.

Also, using spies is not justified in this case.

It doesn't make much sense to use a before hook that is only run along with a single example, so I've simplified it as well.

However, I've faced another cop, RSpec/MessageSpies, that complains that receive is used instead of have_received. What confuses me is that according to the chat from 10 Oct 2016:

If a spec example has multiple stubs (allow) and you want a focused spec with just one expectation per example – and RuboCop is now telling you that you must change all those stubs into expectations. Not good.

However, in rubocop this is not a problem, as multiple expectations per example are allowed:

RSpec/MultipleExpectations:
  Max: 25

So I decided to give it a shot and take a look what happens if I switch the default enforced style of RSpec/MessageSpies from have_received to receive, and this pull request is the result of this switch.

Why it makes sense to merge this?

  1. rubocop-rspec's SubjectStub has a special case that deals with a single known example, here, in rubocop's formatter_set_spec.rb.
  2. Less hooks are used, the specs are more straightforward.
  3. Spies have to be additionally instantiated, but now anymore.
  4. Base formatter spec is now "Runner formatter invocation spec", which reflects what it tests.

Why it wouldn't make sense to merge this?

  1. Usage of expect(...).to receive breaks Arrange-Act-Assert pattern.

What is redundant

I've made a couple of changes along the way not directly related to this change, specifically changed receive { ... } to .and_return. Don't remember the details, but I think it's recommended to use this syntax.

Appreciate any feedback on this.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • [-] Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • [-] Added tests.
  • [-] Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

pirj added some commits May 21, 2019

Simplify spec for FormatterSet
This line:

    expect(formatter_set).to all(have_received(:started).with(files))

previously was

    expect(formatter_set).to all(receive(:started).with(files))

but was triggering a RSpec/SubjectStub offence, and there was a special
case treatment introduced to `rubocop-rspec` to relax detection of
stubbing when `all` matcher is used together with a message expectation
matcher.

The cop is not at the moment able to detect `have_received` matcher, but
there is an open pull request to improve the detection.

That pull request (rubocop-hq/rubocop-rspec#770)
is also aiming to remove that special case treatment, because, as
experiment has shown, across
[`real-world-ruby-apps`](https://github.com/jeromedalbert/real-world-ruby-apps)
and
[`real-world-rails`](https://github.com/eliotsykes/real-world-rails),
there is no other offences detected apart from this `FormatterSet` spec.

In any case, the rest of the spec don't use `all`, and are explicit of
which object the expectation is made against (by using `[0]`, `[1]`).

Also, using spies is not justified in this case.

It doesn't make much sense to use a `before` hook that is only run along
with a single example.
Switch to `receive` message expectation syntax
Base formatter spec was actually testing how the runner is calling the
formatter, so the subject of it is different.

@pirj pirj force-pushed the pirj:prefer-receive-message-spies-enforced-style branch from e6ed625 to b159838 May 21, 2019

@bbatsov bbatsov merged commit cd95386 into rubocop-hq:master May 22, 2019

22 checks passed

ci/circleci: cc-setup Your tests passed on CircleCI!
Details
ci/circleci: cc-upload-coverage Your tests passed on CircleCI!
Details
ci/circleci: documentation-checks Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-rubocop Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-spec Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented May 22, 2019

Nicely done! Thanks!

@pirj pirj deleted the pirj:prefer-receive-message-spies-enforced-style branch May 22, 2019

@bquorning

This comment has been minimized.

Copy link
Member

commented May 28, 2019

😢

I find the main benefit of using RSpec/MessageSpies is that the specs can adhere to the four-phase test pattern, specifically that the “verify” step is after “exercise”.

After this PR, the expectations are sometimes at the bottom of a test example, sometimes at the top, and sometimes a bit of both.

@pirj

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

@bquorning I had doubts about this change as well, but looking at the changes one by one I was more inclined to do this change rather than keep things as is:

  1. This is an indicative example of a case when message expectations are used together with value comparison expectations.
-     allow(File).to receive(:open).and_call_original
+     expect(File).not_to receive(:open).with(%r{/ignored/})
      expect(cli.run(%w[--format simple example])).to eq(0)
      expect($stdout.string).to eq(<<~OUTPUT)
        0 files inspected, no offenses detected
      OUTPUT
-     expect(File).not_to have_received(:open).with(%r{/ignored/})

There was (and still is) no clear Act/Excercise stage, it was mixed with value comparison expectation in a single statement.

The combination of allow...to and expect...not_to is a bit confusing. It's a technical construct, but it reads quite poorly.

A necessity to and_call_original is not clear, especially keeping in mind the not_to expectation.

  1. Another example when allow.to .and_call_original is used with expect.not_to.

There is no Arrange/Setup phase.

To avoid incidental state (more like behaviour than a state), a block style expectation could be used, e.g. send_message from saharspec.

It reads better, a hook is not used.

Also, I would disagree that setting up a spy counts towards the Setup/Arrange phase.

  1. Swapping.

With swapping two parts of the example, e.g.

when I load the configuration, it's being loaded from a file
vs
configuration is loaded from a file when I load it

Both sentences read quite similarly, and I hope the examples as well.

  1. Overstubbing

Verified doubles are not used, incidental behaviour might strike, since there are at least two statements in Act/Excercise phase, parse_source and commissioner.investigate.

  1. Scattered expectations.

Can't disagree with this. Found two cases in this pull request, 1 (mentioned earlier and 2. I can imagine an example where a number of message expectations are sent, and then the results are compared to the reference expected values.
Although, the first example had more downsides than just that.

Hiding the Act/Excercise phase in a hook is a bad practice, and this pull request changes this by making a call to run visible in the example itself.

I really respect AAA or 4PT, but the usages of message spies that were changed in this specific pull request did not look like a canonical example of it.

Went through the changes once again. I might have a bias for this since my goal was to get rid of message spy without breaking ExampleSize and MessageSpies. I might have made a really bad change, appreciate if you point at the worst one. I'm all for improving it in a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.