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/jest-matcher-utils: Improve report when assertion fails, part 5 #7557

Merged
merged 14 commits into from Jan 11, 2019

Conversation

Projects
None yet
5 participants
@pedrottimark
Copy link
Collaborator

pedrottimark commented Dec 28, 2018

Summary

Goal: people can quickly see information that is relevant to them

To replace redundant descriptions, and be consistent with stack frame, format as regular black:

  • promise resolves or rejects
  • not
  • matcher name

Added promise property to this of matcher methods and to options object argument of matcherHint function.

For backward compatibility of community matchers and so we can update snapshots in batches, remove period from matcher name to enable the improvement. For example, replace '.toBeDefined' with 'toBeDefined' in first argument of matcherHint function call.

Test plan

The change column in the following table refers to one or more of the following:

  • omit extra escape sequences to close and reopen dim gray color of closing paren when matcher has no expected value
  • matcher name in regular black instead of dim gray color
  • lowercase promise to omit unnecessary case distinction in error message
  • separate description of why assertion failed from the received value
module file tests change
1 expect assertionCounts expected
50 expect spyMatchers expected
2 expect toThrowMatchers expected
26 expect matchers resolves and rejects expected, name, promise, separate
20 expect matchers toBeDefined and toBeUndefined expected, name
32 expect matchers toBeFalsy and toBeTruthy expected, name
14 expect matchers toBeNaN expected, name
10 expect matchers toBeNull expected, name; also corrected test string
2 jest-matcher-utils index ensureNoExpected replace obsolete test with relevant test
1 e2e failures async separate

See also pictures in following comment

Fixes #7105

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Dec 28, 2018

Pictures of improved at right in which assertion line is clear and consistent with stack frame.

When received value is incorrect according to matcher criterion:

message

When received value must be a promise:

received

When received promise rejected/resolved instead of resolved/rejected

Updated the following picture according to #7557 (comment)

instead improved

When matcher must not have an expected argument:

Updated the following picture according to #7557 (comment)

ensurenoexpected improved

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 28, 2018

Codecov Report

Merging #7557 into master will increase coverage by 0.06%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7557      +/-   ##
==========================================
+ Coverage   67.99%   68.06%   +0.06%     
==========================================
  Files         248      248              
  Lines        9490     9522      +32     
  Branches        6        5       -1     
==========================================
+ Hits         6453     6481      +28     
- Misses       3035     3039       +4     
  Partials        2        2
Impacted Files Coverage Δ
packages/expect/src/matchers.js 99.43% <100%> (+0.01%) ⬆️
packages/expect/src/index.js 93.96% <100%> (ø) ⬆️
packages/jest-matcher-utils/src/index.js 94.73% <86.66%> (-5.27%) ⬇️

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 b2ffe3d...e949202. Read the comment docs.

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Dec 28, 2018

When expected value must be omitted or undefined

I know this is behavior that we have currently, but using "expected" there seems out of place, because there's no such in the matcher hint. How about just using the "received" as in improved messages for resolves/rejects matchers?

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Dec 28, 2018

Your second pair of eyes is always helpful. Yes, I agree that hint and message don’t add up.

Here is a picture with expected in the matcher hint only for this error message:

ensurenoexpected-6-rejects-true copy

I’m not sure if I understood the alternative htat you meant. Can you say it in other words?

I will let it marinate in my mind whether there is another way to communicate the problem.

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Dec 28, 2018

I was thinking about:

Matcher error: received value must be omitted or undefined

Received has value: null

Does that sound right? It seems on pair with the new promise-related messages.

Here is a picture with expected in the matcher hint only for this error message:

Pretty sure adding the "expected" as an argument of argument-less fn is pretty confusing as well.

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Dec 29, 2018

Received seems like it applies to an incorrect assertion like expect(null).hasAssertions()

What do you think about the phrase expected argument in black?

ensurenoexpected-6-rejects-true copy

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Dec 29, 2018

Sounds good 👍

@thymikee thymikee referenced this pull request Jan 4, 2019

Closed

Issue7105 #7540

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Jan 4, 2019

A jest-circus test timing out indicates a slowdown in execution. Worth investigating. I've re-run it 2 times just be sure, still failing.

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jan 4, 2019

Yeah, some of the CI results have been a puzzle to me.

  • When node 10 failed, I merged some changes from master, and then jest-test-circus failed.
  • After merging most recent changes from master, now they both pass.
@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Jan 4, 2019

@SimenB can you have a final look? I think it's good to merge now

@thymikee thymikee requested a review from rickhanlonii Jan 4, 2019

@thymikee thymikee requested a review from SimenB Jan 11, 2019

@SimenB

SimenB approved these changes Jan 11, 2019

@thymikee thymikee merged commit f425bd4 into facebook:master Jan 11, 2019

10 checks passed

ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@pedrottimark pedrottimark deleted the pedrottimark:improve-report-5 branch Feb 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment