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 an error when passed non-array to match_array #135

Closed
wants to merge 3 commits into from

Conversation

wata727
Copy link
Contributor

@wata727 wata727 commented May 21, 2021

Fixes #132

This issue is caused by a difference in handling arguments to the monkey patched match_array. In the original matcher implementation, the arguments are splatted:

https://github.com/rspec/rspec-expectations/blob/040d1b0bba83031c9ae432fd3a4462fdd1cc74e7/lib/rspec/matchers.rb#L715-L717
https://github.com/rspec/rspec-expectations/blob/040d1b0bba83031c9ae432fd3a4462fdd1cc74e7/lib/rspec/matchers.rb#L510-L512

Comment on lines +785 to +786
items = *items
BuiltIn::MatchArray.new(items)

Choose a reason for hiding this comment

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

Shouldn't this be?

Suggested change
items = *items
BuiltIn::MatchArray.new(items)
BuiltIn::MatchArray.new(items.is_a?(Array) ? items : [items] )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original implementation is splatting the given argument, so I believe it would be more strict to do the same conversion.
https://github.com/rspec/rspec-expectations/blob/040d1b0bba83031c9ae432fd3a4462fdd1cc74e7/lib/rspec/matchers.rb#L715-L717

Choose a reason for hiding this comment

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

👍🏼

Copy link
Owner

Choose a reason for hiding this comment

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

Good call!

@mcmire
Copy link
Owner

mcmire commented Feb 4, 2024

Hi @wata727, sorry it has taken me so long to get to this PR. I took a closer look at the current behavior of match_array. It looks like it doesn't matter what you pass to match_array — if you pass a single non-array, then it should treat it as it though you pass it an array with that single value. It also looks like that issue was raised in #97 and a fix was attempted in #110, but I must have misunderstood that it doesn't matter whether the value is a string, an rspec-mocks argument matcher, a hash, or whatever — just as long as it's not an array. I rebased this PR and removed the extra tests you added. It makes sense that we'd want to prove that match_array can take a non-array; but I think the existing tests already did this, although perhaps we should have used something that clearly cannot be turned into an array on its own, and the test name also didn't spell this out. I made some adjustments and merged #213 and gave you credit. Thank you!

@mcmire mcmire closed this Feb 4, 2024
@mcmire
Copy link
Owner

mcmire commented Feb 10, 2024

This fix has been included in v0.11.0. Thanks again!

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.

match_array + hash_including raises exception
3 participants