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

Issue7105 #7540

Closed
wants to merge 5 commits into from
Closed

Issue7105 #7540

wants to merge 5 commits into from

Conversation

joanasmramos
Copy link

Summary

This branch is aiming to fix issue #7105 providing clearer fail messages for toBeGreaterThan and toBeLessThan

Test plan

I made two test cases for a function that returns the sum of its two arguments. The first test case verifies if 5+5 is greater than 10. This was the output before:
tobegreaterthan_reproduce
And now:
tobegreaterthan_fixed

The second test verifies if 5+5 is less than 10. This was the output before:
tobelessthan_reproduce
And now:
tobelessthan_fixed

@pedrottimark
Copy link
Contributor

@joanasmramos Thank you for making a pull request to fix the issue.

We agree there is room to improve the report when an assertion fails in Jest.

A goal for changes is let people quickly see information that is relevant to them.

An early step is some matchers like toBeGreaterThan and toBeLessThan now have short parallel labels Expected and Received instead of previous wordy message.

Because the labels and values are aligned like two columns in a table, I prefer not to extend the first label from Expected to Expected greater than or Expected less than

A next step (which we have been slow to achieve) is to replace the deleted words with visual contrast: matcher name as regular black instead of dim gray in the line at the beginning of the report:

expect(received).toBeGreaterThan(expected)
expect(received).toBeLessThan(expected

Improved assertion line will also display any not or rejects or resolves in regular black.

To put it another way, goal is people can choose to focus on the values, or the assertion, or both.

Because there are hundreds of updated snapshots to review, we need to change format of matcher name in small batches of a few related matchers (like these two matchers) in each pull request.

We would be happy for you to take a share in this effort:

  • The good news is there will be opportunity for you to make several pull requests.
  • The bad news is I ask you to let us leave this pull request waiting until we are ready.

When we have merged the prerequisite improvement, I can add a comment to this pull request:

  • with a link to pull request for other matchers so you can see which changes that I made
  • and a mention of your user name, so you receive a notification

If we (especially the we who is me :) can get our act together sooner than later, it would be an honor if Jest can merge your first pull requests on a public project. If developer experience and information design interest you, there are also other rough edges in Jest that deserve attention in 2019.

P.S. one source that has helped me most is Non Designer’s Design Book by Robin Williams

@thymikee
Copy link
Collaborator

thymikee commented Jan 4, 2019

Thanks @joanasmramos for tackling that!

There's ongoing effort from @pedrottimark to improve the expect output with #7557 adding bold matcher names for readability. Let's close this and focus on mentioned PR.

@thymikee thymikee closed this Jan 4, 2019
@pedrottimark
Copy link
Contributor

@joanasmramos Thank you for your patience.

#7557 has been merged which will help you open a new pull request.

The goal is to “divide and conquer” visually so readers can focus on either or both of the following:

  • clear and correct summary of the assertion that failed
  • consistent simple labels Expected and Received

Please look at two of the Changed Files in the recently merged PR as an example:

Adapting the changes to toBeDefined in expect/src/matchers.js file, update toBeGreaterThan and toBeGreaterThanOrEqual and toBeLessThan and toBeLessThanOrEqual functions:

  • Rename actual as received so variable and label is consistent :)
  • Delete . from matcher name argument in ensureNumbers and matcherHint function calls to display it as regular black instead of dim gray
  • Factor out fourth options argument in matcherHint function call as options variable so you can add options as fourth argument of ensureNumbers function call
  • Add promise: this.promise property to options object (in case the matcher is called with .rejects or .resolves modifier, but you don’t need to add tests for that possibility)

Adapting changes to ensureNoExpected in jest-matcher-utils/src/index.js file update ensureActualIsNumber, ensureExpectedIsNumber, and ensureNumbers functions:

  • Add optional options?: MatcherHintOptions as new last argument
  • Delete matcherName || (matcherName = 'This matcher'); default assignment
  • Add const matcherString = (options ? '' : '[.not]') + matcherName; assignment
  • Replace matcherHint('[.not]' + matcherName) with matcherHint(matcherString, undefined, undefined, options)

Because change to exported name ensureActualIsNumber would break dependents on the package, let’s leave the actual argument name as it is.

After you make the changes and verify that the changes in failing snapshots are correct, you can update them with command yarn jest matchers --testNamePattern="toBeGreaterThan" -u

@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 12, 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