-
Notifications
You must be signed in to change notification settings - Fork 87
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
bag has issues if an item matches multiple elements #996
Comments
This was discovered when looking at Test-More/Test2-Suite#43. |
FWIW, Test::Deep has the same problem, and they just documented it. |
basic yes/no tests pass, deltas are wrong most of the time
I'm still not clear on what kind of deltas we should return in some cases, and I suspect that my logic in Bag::deltas is too convoluted, but it's a start
I have rewritten Bag in a maybe-too-complicated way. It builds a matrix of inputs × checks, then looks at it to see what went wrong. |
First off, those commits are failing in travis, so that needs to be fixed. I have not yet done a full code review, I will when I have a chance. I would like an explanation of how it handles things after the matrix is created. Say 1 check matches 2 items in the array, but no other checks match either item, will it pass or fail? What about more complicated scenarios where all items in the array have matched at least 1 check, but some checks matched multiple items, but there are less checks than matched items and 'end' was used? I am actually not 100% sure what the correct way to handle these scenarios are, so right now I am interested in how you handle them. |
Re: failing, yes, I know, the tests that are failing are those that check the deltas returned in cases where I'm not sure what we should be returning. My current logic is:
Regarding deltas:
So the open questions are:
|
Yes, it makes sense. Once again I have not looked at the code. That said I worked it out, and here is how I think the deltas should work: First check that the array has at least as many elements as the bag, if it does not then add a delta that indicates we expected more items:
If end is specified we want to do the same for the opposite, more array items than bag items:
After those deltas add the ones for any checks that didn't match an item
if end() is specified add deltas for any extra array items, these should indicate we expected them to not exist.
if end() is not specified, but we have deltas for any failure, add deltas for all unmatched items, this should NOT indicate we expected them to not exist, the 'expected' and 'op' columns should be empty. This is just informative diagnostics
To summarize
|
In this case we get a failure cause both items match the first element, so the second element is considered 'extra' for the end check. A quick-fix would be to never re-check an element, but that is not a valid fix:
This one will still fail because the less greedy bag item got 'fab' and 'ab' will not match against 'bad' but a human reader knows this should pass.
I think bag as it is currently needs a bit of an overhaul. We need to iterate over the array, not the bag items. We need to record all matches. When 'end; is not specified we just need to check that all items have at least 1 match in the array, easy. When 'end' is used we need to make sure both lists have a match in the other, but in cases where there are multiple matches we need to pick one. This can get complicated.
The text was updated successfully, but these errors were encountered: