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: not addressing to Sets and Maps as object without keys #14873

Merged
merged 2 commits into from Apr 30, 2024

Conversation

ofekm97
Copy link
Contributor

@ofekm97 ofekm97 commented Jan 29, 2024

Summary

Fixes #14599
The problem is that comparing list of nulls with Set or list of Sets:

expect([null]).toMatchObject([new Set()]);
expect([null]).toMatchObject(new Set());

In both cases the comparison does not throw an exception.


Update:
Same issue is happening with Map instead of set, I have fixed it too

expect([null]).toMatchObject([new Map()]);
expect([null]).toMatchObject(new Map());

Test plan

Unit tests added
Both cases described in the Summery have been tested locally with the fix and reproduced without the fix

Copy link

linux-foundation-easycla bot commented Jan 29, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ofekm97 / name: Ofek Meoded (f2346a7)
  • ✅ login: SimenB / name: Simen Bekkhus (ae8284e)

Copy link

netlify bot commented Jan 29, 2024

Deploy Preview for jestjs ready!

Name Link
🔨 Latest commit ae8284e
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/6627f0cb8cb00600097fce10
😎 Deploy Preview https://deploy-preview-14873--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -336,7 +336,8 @@ const isObjectWithKeys = (a: any) =>
isObject(a) &&
!(a instanceof Error) &&
!Array.isArray(a) &&
!(a instanceof Date);
!(a instanceof Date) &&
!(a instanceof Set);

export const subsetEquality = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case of subset as a Set the getObjectKeys(subset) at line 358 returns empty array ([]), so the .every(...) in the same line is vacuous truth, resulting the comparison not to raise an exception

@ofekm97 ofekm97 changed the title fix: Not addressing to Sets as object without keys fix: not addressing to Sets as object without keys Jan 29, 2024
@ofekm97 ofekm97 requested a review from SimenB April 4, 2024 13:51
@ofekm97 ofekm97 changed the title fix: not addressing to Sets as object without keys fix: not addressing to Sets and Maps as object without keys Apr 4, 2024
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks!

@SimenB SimenB enabled auto-merge (squash) April 23, 2024 17:33
@ofekm97
Copy link
Contributor Author

ofekm97 commented Apr 30, 2024

@SimenB Hey, I see one of the tests failed after the squash - is there an option to rerun it only? 🙌

@SimenB SimenB disabled auto-merge April 30, 2024 10:47
@SimenB SimenB merged commit b028e50 into jestjs:main Apr 30, 2024
72 of 73 checks passed
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.

[Bug]: toMatchObject incorectly handles nulls in lists of sets
2 participants