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.

Sign up for GitHub

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

Jump to bottom

Add an else condition to correct StubbedMock behavior #1994

Merged
merged 1 commit into from
Nov 17, 2024
Merged

Add an else condition to correct StubbedMock behavior #1994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions lib/rubocop/cop/rspec/stubbed_mock.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ module RSpec
# expect(foo).to receive(:bar).with(42)
#
class StubbedMock < Base
MSG = 'Prefer `%<replacement>s` over `%<method_name>s` when ' \
MSG = 'Prefer %<replacement>s over `%<method_name>s` when ' \
'configuring a response.'
RESTRICT_ON_SEND = %i[to].freeze

# @!method message_expectation?(node)
# Match message expectation matcher
Expand Down Expand Up @@ -133,8 +134,6 @@ class StubbedMock < Base
}
PATTERN

RESTRICT_ON_SEND = %i[to].freeze

def on_send(node)
expectation(node) do |expectation, method_name, matcher|
on_expectation(expectation, method_name, matcher)
Expand All @@ -155,19 +154,23 @@ def on_expectation(expectation, method_name, matcher)
end

def msg(method_name)
format(MSG,
method_name: method_name,
replacement: replacement(method_name))
format(
MSG,
method_name: method_name,
replacement: replacement(method_name)
)
end

def replacement(method_name)
case method_name
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer a case.
Why not?

Copy link
Collaborator

Choose a reason for hiding this comment

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

SimpleCov pointed out that we don’t have any test coverage for when method_name didn’t match any of the options. And correctly so.

I also prefer the case statement, but perhaps we should add an else raise ArgumentError or KeyError or similar, and add test coverage (where relevant).

Copy link
Member

@pirj pirj Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ha! We can actually configure expectations DSL, and add eg the mentioned are_expected, for which this cop won’t find a substitute.

But there is no substitute for eg are_expected. Can we skip correction in this case instead of throwing an argument error?

fetch would raise, and this is also an undesirable outcome, and a breaking change.

I was never a big fan of 100% coverage, but it really starts to manifest its benefits here!

Copy link
Member

Choose a reason for hiding this comment

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

Correcting myself. Dsl conf link. are_expected, and should_* are included.

Quick suggestion: keep the case, but limit matching to just the three options we have substitutes for. Wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Quick suggestion: keep the case, but limit matching to just the three options we have substitutes for. Wdyt?

That is what we had before, no? Should we add an else case, and how should it respond?

Copy link
Member

Choose a reason for hiding this comment

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

Previously I think it was returning nil, and we auto-corrected to nonsense. Now, I think, it should just raise an offence and don’t autocorrect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, I think, it should just raise an offence and don’t autocorrect

I'm not exactly sure how to accomplish this.

Would you like me to close this PR so you can take it a new direction?

Or I'm happy to continue helping out here if you are willing to coach me through it.

Copy link
Member

@pirj pirj Nov 16, 2024 •
edited by bquorning
Loading

Choose a reason for hiding this comment

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

My bad - this cop does not autocorrect 🙈

Since there’s no valid suggestion for the message:

  1. add a new MSG_NO_REPLACEMENT with a different message
  2. use it for anything that doesn’t have a built-in counterpart

I insist on keeping the case statement, as ‘fetch’ may conceal execution paths from our coverage detection

when :expect
:allow
'`allow`'
when :is_expected
'allow(subject)'
'`allow(subject)`'
when :expect_any_instance_of
:allow_any_instance_of
'`allow_any_instance_of`'
else
'an allow statement'
end
end
end
Expand Down
9 changes: 8 additions & 1 deletion spec/rubocop/cop/rspec/stubbed_mock_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,19 @@
RUBY
end

it 'tolerates passed arguments without parentheses' do
it 'flags even when passed arguments without parentheses' do
expect_offense(<<~RUBY)
expect(Foo)
^^^^^^^^^^^ Prefer `allow` over `expect` when configuring a response.
.to receive(:new)
.with(bar).and_return baz
RUBY
end

it 'flags `are_expected`' do
expect_offense(<<~RUBY)
are_expected.to receive(:bar).and_return(:baz)
Copy link
Member

Choose a reason for hiding this comment

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

Is this from rspec-its? It’s soft-maintained, and is unlikely can be named an integral part of RSpec. I’d rather documented how to add it to the co figuration if the cop was configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes: https://github.com/search?q=org%3Arspec%20are_expected&type=code

My understanding is that it is included in the Expectations.all that the current Cop is using.

^^^^^^^^^^^^ Prefer an allow statement over `are_expected` when configuring a response.
RUBY
end
end