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

Update toStrictEqual failure message #7224

Merged
merged 2 commits into from
Oct 29, 2018

Conversation

natealcedo
Copy link
Contributor

Summary

This pull request updates the failure message returned by toStrictEqual so that it is more inline with what #7105 envisions.

Test plan

The matcher is tested with a snapshot test.

Here are a few screenshots of the before and after along with the test file.

Before: (I had to minimize the image to fit both tests in one screen)

image

After:

image

@codecov-io
Copy link

codecov-io commented Oct 20, 2018

Codecov Report

Merging #7224 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7224      +/-   ##
==========================================
+ Coverage   66.55%   66.59%   +0.03%     
==========================================
  Files         237      237              
  Lines        9317     9318       +1     
  Branches        3        4       +1     
==========================================
+ Hits         6201     6205       +4     
+ Misses       3115     3112       -3     
  Partials        1        1
Impacted Files Coverage Δ
packages/expect/src/matchers.js 99.37% <100%> (+1.89%) ⬆️

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 cdf7a22...1eaf254. Read the comment docs.

CHANGELOG.md Outdated
@@ -49,6 +49,7 @@
- `[jest-runtime]` Fix missing coverage when using negative glob pattern in `testMatch` ([#7170](https://github.com/facebook/jest/pull/7170))
- `[*]` Ensure `maxWorkers` is at least 1 (was 0 in some cases where there was only 1 CPU) ([#7182](https://github.com/facebook/jest/pull/7182))
- `[jest-runtime]` Fix transform cache invalidation when requiring a test file from multiple projects ([#7186](https://github.com/facebook/jest/pull/7186))
- `[expect]` Improves the failing message for `toStrictEqual` matcher. ([#7224])
Copy link
Member

Choose a reason for hiding this comment

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

please include the URL as well, so it's clickable outside of github 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Small hickup. Will do!

@pedrottimark
Copy link
Contributor

Thank you very much. Left one suggestion (first time I used the new feature in GitHub :)

To document the goal:

  • .toStrictEqual omit minified values because they distract from Differences
  • .not.toStrictEqual align Expected and Received values

@natealcedo
Copy link
Contributor Author

natealcedo commented Oct 21, 2018

@pedrottimark I don't see any suggestions on my end.

Could you further explain both points? I don't quite understand the first point. Your second point, what do you mean by align Expected and Received values. Do you mean something like this?

Expected: 
  { "a": "a" }

Received:
  { "b": "b" }

I was just following the style on the right as per our previous discussion.

47158284-4e586a00-d2b9-11e8-9f5e-d8a92b5d8f63

`Received:\n` +
` ${printReceived(received)}`
`Expected: ${printExpected(expected)}\n` +
`Received: ${printReceived(received)}\n`
Copy link
Contributor

Choose a reason for hiding this comment

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

Jest trims extra space in message, but let’s omit trailing newline for other dependents of expect package:

Suggested change
`Received: ${printReceived(received)}\n`
`Received: ${printReceived(received)}`

Copy link
Contributor Author

@natealcedo natealcedo Oct 23, 2018

Choose a reason for hiding this comment

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

I've updated this in the code. I just did a commit --amend --no-edit and forced push to my branch. Hence, you're not gonna see a new commit. Just in case you get confused by the lack of commits. :)

@pedrottimark
Copy link
Contributor

@natealcedo You have my apology if the comment was double confusing :)

  1. Sorry such a new feature that I didn’t realize I had not submitted the comment:

#7224 (review)

  1. What you did is correct for both positive and negative case.

@natealcedo natealcedo force-pushed the update-toStrictEqual branch 3 times, most recently from fabf094 to 1eaf254 Compare October 23, 2018 02:12
@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.

6 participants