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: avoid unhandled promise rejections when concurrent tests fail #11987

Merged
merged 6 commits into from Nov 29, 2021

Conversation

dmitri-gb
Copy link
Contributor

@dmitri-gb dmitri-gb commented Oct 22, 2021

Summary

This change fixes an issue where a failing concurrent test could trigger the global unhandled promise rejection handler, which would then attribute this failure to a different test (the one that was currently "running" i.e. the one that Jest was currently awaiting on). The issue is present in both jest-jasmine2 and jest-circus.

You can also check existing bug reports (#11691, #11231) for examples.

Fixes #11691
Fixes #11231

Test plan

There is a new integration test that demonstrates the issue.

@@ -93,6 +93,9 @@ export const initialize = async ({
// that will result in this test to be skipped, so we'll be executing the promise function anyway,
// even if it ends up being skipped.
const promise = mutex(() => testFn());
// Avoid triggering the uncaught promise rejection handler in case the test errors before
// being awaited on.
promise.catch(() => {});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this doesn't mean promise will never reject, that is only true for the promise returned by catch(...).

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2021

Codecov Report

Merging #11987 (589c9dc) into main (95f4969) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11987      +/-   ##
==========================================
- Coverage   68.77%   68.76%   -0.01%     
==========================================
  Files         324      324              
  Lines       16670    16672       +2     
  Branches     4814     4814              
==========================================
  Hits        11465    11465              
- Misses       5172     5174       +2     
  Partials       33       33              
Impacted Files Coverage Δ
...us/src/legacy-code-todo-rewrite/jestAdapterInit.ts 0.00% <0.00%> (ø)
packages/jest-jasmine2/src/jasmineAsyncInstall.ts 0.00% <0.00%> (ø)

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 95f4969...589c9dc. Read the comment docs.

@dmitri-gb dmitri-gb changed the title fix: avoid uncaught promise rejections when concurrent tests fail fix: avoid unhandled promise rejections when concurrent tests fail Oct 24, 2021
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.

thanks! I've been meaning to dig into these, thanks for taking the time 👍

@SimenB SimenB merged commit 9d14c5d into jestjs:main Nov 29, 2021
@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 Dec 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants