Prevent false test failures caused by promise rejections handled asynchronously #14110

stekycz · 2023-04-25T11:29:58Z

Summary

I am experiencing a failing test even though it is fine. I create a promise but await it later (asynchronously). The test is failing because it tests a negative condition that rejects.

I believe Jest should register rejectionHandled listener to handle those cases.

Test plan

I have added integration tests to cover all cases I was able to think of. The first commit basically maps the current situation and the following one shows what is actually changed thanks to usage of snapshots. Feel free to suggest other cases I should add.

linux-foundation-easycla · 2023-04-25T11:30:04Z

The committers listed above are authorized under a signed CLA.

✅ login: stekycz / name: Martin Štekl (9337d1e)

netlify · 2023-04-25T11:30:06Z

✅ Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`b775b19`
🔍 Latest deploy log	https://app.netlify.com/sites/jestjs/deploys/6470b0044a689b000800ee73
😎 Deploy Preview	https://deploy-preview-14110--jestjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

SimenB · 2023-04-25T12:24:44Z

Ooh, very cool! I didn't know about rejectionHandled. Could you add some integration tests as well?

…chronously

stekycz · 2023-04-25T20:20:48Z

@SimenB I have created integration tests. They actually showed me my original implementation did not work as intended so I reworked it. I am open for additional suggestions.

…jectionHandled event only Awaiting for the next turn is potentially conflicting with fake timers usage.

Legacy fake timers execute original setImmediate in its fake implementation. I believe removing that behaviour would cause more harm than failing the test here that seems to be synthetic anyway.

stekycz · 2023-04-26T14:48:00Z

e2e/promise-async-handling/__tests__/unhandledRejectionTest.test.js

+
+test('w/o event loop turn after rejection', () => {
+  Promise.reject(new Error('REJECTED'));
+});


Failures of this test made me update other tests in this PR. I have to force event loop to make a single turn to catch this as unhandled promise rejection.

stekycz · 2023-04-26T14:50:46Z

e2e/promise-async-handling/__tests__/rejectionHandled.test.js

+
+  await promisify(setTimeout)(0);
+
+  await expect(promise).rejects.toThrow(/REJECTED/);


I have to force event loop to make one turn at least when there is an unhandled rejection processed before. Otherwise, this test would be still failing because rejectionHandled needs one event loop turn to be processed. I could make this when there is at least one unhandled rejection before to not modify tests mentioned in the previous comment. However, I would hide those consequences only. Those cases can still happen IMO in situations like this one.

packages/jest-circus/src/unhandledRejectionHandler.ts

packages/jest-types/src/Circus.ts

SimenB · 2023-05-12T09:36:30Z

e2e/require-after-teardown/__tests__/lateRequire.test.js

@@ -11,5 +11,5 @@ test('require after done', () => {
    const double = require('../');

    expect(double(5)).toBe(10);
-  }, 0);
+  }, 100);


Are 0 still detected? If not, we should try to set some other flag so we can detect this case immediately after tests are done

Yes, they are detected. That is why I had to update this test (and the other one as well).

If it's still detected I'm not 100% sure why this had to change? Is the error just different?

Oh, I miss-understood originally. Unfortunately 0 are not detected after my changes. Possibly even 1 or other small count if ms are not detected. It is cased by the extra event loop turnaround I need to ensure detection of handled and unhandled rejections.

Let me think a bit about the possible solution.

I took a deep loop into both cases (jest environment method access after teardown and late require) and both seems fragile already.

Any async event handler can break it. No matter what the even handler would do, anytime it takes at least one even-loop turnaround, this gets broken. More even-loop turnarounds in an async event handler, the more "suppressed" errors like this. Both errors are thrown on runtime level when the circus is out of game already. The runtime assumes the the time between a test and tearing runtime down is strictly sync (which may not be).

"jest environment method access after teardown" validates a single timers method access only. All other Jest environment methods usages are not reported at all, even on the current main branch.

I introduced the first truly async event handler. Therefore, the detection has changed because of the first point.

I was thinking about a new flag, however, I would need to make a hard choice as a user between the following variants:

A)

I can keep the current behaviour detecting late require as well as late global method usage.

I won't be able to detect unhandled rejections when the promise rejection happens in the same event loop turn as the test ends.

I would still get false failures for unhandled rejections when the rejected promise handling happens in the same event loop turn as the test ends.

I would still get global unhandled rejection error in case there is a floating promise being rejected way after its creation (e.g. when tearing down the env).

B)

I get all promise rejections error accurately (except rejections happening way after the promise creation).

I won't get reported on late requires nor late global method usage when it happens within the same event loop turn as the test ends (as it is still considered as part of the test).

I would prefer variant B) as you would probably guess. What is the reasoning behind those checks though?

I think there is a variant C) though. It would mean those 2 specific conditions get changed so they are detect on circus level rather than on runtime level. However, I am not sure how feasible that.

