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 11 #8008

Merged
merged 5 commits into from Mar 2, 2019

Conversation

@pedrottimark
Copy link
Collaborator

pedrottimark commented Feb 28, 2019

Summary

For .toContain, .toContainEqual, and .toMatch:

  • Display matcher name in regular black instead of dim color
  • Display rejects or resolves in matcher hint
  • Display comment in hint when toContain or toContainEqual throw matcher error
  • Display not following expected label
  • Inverse highlight the not-expected substring or array item in received value

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

Your critique about clarity is especially welcome, because I had to wrestle with the code not to add any more any type than the one already in toContain matcher.

Design discussion your thoughts are welcome for a possible future pull request:

  • Is // indexOf comment in matcher hint for toContain confusing if received is iterable (for example, set) instead of array?
  • What do y’all think when .toContain referential identity fails for item in array but deep equality succeeds, to highlight the item?
  • Let’s brainstorm how to rewrite the message that suggests .toContainEqual matcher to be less wordy and more neutral (for example, to fix mistake in Redux reducer, .toContain might fail in first phase of TDD even though referential identity is the correct criterion)

Residue for future pull requests:

  • Inverse highlight the not-expected substring in received value for .toThrow(string | RegExp) matcher
  • Breaking change to require .exec method in addition to .test method
  • Better organize print functions after all reports have been improved

Test plan

Updated existing snapshot tests:

matcher
14 .toContain
10 .toContainEqual
17 .toMatch
41 total

See also pictures in following comment.

@pedrottimark pedrottimark requested review from SimenB and thymikee Feb 28, 2019
@SimenB
SimenB approved these changes Feb 28, 2019
Copy link
Collaborator

SimenB left a comment

This looks great!

packages/expect/src/matchers.ts Outdated Show resolved Hide resolved
@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Feb 28, 2019

Is // indexOf comment in matcher hint for toContain confusing if received is iterable (for example, set) instead of array?

For now, I'd say "no". If (when) we add support for Set to toContain, we might want to tweak it.


Looking forward to your screenshots 😀

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Feb 28, 2019

Example pictures baseline at left and improved at right

Matcher name is regular black:

tocontain false item

Possible future improvement to highlight item found according to deep equality:

tocontain false deep equality

Can highlight substring in string:

tocontain true substring

Cannot (yet) highlight value in non-array iterable object:

tocontain true value

Matcher name is regular black:

tocontainequal false

Can highlight item in array:

tocontainequal true

Align expected and received values:

tomatch false

Can highlight pattern in string:

tomatch true

packages/expect/src/matchers.ts Outdated Show resolved Hide resolved
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 1, 2019

Codecov Report

Merging #8008 into master will increase coverage by 0.12%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8008      +/-   ##
==========================================
+ Coverage   64.12%   64.24%   +0.12%     
==========================================
  Files         259      260       +1     
  Lines       10150    10162      +12     
  Branches     1754     1764      +10     
==========================================
+ Hits         6509     6529      +20     
+ Misses       3255     3245      -10     
- Partials      386      388       +2
Impacted Files Coverage Δ
packages/jest-matcher-utils/src/index.ts 91.83% <100%> (+0.08%) ⬆️
packages/expect/src/print.ts 88.88% <88.88%> (ø)
packages/expect/src/matchers.ts 95.52% <90%> (-1.81%) ⬇️
packages/jest-get-type/src/index.ts 89.65% <0%> (-0.35%) ⬇️
packages/jest-jasmine2/src/jasmine/Env.js 0% <0%> (ø) ⬆️
packages/jest-each/src/bind.js 100% <0%> (ø) ⬆️
packages/jest-circus/src/utils.ts 16.12% <0%> (+0.5%) ⬆️
packages/jest-haste-map/src/index.ts 84.24% <0%> (+0.95%) ⬆️

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 591eb99...ed4f245. Read the comment docs.

const converted: any =
isArray || isString ? received : Array.from(received);
if (typeof received === 'string') {
const index = received.indexOf(String(expected));

This comment has been minimized.

Copy link
@SimenB

SimenB Mar 1, 2019

Collaborator

too lazy to check, but this reads to be like

expect('hello null in the middle').toContain(null) would pass? Since String(null) === 'null'

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Mar 1, 2019

Author Collaborator

Yes, good eyes. It is for correct typing of dubious behavior.

  1. Do you think it should throw matcher error if received is string and expected is not?
  2. If yes, is that a breaking change that we wait to add at Jest 25.

This comment has been minimized.

Copy link
@SimenB

SimenB Mar 1, 2019

Collaborator

it fails today, doesn't it? I'd say it's a bug. If an expect fails with "matcher error" or "null is not in string 'null'" shouldn't matter?

Or does expect('hello null in the middle').toContain(null) pass today?

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Mar 1, 2019

Author Collaborator

I will double-check but the original code is .indexOf(value) which implicitly coerces to string:

  test('null', () => {
    expect('isnullornot'.indexOf(null)).toBe(2);
  });

This comment has been minimized.

Copy link
@SimenB

SimenB Mar 1, 2019

Collaborator

Oh, I didn't know indexOf coerced to string! TIL. Same for includes actually ('isnullornot'.includes(null) === true). I'd still say it's a false positive, but I don't feel strongly about it

This comment has been minimized.

Copy link
@pedrottimark

pedrottimark Mar 2, 2019

Author Collaborator

What do you think if I include the code and tests commented out, with a comment that we can find: // BREAKING change to include in Jest 25

This comment has been minimized.

Copy link
@jeysal

jeysal Mar 2, 2019

Collaborator

For tracking the breaking change to be included, we should create an issue assigned to the milestone like #7749.
I'd prefer not to put significant amounts of commented out code into master, if you've already written it maybe push it to a branch and link that in the issue so it doesn't get lost?

This comment has been minimized.

Copy link
@SimenB

SimenB Mar 2, 2019

Collaborator

feel free to open one - we can stick all the tiny breakages we wanna make there

This comment has been minimized.

Copy link
@jeysal

jeysal Mar 2, 2019

Collaborator

Created #8023

This comment has been minimized.

Copy link
@jeysal

jeysal Mar 2, 2019

Collaborator

@SimenB Do you mean one for all of them? I think I'd prefer separate issues unless there is a group of breaking changes that have something in common. The small issues will already be grouped using the milestone.

packages/expect/src/matchers.ts Outdated Show resolved Hide resolved
packages/expect/src/matchers.ts Show resolved Hide resolved
@SimenB SimenB merged commit c3a0167 into facebook:master Mar 2, 2019
12 checks passed
12 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/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
facebook.jest #20190302.11 succeeded
Details
@pedrottimark pedrottimark deleted the pedrottimark:improve-report-11 branch Mar 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.