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

Add RSpec/StringAsInstanceDoubleConstant #1957

Merged
merged 1 commit into from
Sep 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -253,4 +253,5 @@ Style/YAMLFileRead: {Enabled: true}

# Enable our own pending cops.
#
# No pending cops yet.
RSpec/StringAsInstanceDoubleConstant:
Enabled: true
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## Master (Unreleased)

- Add `RSpec/StringAsInstanceDoubleConstant` to check for and correct strings used as instance_doubles. ([@corsonknowles])
- Fix false-positive for `RSpec/UnspecifiedException` when a method is literally named `raise_exception`. ([@aarestad])
- Fix false-positive for `RSpec/UnspecifiedException` when `not_to raise_error` is used within a block. ([@aarestad], [@G-Rath])

Expand Down Expand Up @@ -924,6 +925,7 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features.
[@cfabianski]: https://github.com/cfabianski
[@clupprich]: https://github.com/clupprich
[@composerinteralia]: https://github.com/composerinteralia
[@corsonknowles]: https://github.com/corsonknowles
[@corydiamand]: https://github.com/corydiamand
[@darhazer]: https://github.com/Darhazer
[@daveworth]: https://github.com/daveworth
Expand Down
7 changes: 7 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -929,6 +929,13 @@ RSpec/SpecFilePathSuffix:
- "**/spec/**/*"
Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/SpecFilePathSuffix

RSpec/StringAsInstanceDoubleConstant:
Description: Do not use a string as `instance_double` constant.
Enabled: pending
Safe: false
corsonknowles marked this conversation as resolved.
Show resolved Hide resolved
VersionAdded: "<<next>>"
Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/StringAsInstanceDoubleConstant

RSpec/StubbedMock:
Description: Checks that message expectations do not have a configured response.
Enabled: true
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@
* xref:cops_rspec.adoc#rspecsortmetadata[RSpec/SortMetadata]
* xref:cops_rspec.adoc#rspecspecfilepathformat[RSpec/SpecFilePathFormat]
* xref:cops_rspec.adoc#rspecspecfilepathsuffix[RSpec/SpecFilePathSuffix]
* xref:cops_rspec.adoc#rspecstringasinstancedoubleconstant[RSpec/StringAsInstanceDoubleConstant]
* xref:cops_rspec.adoc#rspecstubbedmock[RSpec/StubbedMock]
* xref:cops_rspec.adoc#rspecsubjectdeclaration[RSpec/SubjectDeclaration]
* xref:cops_rspec.adoc#rspecsubjectstub[RSpec/SubjectStub]
Expand Down
35 changes: 35 additions & 0 deletions docs/modules/ROOT/pages/cops_rspec.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -5493,6 +5493,41 @@ spec/models/user.rb # shared_examples_for 'foo'
* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/SpecFilePathSuffix
== RSpec/StringAsInstanceDoubleConstant
|===
| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed
| Pending
| No
| Always (Unsafe)
| <<next>>
| -
|===
Do not use a string as `instance_double` constant.
=== Safety
This cop is unsafe because the correction requires loading the class.
Loading before stubbing causes RSpec to only allow instance methods
to be stubbed.
=== Examples
[source,ruby]
----
# bad
instance_double('User', name: 'John')
# good
instance_double(User, name: 'John')
Copy link

Choose a reason for hiding this comment

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

👋 Hello! I was looking through the offences this cop turned up in the codebase I work on, and noticed they're identical to those produced by RSpec/VerifiedDoubleReference (which we had disabled via TODO file). The docs would suggest they're doing largely the same thing — is there any meaningful difference that could be clarified in the docs?

Copy link
Member

Choose a reason for hiding this comment

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

Hi!
Nice catch, we’ve missed this completely.

The new cop is essentially the old one, but with no option to select the style. And this is certainly the direction we choose to enforce.

Your input is valuable, why did you decide to turn the cop off?

In a nutshell, we’ve discovered that an instance_double with a string argument does not verify, and behaves identically to a regular double. It makes sense to make the usage explicit, and to use a double for abstract doubles, and instant_double for specific ones, and verify them.