I was also thinking about variant D) - capturing all pending timers, leaving there the one waiting for the next even loop turn and the reapply the remaining timers. However, it feels hacky and I was not able to think of a way for other envs than Node.js (and even that is questionable).

Thanks for digging! I think your change is fine considering the tradeoffs.

I guess what we could do was signalling from Circus that we are done running tests once the teardown event fires, at which point the runtime would start rejecting new requires etc. even though it hasn't started its own teardown. Might not really be feasible in the current architecture, tho?

I actually got the same idea in the evening but I did not have time to look into it yet 😃 I will give it a shot

I believe I have the solution.

…est-circus

stekycz · 2023-05-26T11:43:23Z

packages/jest-runtime/src/index.ts

@@ -208,6 +208,7 @@ export default class Runtime {
  private readonly esmConditions: Array<string>;
  private readonly cjsConditions: Array<string>;
  private isTornDown = false;
+  private isInsideTestCode: boolean | undefined;


I am using value undefined as a sign that no test started yet so Jest can still use requires of modules prior running the test module (e.g. requiring the test module itself).

stekycz · 2023-05-26T11:44:03Z

packages/jest-runtime/src/index.ts

@@ -2169,6 +2193,11 @@ export default class Runtime {
        );
        process.exitCode = 1;
      }
