Expose JestAssertionError to custom matchers

[bvaughn]

Resolves #5136

Expose JestAssertionError to custom matchers as expect.JestAssertionError. The motivation for this is described in more detail in issue #5136, along with alternate solutions.

It's unclear to me if the current Jest behavior is the right default. (I don't have sufficient context to have an opinion about this.) I assume for now that it is, and so propose the smallest change to enable custom matchers to throw errors that will not have the stack overridden.

I didn't update website/documentation yet since I'm unsure if this PR will be accepted. I would be happy to make that change as well though.

Summary

With this change, it is possible to write custom matchers that preserve the original error stack, eg:

function toCustomMatch(callback, expectation) {
  try {
    const actual = callback();

    if (actual !== expectation) {
      return {
        pass: false,
        message: () => `Expected "${expectation}" but got "${actual}"`
      };
    }
  } catch (error) {
    // Explicitly wrap caught errors to preserve their stack
    // Without this, Jest will override stack to point to the matcher
    const assertionError = new expect.JestAssertionError();
    assertionError.message = error.message;
    assertionError.stack = error.stack;
    throw assertionError;
  }

  return {pass: true};
}

expect.extend({
  toCustomMatch,
});

describe('Custom matcher', () => {
  // This test will pass
  it('passes', () => {
    expect(() => 'foo').toCustomMatch('foo');
  });

  // This test should fail
  it('fails', () => {
    expect(() => 'foo').toCustomMatch('bar');
  });

  // This test fails due to an unrelated/unexpected error
  // It will show a helpful stack trace though
  it('preserves error stack', () => {
    const foo = () => bar();
    const bar = () => baz();
    const baz = () => qux();

    expect(() => {
      foo();
    }).toCustomMatch('test');
  });
});

Running the above test would result in the following output:

FAIL  path/to/test.js
  Custom matcher
    ✓ passes (3ms)
    ✕ fails (10ms)
    ✕ preserves error stack

  ● Custom matcher › fails

    Expected "bar" but got "foo"

      at Object.<anonymous> (path/to/test.js:35:41)

  ● Custom matcher › preserves error stack

    ReferenceError: qux is not defined

      at baz (path/to/test.js:43:28) # This stack is important
      at bar (path/to/test.js:42:35)
      at foo (path/to/test.js:41:35)
      at path/to/test.js:46:7
      at Object.toCustomMatch (path/to/test.js:3:18)
      at Object.throwingMatcher [as toCustomMatch] (node_modules/expect/build/index.js:198:24)
      at Object.<anonymous> (path/to/test.js:47:8)

Test Suites: 1 failed, 1 total
Tests:       2 failed, 1 passed, 3 total

Test plan

Above results are shown for a custom matcher I've written locally that uses this newly-exposed property. I would be happy to contribute automated test(s) as well if it's desired.

[codecov-io]

Codecov Report

Merging #5138 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #5138   +/-   ##
======================================
  Coverage    60.7%   60.7%           
======================================
  Files         201     201           
  Lines        6691    6691           
  Branches        4       4           
======================================
  Hits         4062    4062           
  Misses       2628    2628           
  Partials        1       1

---

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 774d457...9f64ea7. Read the comment docs.

[SimenB]

It might be a bit overkill, but could you add an integration test showing that extending this error ends up with the correct trace?

More as a guard for future regressions than as proof of this change working wonders.

[bvaughn]

Yeah, definitely. I added one to stacktrace.test.js (via 8aeb67d)

[SimenB]

The way #4516 was implemented, I think this makes sense, but I'll let @cpojer be the judge 🙂

I added an integration test as well, just copying your example from the OP (making it pass lint being the only diff)

[SimenB]

Thinking about this on the bus, I wonder if https://github.com/facebook/jest/blob/78184f26c3fea0ba85b95ec3aed484a7a2d4d44b/packages/expect/src/index.js#L216-L225 is just completely wrong... (And that #4787 was the wrong approach)

If it's not an assertion error, I think we should just wrap the error (using something like https://www.npmjs.com/package/verror), not override its stack. This'll give us a caused by in the stack, preserving the original trace. WDYT?

[bvaughn]

That's sort of what my comment in the PR description, about it being "unclear to me if the current Jest behavior is the right default", referred to. But I didn't have the time to trace back through past PRs to understand why the current behavior exists.

Having written many Jest tests, as well as several matchers, I am confident though that the behavior of clobbering the original error stack is- at least in some cases- definitely harmful to the end user, since it masks the source of an error and requires more effort to identify and fix.

If Jest, by default, preserved the original stack (eg something like a "caused by" snippet with the original stack) that would be even better and would make this PR unnecessary.

[cpojer]

Could we change it to only have this behavior for Jest’s own inbuilt marchers? For those, we don’t want the deep stack traces to show up because they aren’t adding value.

For any third-party marchers I suggest not cleaning the stack trace like this.

[bvaughn]

Could we change it to only have this behavior for Jest’s own inbuilt marchers? For those, we don’t want the deep stack traces to show up because they aren’t adding value.

I think this sounds like a better default behavior, at least from the perspective of an outsider. Not sure how simple it would be to implement though. I'll take a look.

One thing I'm not clear on: Is the stack-swallowing behavior limited to custom matchers? Are there any built-in matchers that might also override the meaningful stack with their own? I think this would only apply for the to-throw matchers, which seem to work correctly.

[bvaughn]

Regarding the above suggestion, I pushed PR #5162 for discussion purposes. ^

[cpojer]

Closing in favor of the other PR.

[github-actions]

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.