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

pedrottimark
Copy link
Contributor

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
Copy link
Contributor Author

pedrottimark 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
Copy link
Contributor Author

pedrottimark 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

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.

Hell yeah!

@codecov-io
Copy link

codecov-io 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</>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of this?

not_toBeInstanceOf_base_class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, your suggestion is very intuitive!

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Nice!

@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 11, 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.

5 participants