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 4 #7241

Merged
merged 15 commits into from Dec 18, 2018

Conversation

Projects
None yet
7 participants
@pedrottimark
Copy link
Collaborator

pedrottimark commented Oct 22, 2018

Summary

Improve report when matcher throws error:

  • contrast with reports for ordinary test failures
  • consistent format and wording
  • remove ambiguity when Expected was used with received values
  • remove ambiguity when Received was used with expected values

Added condition Expected value must be number to toHaveLength matcher

Residue for future pull request:

  • toMatchObject: find and fix pass must be initialized
  • replace indefinite [.not] with definite presence or absence of .not in to_throw_matchers.js and spy_matchers.js

Test plan

package test file
expect assertion_counts
expect matchers
expect spy_matchers
expect to_throw_matchers.test.js
jest-matcher-utils index

See pictures in following comments but see also improvements in #7241 (comment)

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 22, 2018

Examples from matchers.js

matchers

Example that hint displays .not matcher

not-tomatchobject-throw

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 22, 2018

Example from to_throw_matchers.js

to_throw_matchers

Baseline doesn’t display .not correctly. Improved displays it indefinitely for now.

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 22, 2018

Examples from spy_matchers.js

spy_matchers

Baseline and improved display .not indefinitely.

@pedrottimark pedrottimark changed the title Improve report 4 expect: Improve report when assertion fails, part 4 Oct 22, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 23, 2018

Codecov Report

Merging #7241 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7241      +/-   ##
==========================================
+ Coverage   67.46%   67.48%   +0.02%     
==========================================
  Files         247      247              
  Lines        9516     9522       +6     
  Branches        5        5              
==========================================
+ Hits         6420     6426       +6     
  Misses       3094     3094              
  Partials        2        2
Impacted Files Coverage Δ
packages/expect/src/toThrowMatchers.js 92.3% <ø> (ø) ⬆️
packages/expect/src/index.js 94.01% <ø> (ø) ⬆️
packages/expect/src/spyMatchers.js 97.44% <ø> (ø) ⬆️
packages/expect/src/matchers.js 99.39% <100%> (ø) ⬆️
packages/jest-matcher-utils/src/index.js 100% <100%> (ø) ⬆️

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 df78255...09e5bc9. Read the comment docs.

@thymikee
Copy link
Collaborator

thymikee left a comment

Nice work! I love these little improvements.
I've left some comments regarding the language (should be applied throughout this diff, not only in marked places)

Show resolved Hide resolved packages/expect/src/__tests__/__snapshots__/to_throw_matchers.test.js.snap Outdated
Show resolved Hide resolved packages/expect/src/__tests__/__snapshots__/to_throw_matchers.test.js.snap Outdated
Show resolved Hide resolved packages/expect/src/index.js Outdated
Show resolved Hide resolved packages/expect/src/matchers.js Outdated
Show resolved Hide resolved packages/expect/src/matchers.js Outdated
@natealcedo

This comment has been minimized.

Copy link
Contributor

natealcedo commented Oct 24, 2018

@pedrottimark Thanks for the good work! From what I've observed so far, the work you've been doing does two things.

  1. Standardise validation errors via matcherUtils.matcherErrorMessage
  2. Making the .not case highlighted.

I think it would be good to document the standard you have somewhere in the contributing.md under a section like For library authors since matcherUtils is what is being used as well. This way libraries like jest-extended and jest-enzyme have a point of reference. I imagine we'd like to have all matchers to look similar for error messages and some work is required to replicate this standard across.

Thoughts @SimenB @mattphillips ?

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Oct 24, 2018

Should be added in a readme for https://github.com/facebook/jest/tree/master/packages/jest-matcher-utils and maybe under https://jestjs.io/docs/en/expect#expectextendmatchers

Not sure it should be in contributing?

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Oct 24, 2018

Just an idea, but what about, instead of Expected number: <red>17</red> we do Expected: <red>Number<17></red>? I feel like the fact that "you passed the number 17 as expected" is a bit more.... obscured now. The part the user did is both before and after the colon.

Clarity and scannability of the assertion error is very important, IMO. I should be able to at a glance see what I did wrong without having to read to much.

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 24, 2018

@thymikee Thank you for review with helpful suggestions:

package file updated
expect matchers.test.js 59 snapshots
expect spy_matchers.test.js 68 snapshots
expect to_throw_matchers.test.js 4 snapshots
jest-matcher-utils index.test.js 2 tests

EDIT 2018-12-12 replaced cannot with must not in 3 messages see #7241 (comment)

function message fixed
makeResolveMatcher Received value must be a Promise fixed
makeRejectMatcher Received value must be a Promise fixed
toBeInstanceOf Expected value must be a function fixed
toContain Received value must not be null nor undefined fixed
toContainEqual Received value must not be null nor undefined fixed
toHaveLength Received value must have a length property whose value must be a number fixed
toHaveLength Expected value must be a number fixed
toHaveProperty Received value must not be null nor undefined fixed
toHaveProperty Expected path must be a string or array fixed
toMatch Received value must be a string fixed
toMatch Expected value must be a string or regular expression fixed
toMatchObject Received value must be a non-null object fixed
toMatchObject Expected value must be non-null object fixed
ensureMock Received value must be a mock or spy function fixed
createMatcher Received value must be a function fixed
createMatcher Expected value must be a string or regular expression or Error fixed
ensureNoExpected Expected value must be omitted or undefined
ensureActualIsNumber Received value must be a number fixed
ensureExpectedIsNumber Expected value must be a number fixed
@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 24, 2018

