-
-
Notifications
You must be signed in to change notification settings - Fork 272
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
Don’t stub when you’re mocking #226
Conversation
1cbad46
to
ce9cd8d
Compare
@bquorning I played with the branch locally to get the highlighting you are interested in. This should work: diff --git i/lib/rubocop/cop/rspec/mock_not_stub.rb w/lib/rubocop/cop/rspec/mock_not_stub.rb
index 286df07..aba2985 100644
--- i/lib/rubocop/cop/rspec/mock_not_stub.rb
+++ w/lib/rubocop/cop/rspec/mock_not_stub.rb
@@ -27,8 +27,8 @@ module RuboCop
(send
(send nil :expect ...) :to
{
- (send #receive :and_return $_)
- (block #receive (args) $_)
+ $(send #receive :and_return _)
+ $(block #receive (args) _)
}
)
PATTERN
@@ -42,7 +42,24 @@ module RuboCop
def on_send(node)
message_expectation_with_return_block(node) do |match|
- add_offense(match, :expression)
+ source_map = match.loc
+
+ offending_range =
+ if match.send_type?
+ Parser::Source::Range.new(
+ source_map.expression.source_buffer,
+ source_map.selector.begin_pos,
+ source_map.end.end_pos
+ )
+ else
+ Parser::Source::Range.new(
+ source_map.expression.source_buffer,
+ source_map.begin.begin_pos,
+ source_map.end.end_pos
+ )
+ end
+
+ add_offense(match, offending_range)
end
end
end I'd refactor it a bit of course to make it not just one big method. |
Nice job with the node pattern stuff btw. I wish people would use it more on rubocop proper |
Thanks for the code snippet there, @backus. I think I’ll use |
Should this cop be configurable? It seems like |
Configurable? You can just disable it. Perhaps I misunderstand you? |
No I guess I was curious if there was an alternate style that this sort of thing could also enforce if we're just saying "do not use this part of the RSpec API." I look forward to hearing outside input for @jcoyne. I'm wondering how users will respond to this cop. |
Should this cop also reject things like |
Yes, good point. All three cases should be policed by this cop. |
Also |
Can you say why this is good style? What's the justification? |
The argumentation / justification is in #221. |
It sounds like you want to change the rspec-mocks API via rubocop. Have you considered this change as a deprecation in rspec-mocks? |
Yes :-) I thought it would be easier to start here. I’ve also considered mentioning someone from rspec-mocks in this PR. |
@bquorning I think that would be a good idea. If you want to influence the usage of a library, it makes sense to consult the authors thereof. |
After a brief discussion in rspec/rspec-mocks#1123 I’m convinced that rspec-mocks will not change its DSL. Which makes this a personal style preference – perhaps suitable for RuboCop-RSpec? |
I think seeing myron call this a code smell is more than enough to convince me that this is worth making a cop |
I'll try to do another review tonight |
Just a personal opinion (which may or may not be shared with my other rspec team members) but this cop should be disabled by default it's very much a personal opinion that creates extra code for people, and rubocop tends to give the impression that something is "recommended best practise" which this isn't. |
@JonRowe thanks I'll keep that in mind. If you have a minute, would you be able to look at the other cops and say whether you think this is true for the others? I'm curious how broad you view things as style vs. best practice. On an unrelated note, I'm not sure how much rubocop is "recommended best practice" anymore given that most cops with more than one possible style tend to be configurable in different ways. |
I see people using the default config a fair bit, and just accepting them as "the way" |
5ae0d4f
to
3ab2326
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
I kind of lean with the disable-by-default crowd here. This feels a little obscure/confusing to me, though @bquorning motivating example in the linked issue seems reasonable and I would probably write the specs the same way he would. I strongly suspect we'll get very confused users asking what we mean by this cop though and why it's better to do it another way, so my intuition is that it's a little too surgical for the average user. If this gets merged and enabled by default, the explanation/motivation should probably be documented more clearly, imo. |
67018b7
to
486ce5a
Compare
|
486ce5a
to
9cccce4
Compare
Even though it's your own pull request, I'd love to hear your thoughts on the recent changes before merging it @bquorning |
8915b31
to
7906f23
Compare
|
||
private | ||
|
||
def on_expectation(expectation, method_name, matcher) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I’m having a bad day, but I have a really hard time understanding this code. It’s not immediately obvious what the node matchers actually match. The names are easy enough to understand (maybe except matcher_with_blockpass_or_hash
which I’m not sure I get), but… Maybe we should start adding adding comments to our node matchers, with one or two examples of what the would match?
It can also be that I lack training in reading node patterns (and parser
output). I wonder how others feel about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fresh view on the cop.
It's quite common to miss the complexity when working on it, but as time goes, this complexity becomes apparent.
In addition to node matcher YARD docs that I believe now help a lot to understand RSpec/EmptyExampleGroup
's node patterns, there's also Node pattern comment feature.
I intend to add both to this cop's node patterns.
Absolutely. Sorry for sometimes taking several days before responding; it feels like there are fewer hours in a day than there used to be 😕 Perhaps the spec for |
7906f23
to
fb9ea45
Compare
Co-authored-by: Phil Pirozhkov <hello@fili.pp.ru>
fb9ea45
to
ee4f9fd
Compare
def_node_matcher :matcher_with_blockpass, <<~PATTERN | ||
{ | ||
(send nil? { :receive :receive_message_chain } ... block_pass) # receive(:foo, &canned) | ||
(send (send nil? :receive ...) :with ... block_pass) # receive(:foo).with('foo', &canned) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's kind of odd that LineLength
ignores HEREDOC
def_node_matcher :matcher_with_blockpass, <<~PATTERN | ||
{ | ||
(send nil? { :receive :receive_message_chain } ... block_pass) # receive(:foo, &canned) | ||
(send (send nil? :receive ...) :with ... block_pass) # receive(:foo).with('foo', &canned) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if you guys find that kind of node pattern comments helpful @bquorning @Darhazer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found them very useful, thank you @pirj
# bad
expect(foo).to receive(:bar).with(42).and_return("hello world")
# good (without spies)
allow(foo).to receive(:bar).with(42).and_return("hello world")
expect(foo).to receive(:bar).with(42) I strongly disagree that an expressive and DRY form should be broken into two repetitive forms for no benefit. I would support a cop that does the reverse. I really dislike when cops complain about valid code, especially when the solution is not clearly better (i.e. clearly worse for many, me included). Of course, I fully realize that yes, we can disable it. That mere fact does not imply that the cop should be enabled by default. I believe having cops like these, enabled by default, give RuboCop a bad name. Note: I'm sorry, I didn't read the whole thread as it contains comments on the code as well as discussion about the usefulness of the cop itself. I'll just comment on this:
He did not specifically call this a code smell (and I would disagree with him if he did) |
There has been a lot of discussion for and against whether this cop should be enabled by default. I am actually not sure if we really made any decision either way. @pirj I remember you revived this PR in #226 (comment), but I do not recall if we really decided to let this cop be enabled or disabled by default. I think it just … happened. |
Maybe disabling it and issuing a 2.0.1 release is the way to go then. |
Let's take a closer look: context 'when team external_dependency_checksum changes' do
it 'is invalid' do
cache.save(offenses)
expect(team).to(
receive(:external_dependency_checksum).and_return('bar')
)
cache2 = described_class.new(
file, team, options, config_store, cache_root
)
expect(cache2.valid?).to eq(false)
end
end If I would be reading this, I would ask myself:
we didn't limit ourselves to set the input and rather decided to increase specs' brittleness by tying ourselves to the internal implementation? You may also notice that: before do
#...
allow(team).to receive(:external_dependency_checksum).and_return('foo')` This means we're double-stubbing
warning, but we don't, not sure why exactly, probably because it's only implemented for pure doubles and doesn't work on partial doubles, or warnings are muted somehow. The fact that this spec works doesn't mean it's good. I can lend you a hand with fixing it and turning I don't have other specs on hand to reason about, but I'm all ears to know when |
The first
The spec assumes an implementation detail. If that implementation detail, the spec needs to be changed. That's why we stub only when there is no better alternative. I see the spec failing as a virtue. Other remarks in rubocop/rubocop#9006 |
Just for the record, I take back my argument that double-stubbing is an offence, it's my false belief. |
Fixes #221.
TODO
and_raise
,and_yield
,and_throw
,and_call_original
, andand_wrap_original
.cc @backus
If you have created a new cop:
config/default.yml
.VersionAdded
indefault/config.yml
to the next minor version.