expect: Improve report when assertion fails, part 6 #7621

pedrottimark · 2019-01-13T23:25:43Z

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

Codecov Report

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

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

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

thymikee · 2019-01-14T13:01:14Z

packages/expect/src/__tests__/__snapshots__/toThrowMatchers.test.js.snap

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


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

Proximity for the win :)

thymikee · 2019-01-14T13:14:28Z

packages/expect/src/toThrowMatchers.js

+        // 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 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

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

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; }

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

Yes

packages/expect/src/__tests__/toThrowMatchers.test.js

jeysal

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

jeysal · 2019-01-14T21:21:16Z

packages/expect/src/toThrowMatchers.js


-  const pass = equals(error, expectedError);


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)

@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 Confusing output from expect.toThrow #4724 although I have reservations about the specific request to match concatenated name and message

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

@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

Sure, sent you an email! 👍

pedrottimark · 2019-01-14T23:22:11Z

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 · 2019-01-15T20:18:01Z

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:

Expected value is substring of error message or exception value:

SimenB · 2019-01-15T20:27:57Z

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 · 2019-01-15T20:34:18Z

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 · 2019-01-15T20:57:05Z

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

pedrottimark · 2019-01-15T21:03:29Z

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 · 2019-01-15T21:09:03Z

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 · 2019-01-15T21:09:21Z

+1 for "expected substring"

pedrottimark · 2019-01-16T22:18:44Z

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:

Expected value is regexp to test error message:

Expected value is class to test error instance:

Expected value is object to test error message:

Expected value is undefined:

packages/expect/src/toThrowMatchers.js

thymikee

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

pedrottimark · 2019-01-17T14:57:30Z

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:

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

pedrottimark · 2019-01-17T15:51:10Z

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:

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 · 2019-01-20T15:39:49Z

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

* expect: Improve report when assertion fails, part 6 * Fix flow errors * Move new function to reduce noise in diff * Update CHANGELOG.md * Convert ANSI codes in promise/async snapshots * Rewrite promise/async snapshot tests * Reduce length of labels and handle non-error values * Add tests for non-error values * Fix prettier lint error * Update ExpectAPI.md * Improve grammar in ExpectAPI.md * Delete an exception from Received function did not throw * Correct typo in ExpectAPI.md * Correct type and report for non-error class

github-actions · 2021-05-12T03:12:24Z

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.

pedrottimark added 4 commits January 13, 2019 16:57

expect: Improve report when assertion fails, part 6

f04e1b1

Merge dependency upgrades especially flow

eb7902d

Fix flow errors

f09ad0a

Move new function to reduce noise in diff

9747691

facebook-github-bot added the cla signed label Jan 13, 2019

SimenB approved these changes Jan 13, 2019

View reviewed changes

packages/expect/src/__tests__/__snapshots__/toThrowMatchers.test.js.snap Outdated Show resolved Hide resolved

thymikee reviewed Jan 14, 2019

View reviewed changes

pedrottimark added 2 commits January 14, 2019 10:15

Update CHANGELOG.md

2a46840

Convert ANSI codes in promise/async snapshots

39daf2c

SimenB reviewed Jan 14, 2019

View reviewed changes

packages/expect/src/__tests__/toThrowMatchers.test.js Show resolved Hide resolved

Rewrite promise/async snapshot tests

fc766f1

jeysal suggested changes Jan 14, 2019

View reviewed changes

jeysal approved these changes Jan 14, 2019

View reviewed changes

pedrottimark added 2 commits January 16, 2019 15:12

Reduce length of labels and handle non-error values

495db2f

Add tests for non-error values

0c94113

pedrottimark added 2 commits January 16, 2019 17:21

Fix prettier lint error

44118a3

Update ExpectAPI.md

6d8a416

jeysal reviewed Jan 16, 2019

View reviewed changes

packages/expect/src/toThrowMatchers.js Outdated Show resolved Hide resolved

Improve grammar in ExpectAPI.md

d576e72

pedrottimark added 2 commits January 16, 2019 18:26

