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

Special case maps with only value keys #2257

Merged
merged 6 commits into from
Oct 2, 2024
Merged

Special case maps with only value keys #2257

merged 6 commits into from
Oct 2, 2024

Conversation

natebosch
Copy link
Member

Closes #2256

When checking deep equality on Map instances (including maps nested in
other collection types) we allow keys which are instances of other
collection (and don't have a useful operator ==) and keys which are
Condition callbacks that could potentially match multiple keys of the
actual value. This is maximally flexible, but it loses the nested path
information we are otherwise able to keep for iterables compared by
index.

Check whether expectation maps have any keys which need this special
treatment, and if not, special case to an algorithm close to the
Iterable algorithm which can enqueue nested checks and maintain the
direct known path into the collection.

Update the doc comment to describe the new behavior. This does
potentially introduce confusion since a change which adds a Condition
key has to potential to make other keys in the same expectation
behaved differently. The benefit is a more actionable failure output for
the much more common case of checking expectations with hardcoded keys.

Closes #2256

When checking deep equality on Map instances (including maps nested in
other collection types) we allow keys which are instances of other
collection (and don't have a useful `operator ==`) and keys which are
`Condition` callbacks that could potentially match multiple keys of the
actual value. This is maximally flexible, but it loses the nested path
information we are otherwise able to keep for iterables compared by
index.

Check whether expectation maps have any keys which need this special
treatment, and if not, special case to an algorithm close to the
`Iterable` algorithm which can enqueue nested checks and maintain the
direct known path into the collection.

Update the doc comment to describe the new behavior. This does
potentially introduce confusion since a change which adds a `Condition`
key has to potential to make _other_ keys in the same expectation
behaved differently. The benefit is a more actionable failure output for
the much more common case of checking expectations with hardcoded keys.
Comment on lines +30 to +31
/// Deep equality checks for [Map] instances depend on the structure of the
/// expected map.
Copy link
Member Author

Choose a reason for hiding this comment

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

@jakemac53 - WDYT about this behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds pretty reasonable to me.

@gnprice
Copy link

gnprice commented Jul 23, 2024

This looks great, thank you!

This does
potentially introduce confusion since a change which adds a Condition
key has to potential to make other keys in the same expectation
behaved differently

To make sure it's visible on this thread too: I just replied to a remark about this on the issue. It's possible to give a fun demo for this, in fact (#2256 (comment)), using as actual a SplayTreeMap with both 0.0 and -0.0 as keys, so that turning a different expected key from a plain value to a Condition turns the check from passing to failing.

But I think it's the case that that sort of behavior can only come about when one of the maps involved doesn't behave consistent with how a Map is supposed to behave (like in that demo, where it has two ==-equal keys). And I think it's appropriate for a general-purpose test-assertion library like this one to rely on the invariants of Map and so on; it needs some kind of structure in order to make sense of things at all.

@kevmoo kevmoo added the package:checks Issues related to pkg:checks label Jul 29, 2024
@natebosch natebosch marked this pull request as ready for review September 27, 2024 23:16
@natebosch natebosch requested a review from a team as a code owner September 27, 2024 23:16
@natebosch
Copy link
Member Author

I'm pretty happy with how this is working as I use it. I think we should land this.

Copy link

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Package publish validation ✔️
Package Version Status
package:checks 0.3.1-wip WIP (no publish necessary)
package:test 1.25.9-wip WIP (no publish necessary)
package:test_api 0.7.4-wip WIP (no publish necessary)
package:test_core 0.6.6-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@natebosch natebosch merged commit d872cf8 into master Oct 2, 2024
56 checks passed
@natebosch natebosch deleted the map-paths branch October 2, 2024 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:checks Issues related to pkg:checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deepEquals on Map should give details on the value that didn't match
4 participants