Allow matchers to be reused #1326
Conversation
bclayman-sq
force-pushed
the
bclayman/allow-matcher-reuse
branch
2 times, most recently
from
e9dcb9b
to
b2d8d78
Compare
|
@pirj Thanks for kicking off that build! It looks like this PR passes for all CI environments except Ruby 2.2 on Windows. I looked at the logs but didn't see much info that would be helpful in debugging (I see an error, but no failing tests). Could you help me understand what might have happened here? |
There was a problem hiding this comment.
It seems like that the original problem is caused by rose memoization @... ||=.
Even though other built-in matchers use it, too, I couldn't make them cache anything between usages.
I could suggest localizing the change to contain_exactly, but that would require using an additional instance variable (@match_called_previously?) in match, as @actual would be always defined there, or overriding matches?. Unless you have a more elegant solution, let's keep the call to reinitialize in BaseMatcher.
And thanks for yet another contribution!
bclayman-sq
force-pushed
the
bclayman/allow-matcher-reuse
branch
from
b2d8d78
to
65b0241
Compare
👋 @pirj Happy to! I just incorporated your feedback, thanks for the review! Do you have any insight into why ruby 2.2 on windows fails in CI? I don't see any failing tests, just an error code in the logs... |
It segfaulted last time, passed this time, and crashed with no message the first time. I don't think it is caused by your changes or can be fixed. It is not a blocker to merge this PR. |
| # if @actual is already set, we know this matcher was used previously and want | ||
| # to reset memoized fields | ||
| reset if defined?(@actual) |
There was a problem hiding this comment.
| # if @actual is already set, we know this matcher was used previously and want | |
| # to reset memoized fields | |
| reset if defined?(@actual) |
| def reset; end | ||
|
|
There was a problem hiding this comment.
| def reset; end |
| private | ||
|
|
||
| def reset | ||
| @pairings_maximizer = nil | ||
| @best_solution = nil | ||
| @extra_items = nil | ||
| @missing_items = nil | ||
| end |
There was a problem hiding this comment.
| private | |
| def reset | |
| @pairings_maximizer = nil | |
| @best_solution = nil | |
| @extra_items = nil | |
| @missing_items = nil | |
| end | |
| def matches?(actual) | |
| @pairings_maximizer = nil | |
| @best_solution = nil | |
| @extra_items = nil | |
| @missing_items = nil | |
| super(actual) | |
| end |
There was a problem hiding this comment.
@JonRowe Yep, this is even nicer than my approach: it keeps the change local to ContainExactly. Just incorporated. Mind kicking off a build?
There was a problem hiding this comment.
One more build and should be good to go!
bclayman-sq
force-pushed
the
bclayman/allow-matcher-reuse
branch
from
65b0241
to
f17bdf9
Compare
This addresses issue rspec#1287. Previously, matchers retained state after being used as an argument to RSpec::Expectations::ExpectationTarget#to. This caused incorrect results when reusing a matcher in subsequent expectations. This PR addresses the issue by overriding ContainExactly#matches?, reinitializing its internal state when the matcher is reused. Co-authored by: Rai Feren <rferen@squareup.com>
bclayman-sq
force-pushed
the
bclayman/allow-matcher-reuse
branch
from
f17bdf9
to
0233b10
Compare
|
Thank you! |
This addresses issue #1287.
Previously, matchers retained state after being used as an argument to
RSpec::Expectations::ExpectationTarget#to. In particular, it leaked@extra_items,@missing_items,@best_solution, and@pairings_maximizer. This caused incorrect results when reusing a matcher in subsequent expectations.This PR addresses the issue by reinitializing the matcher's state when the matcher is reused.
Co-authored by: Rai Feren rferen@squareup.com