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

test: fix flaky test-cluster-send-handle-twice #19700

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 30, 2018

Use common.mustCall() to make sure connection callback runs exactly
once.

Use connect event instead of setTimeout to avoid test failing if
timer runs before client is connected.

Remove cluster.worker.disconnect() after assert.fail(). It is
unreachable code that is unnecessary.

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

Use `common.mustCall()` to make sure connection callback runs exactly
once.

Use `connect` event instead of `setTimeout` to avoid test failing if
timer runs before client is connected.

Remove `cluster.worker.disconnect()` after `assert.fail()`. It is
unreachable code that is unnecessary.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 30, 2018
@Trott
Copy link
Member Author

Trott commented Mar 30, 2018

Example failure of the current form of the test in CI: https://ci.nodejs.org/job/node-test-commit-freebsd/16565/nodes=freebsd10-64/console

not ok 474 parallel/test-cluster-send-handle-twice
  ---
  duration_ms: 120.28
  severity: fail
  stack: |-
    timeout
  ...

@Trott
Copy link
Member Author

Trott commented Mar 30, 2018

Failure of current form of test reproduced locally:

$ tools/test.py -j 96 --repeat 192 test/parallel/test-cluster-send-handle-twice.js 
=== release test-cluster-send-handle-twice ===                    
Path: parallel/test-cluster-send-handle-twice
Command: out/Release/node /Users/trott/io.js/test/parallel/test-cluster-send-handle-twice.js
--- TIMEOUT ---
...
=== release test-cluster-send-handle-twice ===                   
Path: parallel/test-cluster-send-handle-twice
Command: out/Release/node /Users/trott/io.js/test/parallel/test-cluster-send-handle-twice.js
--- TIMEOUT ---
[04:01|% 100|+ 134|-  58]: Done   
$

@Trott
Copy link
Member Author

Trott commented Mar 30, 2018

Success of test in this PR locally:

$ tools/test.py -j 96 --repeat 192 test/parallel/test-cluster-send-handle-twice.js 
[00:17|% 100|+ 192|-   0]: Done                
$

@Trott
Copy link
Member Author

Trott commented Mar 30, 2018

process.send('send-handle-1', socket);
process.send('send-handle-2', socket);
});
}));
Copy link
Member

Choose a reason for hiding this comment

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

This is very likely tested for very very often already.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 30, 2018
@Trott
Copy link
Member Author

Trott commented Apr 2, 2018

Single CI failure seems unrelated. Re-running just that task: https://ci.nodejs.org/job/node-test-commit-plinux/16508/

(EDIT: Re-run is green.)

Trott added a commit to Trott/io.js that referenced this pull request Apr 2, 2018
Use `common.mustCall()` to make sure connection callback runs exactly
once.

Use `connect` event instead of `setTimeout` to avoid test failing if
timer runs before client is connected.

Remove `cluster.worker.disconnect()` after `assert.fail()`. It is
unreachable code that is unnecessary.

PR-URL: nodejs#19700
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Apr 2, 2018

Landed in 4d749e1

@Trott Trott closed this Apr 2, 2018
targos pushed a commit that referenced this pull request Apr 3, 2018
Use `common.mustCall()` to make sure connection callback runs exactly
once.

Use `connect` event instead of `setTimeout` to avoid test failing if
timer runs before client is connected.

Remove `cluster.worker.disconnect()` after `assert.fail()`. It is
unreachable code that is unnecessary.

PR-URL: #19700
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Apr 4, 2018
BethGriggs pushed a commit that referenced this pull request Dec 4, 2018
Use `common.mustCall()` to make sure connection callback runs exactly
once.

Use `connect` event instead of `setTimeout` to avoid test failing if
timer runs before client is connected.

Remove `cluster.worker.disconnect()` after `assert.fail()`. It is
unreachable code that is unnecessary.

PR-URL: #19700
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2018
@Trott Trott deleted the fix-send-twice branch January 13, 2022 22:49
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants