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

Handle dynamic mocks specially for null-object doubles #1455

Conversation

nevinera
Copy link
Contributor

@nevinera nevinera commented May 7, 2024

This hopefully resolves #1132 and #1288 - essentially, if you leave strict_predicate_matchers disabled and use a null-object spy like this:

let(:thing) { spy('my thing') }

it "does the thing right" do
  do_it
  expect(thing).to have_receivedd(:foo)
end

It'll pass. Note the typo in have_receivedd? The dynamic matcher will call has_receivedd? on the spy, the spy will return itself (that's what it does), and the matcher will note that the result is truthy, and pass the spec. That's kind of an issue - turning off that setting does solve it, but that won't be the default until rspec 4, and even then it's not the ideal behavior, just a lot better.

This PR updates the dynamic matchers to notice when their target is a Double, and change behavior slightly - for Doubles, don't just check if they respond to the method, also check if it's in the methods list. That'll suffice to confirm that it's explicitly mocked on the double, rather than being an automatic response.

Specifically, when applying these dynamic matchers to a spy, only
consider them to have the method in question if it's explicitly defined
on the double; don't let the null-object double pretend.
This should short-circuit a common mistake, where someone typos

    expect(my_spy).to have_receivedd(:foo)

and gets an evergreen test, because strict_predicate_matchers is
disabled, and null-object spies return a truthy value (themselves) when
asked `has_receivedd?`.
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Looks mostly good, thanks for the fix!

spec/rspec/matchers/built_in/be_spec.rb Show resolved Hide resolved
spec/rspec/matchers/built_in/be_spec.rb Outdated Show resolved Hide resolved
@pirj
Copy link
Member

pirj commented May 8, 2024

22 deprecation warnings total

Wondering why we don’t print them 🧐

Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

Can you please use our fail matcher in the specs

@nevinera
Copy link
Contributor Author

nevinera commented May 8, 2024

Can you please use our fail matcher in the specs

Thank you! This was so ugly, I should have known there would be a matcher for that :-D

BasicObjects don't even have `is_a?` (or `class`, or `===`, etc), so the
only way I see to behave correctly for that case is to _try_ to check if
they're a Double, and also handle `NoMethodError` with "Oh, you're that
basic? Not a Double then."
@nevinera
Copy link
Contributor Author

nevinera commented May 8, 2024

  1) Object#should on interpreters that have BasicObject does not break the deprecation check on BasicObject-subclassed proxy objects
     Failure/Error: @target.send(name, *args)

     NoMethodError:
       undefined method `send' for #<BasicObject:0x000000010493faf8>

.. BasicObject doesn't have is_a?, so this line breaks for those. Good test, I'd have never thought of that!

I'm not sure how to safely detect that I'm dealing with a BasicObject though, since it doesn't have respond_to? or class methods (or anything else I can think of). Incidentally, it also doesn't have a send method, so this bit here just produces a NoMethodError: undefined method send' for #BasicObject:0x000000012ca06360`, which seems probably unintended?

Any advice? I could just catch NoMethodError on really_responds_to? and assume it means we're on a BasicObject (and therefore not a spy). I'll push that up, but I feel like there ought to be a better way to do this - possibly behavior to add in the Proxy itself?

(I think the current changes pass the tests, I just don't love using error-handling for this kind of thing unless it's actually necessary)

@nevinera
Copy link
Contributor Author

nevinera commented May 8, 2024

Oh, and please let me know if I'm using PRs wrong here - I've run into a variety of expectations regarding who resolves conversations, how much feedback to ask for, etc. You guys seem super-active, so I'm hoping I can be helpful.

@pirj
Copy link
Member

pirj commented May 9, 2024

No worries on the code, it feels that we’re getting along quite good. Thanks for your tireless contributions!

@nevinera
Copy link
Contributor Author

nevinera commented May 9, 2024

Darn, looks like I still have some kind of problem on ruby-1.8.7. I've never actually gotten it to install on an m1, guess I'll spend some time on that today. It's obviously going to keep coming up :-)

Luckily, we already have some code in this very file that handles that
situation, so we can just ride along.
@nevinera nevinera force-pushed the nev--1132--handle-dynamic-matches-on-mocks-specially branch from 7a56da1 to 5cf5ad3 Compare May 9, 2024 13:38
@pirj pirj requested a review from JonRowe May 9, 2024 14:21
Much nicer, and using === instead of is_a? also saves us the rescue
clause!
@nevinera nevinera requested a review from JonRowe June 14, 2024 16:11
@JonRowe JonRowe merged commit c0ea3eb into rspec:main Jun 15, 2024
29 of 30 checks passed
JonRowe added a commit that referenced this pull request Jun 15, 2024
@pirj
Copy link
Member

pirj commented Jun 16, 2024

Thank you!

JonRowe added a commit that referenced this pull request Jun 16, 2024
…hes-on-mocks-specially

Handle dynamic mocks specially for null-object doubles
JonRowe added a commit that referenced this pull request Jun 16, 2024
@JonRowe
Copy link
Member

JonRowe commented Aug 20, 2024

Released in 3.13.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants