Add RSpec/StringAsInstanceDoubleConstant #1957

corsonknowles · 2024-09-17T23:28:24Z

Begins Addressing: #1136

Adds a cop which can autocorrect from String declarations for instance_double to Class declarations. Passing a variable -- which could hold a String or a Class -- is not affected by this cop.

Autocorrect is not marked as safe, because the correction will cause instantiation of the Class, which depending on test setup may or may not have been loaded yet. In addition, instantiating the Class before stubbing an instance_double means that RSpec will treat it as a verifying double. A string reference to a Class that is not loaded is treated as a normal non-verifying double.

We have been running this as safe autocorrect internally for years, and have not met with any issues as we use Zeitwerk and eager load once before running specs. In pushing this upstream, I don't want to assume that all test suites will have autoloading in place.

Before submitting the PR make sure the following are checked:

Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Updated documentation.
Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
The build (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:

Added the new cop to config/default.yml.
The cop is configured as Enabled: pending in config/default.yml.
The cop is configured as Enabled: true in .rubocop.yml.
The cop documents examples of good and bad code.
The tests assert both that bad code is reported and that good code is not reported.
Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

Set VersionChanged: "<<next>>" in config/default.yml.

corsonknowles · 2024-09-17T23:45:49Z

Getting

* generated /Users/.../Documents/GitHub/rubocop-rspec/docs/modules/ROOT/pages//cops_rspec.adoc
rake aborted!
Please run `bundle exec rake generate_cops_documentation` and add docs/ to the commit.
/Users/.../Documents/GitHub/rubocop-rspec/Rakefile:79:in `block in <top (required)>'
/Users/.../.rbenv/versions/3.3.0/bin/ruby_executable_hooks:22:in `eval'
/Users/.../.rbenv/versions/3.3.0/bin/ruby_executable_hooks:22:in `<main>'
Tasks: TOP => default => confirm_documentation
(See full trace by running task with --trace)

However, have run rake generate_cops_documentation

.rubocop.yml

CHANGELOG.md

lib/rubocop/cop/rspec/no_stringified_instance_double_constant.rb

spec/rubocop/cop/rspec/no_stringified_instance_double_constant_spec.rb

.rubocop.yml

bquorning · 2024-09-18T08:52:10Z

However, have run rake generate_cops_documentation

Did you also “add docs/ to the commit”? When I run the rake task I see a diff in the docs folder.

pirj · 2024-09-18T10:35:47Z

There are some wip rubocop cop naming guidelines, and they insist on naming cops so that they highlight the problem. This should be “StringifiedInstanceDoubleConstant”

corsonknowles · 2024-09-18T13:42:22Z

Did you also “add docs/ to the commit”? When I run the rake task I see a diff in the docs folder.

Looks like my rake problems were mostly self-inflicted, I tried to add my own documentation while I was following the contribution guidelines.

For ease of new development, I'd recommend updating all of our rake task feedback to say "bundle exec rake etc" so it's easier to copy and execute

technicalpickles

I am biased since it was extracted from our app, but in favor of this 👍🏻

Interestingly, instance_double does support a string as the name, at least according to its rdoc. I am looking at some of the implementation (instance_double and ObjectReference.for(doubled_class) but I couldn't tell you what happens for classes vs strings.

bquorning

Two questions:

Could / should we make the same rule for class_double too?
Shouldn’t we also detect and replace symbols? As I understand the source, calling instance_double with a symbol won’t even work.

corsonknowles · 2024-09-22T23:11:28Z

I don't think we need to extend the cop to cover symbols because RSpec will raise like this:

     ArgumentError:
       Module or String expected, got :Task

I have updated the PR description to be clearer about that.

corsonknowles · 2024-09-22T23:14:47Z

I think enforcing this for class_double would be another Cop, right?

bquorning · 2024-09-23T06:34:52Z

I think enforcing this for class_double would be another Cop, right?

@pirj What’s your take on this?

spec/rubocop/cop/rspec/string_as_instance_double_constant_spec.rb

pirj

No errors whatsoever during a run against real-world-rspec. 499 offences look legit at a glance.

Thank you!

A few minor tweaks, and good to go.

config/default.yml

spec/rubocop/cop/rspec/string_as_instance_double_constant_spec.rb

lib/rubocop/cop/rspec/string_as_instance_double_constant.rb

spec/rubocop/cop/rspec/string_as_instance_double_constant_spec.rb

corsonknowles · 2024-09-24T10:02:19Z

As an aside, while following the test coverage results, I noticed what appears to be a dead code method:
top_level_group?

https://github.com/search?q=repo%3Arubocop%2Frubocop-rspec%20top_level_group%3F&type=code

All specs pass without it.

The only other uncovered line is raise ::NotImplementedError

I'd be happy to raise another PR after this, if its useful. I don't know if you prefer to handle all maintenance PRs internally.

bquorning · 2024-09-24T10:57:12Z

As an aside, while following the test coverage results, I noticed what appears to be a dead code method:
top_level_group?

Yeah, it seems its last use was removed in 6276370 / #977.

I'd be happy to raise another PR after this, if its useful. I don't know if you prefer to handle all maintenance PRs internally.

You’re more than welcome to open a PR to remove that method 👍🏼

corsonknowles · 2024-09-25T23:42:11Z

Are we ready to merge here?

bquorning · 2024-09-26T06:58:37Z

The original issue #1957 mentions all of double/instance_double/class_double/object_double/spy etc. Since the cop name includes the method name instance_double, I think before we merge we should talk a bit more about if this cop should also police some of the other methods, of we want to have separate cops for each.

pirj · 2024-09-26T10:26:01Z

Even though for the rest of double methods we may want to enforce consistency, the reason for that would be … just consistency. While for instance_double that is to enforce verification.

My only question is if the class_double is the same?

For the rest I’d vote for a separate cop.

corsonknowles · 2024-09-26T16:59:25Z

Thanks @pirj --- so the problem with adding class_double is that it is a complete re-write.
We need to:

Rename the cop
Rewrite the description
Redo the matcher
Double the specs
Re-run the new rule on live codebases

Then I have an additional downstream problem where that rule doesn't exist in my codebase, so instead of merging this and then pulling the upstream, I need to go work through all class_doubles at the same time.

I guess what I'm saying is it's much more preferable for me to decouple these things.

In comparison to all of the above, it's relatively trivial for anyone to duplicate this new Cop and adapt it to work for class_double -- it's just a find and replace exercise, very modular.

I can't think of any performance reasons to combine them either, with restrict on send in place. A combined rule would need an OR matcher like {instance_double class_double} , or to leverage something to insert the method name in the pattern, if something like that exists.

pirj · 2024-09-26T20:27:38Z

Makes total sense to me. Thanks for the explanation.

bquorning · 2024-09-26T20:36:29Z

Yep, that makes sense. Thank you 👍🏼

bquorning · 2024-09-27T21:24:18Z

Hi @corsonknowles, I think you rebased the branch yesterday, but it looks like you have got a few extra/unintended changes in. Would you mind taking a look before we merge?

Addresses rubocop#1136 Adds a cop which can autocorrect from String declarations for instance_double to Class declarations. Symbol declarations are not affected.

corsonknowles · 2024-09-28T00:03:42Z

Thanks @bquorning, amended!

bquorning

Thank you for the contribution, and for your patience with our nitpicking 😅

lucthev · 2024-10-02T14:15:06Z

docs/modules/ROOT/pages/cops_rspec.adoc

+instance_double('User', name: 'John')
+
+# good
+instance_double(User, name: 'John')


👋 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?

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.

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.

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.

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

deprecate StringAsInstanceDoubleConstant -- is there a way to make it an alias of the other cop?

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.

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

corsonknowles force-pushed the master branch from 63c936b to 9f1a248 Compare September 17, 2024 23:44

corsonknowles mentioned this pull request Sep 17, 2024

double/instance_double argument type #1136

Closed

corsonknowles force-pushed the master branch from 9f1a248 to d3d3328 Compare September 17, 2024 23:56

bquorning reviewed Sep 18, 2024

View reviewed changes

.rubocop.yml Outdated Show resolved Hide resolved

bquorning mentioned this pull request Sep 18, 2024

Sort RuboCop config file #1958

Merged

6 tasks

corsonknowles force-pushed the master branch from d3d3328 to 53de88b Compare September 18, 2024 13:37

corsonknowles changed the title ~~Add RSpec/NoStringifiedNoStringifiedInstanceDoubleConstant~~ Add RSpec/StringAsInstanceDoubleConstant Sep 18, 2024

corsonknowles marked this pull request as ready for review September 18, 2024 13:40

corsonknowles requested a review from a team as a code owner September 18, 2024 13:40

corsonknowles requested a review from bquorning September 18, 2024 13:40

corsonknowles force-pushed the master branch 2 times, most recently from 7c794c1 to 91101d8 Compare September 18, 2024 13:49

technicalpickles reviewed Sep 18, 2024

View reviewed changes

bquorning reviewed Sep 20, 2024

View reviewed changes

bquorning reviewed Sep 23, 2024

View reviewed changes

spec/rubocop/cop/rspec/string_as_instance_double_constant_spec.rb Outdated Show resolved Hide resolved

pirj reviewed Sep 23, 2024

View reviewed changes

corsonknowles force-pushed the master branch 4 times, most recently from 04d0955 to 82e9d17 Compare September 24, 2024 09:27

bquorning approved these changes Sep 26, 2024

View reviewed changes

corsonknowles force-pushed the master branch from 9802d19 to befbe23 Compare September 27, 2024 00:06

Add RSpec/StringAsInstanceDoubleConstant

324552c

Addresses rubocop#1136 Adds a cop which can autocorrect from String declarations for instance_double to Class declarations. Symbol declarations are not affected.

corsonknowles force-pushed the master branch from befbe23 to 324552c Compare September 28, 2024 00:01

bquorning approved these changes Sep 28, 2024

View reviewed changes

bquorning merged commit dea7de9 into rubocop:master Sep 28, 2024
24 checks passed

lucthev reviewed Oct 2, 2024

View reviewed changes

This was referenced Oct 13, 2024

Obsolete StringAsInstanceDoubleConstant in favor of VerifiedDoubleReference for constants only #1968

Open

Complete line coverage for Metadata mixin #1970

Merged

Deprecate top_level_group? and test it in a Cop class #1976

Merged

pirj mentioned this pull request Nov 8, 2024

Incorrect autocorect for RSpec/StringAsInstanceDoubleConstant #1965

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RSpec/StringAsInstanceDoubleConstant #1957

Add RSpec/StringAsInstanceDoubleConstant #1957

corsonknowles commented Sep 17, 2024 •

edited

Loading

corsonknowles commented Sep 17, 2024 •

edited

Loading

bquorning commented Sep 18, 2024

pirj commented Sep 18, 2024 •

edited by bquorning

Loading

corsonknowles commented Sep 18, 2024 •

edited

Loading

technicalpickles left a comment

bquorning left a comment

corsonknowles commented Sep 22, 2024

corsonknowles commented Sep 22, 2024

bquorning commented Sep 23, 2024

pirj left a comment

corsonknowles commented Sep 24, 2024

bquorning commented Sep 24, 2024

corsonknowles commented Sep 25, 2024

bquorning commented Sep 26, 2024

pirj commented Sep 26, 2024

corsonknowles commented Sep 26, 2024 •

edited

Loading

pirj commented Sep 26, 2024

bquorning commented Sep 26, 2024

bquorning commented Sep 27, 2024

corsonknowles commented Sep 28, 2024

bquorning left a comment •

edited

Loading

lucthev Oct 2, 2024

pirj Oct 2, 2024

lucthev Oct 2, 2024

pirj Oct 2, 2024

corsonknowles Oct 3, 2024 •

edited

Loading

corsonknowles Oct 3, 2024

Add RSpec/StringAsInstanceDoubleConstant #1957

Add RSpec/StringAsInstanceDoubleConstant #1957

Conversation

corsonknowles commented Sep 17, 2024 • edited Loading

corsonknowles commented Sep 17, 2024 • edited Loading

bquorning commented Sep 18, 2024

pirj commented Sep 18, 2024 • edited by bquorning Loading

corsonknowles commented Sep 18, 2024 • edited Loading

technicalpickles left a comment

Choose a reason for hiding this comment

bquorning left a comment

Choose a reason for hiding this comment

corsonknowles commented Sep 22, 2024

corsonknowles commented Sep 22, 2024

bquorning commented Sep 23, 2024

pirj left a comment

Choose a reason for hiding this comment

corsonknowles commented Sep 24, 2024

bquorning commented Sep 24, 2024

corsonknowles commented Sep 25, 2024

bquorning commented Sep 26, 2024

pirj commented Sep 26, 2024

corsonknowles commented Sep 26, 2024 • edited Loading

pirj commented Sep 26, 2024

bquorning commented Sep 26, 2024

bquorning commented Sep 27, 2024

corsonknowles commented Sep 28, 2024

bquorning left a comment • edited Loading

Choose a reason for hiding this comment

lucthev Oct 2, 2024

Choose a reason for hiding this comment

pirj Oct 2, 2024

Choose a reason for hiding this comment

lucthev Oct 2, 2024

Choose a reason for hiding this comment

pirj Oct 2, 2024

Choose a reason for hiding this comment

corsonknowles Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

corsonknowles Oct 3, 2024

Choose a reason for hiding this comment

corsonknowles commented Sep 17, 2024 •

edited

Loading

corsonknowles commented Sep 17, 2024 •

edited

Loading

pirj commented Sep 18, 2024 •

edited by bquorning

Loading

corsonknowles commented Sep 18, 2024 •

edited

Loading

corsonknowles commented Sep 26, 2024 •

edited

Loading

bquorning left a comment •

edited

Loading

corsonknowles Oct 3, 2024 •

edited

Loading