Delete an exception from Received function did not throw

4218be0

Correct typo in ExpectAPI.md

4ea8044

thymikee approved these changes Jan 17, 2019

View reviewed changes

Correct type and report for non-error class

2a3f1c8

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

pedrottimark deleted the improve-report-6 branch January 20, 2019 15:42

rubennorte mentioned this pull request Jan 24, 2019

toThrow does not behave like toEqual for non-errors #7692

Closed

pedrottimark mentioned this pull request Jan 24, 2019

expect: fix regression in toThrow for message prop and asymmetric matchers #7697

Merged

olivierbeaulieu mentioned this pull request Jan 30, 2019

toThrow assertion message includes Error name, but does not match on it #6688

Closed

mattphillips mentioned this pull request Mar 18, 2019

Invalid toThrow verification #6805

Closed

pedrottimark mentioned this pull request Apr 23, 2019

expect: Improve report when matcher fails, part 19 #8367

Merged

pedrottimark mentioned this pull request Aug 3, 2019

Docs - Expect API - toThrow - Added a note about the type of value thrown #8772

Closed

GerkinDev mentioned this pull request Apr 27, 2021

Feature: Add support for .rejects.toThrowWithMessage jest-community/jest-extended#315

Merged

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expect: Improve report when assertion fails, part 6 #7621

expect: Improve report when assertion fails, part 6 #7621

pedrottimark commented Jan 13, 2019 •

edited

Loading

SimenB left a comment

pedrottimark commented Jan 13, 2019

codecov-io commented Jan 13, 2019 •

edited

Loading

thymikee left a comment

thymikee Jan 14, 2019

pedrottimark Jan 14, 2019

thymikee Jan 14, 2019

pedrottimark Jan 14, 2019

pedrottimark Jan 14, 2019

SimenB Jan 14, 2019

jeysal left a comment

jeysal Jan 14, 2019

pedrottimark Jan 14, 2019

jeysal Jan 14, 2019

SimenB Jan 22, 2019 •

edited

Loading

jeysal Jan 22, 2019

pedrottimark commented Jan 14, 2019

pedrottimark commented Jan 15, 2019

SimenB commented Jan 15, 2019

pedrottimark commented Jan 15, 2019 •

edited

Loading

jeysal commented Jan 15, 2019

pedrottimark commented Jan 15, 2019

jeysal commented Jan 15, 2019

SimenB commented Jan 15, 2019

pedrottimark commented Jan 16, 2019 •

edited

Loading

thymikee left a comment

pedrottimark commented Jan 17, 2019

pedrottimark commented Jan 17, 2019

SimenB commented Jan 20, 2019

github-actions bot commented May 12, 2021

expect: Improve report when assertion fails, part 6 #7621

expect: Improve report when assertion fails, part 6 #7621

Conversation

pedrottimark commented Jan 13, 2019 • edited Loading

Summary

Test plan

SimenB left a comment

Choose a reason for hiding this comment

pedrottimark commented Jan 13, 2019

codecov-io commented Jan 13, 2019 • edited Loading

Codecov Report

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeysal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrottimark commented Jan 14, 2019

pedrottimark commented Jan 15, 2019

SimenB commented Jan 15, 2019

pedrottimark commented Jan 15, 2019 • edited Loading

jeysal commented Jan 15, 2019

pedrottimark commented Jan 15, 2019

jeysal commented Jan 15, 2019

SimenB commented Jan 15, 2019

pedrottimark commented Jan 16, 2019 • edited Loading

thymikee left a comment

Choose a reason for hiding this comment

pedrottimark commented Jan 17, 2019

pedrottimark commented Jan 17, 2019

SimenB commented Jan 20, 2019

github-actions bot commented May 12, 2021

pedrottimark commented Jan 13, 2019 •

edited

Loading

codecov-io commented Jan 13, 2019 •

edited

Loading

SimenB Jan 22, 2019 •

edited

Loading

pedrottimark commented Jan 15, 2019 •

edited

Loading

pedrottimark commented Jan 16, 2019 •

edited

Loading