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
Contributor

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.

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.

This looks great!

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

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

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

@codecov-io
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));
Copy link
Member

Choose a reason for hiding this comment

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

too lazy to check, but this reads to be like

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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 if I include the code and tests commented out, with a comment that we can find: // BREAKING change to include in Jest 25

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Created #8023

Copy link
Contributor

Choose a reason for hiding this comment

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

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