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

Fix for NoMethodError: undefined method `supports_value_expectations?' #48

Closed
wants to merge 1 commit into from

Conversation

guigs
Copy link

@guigs guigs commented Mar 4, 2021

In order to upgrade an application to Rails 6.1, it is necessary to use newer version of rspec-rails 4.1.0.pre, which requires rspec-expectations 3.11.0.pre.

Turns out that rspec-expectations 3.11.0.pre is incompatible with current rspec-collection_matchers, because of this change, a call to expect(my_object).to have(...) is raising a NoMethodError: undefined method 'supports_value_expectations?' for <MyObject:0x000055a1b6e065c0>.

This is because Spec::Expectations::ExpectationTarget calls matcher.supports_value_expectation here.

Which ends up being processed by Have#method_missing which sets supports_value_expectations? to @collection_name. Then, later in Have#determine_collection it tries to call supports_value_expectations? on my_object, which causes the NoMethodError.

The fix is simple, just implement Have#supports_value_expectations?.

With [this change](rspec/rspec-expectations#1139) in rspec-expectations, a call to `expect(my_object).to have(...)` is raising a `NoMethodError: undefined method `supports_value_expectations?' for <MyObject:0x000055a1b6e065c0>`. 

This is because ExpectationTarget calls `supports_value_expectation` here:

https://github.com/rspec/rspec-expectations/blob/4e7011fe8ac68b74621336b07f894879c8da202d/lib/rspec/expectations/expectation_target.rb#L127

Which ends up being processed by `Have#method_missing` which sets `supports_value_expectations?` to @collection_name. Later in `Have#determine_collection` it tries to call `supports_value_expectations?` on my_object, which causes the NoMethodError.

The fix is simple, just implement `Have#supports_value_expectations?`.
@JonRowe
Copy link
Member

JonRowe commented Mar 4, 2021

Your link cannot be the culprit, as:

def supports_value_expectations?(matcher)
  matcher.supports_value_expectations?
rescue NoMethodError
  true
end

So it would be useful to find out where a NoMethodError is coming from, in order to fix upstream, cc @pirj

@guigs
Copy link
Author

guigs commented Mar 4, 2021

@JonRowe That puzzled me too, initially.

But what happens is that matcher.supports_value_expectations? ends up being handled my Have#method_missing which sets the value of @collection_name as "supports_value_expectations?".

Then the NoMethodError is raised here:

collection_or_owner.__send__(@collection_name, *@args, &@block)
when it tries to send "supports_value_expectations?" to my object instance.

@pirj
Copy link
Member

pirj commented Mar 4, 2021

Can you please add a test case that is failing prior to the fix?

@JonRowe
Copy link
Member

JonRowe commented Mar 4, 2021

hah lol the matcher thinks its an expectation
yeah this pretty valid, can you add a test?

@JonRowe
Copy link
Member

JonRowe commented Mar 4, 2021

Actually this will probably cause issues for other dynamic matchers, so I'm still wondering if this needs a fix upstream 🤔

@pirj
Copy link
Member

pirj commented Mar 4, 2021

Can you please add a test case that is failing prior to the fix?

@pirj
Copy link
Member

pirj commented Mar 4, 2021

I still don't get what is going on 🤷 😄

@guigs
Copy link
Author

guigs commented Mar 4, 2021

I'm not sure if it need specific test for this.

If you just run bundle install with fresh repository clone, rspec-expectations version 3.11.0.pre will be installed. Then if you run the specs, many fail with error similar to:

NoMethodError:
        undefined method `supports_value_expectations?' for #<RSpec::Expectations::Helper::CollectionOwner:0x0000563c53093e00>

and

Failure/Error: collection_or_owner.__send__(@collection_name, *@args, &@block)
        #<Double "target"> received unexpected message :supports_value_expectations? with (no args)

After this patch, all tests pass.

@JonRowe If by "dynamic matchers" you mean implement method_missing with similar logic, then maybe yes. I have no idea if this is common practice.

Anyway, I think the fix here is still pretty reasonable and practical.

pirj added a commit to rspec/rspec-expectations that referenced this pull request Mar 5, 2021
See rspec/rspec-collection_matchers#48

If a matcher supports dynamic arbitrary methods called on it, e.g.
`have_at_least(100).bucks`, it would also consider a
`supports_value_expectations?` call as a dynamic part.

Instead of calling `supports_*?` methods on a matcher, explicitly
check if it responds to it.
@pirj
Copy link
Member

pirj commented Mar 5, 2021

@guigs rspec/rspec-expectations#1294 should fix it for rspec-collection_matchers and other extensions with dynamic matchers. Can you please check if it fixes the problem you're experiencing?

pirj added a commit to rspec/rspec-expectations that referenced this pull request Mar 5, 2021
See rspec/rspec-collection_matchers#48

If a matcher supports dynamic arbitrary methods called on it, e.g.
`have_at_least(100).bucks`, it would also consider a
`supports_value_expectations?` call as a dynamic part.

Instead of calling `supports_*?` methods on a matcher, explicitly
check if it responds to it.
pirj added a commit to rspec/rspec-expectations that referenced this pull request Mar 5, 2021
See rspec/rspec-collection_matchers#48

If a matcher supports dynamic arbitrary methods called on it, e.g.
`have_at_least(100).bucks`, it would also consider a
`supports_value_expectations?` call as a dynamic part.

Instead of calling `supports_*?` methods on a matcher, explicitly
check if it responds to it.
@guigs
Copy link
Author

guigs commented Mar 5, 2021

@pirj I confirm that rspec/rspec-expectations#1294 also fixes the issue for me.

@pirj
Copy link
Member

pirj commented Mar 5, 2021

Great, thank you!
I'll close this one, and we'll get the upstream fix merged.

@pirj pirj closed this Mar 5, 2021
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021
See rspec/rspec-collection_matchers#48

If a matcher supports dynamic arbitrary methods called on it, e.g.
`have_at_least(100).bucks`, it would also consider a
`supports_value_expectations?` call as a dynamic part.

Instead of calling `supports_*?` methods on a matcher, explicitly
check if it responds to it.

---
This commit was imported from rspec/rspec-expectations@c6475b5.
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 19, 2021
See rspec/rspec-collection_matchers#48

If a matcher supports dynamic arbitrary methods called on it, e.g.
`have_at_least(100).bucks`, it would also consider a
`supports_value_expectations?` call as a dynamic part.

Instead of calling `supports_*?` methods on a matcher, explicitly
check if it responds to it.

---
This commit was imported from rspec/rspec-expectations@d8f272a.
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