Skip to content
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

Fix stack trace for errors thrown by snapshot serializers #4787

Merged
merged 5 commits into from
Oct 30, 2017
Merged

Fix stack trace for errors thrown by snapshot serializers #4787

merged 5 commits into from
Oct 30, 2017

Conversation

nicolasiensen
Copy link
Contributor

Summary

In this commit, we are creating a new custom error called PrettyFormatPluginError, this error is thrown by prettyFormat function when a serializer throws an error.

In the expect module, we skip Error.captureStackTrace if the error name is PrettyFormatPluginError, this way the stack trace stays intact.

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:

screen shot 2017-10-29 at 12 00 50 pm

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

screen shot 2017-10-29 at 12 02 12 pm

In this commit, we are creating a new custom error called
`PrettyFormatPluginError`, this error is thrown by `prettyFormat`
function when a serializer throws an error.

In the `expect` module, we skip `Error.captureStackTrace` if the error
name is `PrettyFormatPluginError`, this way the stack trace stays
intact.

Fixes #3302
@SimenB
Copy link
Member

SimenB commented Oct 29, 2017

I like this. Mind adding a note in the changelog?

@Nargonath
Copy link

Yeah this looks nice. Good job @nicolasiensen

prettyFormat('', options);
} catch (error) {
expect(error.name).toBe('PrettyFormatPluginError');
}
Copy link
Contributor

@pedrottimark pedrottimark Oct 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If for any reason the prettyFormat call doesn’t throw, then the test passes?

What do you think about: DISREGARD FOLLOWING CODE

let name;
try {
  prettyFormat('', options);
} catch (error) {
  name = error.name;
}
expect(name).toBe('PrettyFormatPluginError');

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either do expect().toThrow() or expect.hasAssertions instead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimenB Thank you! Because toThrow matches the message but the name is the test criterion, then our requested change is keep try-catch as originally written and insert expect.hasAssertions() at beginning of each of the 3 test blocks?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Or .toThrow(PrettyFormatPluginError) if it's exported.

Copy link
Contributor

@pedrottimark pedrottimark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. One suggestion about the 3 added tests.

// Try to remove this and deeper functions from the stack trace frame.
if (
!(error instanceof JestAssertionError) &&
error.name != 'PrettyFormatPluginError' &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Triple equals?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing as the linter doesn't complain, maybe not needed

CHANGELOG.md Outdated
@@ -17,6 +17,8 @@
* `[pretty-format]` Prevent error in pretty-format for window in jsdom test env ([#4750](https://github.com/facebook/jest/pull/4750))
* `[jest-resolve]` Preserve module identity for symlinks ([#4761](https://github.com/facebook/jest/pull/4761))
* `[jest-config]` Include error message for `preset` json ([#4766](https://github.com/facebook/jest/pull/4766))
* `[pretty-format]` Throw `PrettyFormatPluginError` if a plugin halts with an exception ([#4787](https://github.com/facebook/jest/pull/4787))
* `[expect]` Keep the stack trace unchanged when `PrettyFormatPluginError` is thrown by pretty-format
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing link to PR

@cpojer cpojer merged commit cc802fb into jestjs:master Oct 30, 2017
@cpojer
Copy link
Member

cpojer commented Oct 30, 2017

Thank you everyone for working on this PR :)

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in snapshotSerializer test doesn't have useful stack trace
6 participants