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

Revert "test_runner: abort unfinished tests on async error" #52140

Closed
wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 18, 2024

This reverts commit 29b2317.

This appears to fix the flaky test. I'll take a look at fixing the original bug in more detail when I have the time.

Fixes: #52139

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Mar 18, 2024
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

On main: [03:05|% 20|+ 201|- 3]: Done

With this PR: [03:23|% 20|+ 204|- 0]: Done

@aduh95 aduh95 added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v21.x labels Mar 18, 2024
Copy link
Contributor

Fast-track has been requested by @aduh95. Please 👍 to approve.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2024
@nodejs-github-bot
Copy link
Collaborator

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 18, 2024

FWIW, I think 16579d7 (by @aduh95) just needs to be applied to test/fixtures/test-runner/output/output.js instead of a revert. I made that change and haven't seen any errors in my local stress test.

$ ./tools/test.py --repeat=1000 test/parallel/test-runner-output.mjs 
[08:21|% 100|+ 1000|-   0]: Done                     

All tests passed.
diff --git a/test/fixtures/test-runner/output/lcov_reporter.snapshot b/test/fixtures/test-runner/output/lcov_reporter.snapshot
index 11b2bc1039..adc3abb005 100644
--- a/test/fixtures/test-runner/output/lcov_reporter.snapshot
+++ b/test/fixtures/test-runner/output/lcov_reporter.snapshot
@@ -81,24 +81,26 @@ FN:310,anonymous_77
 FN:313,anonymous_78
 FN:318,anonymous_79
 FN:319,anonymous_80
-FN:324,anonymous_81
-FN:329,anonymous_82
-FN:330,anonymous_83
+FN:320,anonymous_81
+FN:327,anonymous_82
+FN:328,anonymous_83
 FN:335,anonymous_84
-FN:339,anonymous_85
-FN:342,get then
+FN:336,anonymous_85
+FN:341,anonymous_86
 FN:345,anonymous_87
-FN:350,anonymous_88
-FN:353,get then
+FN:348,get then
+FN:351,anonymous_89
 FN:356,anonymous_90
-FN:361,anonymous_91
+FN:359,get then
 FN:362,anonymous_92
-FN:363,anonymous_93
-FN:367,anonymous_94
-FN:368,anonymous_95
-FN:369,anonymous_96
-FN:375,anonymous_97
-FN:379,anonymous_98
+FN:367,anonymous_93
+FN:368,anonymous_94
+FN:369,anonymous_95
+FN:373,anonymous_96
+FN:374,anonymous_97
+FN:375,anonymous_98
+FN:381,anonymous_99
+FN:385,anonymous_100
 FNDA:1,anonymous_0
 FNDA:1,anonymous_1
 FNDA:1,anonymous_2
@@ -185,12 +187,12 @@ FNDA:1,anonymous_82
 FNDA:1,anonymous_83
 FNDA:1,anonymous_84
 FNDA:1,anonymous_85
-FNDA:1,get then
+FNDA:1,anonymous_86
 FNDA:1,anonymous_87
-FNDA:1,anonymous_88
 FNDA:1,get then
+FNDA:1,anonymous_89
 FNDA:1,anonymous_90
-FNDA:1,anonymous_91
+FNDA:1,get then
 FNDA:1,anonymous_92
 FNDA:1,anonymous_93
 FNDA:1,anonymous_94
@@ -198,8 +200,10 @@ FNDA:1,anonymous_95
 FNDA:1,anonymous_96
 FNDA:1,anonymous_97
 FNDA:1,anonymous_98
-FNF:99
-FNH:96
+FNDA:1,anonymous_99
+FNDA:1,anonymous_100
+FNF:101
+FNH:98
 BRDA:1,0,0,1
 BRDA:8,1,0,1
 BRDA:12,2,0,1
@@ -279,30 +283,32 @@ BRDA:310,75,0,1
 BRDA:313,76,0,1
 BRDA:318,77,0,1
 BRDA:319,78,0,1
-BRDA:324,79,0,1
-BRDA:329,80,0,1
-BRDA:330,81,0,1
+BRDA:320,79,0,1
+BRDA:327,80,0,1
+BRDA:328,81,0,1
 BRDA:335,82,0,1
