-
-
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 assertion fails, part 3 #7152
Conversation
Really like this improvement (especially how it simplifies the mather code) What happens when the received value is a multiline string? Like a pretty formatted object? My thinking is that we may still want the received value to be on a new line |
Just realized that the received value can only be an object and we control the printing so nvm |
Could we try to align the right side of the colons? VIsual comparison is easier if the line up. Thankfully "received" and "expected" are both 8 characters longs, so it's a matter of padding enough for "value"/"string"/"length"/"array" etc |
@rickhanlonii Here is picture of multiline string as received value: @SimenB More alignment is more better! I will make a new picture formatted as you suggest. |
Before I push changes, I need think more about use cases for Here is code I wrote to make the picture: type PrintLabel = string => string; // append colon, space, and padding spaces
const getLabelPrinter = (...labels: Array<string>): PrintLabel => {
const maxLength = Math.max(...labels.map(arg => arg.length));
return label => `${label}: ${' '.repeat(maxLength - label.length)}`;
};
// in toContainEqual matcher
const labelExpected = 'Expected value';
const labelReceived = `Received ${collectionType}`;
const printLabel = getLabelPrinter(labelExpected, labelReceived);
const message = () =>
matcherHint('.toContainEqual', collectionType, 'value', {
comment,
isNot: this.isNot,
}) +
'\n\n' +
`${printLabel(labelExpected)}${printExpected(value)}\n` +
`${printLabel(labelReceived)}${printReceived(collection)}`; |
Test plan
|
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 is so good
|
||
return ( | ||
matcherHint('.toHaveLength', 'received', 'length', { | ||
isNot: this.isNot, |
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.
I feel like we should make isNot
option mandatory, it's too easy to forget.
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, while we walk forward by incremental steps, let’s think about end goals for interface.
By the way, it took me a while to understand this.isNot
is equivalent to pass
for message
function: if test passes, print message only if assertion has .not
modifier.
But this.isNot
property removes [.not]
uncertainty in messages about invalid expected or received values. During this pass (pardon pun :) I have been letting marinate in my mind the patterns that we will need to improve those messages.
Codecov Report
@@ Coverage Diff @@
## master #7152 +/- ##
==========================================
+ Coverage 67.98% 68.01% +0.03%
==========================================
Files 248 248
Lines 9476 9487 +11
Branches 6 5 -1
==========================================
+ Hits 6442 6453 +11
Misses 3032 3032
Partials 2 2
Continue to review full report at Codecov.
|
Let's do it @pedrottimark! |
* expect: Improve report when assertion fails, part 3 * Display lines of labeled values as two columns with monospace alignment * Merge and then update CHANGELOG.md * Add test for getLabelPrinter * Include received type in label for toHaveLength matcher * Fix pretty lint error * Refactor message function in toContain and toContainEqual * Align values for toBeCloseTo
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
Delete paraphrased info from value labels which the description line can communicate by formatting.
Expected:
andReceived:
have same length without paraphrased info, display values starting on same line instead of indented on following line.Will continue in small chunks picking up from #5437 and #5512
After small chunks, matcher name from dim to regular font weight will affect hundreds of snapshots.
Test plan
Updated 33 snapshots.
Examples of baseline and improved reports:
Examples of baseline and improved reports with
.not
notice regular font weight: