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 assertion fails, part 3 #7152

Merged
merged 11 commits into from Dec 28, 2018

Conversation

Projects
None yet
6 participants
@pedrottimark
Copy link
Collaborator

pedrottimark commented Oct 12, 2018

Summary

Delete paraphrased info from value labels which the description line can communicate by formatting.

  • “Expected …” phrase above received value makes me stop and reread every time :(
  • Especially because Expected: and Received: have same length without paraphrased info, display values starting on same line instead of indented on following line.

Will continue in small chunks picking up from #5437 and #5512

After small chunks, matcher name from dim to regular font weight will affect hundreds of snapshots.

Test plan

Updated 33 snapshots.

Examples of baseline and improved reports:

tocontain

Examples of baseline and improved reports with .not notice regular font weight:

not-tocontain

@SimenB SimenB requested review from rickhanlonii and cpojer and removed request for rickhanlonii Oct 12, 2018

@rickhanlonii

This comment has been minimized.

Copy link
Member

rickhanlonii commented Oct 13, 2018

Really like this improvement (especially how it simplifies the mather code)

What happens when the received value is a multiline string? Like a pretty formatted object? My thinking is that we may still want the received value to be on a new line

@rickhanlonii

This comment has been minimized.

Copy link
Member

rickhanlonii commented Oct 13, 2018

Just realized that the received value can only be an object and we control the printing so nvm

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Oct 13, 2018

Could we try to align the right side of the colons? VIsual comparison is easier if the line up. Thankfully "received" and "expected" are both 8 characters longs, so it's a matter of padding enough for "value"/"string"/"length"/"array" etc

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 13, 2018

@rickhanlonii Here is picture of multiline string as received value:

tocontain-multiline-string

@SimenB More alignment is more better! I will make a new picture formatted as you suggest.

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 13, 2018

What do y’all think about padding spaces at right of colon to align values:

tocontain-2

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 13, 2018

Before I push changes, I need think more about use cases for getLabelPrinter higher-order function which jest-matcher-utils package will export so people can call it in custom matchers.

Here is code I wrote to make the picture:

type PrintLabel = string => string; // append colon, space, and padding spaces
const getLabelPrinter = (...labels: Array<string>): PrintLabel => {
  const maxLength = Math.max(...labels.map(arg => arg.length));
  return label => `${label}: ${' '.repeat(maxLength - label.length)}`;
};

// in toContainEqual matcher
    const labelExpected = 'Expected value';
    const labelReceived = `Received ${collectionType}`;
    const printLabel = getLabelPrinter(labelExpected, labelReceived);

    const message = () =>
      matcherHint('.toContainEqual', collectionType, 'value', {
        comment,
        isNot: this.isNot,
      }) +
      '\n\n' +
      `${printLabel(labelExpected)}${printExpected(value)}\n` +
      `${printLabel(labelReceived)}${printReceived(collection)}`;

pedrottimark added some commits Oct 15, 2018

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 15, 2018

Test plan

  • Updated 16 snapshot tests in expect package for aligned value
  • Added 7 tests in jest-matcher-utils package for getLabelPrinter function
@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 15, 2018

Received collection type in toHaveLength matcher and updated 10 snapshot tests.

tohavelength-type

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 15, 2018

Aligned values and updated 12 snapshot tests for toBeCloseTo to restart CI honestly :)

tobecloseto

@thymikee
Copy link
Collaborator

thymikee left a comment

This is so good

return (
matcherHint('.toHaveLength', 'received', 'length', {
isNot: this.isNot,

This comment has been minimized.

@thymikee

thymikee Oct 16, 2018

Collaborator

I feel like we should make isNot option mandatory, it's too easy to forget.

This comment has been minimized.

@pedrottimark

pedrottimark Oct 16, 2018

Author Collaborator

Yes, while we walk forward by incremental steps, let’s think about end goals for interface.

By the way, it took me a while to understand this.isNot is equivalent to pass for message function: if test passes, print message only if assertion has .not modifier.

But this.isNot property removes [.not] uncertainty in messages about invalid expected or received values. During this pass (pardon pun :) I have been letting marinate in my mind the patterns that we will need to improve those messages.

pedrottimark added some commits Dec 22, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 26, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7152      +/-   ##
==========================================
+ Coverage   67.98%   68.01%   +0.03%     
==========================================
  Files         248      248              
  Lines        9476     9487      +11     
  Branches        6        5       -1     
==========================================
+ Hits         6442     6453      +11     
  Misses       3032     3032              
  Partials        2        2
Impacted Files Coverage Δ
packages/jest-matcher-utils/src/index.js 100% <100%> (ø) ⬆️
packages/expect/src/matchers.js 99.41% <100%> (+0.02%) ⬆️

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 f13b97b...dd45440. Read the comment docs.

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Dec 28, 2018

Let's do it @pedrottimark!

@thymikee thymikee merged commit bf2a42d into facebook:master Dec 28, 2018

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

@pedrottimark pedrottimark deleted the pedrottimark:improve-report-3 branch Dec 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment