-
-
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
expect: Improve report when matcher fails, part 15 #8281
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? |
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
deepEqualityName
the onlyif
condition is skipmap
andset
until we review the current questionable logic - Separate and rename
hasConciseReport
to 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) |
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 <pedrottimark@gmail.com>
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
toBe
matcher:rejects
orresolves
in matcher hintWhen negative assertion fails:
not
between expected label and valueWhen positive assertion fails:
Difference
label) or Expected/Received values, not botheither Difference ormessage about deep equality independent of the above, not bothReceived value has no visual difference
in black instead of dimIf the test should pass with deep equality, replace toBe with ${deepEqualityName}
to emphasizetoStrictEqual
more thantoEqual
matcher@jeysal your work on
shouldPrintDiff
in #7605 gave me important example for the logic :)Residue:
pretty-format
serializes[, 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