Skip to content

feat(jest-message-util): Add support for Error cause in test and it - #13935

Merged
SimenB merged 26 commits into
jestjs:mainfrom
dubzzz:rewrite-stack-extraction
Feb 24, 2023
Merged

feat(jest-message-util): Add support for Error cause in test and it#13935
SimenB merged 26 commits into
jestjs:mainfrom
dubzzz:rewrite-stack-extraction

Conversation

@dubzzz

@dubzzz dubzzz commented Feb 19, 2023

Copy link
Copy Markdown
Contributor

Summary

For the moment, Jest only supports Error coming with causes if the Error was thrown outside of any test or it. The current PR adds support to test/it cases.

Test plan

Various tests have been added on the PR to show what it will look like on various configurations of Errors coming with causes.

@dubzzz

dubzzz commented Feb 19, 2023

Copy link
Copy Markdown
Contributor Author

Here is an example of report:

image

For the code snippet:

image

The code snippet is the one that has been used to build the test cases.

@dubzzz

dubzzz commented Feb 19, 2023

Copy link
Copy Markdown
Contributor Author

And here is something close to the second test case:

image

@dubzzz dubzzz changed the title Properly report Error cause from test/it Add support for Error cause in test and it Feb 19, 2023
Comment thread packages/jest-message-util/src/index.ts Outdated
Comment thread packages/jest-message-util/src/index.ts Outdated

@SimenB SimenB left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks for working on this! can you also add integration tests to failures for errors

  • outside of test
  • in test
  • in hook

?

Also, please add a changelog entry 🙂

Comment thread packages/jest-message-util/src/index.ts Outdated
Comment thread packages/jest-message-util/src/index.ts Outdated
@dubzzz

dubzzz commented Feb 20, 2023

Copy link
Copy Markdown
Contributor Author

can you also add integration tests to failures for errors

Is there already a file with such integration tests for normal error cases or should I create a new test file?

@SimenB

SimenB commented Feb 20, 2023

Copy link
Copy Markdown
Member

can you also add integration tests to failures for errors

Is there already a file with such integration tests for normal error cases or should I create a new test file?

https://github.com/facebook/jest/blob/main/e2e/__tests__/failures.test.ts

@dubzzz

dubzzz commented Feb 21, 2023

Copy link
Copy Markdown
Contributor Author

I'll need to fix error with cause tests for node 14. I already had the issue in my first set of tests, I'll update the PR tomorrow with a working versions of the tests for node 14.

On a pretty close subject, @SimenB do you think we should add support for error with cause into expect(...).toThrowErrorMatchingSnapshot(). I can possibly attempt to work on it in another PR if you do think it can be useful.

@dubzzz dubzzz changed the title Add support for Error cause in test and it feat(jest-message-util): Add support for Error cause in test and it Feb 21, 2023
@SimenB

SimenB commented Feb 21, 2023

Copy link
Copy Markdown
Member

On a pretty close subject, @SimenB do you think we should add support for error with cause into expect(...).toThrowErrorMatchingSnapshot(). I can possibly attempt to work on it in another PR if you do think it can be useful.

That makes sense to me - toThrowErrorMatchingSnapshot is supposed to stringify an error, and its causes seems a natural part of that to me. Might be a breaking change, but happy to land in Jest 30.

@dubzzz

dubzzz commented Feb 21, 2023

Copy link
Copy Markdown
Contributor Author

Not sure how I should deal with the last error remaining for jest-jasmine. It seems that my last change trying to handle results coming from jasmine and accessing to failure.error somehow changed the behaviour we see in another test 🤔 I'll re-dig a little bit more as I was not expecting such regression and definitely it's linked to my last update attempting to fix part of jasmine runs.

@dubzzz

dubzzz commented Feb 21, 2023

Copy link
Copy Markdown
Contributor Author

That makes sense to me - toThrowErrorMatchingSnapshot is supposed to stringify an error, and its causes seems a natural part of that to me. Might be a breaking change, but happy to land in Jest 30.

Making this PR pass first, then I might spend some time on it 👍
Actually I discovered them yesterday while writing tests using cause on fast-check side.

@SimenB

SimenB commented Feb 21, 2023

Copy link
Copy Markdown
Member

I'm happy to exclude jasmine for now if it proves troublesome

Comment thread packages/jest-message-util/src/index.ts Outdated
Comment on lines +439 to +449
const rootErrorOrStack: Error | string = failureDetails
? types.isNativeError(failureDetails) || failureDetails instanceof Error
? failureDetails // receiving raw errors for jest-circus
: typeof failureDetails === 'object' &&
'error' in failureDetails &&
failureDetails.error &&
(types.isNativeError(failureDetails.error) ||
failureDetails.error instanceof Error)
? failureDetails.error // receiving instances of FailedAssertion for jest-jasmine
: content
: content;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is hard to read - could you extract to a helper function with returns?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Definitely, will do

@dubzzz

dubzzz commented Feb 21, 2023

Copy link
Copy Markdown
Contributor Author

Ok, I'll drop jasmine support for now then. And retry for it later.