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 6 #7621

Merged
merged 15 commits into from Jan 20, 2019

Conversation

Projects
None yet
6 participants
@pedrottimark
Copy link
Collaborator

pedrottimark commented Jan 13, 2019

Summary

Goal: people can quickly see information that is relevant to them when toThrow fails

To replace redundant descriptions, and be consistent with stack frame, format as regular black:

  • promise resolves or rejects
  • not
  • matcher name

Replace arrow function with classic function to refer to properties of non-lexical this

Because of indentation change increateMatchertry review with option to ignore white space

Replace message separated from stack or serialized by pretty-format with labels and values

Factor out logic for throw statement with value other than Error object

For expected:

  • String: all new function instead of converting to RegExp
  • RegExp: call its test method because that is the condition of else if
  • Object: replace call to equals function with error.message === expected.message

I didn’t DRY out the message functions as much as theoretically possible:

  • To reduce noise in diff
  • To allow possible future improvement also for .toMatch inverse highlight matching substring

Test plan

Updated tests:

description
6 strings
6 regexp
6 error class
6 promise/async throws (and enabled 3 missing .toThrowError tests)
2 invalid arguments (and added .not modifier)
2 invalid actual

See also pictures in following comment

@SimenB

SimenB approved these changes Jan 13, 2019

Copy link
Collaborator

SimenB left a comment

this is awesome, as usual 😀

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jan 13, 2019

Pictures of baseline at left and improved at right with assertion line and labelled values

Expected value is substring of error message:

string

Expected value is regexp to test error message:

regexp

Expected value is class to test error instance:

class

Expected value is object to test error message:

object

Expected value is undefined:

undefined

Matcher errors:

x

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 13, 2019

Codecov Report

Merging #7621 into master will increase coverage by 0.08%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7621      +/-   ##
==========================================
+ Coverage   68.27%   68.35%   +0.08%     
==========================================
  Files         249      249              
  Lines        9623     9648      +25     
  Branches        5        6       +1     
==========================================
+ Hits         6570     6595      +25     
  Misses       3051     3051              
  Partials        2        2
Impacted Files Coverage Δ
packages/expect/src/index.js 94.01% <100%> (ø) ⬆️
packages/expect/src/toThrowMatchers.js 94.8% <94.36%> (+2.49%) ⬆️

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 066f0e3...4ea8044. Read the comment docs.

@thymikee
Copy link
Collaborator

thymikee left a comment

This is great as always. I left a comment about Flow types

Instead, it threw:
<red> Error</>
<red> <dim>at jestExpect (</>packages/expect/src/__tests__/toThrowMatchers-test.js<dim>:24:74)</></>"
Received error message: <red>\\"apple\\"</>

This comment has been minimized.

@thymikee

thymikee Jan 14, 2019

Collaborator

oh, love this separation, makes it way easier to trace

This comment has been minimized.

@pedrottimark

pedrottimark Jan 14, 2019

Author Collaborator

Proximity for the win :)

// Possible improvement also for toMatch inverse highlight matching substring.
// $FlowFixMe: Cannot get error.message because property message is missing in undefined
`Received error message: ${printReceived(error.message)}\n` +
// $FlowFixMe: Cannot get error.stack because property stack is missing in undefined

This comment has been minimized.

@thymikee

thymikee Jan 14, 2019

Collaborator

This is async code and theoretically there is a chance that error is mutated somewhere in between the calls. Can we guard this the same as on the other branch with error !== undefined?

This comment applies to all $FlowFixMes added in this file

This comment has been minimized.

@pedrottimark

pedrottimark Jan 14, 2019

Author Collaborator

As usual, your review comments are very helpful. Instead of the following code:

if (error && !error.message && !error.name) {
  error = new Error(error);
}

which has incorrect edge cases for falsey exception values like throw 0

I will rewrite as robust destructuring to provide message or name as arguments

This comment has been minimized.

@pedrottimark

pedrottimark Jan 14, 2019

Author Collaborator

Do you agree if exception is primitive value, then assertion should fail for expected class?

Now the following assertion passes, because of if statement in preceding comment:

expect(() => { throw true; }).toThrow(Error);

And for your enjoyment, here is the mother of all edge cases: () => { throw undefined; }

This comment has been minimized.

@SimenB

SimenB Jan 14, 2019

Collaborator

Do you agree if exception is primitive value, then assertion should fail for expected class?

Yes

@jeysal
Copy link
Contributor

jeysal left a comment

Left one comment on toThrow(someObj), other than that LGTM!


const pass = equals(error, expectedError);

This comment has been minimized.

@jeysal

jeysal Jan 14, 2019

Contributor

I'm not sure making toThrow(someObj) less strict is a good idea.

  • It's sort of breaking in that people might rely on their tests checking that thrown errors have some extra properties that should be set. After updating Jest and changing some code, their tests may now incorrectly pass.
  • Personally, it's also not the behavior I would expect. I would expect toThrow(someObj) to check more than just the message, because with these changes, it's almost the same as toThrow(someObj.message)

This comment has been minimized.

@pedrottimark

pedrottimark Jan 14, 2019

Author Collaborator

@jeysal Your energy to comment on issues and pull requests is like a New Year gift to Jest!

I will use the concepts from your first bullet point to evaluate changes to this PR so toThrow matcher is correct and report is clear when the received exception isn’t an error object.

When the expected value is an error object, can you believe that equals has following code:

if (a instanceof Error && b instanceof Error) {
  return a.message == b.message;
}

So this pull request:

  • moves similar logic into toThrow and removes ambiguity from its report
  • prepares code for possible future breaking change, see #4724 although I have reservations about the specific request to match concatenated name and message