@SimenB concerning #7241 (comment) yeah, it still feels like room for improvement:

  • Matcher error describes what the value must be or cannot be
  • Received or Expected line displays what type and value occurred, but it’s not obvious that type is incorrect
@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 24, 2018

@SimenB @thymikee @natealcedo What do you think of more verbose printWithType function:

printwithtype

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Oct 24, 2018

Add colors to expected/received and it would be perfect. But I like this proposal as is too :)

@natealcedo

This comment has been minimized.

Copy link
Contributor

natealcedo commented Oct 24, 2018

I personally love it! The benefits I see are as follows:

  1. Error message is less wordy. Less cognitive overhead trying to understand what went wrong.
  2. Easier to write error messages from a developer's point of view.
@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 24, 2018

@thymikee You mean like this:

printwithtype2

Although more consistent, I think it’s an example where less color is more clear information hierarchy.

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Oct 24, 2018

I like both, so let's keep what we have now. Thanks for showing that! :)

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Oct 24, 2018

I think the recieved in Matcher error should be red. Not needed in the has type, has value part, IMO

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 24, 2018

package file updated
expect assertion_counts.test.js replaced 1 regexp test with snapshot
expect matchers.test.js 61 snapshots
expect spy_matchers.test.js 76 snapshots
expect to_throw_matchers.test.js 4 snapshots
jest-matcher-utils index.test.js replaced 4 regexp tests with snapshots

Picture of additional improvements, except that matcher name in regular font weight waits for future pull request:

printwithtype5

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Oct 24, 2018

Could you update the changelog? 🙂

@SimenB

SimenB approved these changes Oct 24, 2018

pedrottimark added some commits Oct 25, 2018

@SimenB

SimenB approved these changes Oct 25, 2018

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Oct 25, 2018

Would love for any of @cpojer @rubennorte and @mjesun to chime in on these changes

@thymikee thymikee requested review from cpojer and rubennorte Oct 29, 2018

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Nov 16, 2018

I'd love to get this in. @pedrottimark mind rebasing?
@cpojer might taking a look?

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Nov 18, 2018

@thymikee Tests passing locally but CI failing. Maybe I messed up merging upstream changes and also your push to my origin (forgot those at first and then forgot to save my edits to resolve conflicts :)

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Nov 18, 2018

tests passed but exited with code 1? wtf

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Nov 18, 2018

tests passed but exited with code 1? wtf

Failed with an obsolete snapshot?

EDIT: Ah, that is not visible using the dot reporter. You can see it on appveyor, though

@rickhanlonii pls halp

@borilla

This comment has been minimized.

Copy link
Contributor

borilla commented Dec 9, 2018

I love the extra information being provided in these fail reports but, although it's kinda minor, feel there's a failing in the semantics of the newer messages: Given that we write some code along the lines of:

expect(X).toBe(Y);

where we previously had a message along the lines of, "Expected X to be Y", we now have a message saying "X must be Y"

IMO this breaks the semantics of the testing code and even the implication of the failure message; implying that the result is not just unexpected, but is logically incorrect

In plain English this is the equivalent of meeting someone new and saying to them either, "Oh, I expected you to be taller" or, "Oh, you should be taller"

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Dec 10, 2018

@borilla Thank you for your thoughts.

The phrase “…when assertion fails” in title of this PR might not have clearly communicated that there are only about 10 of these Matcher error reports for expected or received values that are incorrect according to the purpose of the matcher. That is, the assertion fails because the matcher is not written correctly, instead of because the received value does not match the expected criterion.

I am happy to receive critique about fluent meaning of words in international technical English :)

For example, I consulted https://www.ietf.org/rfc/rfc2119.txt to decide between must and should.

While reading the messages again, I see that cannot in one message is not exact opposite of must.

@thymikee Does a change to must not help or hurt:

  • Received value cannot be null nor undefined
  • Received value must not be null or undefined
@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Dec 10, 2018

@pedrottimark haha I'm not a native English speaker so I'm not a right person to ask :D But according to quick research "must not" seems better here, and it plays well with "must" we use already in other matchers.

@borilla

This comment has been minimized.

Copy link
Contributor

borilla commented Dec 10, 2018

@pedrottimark Thank you very much for taking time to clarify

In this case I agree that must and must not are the correct terms. I withdraw my reservations and look forward to the update

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Dec 12, 2018

Pictures of 3 updated reports:

tocontain-throw

tocontainequal-throw

tohaveproperty-throw-1

@thymikee thymikee merged commit 21733c3 into facebook:master Dec 18, 2018

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
@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Dec 18, 2018

@pedrottimark thanks for your patience, we all love your work on improving UI ❤️

@pedrottimark pedrottimark deleted the pedrottimark:improve-report-4 branch Dec 18, 2018

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