-BRDA:339,83,0,1
-BRDA:342,84,0,1
-BRDA:343,85,0,0
-BRDA:345,86,0,1
-BRDA:350,87,0,1
-BRDA:353,88,0,1
-BRDA:354,89,0,0
-BRDA:356,90,0,1
-BRDA:361,91,0,1
-BRDA:364,92,0,0
-BRDA:362,93,0,1
-BRDA:363,94,0,1
-BRDA:367,95,0,1
-BRDA:370,96,0,0
-BRDA:368,97,0,1
-BRDA:369,98,0,1
-BRDA:375,99,0,1
-BRDA:379,100,0,1
-BRF:101
-BRH:97
+BRDA:336,83,0,1
+BRDA:341,84,0,1
+BRDA:345,85,0,1
+BRDA:348,86,0,1
+BRDA:349,87,0,0
+BRDA:351,88,0,1
+BRDA:356,89,0,1
+BRDA:359,90,0,1
+BRDA:360,91,0,0
+BRDA:362,92,0,1
+BRDA:367,93,0,1
+BRDA:370,94,0,0
+BRDA:368,95,0,1
+BRDA:369,96,0,1
+BRDA:373,97,0,1
+BRDA:376,98,0,0
+BRDA:374,99,0,1
+BRDA:375,100,0,1
+BRDA:381,101,0,1
+BRDA:385,102,0,1
+BRF:103
+BRH:99
 DA:1,1
 DA:2,1
 DA:3,1
@@ -702,6 +708,12 @@ DA:396,1
 DA:397,1
 DA:398,1
 DA:399,1
-LH:397
-LF:399
+DA:400,1
+DA:401,1
+DA:402,1
+DA:403,1
+DA:404,1
+DA:405,1
+LH:403
+LF:405
 end_of_record
diff --git a/test/fixtures/test-runner/output/output.js b/test/fixtures/test-runner/output/output.js
index 1557b4da4e..ff1b295877 100644
--- a/test/fixtures/test-runner/output/output.js
+++ b/test/fixtures/test-runner/output/output.js
@@ -317,12 +317,18 @@ test('subtest sync throw fails', async (t) => {
 
 test('timed out async test', { timeout: 5 }, async (t) => {
   return new Promise((resolve) => {
-    setTimeout(resolve, 100);
+    setTimeout(() => {
+      // Empty timer so the process doesn't exit before the timeout triggers.
+    }, 5);
+    setTimeout(resolve, 30_000_000).unref();
   });
 });
 
 test('timed out callback test', { timeout: 5 }, (t, done) => {
-  setTimeout(done, 100);
+  setTimeout(() => {
+    // Empty timer so the process doesn't exit before the timeout triggers.
+  }, 5);
+  setTimeout(done, 30_000_000).unref();
 });
 
 

cjihrig added a commit to cjihrig/node that referenced this pull request Mar 18, 2024
This commit is similar to nodejs#51952. When the system is under load
it is possible for these timeout tests to become flaky. We
work around that by using a much longer setTimeout() in the test
so that it is not racing against the test's timeout. But, we have
to unref() such a large timeout. And, because test timeouts do
not currently keep the event loop alive, we use a different
setTimeout() for that purpose.

Fixes: nodejs#52139
Refs: nodejs#52140
@aduh95 aduh95 removed the fast-track PRs that do not need to wait for 48 hours to land. label Mar 18, 2024
@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 18, 2024

Closing in favor of #52146

@cjihrig cjihrig closed this Mar 18, 2024
@cjihrig cjihrig deleted the revert branch March 18, 2024 23:57
nodejs-github-bot pushed a commit that referenced this pull request Mar 19, 2024
This commit is similar to #51952. When the system is under load
it is possible for these timeout tests to become flaky. We
work around that by using a much longer setTimeout() in the test
so that it is not racing against the test's timeout. But, we have
to unref() such a large timeout. And, because test timeouts do
not currently keep the event loop alive, we use a different
setTimeout() for that purpose.

Fixes: #52139
Refs: #52140
PR-URL: #52146
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
This commit is similar to nodejs#51952. When the system is under load
it is possible for these timeout tests to become flaky. We
work around that by using a much longer setTimeout() in the test
so that it is not racing against the test's timeout. But, we have
to unref() such a large timeout. And, because test timeouts do
not currently keep the event loop alive, we use a different
setTimeout() for that purpose.

Fixes: nodejs#52139
Refs: nodejs#52140
PR-URL: nodejs#52146
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
This commit is similar to #51952. When the system is under load
it is possible for these timeout tests to become flaky. We
work around that by using a much longer setTimeout() in the test
so that it is not racing against the test's timeout. But, we have
to unref() such a large timeout. And, because test timeouts do
not currently keep the event loop alive, we use a different
setTimeout() for that purpose.

Fixes: #52139
Refs: #52140
PR-URL: #52146
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
This commit is similar to #51952. When the system is under load
it is possible for these timeout tests to become flaky. We
work around that by using a much longer setTimeout() in the test
so that it is not racing against the test's timeout. But, we have
to unref() such a large timeout. And, because test timeouts do
not currently keep the event loop alive, we use a different
setTimeout() for that purpose.

Fixes: #52139
Refs: #52140
PR-URL: #52146
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flaky: test-runner-output
5 participants