This comment has been minimized.

@jeysal

jeysal Jan 14, 2019

Contributor

@jeysal Your energy to comment on issues and pull requests is like a New Year gift to Jest!

Been considering getting involved in this project for a few months now, Jest is so awesome and miles ahead of most other test runners 😄 No better time to start than the holidays, let's see if I can keep this up alongside work.

When the expected value is an error object, can you believe that equals has following code:

Oh okay, I didn't consider that equals might have such special handling.
So something like

const expected = new Error('stuff');
expected.extra = 1337;
expect(() => {
  const actual = new Error('stuff');
  actual.extra = 42;
  throw actual;
}).toThrow(expected);

already passes, I see (although it still feels weird).

This comment has been minimized.

@SimenB

SimenB Jan 22, 2019

Collaborator

@jeysal I agree with Mark, your contributions are super appreciated! I'd like to invite you to the team's chat on discord - if you're interested, could you send me an email so I can give you an invite link? 🙂 My email is in my profile

This comment has been minimized.

@jeysal

jeysal Jan 22, 2019

Contributor

Sure, sent you an email! 👍

@jeysal

jeysal approved these changes Jan 14, 2019

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jan 14, 2019

3 code snippets if y’all want to comment while I let them simmer in mental slow cooker tonight:

type ThrownProps = {
  message: string, // either exception.message or String(exception)
  name?: string, // undefined if exception is not error object
  stack?: string, // undefined if exception is not error object
};
  let thrownProps = null;

  if (fromPromise && isError(received)) {
    thrownProps = getThrownProps(received);
  } else {
    if (typeof received !== 'function') {
      //
    } else {
      try {
        received();
      } catch (exception) {
        thrownProps = getThrownProps(exception);
      }
    }
  }
const formatReceived = (
  label: string,
  thrownProps: ThrownProps | null,
  key: string
) => thrownProps !== null && typeof thrownProps[key] === 'string'
  ? `${label}${printReceived(thrownProps[key])}\n`
  : '';

// This helper is so we can easily see that corresponding labels and values align.
const formatExpected = (label, expected) => `${label}${printExpected(expected)}\n`;
@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jan 15, 2019

Before I commit code changes, here are pictures for y’all to critique.

The currently approved improved is at left and proposed improved is at right.

  • Reduce labels from 3 words to 2 words
  • Include logic and contextual labels for non-error exception value
  • Rewrite code to be clearly type safe

Expected value is undefined:

undefined

Expected value is substring of error message or exception value:

string

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 15, 2019

I'd prefer "thrown value" over "exception value"

And maybe expected pattern "pattern" should be expected pattern /pattern/ to make it a bit more obvious it's a regex?

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jan 15, 2019

Yes, I will change label to Thrown value: by the way it is only when .toThrow has no expected argument, otherwise the label is Received value: to contrast with Expected whatever:

Here are the corresponding pretty formatted values for string versus regexp argument:

Expected pattern: "an error"
Expected pattern: /an error/

If I understand what you mean, when the pattern is a string value, it is not as obvious that it means the criterion is whether the received message includes it as a substring?

@jeysal

This comment has been minimized.

Copy link
Contributor

jeysal commented Jan 15, 2019

Expected error message including: "an error" for strings?

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jan 15, 2019

How about Expected substring: for expected string argument?

Possible future improvement for .not.toMatch and .not.toThrow for regexp and string, inverse highlight the matched substring in the received message.

Also for .toThrow(error) display string diff of expected and received messages.

@jeysal

This comment has been minimized.

Copy link
Contributor

jeysal commented Jan 15, 2019

Expected substring doesn't make it clear that it only looks at the message IMO, I wouldn't mind making it a bit more verbose to clarify that.

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 15, 2019

+1 for "expected substring"

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jan 16, 2019

Changes indirectly motivated by feedback: reduce length of labels and support non-error values.

added updated description
4 6 strings
4 6 regexp
2 6 class
2 0 undefined
0 4 promise/async

Pictures of baseline at left and improved at right supersede preceding comments.

Expected value is substring of error message:

string

Expected value is regexp to test error message:

regexp

Expected value is class to test error instance:

class

Expected value is object to test error message:

object

Expected value is undefined:

undefined

pedrottimark added some commits Jan 16, 2019

@thymikee
Copy link
Collaborator

thymikee left a comment

Looks like a nice overall improvement. Ideas why code-frames on the screenshots differ?

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jan 17, 2019

Good eyes to notice inconsistent stack frames.

  • When I took baseline pictures, the functions were inline expect(() => { throw something })
  • Before I took improved pictures, I had factored out the function to see more realistic stack frame.

Complete picture with stack for RangeError followed by stack for test:

stack rangeerror

Complete picture for non-error thrown value therefore only stack for test:

stack non-error

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Jan 17, 2019

Aha, last test in picture for expected class was wrong. When I rewrote as .not.toThrow(Object) for non-error class, it caused me to correct invalid assumptions in type assertion and message function.

Here is updated picture:

class true did throw non-error truthy

Depending on how classes that extend Error set name property of instances, that report can still seem confusing, but I think that will have to wait for a future pull request to sort out :(

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 20, 2019

@pedrottimark this is awesome work! Feel free to merge if you consider it complete 🙂

@pedrottimark pedrottimark merged commit 10bb779 into facebook:master Jan 20, 2019

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 #20190117.3 succeeded
Details

@pedrottimark pedrottimark deleted the pedrottimark:improve-report-6 branch Jan 20, 2019

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