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

Use simple lookup table for replacements in StubbedMock #1994

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+16 −16
Open

Use simple lookup table for replacements in StubbedMock #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
24 changes: 9 additions & 15 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 @@ -16,6 +16,13 @@ module RSpec
class StubbedMock < Base
MSG = 'Prefer `%<replacement>s` over `%<method_name>s` when ' \
'configuring a response.'
REPLACEMENTS = {
expect: :allow,
is_expected: 'allow(subject)',
expect_any_instance_of: :allow_any_instance_of
}.freeze
REPLACABLE_METHODS = Set.new(REPLACEMENTS.keys).freeze
RESTRICT_ON_SEND = %i[to].freeze

# @!method message_expectation?(node)
# Match message expectation matcher
Expand Down Expand Up @@ -60,7 +67,7 @@ class StubbedMock < Base
# @yield [RuboCop::AST::Node] expectation, method name, matcher
def_node_matcher :expectation, <<~PATTERN
(send
$(send nil? $#Expectations.all ...)
$(send nil? $REPLACABLE_METHODS ...)
:to $_)
PATTERN

Expand Down Expand Up @@ -133,8 +140,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 @@ -157,18 +162,7 @@ def on_expectation(expectation, method_name, matcher)
def msg(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

when :expect
:allow
when :is_expected
'allow(subject)'
when :expect_any_instance_of
:allow_any_instance_of
end
replacement: REPLACEMENTS.fetch(method_name))
end
end
end
Expand Down
8 changes: 7 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,18 @@
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 'does not flag `are_expected`' do
expect_no_offenses(<<~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.

RUBY
end
end