Skip to content

Conversation

@Trott
Copy link
Member

@TrottTrott 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-botnodejs-github-bot added the test Issues and PRs related to the tests. label Nov 5, 2017
@mscdexmscdex added the https Issues or PRs related to the https subsystem. label Nov 5, 2017
@mscdex
Copy link
Contributor

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
MemberAuthor

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-timeoutMismatched <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:3Command: 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
MemberAuthor

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
MemberAuthor

Landed in 780c2d3.

@TrottTrott 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]>
@MylesBorinsMylesBorins 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]>
@gibfahngibfahn 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]>
@gibfahngibfahn mentioned this pull request Dec 20, 2017
@TrottTrott deleted the 2 branch October 19, 2021 13:18
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

httpsIssues or PRs related to the https subsystem.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@Trott@mscdex@jasnell@cjihrig@XadillaX@gireeshpunathil@MylesBorins@gibfahn@nodejs-github-bot