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

Inverted include matcher never returns for infinite enumerables #1097

Open
iainbeeston opened this issue Jan 26, 2019 · 6 comments
Open

Inverted include matcher never returns for infinite enumerables #1097

iainbeeston opened this issue Jan 26, 2019 · 6 comments

Comments

@iainbeeston
Copy link

Subject of the issue

I would like to write a spec where I assert that a value should not be included in an enumerable that is lazy and infinite. For example:

expect(fibonacci_sequence).not_to include(4)
# or
expect(1..).not_to include(0)

However, the rspec include matcher goes into an infinite loop and never returns for these specs, despite the fact that (1..).include?(0) does return false immediately (for fibonaccci_sequence it would depend on your implementation).

Your environment

  • Ruby version: 2.6.0
  • rspec-expectations version: 3.8.2

Steps to reproduce

I've created a simple test case on github here, which reproduces the issue using an infinite range. It's run on travis here

Expected behavior

If include? returns false, the expectation should fail.

Actual behavior

It goes into an infinite loop and the expectation never completes.

It looks like this is because the matcher calls any? after include? and that does never return.

@JonRowe
Copy link
Member

JonRowe commented Jan 26, 2019

I would like to write a spec where I assert that a value should not be included in an enumerable that is lazy and infinite.

From a theoretical point of view you cannot, an infinite enumerable may at some point generate that value. The RSpec matchers have no idea of ordering.

It looks like this is because the matcher calls any? after include? and that does never return.

This is for deep matching I believe, a fix might be interesting that works for all scenarios.

It might be possible to fix the "infinite range" issue but I'm still unsure how deep matching would be affected by changing this behaviour

@iainbeeston
Copy link
Author

Thanks for the quick reply @JonRowe

I guess from my point of view, I'd always expect rspec to agree with the return value of the include? method, but any time an enumerable defines it's own (possibly optimised, possibly flawed) version of include?, there's a chance that rspec will disagree (as you said due to deep matching). I wonder if there's a clean way for rspec to distinguish between enumerables that have overridden include? vs those that inherit the default implementation from Enumerable...

@iainbeeston
Copy link
Author

I guess there's also two different intents here. I'm specifically testing the implementation of the include? method, but other developers will be testing that the output of some other method "includes" a particular value, and whether that's determined using include? or any? or some other way is not important

@JonRowe
Copy link
Member

JonRowe commented Jan 27, 2019

Its not the implementation of include? thats the issue, its that we support things like:

expect(values).to include(a_string_matching("My Value")

in addition to checking nested values in arrays and hashes. Hence the use of any? with a fuzzy matcher.

Usually this works ok but evidently the new infinite enumerables behave a bit differently.

Unless we can find a way to make any? return we won't be able to support these new infinite enumerables in our usual way when negated, so we should probably raise an error along the lines of:

`to_not include` with infinite enumerables can not support fuzzy matching.
If you wish to check the behavior of include directly, use:
`expect(thing.include?(value)).to eq false`

Its possible for this edge case we could detect things that require fuzzy matching and issue this warning, otherwise just use include, but I'm unsure if the extra complication is worth it.

@benoittgt
Copy link
Member

but I'm unsure if the extra complication is worth it.

I don't think so

@pirj
Copy link
Member

pirj commented May 8, 2019

@iainbeeston You may use be_include predicate matcher that calls include? directly underneath and thus doesn't go into an endless loop.

include accepts an arbitrary number of arguments, while include? does not.

It's possible to define does_not_match? so that it passes some flag or a strategy to use for comparison of collections. However, this adds complexity.

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

No branches or pull requests

4 participants