Copy link

Choose a reason for hiding this comment

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

why did you decide to turn the cop off

We didn't totally turn off VerifiedDoubleReference, just ignored a number of existing offences via .rubocop_todo.yml — it's a large, old codebase, but Rubocop was only added semi-recently. It's definitely a useful cop for the reasons you've outlined!

That being said, if this new cop is wholly redundant, then I'll likely turn it off 😅. Would it make sense to deprecate one or the other? For what it's worth, VerifiedDoubleReference appears to support e.g. class_double as well, which I see was a concern raised in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for sharing.

Yes, it would make sense to deprecate one of the cops, and I’d remove the EnforcedStyle: string option from the old one if it is less intrusive.

Copy link
Contributor Author

@corsonknowles corsonknowles Oct 3, 2024

Choose a reason for hiding this comment

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

Ah, thank you for immediately reporting this Luc!

I'm happy to deprecate the new cop and focus on updating the original cop.

So the action items are

  1. deprecate StringAsInstanceDoubleConstant -- is there a way to make it an alias of the other cop?
  2. remove String style from VerifiedDoubleReference as it is not consistently verifying. Is there a way to deprecate this or do we simply remove it?

I'll also take a look if there's anything in the new implementation worth porting over to [VerifiedDoubleReference](https://docs.rubocop.org/rubocop-rspec/cops_rspec.html#rspecverifieddoublereference

I can probably get started on this on Monday. If anyone wants to pitch in or get started earlier, you are by all means welcome to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we could have it inherit from VerifiedDoubleReference and add the deprecation message, if that makes sense?

----
=== References
* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/StringAsInstanceDoubleConstant
== RSpec/StubbedMock
|===
Expand Down
45 changes: 45 additions & 0 deletions lib/rubocop/cop/rspec/string_as_instance_double_constant.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# frozen_string_literal: true

module RuboCop
module Cop
module RSpec
# Do not use a string as `instance_double` constant.
#
# @safety
# This cop is unsafe because the correction requires loading the class.
# Loading before stubbing causes RSpec to only allow instance methods
# to be stubbed.
#
# @example
# # bad
# instance_double('User', name: 'John')
#
# # good
# instance_double(User, name: 'John')
#
class StringAsInstanceDoubleConstant < Base
extend AutoCorrector

MSG = 'Do not use a string as `instance_double` constant.'
RESTRICT_ON_SEND = %i[instance_double].freeze

# @!method stringified_instance_double_const?(node)
def_node_matcher :stringified_instance_double_const?, <<~PATTERN
(send nil? :instance_double $str ...)
PATTERN

def on_send(node)
stringified_instance_double_const?(node) do |args_node|
add_offense(args_node) do |corrector|
autocorrect(corrector, args_node)
end
end
end

def autocorrect(corrector, node)
corrector.replace(node, node.value)
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/rspec_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@
require_relative 'rspec/sort_metadata'
require_relative 'rspec/spec_file_path_format'
require_relative 'rspec/spec_file_path_suffix'
require_relative 'rspec/string_as_instance_double_constant'
require_relative 'rspec/stubbed_mock'
require_relative 'rspec/subject_declaration'
require_relative 'rspec/subject_stub'
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::RSpec::StringAsInstanceDoubleConstant,
:config do
context 'when using a class for instance_double' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY)
instance_double(Shape, area: 12)
RUBY
end
end

context 'when passing a variable to initialize instance_double' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY)
instance_double(type_undetectable_in_static_analysis)
RUBY
end
end

context 'when using a string for instance_double' do
it 'replaces the string with the class' do
expect_offense <<~RUBY
instance_double('Shape', area: 12)
^^^^^^^ Do not use a string as `instance_double` constant.
RUBY

expect_correction <<~RUBY
instance_double(Shape, area: 12)
RUBY
end
end
end
corsonknowles marked this conversation as resolved.
Show resolved Hide resolved