Skip to content

test: address flaky worker test - #21893

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:fix-21892
Closed

test: address flaky worker test#21893
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:fix-21892

Conversation

@Trott

@TrottTrott commented Jul 19, 2018

Copy link
Copy Markdown
Member

Move test/parallel/test-worker-message-port-transfer-closed.js to
sequential to avoid race condition with setTimeout() at end of test.

Fixes: #21892

/cc @TimothyGu

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@TrottTrott added test Issues and PRs related to the tests. flaky-test Issues and PRs related to the tests with unstable failures on the CI. worker Issues and PRs related to Worker support. labels Jul 19, 2018
@Trott

Copy link
Copy Markdown
MemberAuthor

@addaleax

Copy link
Copy Markdown
Member

Hm – if I may offer an alternative?

diff --git a/test/parallel/test-worker-message-port-transfer-closed.js b/test/parallel/test-worker-message-port-transfer-closed.js
index 435a3789fca7..e3a13bfcbfe9 100644
--- a/test/parallel/test-worker-message-port-transfer-closed.js+++ b/test/parallel/test-worker-message-port-transfer-closed.js@@ -50,5 +50,9 @@ testSingle(port1, port2);
port2.close(common.mustCall(testBothClosed));
testBothClosed();
-setTimeout(common.mustNotCall('The communication channel is still open'),- common.platformTimeout(1000)).unref();+function tickUnref(n, fn) {+ if (n === 0) return fn();+ setImmediate(tickUnref, n-1, fn).unref();+}++tickUnref(10, common.mustNotCall('The communication channel is still open'));

This removes the timeout too, but we can keep the test in parallel this way…

@Trott

Copy link
Copy Markdown
MemberAuthor

@addaleax SGTM. Do you want to open a PR for that change? Or would you prefer I put it into this one and move it back to parallel here? I'm fine either way, of course.

@addaleax

Copy link
Copy Markdown
Member

@Trott either one is fine :)

Make test/parallel/test-worker-message-port-transfer-closed.js more
reliable by counting ticks rather than using a single setTimeout().
Fixes: nodejs#21892
@Trott

Copy link
Copy Markdown
MemberAuthor

Updated with @addaleax's suggestion.

Re-running stress test to see if this PR passes: https://ci.nodejs.org/job/node-stress-single-test/1970/nodes=freebsd10-64/console

Same stress test failing on master: https://ci.nodejs.org/job/node-stress-single-test/1967/nodes=freebsd10-64/console

@addaleaxaddaleax 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.

😉

@Trott

Copy link
Copy Markdown
MemberAuthor

@Trott

Copy link
Copy Markdown
MemberAuthor

Trott added a commit to Trott/io.js that referenced this pull request Jul 24, 2018
Make test/parallel/test-worker-message-port-transfer-closed.js more
reliable by counting ticks rather than using a single setTimeout().
Fixes: nodejs#21892
PR-URL: nodejs#21893
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
@Trott

Copy link
Copy Markdown
MemberAuthor

Landed in f93a19b

@TrottTrott closed this Jul 24, 2018
targos pushed a commit that referenced this pull request Jul 26, 2018
Make test/parallel/test-worker-message-port-transfer-closed.js more
reliable by counting ticks rather than using a single setTimeout().
Fixes: #21892
PR-URL: #21893
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
@targostargos mentioned this pull request Jul 31, 2018
@Trott
Trott deleted the fix-21892 branch January 13, 2022 22:50
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flaky-testIssues and PRs related to the tests with unstable failures on the CI.testIssues and PRs related to the tests.workerIssues and PRs related to Worker support.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate flaky test/parallel/test-worker-message-port-transfer-closed.js

4 participants

@Trott@nodejs-github-bot@addaleax@hiroppy