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 15 #8281

Merged
merged 8 commits into from
Apr 8, 2019

Conversation

pedrottimark
Copy link
Contributor

@pedrottimark pedrottimark commented Apr 6, 2019

Summary

For toBe matcher:

  • Display matcher name in regular black instead of dim color
  • Display promise rejects or resolves in matcher hint
  • Omit redundant or irrelevant information, as described below

When negative assertion fails:

  • Display not between expected label and value
  • Omit received value because it is redundant

When positive assertion fails:

  • Display either diff (without Difference label) or Expected/Received values, not both
  • Display either Difference or message about deep equality independent of the above , not both
  • Display Received value has no visual difference in black instead of dim
  • Display If the test should pass with deep equality, replace toBe with ${deepEqualityName} to emphasize toStrictEqual more than toEqual matcher

@jeysal your work on shouldPrintDiff in #7605 gave me important example for the logic :)

Residue: pretty-format serializes [, 1] and [undefined, 1] the same (it would be a breaking change to fix for sparse arrays, but it is an example for improvement via data-driven diff)

Test plan

  • Updated 18 snapshot tests
  • Added 7 9 snapshot tests

Friendly reviewers, you have my apology for mess of some added and updated snapshots :(

See also pictures in following comments with baseline at left and improved at right

@pedrottimark pedrottimark changed the title expect: Improve report when matcher fail, part 15 expect: Improve report when matcher fails, part 15 Apr 6, 2019
@pedrottimark
Copy link
Contributor Author

Received value is redundant in negative assertion of referential identity:

true string

Display difference if it is relevant:

false truthy object unequal

false truthy string 4-3

@pedrottimark
Copy link
Contributor Author

Display message about no visual difference without irrelevant Difference label:

false falsey no-visual function

false falsey no-visual symbol

@pedrottimark
Copy link
Contributor Author

pedrottimark commented Apr 6, 2019

See #8281 (comment) instead of this comment

Your critique and improvement of the proposed message is welcome:

  • If the testit should pass with deep equality, replace toBe with toStrictEqual
  • If the testit should pass with deep equality, replace toBe with toEqual

Rewrite message not to mislead for expected failures during first phase of TDD and to emphasize toStrictEqual more than toEqual matcher, sorry about ruthless cropping at left edge of pictures:

false falsey no-visual object toStrictEqual

false falsey no-visual object toEqual

If referential identity test fails and deep equality test would pass, is it okay to omit the diff?

false falsey object toStrictEqual

false falsey object toEqual

@pedrottimark
Copy link
Contributor Author

Display primitive as Expected/Received values:

false falsey boolean

false falsey number

false falsey named symbol

Different types:

false falsey primitive types

false falsey structure types

At least one string is not multiline:

false falsey string 1-1

false falsey string 2-1

Specific other types:

false falsey error

false falsey regexp

false falsey named function

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.

This output is a lot smarter, thanks @pedrottimark.
I love the part about trying toStrictEqual and then toEqual to suggest what's likely the better fit.

packages/expect/src/matchers.ts Outdated Show resolved Hide resolved
(received instanceof Error && expected instanceof Error);

const deepEqualityName =
isShallow || (expectedType !== 'object' && expectedType !== 'array')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why the checks on this line are necessary and why deepEqualityName affects difference a few lines below? I'm probably missing something, I don't quite understand why the logic needs to be this complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed isShallowInequality and refactored assignments to deepEqualityName

Your intuition was correct that difference was too complex.

I moved the message below hint and above difference or Expected/Received lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, now the deepEqualityName and difference concerns are nicely separated. I've just tried it locally and it looks like you can also remove the deepEqualityName conditions? At least it doesn't seem to fail any test.

-          if (!isShallowInequality) {
-            if (expectedType === 'object' || expectedType === 'array') {

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 indeed, thank you for patient critique!

  • For deepEqualityName the only if condition is skip map and set until we review the current questionable logic
  • Separate and rename hasConciseReport to make its purpose clear to short-circuit a line diff (while we improve the remaining matchers, we will discover whether it is reusable)
  • Added condition and 2 snapshot tests for deep equal and unequal dates

@pedrottimark
Copy link
Contributor Author

Updated pictures supersede #8281 (comment) and illustrate #8281 (comment)

false falsey object toStrictEqual 2

false falsey object toEqual 2

false falsey no-visual object toStrictEqual 2

false falsey no-visual object toEqual 2

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

👍

packages/expect/src/matchers.ts Outdated 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