+      if (this.isInsideTestCode === false) {
+        throw new ReferenceError(
+          'You are trying to access a property or method of the Jest environment outside of the scope of the test code.',


I am open for suggestions about wording of both kinds of errors.

stekycz · 2023-06-08T14:18:07Z

@SimenB Is there anything remaining? What can I do to make this land and be in the next release? Thanks

SimenB

works for me, thanks! And thanks for your patience

stekycz · 2023-06-21T10:14:02Z

@SimenB Perfect! Thank you

SimenB · 2023-07-04T15:28:40Z

https://github.com/jestjs/jest/releases/tag/v29.6.0

juan-fernandez · 2023-07-06T07:03:27Z

thanks for the contribution, @stekycz. We're having some issues with bad tests (see #14302) and the new isInsideTestCode attribute. While the end goal would be to fix those tests, we currently can only fix them at once before being able to benefit from bumping to 29.6.0 (which has some bugfixes we need). Could some of these ReferenceErrors (such as https://github.com/jestjs/jest/pull/14110/files#diff-c0d5b59e96fdc7ffc98405e8afb46d525505bc7b1c24916b5c8482de5a186c00R1508-R1510) be warnings instead? At least until (maybe) a major release?

SimenB · 2023-07-06T08:05:35Z

Hmm, might be a good idea to defer this to a major release, yeah.

I can revert, release and prepare a PR for v30 later today

…led asynchronously (jestjs#14110)" This reverts commit 57e1d4e.

…led asynchronously (#14110)" (#14304)

SimenB · 2023-07-06T14:19:03Z

https://github.com/jestjs/jest/releases/tag/v29.6.1

stekycz · 2023-07-10T06:38:13Z

@SimenB Is there something I can help with making it to v30?

SimenB · 2023-07-10T06:58:14Z

@stekycz if you can open up a new PR essentially just reverting my revert, that'd be lovely! 🙂 I'll add it to the milestone for Jest 30 so I don't forget it

…ons handled asynchronously (jestjs#14110)" (jestjs#14304)" This reverts commit 208f2f1.

stekycz · 2023-07-10T07:06:26Z

@SimenB No problem! Here it is #14315

github-actions · 2023-08-10T00:08:54Z

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.

stekycz added 3 commits April 25, 2023 22:08

Add failing integration tests

91cb749

Prevent false test failures caused by promise rejections handled asyn…

bc66d54

…chronously

Update changelog

67966d7

stekycz force-pushed the stekycz-jest-handled-rejection branch from 9337d1e to 67966d7 Compare April 25, 2023 20:16

stekycz added 7 commits April 25, 2023 22:44

Skip promise async handling tests for Jasmine

302e12d

Wait until next event loop turn when there is a reason to wait for re…

9efe383

…jectionHandled event only Awaiting for the next turn is potentially conflicting with fake timers usage.

Fix failing test for detection of unhandled rejections

644b766

Update fake timers legacy test

e564012

Legacy fake timers execute original setImmediate in its fake implementation. I believe removing that behaviour would cause more harm than failing the test here that seems to be synthetic anyway.

Fix fake timers legacy test for Jasmine runner

fc973e2

Update failing tests because of one extra event loop turn

f2592e5

Give event loop more time to proceed in background

d7be001

stekycz commented Apr 26, 2023

View reviewed changes

Merge branch 'main' into stekycz-jest-handled-rejection

c639e85

SimenB reviewed May 12, 2023

View reviewed changes

stekycz added 5 commits May 12, 2023 22:41

Make usage of original timer function consistent with other code in j…

64ef973

…est-circus

Fix usage of Node.js timer function 🤦

d7b963c

Update comments

00ecc46

Merge branch 'main' into stekycz-jest-handled-rejection

b8d9c41

Merge branch 'main' into stekycz-jest-handled-rejection

5fd3805

stekycz commented May 26, 2023

View reviewed changes

Fix reporting of requires or Jest env method access after "teardown"

b412305

stekycz force-pushed the stekycz-jest-handled-rejection branch from 0681959 to b412305 Compare May 26, 2023 12:05

Fix tests for Jasmine

b775b19

SimenB approved these changes Jun 21, 2023

View reviewed changes

SimenB merged commit 57e1d4e into jestjs:main Jun 21, 2023
77 of 78 checks passed

stekycz deleted the stekycz-jest-handled-rejection branch June 21, 2023 10:13

juan-fernandez mentioned this pull request Jul 5, 2023

[Bug]: Multiple You are trying to import a file outside of the scope of the test code. errors when bumping to 29.6.0 #14302

Closed

SimenB added a commit to SimenB/jest that referenced this pull request Jul 6, 2023

Revert "Prevent false test failures caused by promise rejections hand…

aabae57

…led asynchronously (jestjs#14110)" This reverts commit 57e1d4e.

SimenB mentioned this pull request Jul 6, 2023

Revert "Prevent false test failures caused by promise rejections handled asynchronously (#14110)" #14304

Merged

SimenB added a commit that referenced this pull request Jul 6, 2023

Revert "Prevent false test failures caused by promise rejections hand…

208f2f1

…led asynchronously (#14110)" (#14304)

stekycz added a commit to stekycz/jest that referenced this pull request Jul 10, 2023

Revert "Revert "Prevent false test failures caused by promise rejecti…

100d8ed

…ons handled asynchronously (jestjs#14110)" (jestjs#14304)" This reverts commit 208f2f1.

stekycz mentioned this pull request Jul 10, 2023

Prevent false test failures caused by promise rejections handled asynchronously #14315

Merged

ms-bot mentioned this pull request Jul 24, 2023

chore(deps): bump 🧳 jest to ^29.7.0 mobsuccess-devops/pmd-github-action#9

Draft

github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent false test failures caused by promise rejections handled asynchronously #14110

Prevent false test failures caused by promise rejections handled asynchronously #14110

stekycz commented Apr 25, 2023 •

edited

linux-foundation-easycla bot commented Apr 25, 2023 •

edited

netlify bot commented Apr 25, 2023 •

edited

SimenB commented Apr 25, 2023 •

edited

stekycz commented Apr 25, 2023

stekycz Apr 26, 2023

stekycz Apr 26, 2023

SimenB May 12, 2023

stekycz May 12, 2023

SimenB May 22, 2023

stekycz May 22, 2023

stekycz May 22, 2023

SimenB May 24, 2023

stekycz May 24, 2023

stekycz May 26, 2023 •

edited

stekycz May 26, 2023

stekycz May 26, 2023

stekycz commented Jun 8, 2023

SimenB left a comment

stekycz commented Jun 21, 2023

SimenB commented Jul 4, 2023

juan-fernandez commented Jul 6, 2023

SimenB commented Jul 6, 2023

SimenB commented Jul 6, 2023

stekycz commented Jul 10, 2023

SimenB commented Jul 10, 2023

stekycz commented Jul 10, 2023

github-actions bot commented Aug 10, 2023


		await promisify(setTimeout)(0);

		await expect(promise).rejects.toThrow(/REJECTED/);

Prevent false test failures caused by promise rejections handled asynchronously #14110

Prevent false test failures caused by promise rejections handled asynchronously #14110

Conversation

stekycz commented Apr 25, 2023 • edited

Summary

Test plan

linux-foundation-easycla bot commented Apr 25, 2023 • edited

netlify bot commented Apr 25, 2023 • edited

✅ Deploy Preview for jestjs ready!

SimenB commented Apr 25, 2023 • edited

stekycz commented Apr 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stekycz May 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stekycz commented Jun 8, 2023

SimenB left a comment

Choose a reason for hiding this comment

stekycz commented Jun 21, 2023

SimenB commented Jul 4, 2023

juan-fernandez commented Jul 6, 2023

SimenB commented Jul 6, 2023

SimenB commented Jul 6, 2023

stekycz commented Jul 10, 2023

SimenB commented Jul 10, 2023

stekycz commented Jul 10, 2023

github-actions bot commented Aug 10, 2023

stekycz commented Apr 25, 2023 •

edited

linux-foundation-easycla bot commented Apr 25, 2023 •

edited

netlify bot commented Apr 25, 2023 •

edited

SimenB commented Apr 25, 2023 •

edited

stekycz May 26, 2023 •

edited