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

Improve RSpec/SubjectStub cop #770

Merged
merged 10 commits into from Jun 10, 2019

Conversation

@pirj
Copy link
Member

commented May 15, 2019

1. Use consistent wording in docstrings

2. Improve stub detection

Previously, the following code would still be detected as an offence:

allow(subject)

even though there are no message expectations.

3. Add detection of negated runners (not_to, to_not) - fixes #705

4. Add support for chain and multiple stub detection (receive_messages, receive_message_chain)

5. Add more reasoning references

6. Improve offence message

7. Remove special case all matcher (see #488)

This special case treatment was added to address one use case, of an object that acts as a container for and a delegator of method calls to containing items by inheriting from an Array.

The original code that suffered from a false detection was:

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

However, it has been reworked to:

  formatter_set.each do |formatter|
    allow(formatter).to receive(:started)
  end
  formatter_set.started(files)
  expect(formatter_set).to all(have_received(:started).with(files))

and can be further simplified to:

formatter_set.each do |formatter|
  expect(formatter).to receive(:started).with(files)
end
formatter_set.started(files)

which in case of this specific object design is more indicative of what happens.

8. Added support for have_received detection, to detect the bad code above.

9. Fixed detection of the unnamed subject stubbing when the subject is named.

Fixes #705


Before submitting the PR make sure the following are checked:

  • 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.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).
@bquorning

This comment has been minimized.

Copy link
Member

commented May 20, 2019

I’m not sure I understand the case for removing the all matcher. Yes, it was implemented after just a single bug report, but that doesn’t mean there are not more examples out there. I assume – since RSpec allows it – that they are valid use cases.

Or is the code removed unnecessarily complex?

I just need a bit more clarification on why we should remove this feature.

@pirj

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

I'm sorry, but I disagree that if RSpec allows something, it's a valid (correct) use case. rubocop-rspec wouldn't have existed then. Did I understand your statement correctly at all?

As an experiment, I've run a modified version of this cop that only detects expect(subject).to all receive(...) (along with not_to, have_received etc) against real-world-ruby-apps (11k files) and real-world-rails (79k files).
The result is that there was no single offence for the cop.

PS Analysis of the latest rubocop (real-world-ruby-apps seem to point to an older version):

spec/rubocop/formatter/formatter_set_spec.rb:27:7: C: RSpec/SubjectStub: Do not stub methods of the object under test.
      expect(formatter_set).to all(have_received(:started).with(files))
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I don't have a strong opinion if this special all case should be removed or kept.

What do you think?

@pirj

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

Tried simplifying that spec on RuboCop side, and it turns out another cop, RSpec/MessageSpies, starts complaining 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

I'll try my chances with changing the enforced style for MessageSpies to receive, and fixing that lonely example.

Would you accept this pull request with the removal of all special case if those two are accepted?

pirj added a commit to pirj/rubocop that referenced this pull request 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(have_received(: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.

pirj added a commit to pirj/rubocop that referenced this pull request 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.
@pirj pirj referenced this pull request May 21, 2019
5 of 5 tasks complete

bbatsov added a commit to rubocop-hq/rubocop that referenced this pull request May 22, 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.
@pirj

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

One last known use case for expect(...).to all receive has been fixed.

I'll be more than happy to bring all exclusion back in case someone complains about it, please feel free to assign the ticket to me when this comes up.

Is there anything else left to be addressed in this pull request?

pirj added some commits May 15, 2019

Improve stub detection
Previously, the following code would still be detected as an offence:

  allow(subject)

even though there are no message expectations.

Also, `is_expected` was not supported, e.g.:

  is_expected.to receive(:bar)

was not flagged.
Improve offence message for SubjectStub
Addresses concerns expressed in #705
Remove special case `all` matcher from SubjectStub
This special case treatment was added to address one use case,
of an object that acts as a container for and a delegator of
method calls to containing items by inheriting from an `Array`.

The original code that suffered from a false detection was:

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

However, it has been [reworked to](https://github.com/rubocop-hq/rubocop/blob/a0aaa233c20e18784d841212e1caa724bcc919f7/spec/rubocop/formatter/formatter_set_spec.rb#18):

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

and all known cases for this specific offence case are gone.
Add spy stubbing in SubjectStub
One specific example of the code is
https://github.com/rubocop-hq/rubocop/blob/70ae6ca7978770ea699a2809f57ca2a6bdeec6be/spec/rubocop/formatter/formatter_set_spec.rb#L17

  formatter_set.each do |formatter|
    allow(formatter).to receive(:started)
  end
  formatter_set.started(files)
  expect(formatter_set).to all(have_received(:started).with(files))

that could be more concisely be expressed as:

  formatter_set.each do |formatter|
    expect(formatter).to receive(:started).with(files)
  end
  formatter_set.started(files)
@pirj

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

Is there anything else how I can improve this?

@pirj

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

@bquorning @Darhazer I've sent a pull request that would quickly restore the all special case.

In case of emergency, it can be immediately merged.

Do you have some other doubts regarding the changes made in this pull request?

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

@bquorning @Darhazer Is there anything else that needs to be addressed in this pull request?

@bquorning

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Hi @pirj, apologies for the slow response rate around here. Your PR is in my review queue, but I just haven’t had time to get a good look at it yet.

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

No worries, please take your time.

@bquorning

This comment has been minimized.

Copy link
Member

commented on lib/rubocop/cop/rspec/subject_stub.rb in 20cd88a Jun 9, 2019

I wonder if we need a Language module for { :receive :receive_messages :receive_message_chain }?

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

@bquorning I was considering this, but haven't found any other cop that would benefit from it immediately. Probably there are some missed cases due to this in the other cops, but I can't tell that off the top of my head.

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

Please don't merge this just now, @Darhazer has found a case this cop is missing, flagging of an unnamed subject (expect(subject).to receive). Will address this quickly.

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

Fixed.

@bquorning
Copy link
Member

left a comment

I expect the failing circleci job (codeclimate) to pass on master.

@bquorning bquorning merged commit 9e90461 into rubocop-hq:master Jun 10, 2019

10 of 11 checks passed

ci/circleci: code-climate Your tests failed on CircleCI
Details
ci/circleci: confirm_config_and_documentation Your tests passed on CircleCI!
Details
ci/circleci: jruby Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-rspec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-rspec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-rspec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-rspec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-rubocop Your tests passed on CircleCI!
Details
@bquorning

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Thank you for the contributions, persistence, and patience 😄

@pirj

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Thanks for accepting @bquorning, @Darhazer!

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.