FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Revert "test_runner: abort unfinished tests on async error" by cjihrig · Pull Request #52140 · nodejs/node · GitHub

/ node Public

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

Closed
cjihrig wants to merge 1 commit into
nodejs:mainfrom
cjihrig:revert
ConversationCommits (1)ChecksFiles changed
Closed

Revert "test_runner: abort unfinished tests on async error"#52140
cjihrig wants to merge 1 commit into
nodejs:mainfrom
cjihrig:revert

Conversation

cjihrig commented Mar 18, 2024

Copy link
Copy Markdown
Contributor

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 commented Mar 18, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

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
cjihrig mentioned this pull request Mar 18, 2024
Closed
aduh95 approved these changes Mar 18, 2024
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

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 outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. dont-land-on-v18.x labels Mar 18, 2024

github-actions Bot commented Mar 18, 2024

Copy link
Copy Markdown
Contributor

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

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 commented Mar 18, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57820/

cjihrig commented Mar 18, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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
test: reduce flakiness of test-runner-output.mjs
21c7305 
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
cjihrig mentioned this pull request Mar 18, 2024
Merged
aduh95 removed the fast-track PRs that do not need to wait for 48 hours to land. label Mar 18, 2024

cjihrig commented Mar 18, 2024

Copy link
Copy Markdown
Contributor Author

Closing in favor of #52146

cjihrig closed this Mar 18, 2024
cjihrig deleted the revert branch March 18, 2024 23:57
nodejs-github-bot pushed a commit that referenced this pull request Mar 19, 2024
test: reduce flakiness of test-runner-output.mjs
978d5a2 
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 2, 2024
test: reduce flakiness of test-runner-output.mjs
3a75341 
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
test: reduce flakiness of test-runner-output.mjs
d7bfb4e 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca approved these changes
MoLow MoLow approved these changes
aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

flaky: test-runner-output

5 participants

Back | FazBrowse Home | New Git URL