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 19 #8367

Merged
merged 5 commits into from Apr 25, 2019

Conversation

Projects
None yet
5 participants
@pedrottimark
Copy link
Collaborator

commented Apr 23, 2019

Summary

Follow up on residue from #7621 and #8077 according to recent improvements

For toBeInstanceOf matcher:

Factor out printConstructorName similar to printWithType function in jest-matcher-utils

  • Positive assertion: show received value only if it is not function with prototype otherwise display received constructor name
  • Negative assertion: instead of received value, show received constructor name only if it is not equal to expected assertion name

For toThrow(Constructor) matcher:

  • Display constructor name consistent with toBeInstanceOf
  • Omit expected and received name property, because it is usually redundant with constructor name

Residue: why TypeScript errors for received: unknown in printReceivedConstructorName

Test plan

matcher pass snapshots
toBeInstanceOf false updated 9
toBeInstanceOf true updated 5
toThrow(Constructor) false updated 8
toThrow(Constructor) true updated 2 and added 2

See also pictures in following comments.

Example pictures baseline at left and improved at right

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 23, 2019

toBeInstanceOf

Positive assertion omits received value if it is function with prototype:

toBeInstanceOf false omit

Positive assertion shows received value otherwise:

toBeInstanceOf false show

Negative assertion omits received constructor name if it is equal:

toBeInstanceOf true omit

Negative assertion shows received constructor name otherwise:

Updated and added picture according to #8367 (comment)

toBeInstanceOf true show 2

toBeInstanceOf true ellipsis

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 23, 2019

toThrow(Constructor)

Positive assertion shows received constructor name when prop is inconsistent:

toThrow false error inconsistent

Positive assertion shows received constructor name when prop is consistent:

toThrow false error consistent

Positive assertion show non-error value instead of constructor name:

toThrow false non-error

Negative assertion omits received constructor name if it is equal when prop is inconsistent:

toThrow true same inconsistent

Negative assertion omits received constructor name if it is equal when prop is consistent:

toThrow true same consistent

Negative assertion shows received constructor name otherwise:

Updated picture according to #8367 (comment)

toThrow sub consistent 2

@SimenB

SimenB approved these changes Apr 23, 2019

Copy link
Collaborator

left a comment

Hell yeah!

@codecov-io

This comment has been minimized.

Copy link

commented Apr 23, 2019

Codecov Report

Merging #8367 into master will increase coverage by 0.01%.
The diff coverage is 76.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8367      +/-   ##
==========================================
+ Coverage   62.29%   62.31%   +0.01%     
==========================================
  Files         266      266              
  Lines       10729    10737       +8     
  Branches     2609     2612       +3     
==========================================
+ Hits         6684     6691       +7     
- Misses       3460     3462       +2     
+ Partials      585      584       -1
Impacted Files Coverage Δ
packages/expect/src/toThrowMatchers.ts 86.48% <100%> (+0.24%) ⬆️
packages/expect/src/matchers.ts 96.79% <100%> (+0.96%) ⬆️
packages/expect/src/print.ts 88.37% <69.23%> (-8.3%) ⬇️

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 4d3c1a1...f55e9ed. Read the comment docs.

"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toThrowError<dim>(</><green>expected</><dim>)</>

Expected constructor: not <green>Err</>
Received constructor: <red>SubErr</>

This comment has been minimized.

Copy link
@jeysal

jeysal Apr 24, 2019

Collaborator

Maybe we should print something for subclasses? It might not always be obvious why this failed if the names look unrelated

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Apr 24, 2019

Author Collaborator

Yeah, it isn’t obvious. Your thoughts how to make it clear are welcome. It reminds me of (positive and negative) assertions for < and so on, where report shows the comparison operator.

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Apr 24, 2019

Author Collaborator

What do you think of this?

not_toBeInstanceOf_base_class

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Apr 24, 2019

Author Collaborator

Reminds of the joke:

I’m glad I don’t like X, because if I liked it, I would have to eat it, but I don’t like it!

This comment has been minimized.

Copy link
@jeysal

jeysal Apr 24, 2019

Collaborator

What do you think of this?

The idea is super awesome, unfortunately it's not quite correct though, C.prototype instanceof B would be but that's clunky.
What do you think about C extends B?

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Apr 24, 2019

Author Collaborator

Yes, your suggestion is very intuitive!

Do we extend the notation (pardon pun ;) for C extends MiddleClass extends B

This comment has been minimized.

Copy link
@jeysal

jeysal Apr 24, 2019

Collaborator

I guess if it's not too much work, why not. Are there many hundred prototypes long chains in the real world? If so, we might need to limit it?

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Apr 24, 2019

Author Collaborator

Here is a compromise that might balance clear and correct without over-explaining:

Expected constructor: not Parent
Received constructor:     Child extends Parent
Expected constructor: not Ancestor
Received constructor:     Descendant extends … extends Ancestor

This comment has been minimized.

Copy link
@jeysal

jeysal Apr 24, 2019

Collaborator

extends ... extends is exactly what I thought of 👌

Show resolved Hide resolved packages/expect/src/__tests__/__snapshots__/toThrowMatchers.test.js.snap
@jeysal

jeysal approved these changes Apr 25, 2019

Copy link
Collaborator

left a comment

Nice!

@jeysal jeysal merged commit bf6c71a into facebook:master Apr 25, 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-11 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/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@pedrottimark pedrottimark deleted the pedrottimark:improve-report-19 branch Apr 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.