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

[jest-util] Add ErrorWithStack class #7067

Merged
merged 11 commits into from
Sep 30, 2018

Conversation

mattphillips
Copy link
Contributor

Summary

Removes the duplication of:

const error = new Error('whatever');
if (Error.captureStackTrace) {
  Error.captureStackTrace(error, someFunction);
}

Motivation: see @SimenB's #7033 (comment)

Test plan

  • Added unit test for new class
  • All existing test should pass

Notes

I've not updated the expect package with this change as I believe it cannot depend on jest-util because of bundling for the browser, is this correct? cc/ @thymikee @rickhanlonii

@mattphillips mattphillips requested a review from SimenB September 28, 2018 21:01
@mattphillips mattphillips changed the title Add ErrorWithStack class [jest-util] Add ErrorWithStack class Sep 28, 2018
@SimenB
Copy link
Member

SimenB commented Sep 28, 2018

I believe it cannot depend on jest-util because of bundling for the browser

Correct, but we should fix that. Or have like jest-isomorphic-utils and jest-node-utils instead.

(Yes, that was a joke, but you know what I mean)

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

So good! A changelog entry in case it breaks some traces would be nice, other than that this looks great

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

so nice <3

@@ -0,0 +1,8 @@
export default class ErrorWithStack extends Error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

mind adding preamble and flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

woops just added :)

@@ -0,0 +1,16 @@
import ErrorWithStack from '../error_with_stack';
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be cool to flow it up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it's worth adding flow to the test, it causes the following issue (which I'd have to disable):

[flow] Cannot assign `jest.fn()` to `Error.captureStackTrace` because property `captureStackTrace` is not writable. (error_with_stack.test.js:17:11)

Happy to add it though if you think we should :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's fine. It's just nice to have flow in tests too so you don't end up testing scenarios that are already covered/prevented by type checker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense, I've updated to use jest.spyOn instead and added @flow with no warnings 🎉

@SimenB
Copy link
Member

SimenB commented Sep 29, 2018

Rebase on master and use for 49b2380?

@codecov-io
Copy link

Codecov Report

Merging #7067 into master will increase coverage by 0.06%.
The diff coverage is 35.29%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #7067      +/-   ##
=========================================
+ Coverage   66.63%   66.7%   +0.06%     
=========================================
  Files         251     252       +1     
  Lines       10542   10521      -21     
  Branches        3       3              
=========================================
- Hits         7025    7018       -7     
+ Misses       3516    3502      -14     
  Partials        1       1
Impacted Files Coverage Δ
packages/jest-jasmine2/src/jasmine/Env.js 0% <0%> (ø) ⬆️
packages/jest-runner/src/run_test.js 3.07% <0%> (+0.09%) ⬆️
packages/jest-jasmine2/src/error_on_private.js 0% <0%> (ø) ⬆️
packages/jest-util/src/index.js 0% <0%> (ø) ⬆️
packages/jest-cli/src/collectHandles.js 0% <0%> (ø) ⬆️
packages/jest-each/src/bind.js 71.29% <100%> (-1.92%) ⬇️
packages/jest-util/src/error_with_stack.js 100% <100%> (ø)
packages/jest-circus/src/index.js 72.54% <33.33%> (+6.92%) ⬆️

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 49b2380...93bb48e. Read the comment docs.

@SimenB SimenB merged commit 29c7786 into jestjs:master Sep 30, 2018
@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 12, 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.

5 participants