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: move timing-sensitive test to sequential #16775

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 5, 2017

test-https-server-keep-alive-timeout relies on server timeouts and
whatnot that will be inherently unreliable on a busy host. The test
fails when run with a high -j value and higher --repeat value passed
to tools/test.py. Move the test to sequential.

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

test https

test-https-server-keep-alive-timeout relies on server timeouts and
whatnot that will be inherently unreliable on a busy host. The test
fails when run with a high `-j` value and higher `--repeat` value passed
to `tools/test.py`. Move the test to `sequential`.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 5, 2017
@mscdex mscdex added the https Issues or PRs related to the https subsystem. label Nov 5, 2017
@mscdex
Copy link
Contributor

mscdex commented Nov 5, 2017

Why does it fail? It's not using a shared port? If it's the setTimeout(), perhaps we should be using the platform timeout variable there?

@Trott
Copy link
Member Author

Trott commented Nov 5, 2017

Why does it fail?

$ tools/test.py -j 96 --repeat 192 test/parallel/test-https-server-keep-alive-timeout.js 
=== release test-https-server-keep-alive-timeout ===                    
Path: parallel/test-https-server-keep-alive-timeout
Mismatched <anonymous> function calls. Expected exactly 3, actual 1.
    at Object.exports.mustCall (/Users/trott/io.js/test/common/index.js:490:10)
    at serverKeepAliveTimeoutWithPipeline (/Users/trott/io.js/test/parallel/test-https-server-keep-alive-timeout.js:34:12)
    at run (/Users/trott/io.js/test/parallel/test-https-server-keep-alive-timeout.js:28:11)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickCallback (internal/process/next_tick.js:180:9)
    at Function.Module.runMain (module.js:684:11)
    at startup (bootstrap_node.js:191:16)
    at bootstrap_node.js:613:3
Command: out/Release/node /Users/trott/io.js/test/parallel/test-https-server-keep-alive-timeout.js
[00:12|% 100|+ 191|-   1]: Done 
$

perhaps we should be using the platform timeout variable there?

common.platformTimeout() doesn't help on anything other than Raspberry Pi and a few other things. For most hosts, it leaves the value untouched. It might have a small number of valid uses, but generally, we overuse it IMO. Most tests that use it should be in sequential or (better) be refactored to eliminate race conditions.

@Trott
Copy link
Member Author

Trott commented Nov 18, 2017

Trott added a commit to Trott/io.js that referenced this pull request Nov 18, 2017
test-https-server-keep-alive-timeout relies on server timeouts and
whatnot that will be inherently unreliable on a busy host. The test
fails when run with a high `-j` value and higher `--repeat` value passed
to `tools/test.py`. Move the test to `sequential`.

PR-URL: nodejs#16775
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member Author

Trott commented Nov 18, 2017

Landed in 780c2d3.

@Trott Trott closed this Nov 18, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
test-https-server-keep-alive-timeout relies on server timeouts and
whatnot that will be inherently unreliable on a busy host. The test
fails when run with a high `-j` value and higher `--repeat` value passed
to `tools/test.py`. Move the test to `sequential`.

PR-URL: #16775
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
test-https-server-keep-alive-timeout relies on server timeouts and
whatnot that will be inherently unreliable on a busy host. The test
fails when run with a high `-j` value and higher `--repeat` value passed
to `tools/test.py`. Move the test to `sequential`.

PR-URL: #16775
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
test-https-server-keep-alive-timeout relies on server timeouts and
whatnot that will be inherently unreliable on a busy host. The test
fails when run with a high `-j` value and higher `--repeat` value passed
to `tools/test.py`. Move the test to `sequential`.

PR-URL: #16775
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
@Trott Trott deleted the 2 branch October 19, 2021 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants