Show exception stack trace when errors are thrown by plugins of pretty-format

[nicolasiensen]

Summary

In this commit, we are changing the function toMatchSnapshot to catch errors thrown by custom snapshot serializers.

When these errors are caught, toMatchSnapshot is going to throw the stack of the respective error, uncovering errors that might occur in custom snapshot serializers.

Fixes #3302

Test plan

Given a test file foo.test.js:

test("should match the snapshot", () => {
  const bar = {
    foo: {
      x: 1,
      y: 2
    }
  };

  expect(bar).toMatchSnapshot();
});

And a custom snapshot serializer my-serializer-module.js

module.exports = {
  test: val => {
    throw new Error("Where is this error?");
  },
  print: val => ""
};

And a package.json:

{
  "jest": {
    "snapshotSerializers": ["./my-serializer-module"]
  }
}

When jest is run, with the current implementation, the failure message has no trace of the error thrown by the custom serializer:

With the new implementation we have a failure message that includes a trace from the error thrown by the custom serializer:

[Nargonath]

Wouldn't it be better to narrow down the position of the try/catch?

I was going to put in https://github.com/facebook/jest/blob/e04e45102b3ebe486462d45db656f9b47f0e70e1/packages/pretty-format/src/index.js#L272 just surrounding the test() call. So it would not swallow other potential errors. Just genuinely asking.

[pedrottimark]

@nicolasiensen Thank you for submitting this pull request!

Extending comment by @Nargonath let’s see if we can adjust PR to catch errors thrown by either:

• test method of plugin
• print or serialize method of plugin

for either:

• snapshot test
• display Expected/Received values after assertion fails, for example toEqual or toMatchObject

Could you see what happens if instead you catch errors in two places in pretty-format package:

• for top-level value: https://github.com/facebook/jest/blob/master/packages/pretty-format/src/index.js#L431-L434
• for descendant value, for example, array item or object property value: https://github.com/facebook/jest/blob/master/packages/pretty-format/src/index.js#L287-L290

[SimenB]

As a reminder, please update the changelog as well

[pedrottimark]

@nicolasiensen You have my apology if the suggestion thinking from-the-inside-out undid your solution from-the-outside-in as a developer actually experiences it in Jest.

Because catching and re-throwing errors is an area for me to learn, I ran a few tests using code from master and then compared to a clone from your branch. I was not able to repeat the improvement that I see in the picture that you took of your original commit.

Can you repeat your original test to confirm whether or not it still gives result you saw before?

If the improvement has been lost, then let’s return to what worked.

[nicolasiensen]

@pedrottimark you don't have to apologize :)

I confirm that the later changes are working, here is a gif where you can compare this branch with the master:

Have you built jest with yarn run build before trying this branch?

[SimenB]

I'm a bit confused about why doing new Error(oldError.stack) helps. Is it us stripping out the useful part of the stacktrace as part of prettifying the error? Are you able to discover what part of the code makes us lose the stack? Or if I'm missing something, please enlighten me! 🙂

[Nargonath]

@SimenB The part that makes us lose the information is this one: https://github.com/facebook/jest/blob/0748e6f76394b58ce24aee42e08c96fb509e8744/packages/expect/src/index.js#L204

As of my understanding, captureStackTrace is used to removed Jest inners as part of the stack traces so it would point to the users' failed tests code. However in this case we want to keep that information because the problem is coming from Jest serializer test() call which may not be in users test code.

I didn't think about that solution though and I must admit that I don't really understand why it is solving our issue here.

EDIT: Perhaps wrapping the stack trace in another error is preventing captureStackTrace to do its job properly.

[pedrottimark]

@SimenB Your experience with improving stack traces in Jest is welcome here :)

[nicolasiensen]

When I started working on this pull request, I didn't fully understand why the stack trace was incomplete. But I realized that we could get the desired outcome by throwing the stack trace itself.

In this pull request, when an error is thrown by a plugin's serialize, print or test function, Jest is going to:

1. collect the stack trace, that will be further overwritten as @Nargonath pointed out
2. throw it, making it the error message the stack trace itself

Does it make sense? Should we, instead of throwing the stack trace, avoid to call Error.captureStackTrace when something goes wrong in a Jest plugin?

[SimenB]

Does it make sense? Should we, instead of throwing the stack trace, avoid to call Error.captureStackTrace when something goes wrong in a Jest plugin?

This feels more correct, as it fixes the issue, and not the symptom.

The purpose of Error.captureStackTrace is to avoid the matcher part of the stack trace, not actual "internal" errors.

[SimenB]

@nicolasiensen why close it?

[Nargonath]

Shall I make a PR as this one is closed and it seemed that we are heading the way I intended to solve it?

[SimenB]

@Nargonath Yes please. Would love to get this is, I'm not sure why the PR was closed

[nicolasiensen]

Hey @SimenB, I closed this one since we agree that there is a better solution. Please, check #4787.

[SimenB]

Ah, OK! We could have just changed this PR, but a new one is fine 🙂

[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.