-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Check constructor equality in .toStrictEqual() #7005
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7005 +/- ##
=======================================
Coverage 66.89% 66.89%
=======================================
Files 251 251
Lines 10488 10488
Branches 4 3 -1
=======================================
Hits 7016 7016
Misses 3471 3471
Partials 1 1
Continue to review full report at Codecov.
|
@emilsjolander mind taking a look? @ollelauribostrom can you update the changelog? |
4e0a130
to
112a51d
Compare
@SimenB Changelog updated ✔️ |
112a51d
to
ad7cfc7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me, should this go in a minor?
it('does not simply compare constructor names', () => { | ||
expect({ | ||
test: new TestClassC(1, 2), | ||
}).not.toStrictEqual({test: new TestClassD(1, 2)}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a test for when constructor.name is equal but the constructors are not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last test case (Line 253, it('does not simply compare constructor names'..
) should cover this if I'm not mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I see it now, thanks!
May be worth adding an expect like:
expect(c.constructor.name).toEqual(d.constructor.name);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added that, should make it a bit more clear
Next release is major, unless anyone wanna be fancy with cherry-picking etc. |
ad7cfc7
to
fdba936
Compare
fdba936
to
6252af6
Compare
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Associated Issue: #6767
Summary
Modified typeEquality to check for constructor equality and not just compare constructor names. This makes
.toStrictEqual()
no longer treat different classes with the same name as equal.Test plan
.toStrictEqual()
no longer treat different classes with the same name as equal.