Skip to content
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 8 #7876

Merged
merged 3 commits into from Feb 13, 2019

Conversation

pedrottimark
Copy link
Contributor

@pedrottimark pedrottimark commented Feb 12, 2019

Summary

For .toBeCloseTo matcher:

  • Display matcher name in regular black instead of dim color
  • Display expected and received difference to make criterion more transparent
  • Repeat not following Expected: and Expected difference: labels
  • Experiment to omit received value if it is equal to the not expected value (am happy if y’all can suggest as an alternative a concise explicit way to communicate that is reason why test fails)

For more information, see discussion with @jeysal in #7795

Residue for future pull requests:

  1. In call to ensureNumbers helper function:

    • delete period in matcher name to display matcher name in regular black instead of dim color
    • provide options as argument to display promise and isNot
  2. Decide whether .toBeCloseTo throws matcher error:

    • when expected or received is NaN
    • when precision argument is not integer number

Test plan

Updated 17 snapshot tests

See also pictures in following comments

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Feb 12, 2019

Review pictures as if they have updated default format for differences in following #7876 (comment)

16 example pictures baseline at left and improved at right

6 expected is finite number

isfinite

4 expected is Infinity

infinity

3 expected is -Infinity

-infinity

3 expected is NaN

nan

JavaScript is consistent if confusing that NaN is not equal to NaN

@jeysal
Copy link
Contributor

jeysal commented Feb 12, 2019

So much nicer 👌

Agreed that it would be great to have a message without duplicate values if expected and received are equal, but one that still shows clearly why the assertion failed. Can't spontaneously think of a good way to do it that would be consistent with the regular message format though :/

Also, unconditionally using scientific notation might not be a good idea:
image
I think the default of Number.toString() that only uses scientific notation if a string representation would otherwise be very long might suffice?

@pedrottimark
Copy link
Contributor Author

Yes, simpler is much clearer to compare expected and received differences! What was I thinking?

Two updated improved examples are side by side:

tostring

@codecov-io
Copy link

Codecov Report

Merging #7876 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7876      +/-   ##
==========================================
+ Coverage   58.46%   58.52%   +0.05%     
==========================================
  Files         178      178              
  Lines        6638     6647       +9     
  Branches        6        5       -1     
==========================================
+ Hits         3881     3890       +9     
  Misses       2755     2755              
  Partials        2        2
Impacted Files Coverage Δ
packages/expect/src/matchers.js 99.48% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb471f0...52dffd0. Read the comment docs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is awesome

@pedrottimark
Copy link
Contributor Author

Almost a year later, we finally found a way to follow up on review #5512 (comment) by Michał:

I'd move precision below received

@SimenB SimenB merged commit 5050654 into jestjs:master Feb 13, 2019
@SimenB
Copy link
Member

SimenB commented Feb 13, 2019

I'm sure he's dancing with joy 💃

@pedrottimark pedrottimark deleted the improve-report-8 branch February 13, 2019 14:24
@thymikee
Copy link
Collaborator

🕺

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants