Add new RSpec/MissingExpectationTargetMethod cop
#1893
Conversation
| def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler | ||
| return unless expect_block?(node) | ||
|
|
||
| check_expect(node) |
There was a problem hiding this comment.
Can you please add some coverage for this? Do we even need on_block?
There was a problem hiding this comment.
on_block is defined to support the expect with block. The test is below.
https://github.com/rubocop/rubocop-rspec/pull/1893/files#diff-5e17efe314d6a76ce15c21be790197c8d4958d545fbb923dc78881b4172f2a3fR36
This was implemented with reference to void_expect.rb.
If I check the node type of the parent, it looks like only on_send would be fine, is that better?
The following is an image of the implementation using only on_send.
def on_send(node)
return unless expect?(node)
if node.parent&.block_type?
check_expect(node.parent)
else
check_expect(node)
end
endThere was a problem hiding this comment.
Thanks for the explanation, it makes total sense to check the block syntax, too.
But it seems that with no regards to the presence or absence of the block it can be detected in on_send, can’t it?
There was a problem hiding this comment.
I removed on_block because I could detect it only with on_send.
| PATTERN | ||
|
|
||
| # @!method expectation_target_method?(node) | ||
| def_node_matcher :expectation_target_method?, <<~PATTERN |
There was a problem hiding this comment.
| def_node_matcher :expectation_target_method?, <<~PATTERN | |
| def_node_matcher :expectation_without_runner?, <<~PATTERN | |
| (send #expect? !#Runners.all …) |
Should be able to detect expects what its name tells.
There was a problem hiding this comment.
Thank you for the suggestion. That looks better, so I will fix it.
|
Nice! This covers a lot of real life cases. |
When I ran it against real-world-rspec, I noticed that it was using |
|
Could you squashed all commits into one? |
krororo
force-pushed
the
add-expectation_target_method
branch
from
eee9e69
to
fd88583
Compare
|
OK. I squashed commits and force push. |
|
I would rather not tolerate send/public_send, because:
|
| module RuboCop | ||
| module Cop | ||
| module RSpec | ||
| # Checks ExpectationTarget method. |
There was a problem hiding this comment.
imo) The word “Checks” seems to be unclear as to whether it is or is not an offense as a result of the check, although this is a formality that exists in many other cops. I think it would be better if it is explained when it is an offense, or what kind of format is recommended by the cop.
There was a problem hiding this comment.
I have come up with the following explanation, what do you think of this explanation?
Checks if the ExpectationTarget methods `.to`, `not_to` or `to_not` are used.
There was a problem hiding this comment.
How about we change the following?
Checks if the RSpec::Expectations::ExpectationTarget methods `.to`, `not_to` or `to_not` are used.
There was a problem hiding this comment.
Thanks for the suggestion! I tried the fix but had problems 😢
First, I tried to write it in one line, but was registered an offense by Layout/LineLength.
lib/rubocop/cop/rspec/missing_expectation_target_method.rb:6:81: C: Layout/LineLength: Line is too long. [106/80]
# Checks if the RSpec::Expectations::ExpectationTarget methods `.to`, `not_to` or `to_not` are used.
^^^^^^^^^^^^^^^^^^^^^^^^^^
Next, the test failed when I included newlines.
1) config/default.yml does not have newlines in cop descriptions
Failure/Error: expect(value).not_to include("\n")
expected "Checks if the RSpec::Expectations::ExpectationTarget methods `.to`,\n`not_to` or `to_not` are used." not to include "\n"
# ./spec/project/default_config_spec.rb:80:in `block (3 levels) in <top (required)>'
# ./spec/project/default_config_spec.rb:79:in `each'
# ./spec/project/default_config_spec.rb:79:in `block (2 levels) in <top (required)>'
In the build_config task, the first line of the document is set to description in default.yml, but it seems that newlines are not allowed.
I tried to simplify the first line and write the details separately as follows.
Checks if `.to`, `not_to` or `to_not` are used.
The RSpec::Expectations::ExpectationTarget must use `to`, `not_to` or
`to_not` to run. Therefore, this cop checks if other methods are used.
fd88583
to
8c814be
Compare
I have considered the name |
krororo
force-pushed
the
add-expectation_target_method
branch
from
8c814be
to
9677b76
Compare
krororo
changed the title
RSpec/ExpectationTargetMethod copRSpec/MissingExpectationTargetMethod cop
| PATTERN | ||
|
|
||
| def on_send(node) | ||
| if node.parent&.block_type? |
There was a problem hiding this comment.
| if node.parent&.block_type? | |
| add_offense(node.loc.selector) if expectation_without_runner?(node) | |
| end |
And remove everything below.
Would it work like that?
There was a problem hiding this comment.
I was looking at reducing it to
def on_send(node)
node = node.parent if node.parent&.block_type?
expectation_without_runner?(node.parent) do
add_offense(node.parent.loc.selector)
end
endThere was a problem hiding this comment.
Is that because we want to highlight the offence differently for the block vs value expectations?
There was a problem hiding this comment.
I don’t know. My suggested code works like the code in the PR (the specs pass without change).
There was a problem hiding this comment.
Sorry @pirj, I only just now understood what you meant.
Your simpler solution is indeed possible, but I think we’d have to remove RESTRICT_ON_SEND.
There was a problem hiding this comment.
value expectations:
$ ruby-parse -e 'expect(foo).to'
(send
(send nil :expect
(send nil :foo)) :to)
with block:
$ ruby-parse -e 'expect{foo}.to'
(send
(block
(send nil :expect)
(args)
(send nil :foo)) :to)
In both cases I wanted to highlight the to location. Is there a better way?
The specs did not pass with expectation_without_runner?(node).
There was a problem hiding this comment.
@bquorning Now when you mentioned RESTRICT_ON_SEND, i started understanding why it is implemented the way it is.
No objections either way.
I thought that a concise nodepattern-only implementation is superior, but with restrict on send the choice is not that trivial.
I’m wondering if we allow our users to configure expectations in the dsl, and if we should account for that. That’s my only argument left for the nodepattern solution.
krororo
force-pushed
the
add-expectation_target_method
branch
from
9677b76
to
87e0e4a
Compare
There was a problem hiding this comment.
It seems false positive:
/ydah/real-world-rspec/rspec-expectations/spec/rspec/expectations/expectation_target_spec.rb:6:28: C: RSpec/MissingExpectationTargetMethod: Use .to, .not_to or .to_not to set an expectation.
expect(expect(7).target).to eq(7)
^^^^^^
/ydah/real-world-rspec/rspec-expectations/spec/rspec/expectations/expectation_target_spec.rb:11:33: C: RSpec/MissingExpectationTargetMethod: Use .to, .not_to or .to_not to set an expectation.
expect(expect(&block).target).to be(block)
^^^^^^
/ydah/real-world-rspec/rspec-expectations/spec/rspec/expectations/expectation_target_spec.rb:33:30: C: RSpec/MissingExpectationTargetMethod: Use .to, .not_to or .to_not to set an expectation.
expect(expect(nil).target).to be_nil
^^^^^^
|
another case: |
|
Is this also? |
|
Here are the all results of running real-world-rspec: |
|
rspec-expectations are fine, these are tests for some internals. Loomio is a legitimate offence I’m not sure I fully understand GitLab’s ActiveAdmin’s ‘ expect(response.body[:url]).present?.should be true’ seems to be a mistake.
I’ve looked closely at every offence on this list above, and to me, this falls in three categories:
What is your take on this @bquorning @ydah ? |
|
After a quick glance at the list I’d say the cop is working fine. For the edge cases (e.g. rspec-expectations) people will probably gladly add a For all other cases I think people should be happy that we point out their unclear code. I mean, that’s kinda our job, right? |
There was a problem hiding this comment.
I can guess it's a form of the implicit block expectation syntax which will be removed from RSpec 4.
Ah, I see. Thank you.
.not.to is a v2.x notation.
I have no idea about expect(Nokogiri::HTML(body).css(selector).to_s).length.to be 0. I actually tried to run it in the environment at hand, and it gives me an error.
1) sandbox something test
Failure/Error: expect('').length.to be 0
NoMethodError: undefined method `length'.
undefined method `length' for an instance of RSpec::Expectations::ValueExpectationTarget
# . /spec/sandbox_spec.rb:8:in `block (2 levels) in <top (required)>'
I was wondering if there were any cases where .to or something like that was written but was offensed, would users be confused because the offense message is "Use .to, .not_to or .to_not to set an expectation." I was wondering if there was a problem with that, but there doesn't seem to be.
It looks good! Thanks!
|
@pirj @bquorning @ydah |
|
Great job and an absolutely wonderful cop. Thanks for the contribution, @krororo ! |
I have noticed that we are using the wrong
expectin our application.These will pass the test without error or warning, but the expected behavior would be code like this.
This PR checks the above code.
Before submitting the PR make sure the following are checked:
master(if not - rebase it).CHANGELOG.mdif the new code introduces user-observable changes.bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).If you have created a new cop:
config/default.yml.Enabled: pendinginconfig/default.yml.Enabled: truein.rubocop.yml.VersionAdded: "<<next>>"indefault/config.yml.