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
Merged

Improve RSpec/SubjectStub cop #770

merged 10 commits into from Jun 10, 2019

Conversation

@pirj
Copy link
Member

@pirj pirj 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).
lib/rubocop/cop/rspec/subject_stub.rb Outdated Show resolved Hide resolved
Loading
lib/rubocop/cop/rspec/subject_stub.rb Show resolved Hide resolved
Loading
@bquorning
Copy link
Collaborator

@bquorning bquorning 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.

Loading

@pirj
Copy link
Member Author

@pirj pirj 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?

Loading

@pirj
Copy link
Member Author

@pirj pirj 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?

Loading

bbatsov added a commit to rubocop/rubocop that referenced this issue May 22, 2019
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/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
Copy link
Member Author

@pirj pirj 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?

Loading

pirj added 9 commits May 22, 2019
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.
Addresses concerns expressed in #705
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.
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
Copy link
Member Author

@pirj pirj commented May 27, 2019

Is there anything else how I can improve this?

Loading

@pirj
Copy link
Member Author

@pirj pirj 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?

Loading

@pirj
Copy link
Member Author

@pirj pirj commented Jun 3, 2019

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

Loading

@bquorning
Copy link
Collaborator

@bquorning bquorning 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.

Loading

@pirj
Copy link
Member Author

@pirj pirj commented Jun 3, 2019

No worries, please take your time.

Loading

bquorning
Copy link
Collaborator

bquorning commented on 20cd88a Jun 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@pirj
Copy link
Member Author

@pirj pirj 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.

Loading

@pirj
Copy link
Member Author

@pirj pirj 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.

Loading

@pirj
Copy link
Member Author

@pirj pirj commented Jun 10, 2019

Fixed.

Loading

Copy link
Collaborator

@bquorning bquorning left a comment

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

Loading

@bquorning bquorning merged commit 9e90461 into rubocop:master Jun 10, 2019
10 of 11 checks passed
Loading
@bquorning
Copy link
Collaborator

@bquorning bquorning commented Jun 10, 2019

Thank you for the contributions, persistence, and patience 😄

Loading

@pirj
Copy link
Member Author

@pirj pirj commented Jun 12, 2019

Thanks for accepting @bquorning, @Darhazer!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants