expect: fix regression in toThrow for message prop and asymmetric matchers #7697
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7697 +/- ##
==========================================
+ Coverage 68.22% 68.26% +0.04%
==========================================
Files 251 251
Lines 9672 9678 +6
Branches 6 6
==========================================
+ Hits 6599 6607 +8
+ Misses 3071 3069 -2
Partials 2 2
Continue to review full report at Codecov.
|
|
Pictures of baseline 23.6 at left and improved at right Received error object which has
Expected value not helpful for custom asymmetric matcher without
|
| return { | ||
| hasMessage, | ||
| isError: false, | ||
| message: hasMessage ? e.message : typeof e === 'string' ? e : String(e), |
There was a problem hiding this comment.
Would prefer
| message: hasMessage ? e.message : typeof e === 'string' ? e : String(e), | |
| message: hasMessage ? e.message : String(e), |
for better readability.
I assume String('already a string') is probably optimized well enough to not be significantly more expensive than the typeof check anyway
There was a problem hiding this comment.
it's not hot code anyways, so doesn't matter. So +1 for the change
| return {message, pass}; | ||
| }; | ||
|
|
||
| type AsymmetricMatcher = { |
There was a problem hiding this comment.
Have you tried putting this here:
https://github.com/facebook/jest/blob/57aeaa21cef41755740e9a8698118fc7947e37a1/types/Matchers.js#L46
In case that causes type errors all over the place (there's probably a reason why it's Object 😄) we should probably save that costly refactoring for another time after Jest 24 is released, but maybe it works just fine?
|
Looks good. Please address the comments and I think it's good to merge. Thanks for fixing this! |
| }; | ||
| const getThrown = (e: any): Thrown => { | ||
| if ( | ||
| e !== null && |
There was a problem hiding this comment.
e != null can replace the separate null and undefined checks
There was a problem hiding this comment.
I'd say being explicit is better, but whatever in this case 🤷♂️
| } | ||
|
|
||
| const hasMessage = | ||
| e !== null && e !== undefined && typeof e.message === 'string'; |
There was a problem hiding this comment.
same here, e != null is the same as the first 2. Could also be e && typeof e.message === 'string', not sure
|
Can do a follow-up with nitpick fixes later |
|
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. |
Summary
Fixes #7692
Received message:in report when assertion fails for any object which has amessagepropTest plan
All existing tests pass
Added 8 tests and 4 snapshots for each of the following scenarios:
.toThrow(expect.any(Class)).toThrow(expect.anything()).toThrow(customMatcher).toThrow(expect.objectContaing(props))expect(() => { throw { message: 'whatever'}; })This quote from original pull request still applies to any-Class scenario:
See also pictures compared to baseline Jest 23.6 in following comments