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

Consolidates asserts#equal branches for keyed collections (Map/Set) and supports deep equality of Map keys #3258

Merged
merged 13 commits into from Nov 4, 2019

Conversation

@jamesseanwright
Copy link
Contributor

jamesseanwright commented Nov 3, 2019

In my fix for #3235, I simply added an additional branch to asserts#equal() to deeply compare Map instances in a similar fashion to Set instances. @kitsonk sensibly raised, however, that I could consolidate these two branches for all iterables. However, it turns out such a unification is not so straightforward.

Foremost, Map and Set are examples of keyed collections, which are not indexable, linear sequences. Hence, equal(new Set([1, 2, 3]), new Set([3, 2, 1])) should return true, but equal([1, 2, 3], [3, 2, 1]) should return false. After some investigation, I can confirm that this also holds true in Jest and Chai. Effectively, if two Maps or Sets have (i.e. Map/Set#has(key)) the same keys which resolve to deeply-equivalent values, then equal(a, b) === true.

Therefore, rather than introduce a common conditional branch for all iterables, I combined the logic for keyed collections only. I'm currently detecting them with duck typing, but please let me know if an explicit instanceof Map || instanceof Set check would be preferable.

I also realised that the current release (0.22) and my fix also fail to determine if the keys of a keyed collection are deeply equal (i.e. equal(new Map([[{ x: 1 }, true]]), new Map([[{ x: 1 }, true]])) should be true, despite the keys being different references), which I have thus addressed in this PR. Once again, Jest and Chai handle these cases in the same way.

I should highlight that there's a potential performance downside to my changes: disregarding recursion, the existing solution for both Maps and Sets has a worst-case complexity of O(n) (O(n) for iterating over a and O(1) for b.has(keyOfA)/b.get(keyOfA)), whereas my changes have an upper bound of O(n²). I understand if this trade-off isn't worth introducing to a standard module, but I don't think it hurts to support true deep equality of keyed collections out of the box if it's going to help with testing while avoiding the need for superfluous third-party dependencies.

@ry
ry approved these changes Nov 4, 2019
Copy link
Collaborator

ry left a comment

LGTM

@ry ry merged commit 429439d into denoland:master Nov 4, 2019
10 checks passed
10 checks passed
test macOS-latest
Details
test_std macOS-latest
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
@jamesseanwright jamesseanwright deleted the jamesseanwright:asserts-deep-keyed-collection-equality branch Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.