-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
expect: Improve report when matcher fails, part 15 #8281
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
Conversation
|
See #8281 (comment) instead of this comment Your critique and improvement of the proposed message is welcome:
Rewrite message not to mislead for expected failures during first phase of TDD and to emphasize If referential identity test fails and deep equality test would pass, is it okay to omit the diff? |
jeysal
left a comment
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.
This output is a lot smarter, thanks @pedrottimark.
I love the part about trying toStrictEqual and then toEqual to suggest what's likely the better fit.
packages/expect/src/matchers.ts
Outdated
| (received instanceof Error && expected instanceof Error); | ||
|
|
||
| const deepEqualityName = | ||
| isShallow || (expectedType !== 'object' && expectedType !== 'array') |
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.
Can you explain why the checks on this line are necessary and why deepEqualityName affects difference a few lines below? I'm probably missing something, I don't quite understand why the logic needs to be this complex.
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.
Renamed isShallowInequality and refactored assignments to deepEqualityName
Your intuition was correct that difference was too complex.
I moved the message below hint and above difference or Expected/Received lines.
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.
Cool, now the deepEqualityName and difference concerns are nicely separated. I've just tried it locally and it looks like you can also remove the deepEqualityName conditions? At least it doesn't seem to fail any test.
- if (!isShallowInequality) {
- if (expectedType === 'object' || expectedType === 'array') {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.
Yes indeed, thank you for patient critique!
- For
deepEqualityNamethe onlyifcondition is skipmapandsetuntil we review the current questionable logic - Separate and rename
hasConciseReportto make its purpose clear to short-circuit a line diff (while we improve the remaining matchers, we will discover whether it is reusable) - Added condition and 2 snapshot tests for deep equal and unequal dates
|
Updated pictures supersede #8281 (comment) and illustrate #8281 (comment) |
thymikee
left a comment
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.
👍
Co-Authored-By: pedrottimark <[email protected]>
|
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
For
toBematcher:rejectsorresolvesin matcher hintWhen negative assertion fails:
notbetween expected label and valueWhen positive assertion fails:
Differencelabel) or Expected/Received values, not botheither Difference ormessage about deep equality independent of the above, not bothReceived value has no visual differencein black instead of dimIf the test should pass with deep equality, replace toBe with ${deepEqualityName}to emphasizetoStrictEqualmore thantoEqualmatcher@jeysal your work on
shouldPrintDiffin #7605 gave me important example for the logic :)Residue:
pretty-formatserializes[, 1]and[undefined, 1]the same (it would be a breaking change to fix for sparse arrays, but it is an example for improvement via data-driven diff)Test plan
79 snapshot testsFriendly reviewers, you have my apology for mess of some added and updated snapshots :(
See also pictures in following comments with baseline at left and improved at right