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 13 #8077

Merged
merged 6 commits into from
Mar 20, 2019

Conversation

pedrottimark
Copy link
Contributor

Summary

For toBeInstanceOf matcher:

  • Display matcher name in regular black instead of dim color
  • Display not following expected label
  • Replace confusing constructor name with sentence if received value has no prototype chain
  • Omit expected or received constructor name in edge cases

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

The report is less helpful than it might be because min option of pretty-format omits class name :(

Residue: Improve report for toThrow with expected error class especially if received error.name is not the same as error.constructor.name

Test plan

  • Updated 11 snapshot tests
  • Added 4 snapshot tests

See also pictures in following comment.

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Mar 7, 2019

8 example pictures baseline at left and improved at right

EDIT: updated 3 pictures according to #8077 (comment) and 3 according to #8077 (comment)

Sentence instead of empty string for null or undefined

tobeinstanceof false no proto undefine 2

Sentence instead of confusing constructor name for other primitive values

tobeinstanceof false no proto string 2

Sentence instead of undefined if object does not have a prototype chain

tobeinstanceof false no proto object create null 2

Omit Sentence about expected if constructor has empty string as its name

toBeInstanceOf false constructor has empty name 2

Omit Sentence about received if constructor function overrides name property

toBeInstanceOf false constructor defines name prop 2

Display not following expected label for literal array

tobeinstanceof true array

Display not following expected label for instance of subclass

tobeinstanceof true subclass

Omit Sentence about expected if class has static name method

toBeInstanceOf true class 2

@jeysal
Copy link
Contributor

jeysal commented Mar 7, 2019

"Received value does not have a prototype chain" is a rather cumbersome message, can we cut it down to "Received value has no prototype"? I don't think "chain" is needed (anything that has a prototype also has a prototype chain of length >= 1), unless you wanted to express something specific with it?

@pedrottimark
Copy link
Contributor Author

Thank you for constructive critique!

@jeysal
Copy link
Contributor

jeysal commented Mar 8, 2019

In the "Omit expected if constructor has empty string as its name" case maybe we should still indicate that Expected was an unnamed constructor? "Expected unnamed constructor" or something? Or maybe it's fine because the user can identify the constructor in the code frame anyway, what do you think?

@pedrottimark
Copy link
Contributor Author

Yes, I agree with short phrases instead of omitted lines. Bonus points to start same as the labels:

  • Expected constructor blah blah blah
  • Received constructor yada yada yada

@pedrottimark
Copy link
Contributor Author

Here are two phrases so length of report is consistent positive 3 lines and negative 2 lines:

  • Expected/Received constructor name is not a string
  • Expected/Received constructor name is an empty string

Omit expected if class has static name method:

expect(received).not.toBeInstanceOf(expected)

Expected constructor name is not a string
Received value: {}

Omit expected if constructor has empty string as its name:

expect(received).toBeInstanceOf(expected)

Expected constructor name is an empty string
Received constructor: RegExp
Received value: /\w+/

P.S. Because pretty-format serializes strings in double quote marks, empty string is not usually a special case. For this matcher (and for toThrow with expected error class) there is a risk for report to give incorrect impression that the constructor name is the criterion.

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.

LGTM!

@pedrottimark
Copy link
Contributor Author

For what it’s worth, I have see this warning in local yarn test with node 8.12.0:

MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 exit listeners added. Use emitter.setMaxListeners() to increase limit

@SimenB
Copy link
Member

SimenB commented Mar 18, 2019

Yeah, it's from watch.test.js - it's not something to worry about

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

4 participants