-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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 http-client-timeout-option-listeners #10224
Conversation
Sample failure on CI: https://ci.nodejs.org/job/node-test-commit-freebsd/5810/nodes=freebsd11-x64/console not ok 465 parallel/test-http-client-timeout-option-listeners
---
duration_ms: 0.551
severity: fail
stack: |-
assert.js:85
throw new assert.AssertionError({
^
AssertionError: 0 === 1
at common.mustCall (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-http-client-timeout-option-listeners.js:24:12)
at /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/common.js:416:15
at _combinedTickCallback (internal/process/next_tick.js:71:11)
at process._tickCallback (internal/process/next_tick.js:98:9)
... |
On my machine, at least, this results in the test reliably failing, demonstrating that concurrency negatively impacts the test: tools/test.py -j 32 test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js test/parallel/test-http-client-timeout-option-listeners.js |
Error when failing with the above command is strikingly similar to the failure in CI, suggesting a similar cause: assert.js:85
throw new assert.AssertionError({
^
AssertionError: 0 === 1
at common.mustCall (/Users/trott/io.js/test/parallel/test-http-client-timeout-option-listeners.js:26:14)
at /Users/trott/io.js/test/common.js:416:15
at _combinedTickCallback (internal/process/next_tick.js:71:11)
at process._tickCallback (internal/process/next_tick.js:98:9) |
The changes LGTM. As an alternative, what about setting a very large timeout? Do you think it makes sense?
|
Sure, we could use |
@Trott I thought about
I agree. I prefer the latter as moving the test to sequential will lead to a minimal increase of the running time of the tests, but any of the proposed solutions are acceptable to me. |
test-http-client-timeout-option-listeners is flaky due to depending on completing operations before a 100ms socket timeout. The socket timeout is an integral part of the test but can be very large. Set to the maximum allowable value.
359a1bf
to
a5e7220
Compare
@santigimeno OK, I did it your way! PTAL! |
test-http-client-timeout-option-listeners is flaky due to depending on completing operations before a 100ms socket timeout. The socket timeout is an integral part of the test but can be very large. Set to the maximum allowable value. PR-URL: nodejs#10224 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Landed in 4a5c719. |
test-http-client-timeout-option-listeners is flaky due to depending on completing operations before a 100ms socket timeout. The socket timeout is an integral part of the test but can be very large. Set to the maximum allowable value. PR-URL: #10224 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
test-http-client-timeout-option-listeners is flaky due to depending on completing operations before a 100ms socket timeout. The socket timeout is an integral part of the test but can be very large. Set to the maximum allowable value. PR-URL: #10224 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
test-http-client-timeout-option-listeners is flaky due to depending on completing operations before a 100ms socket timeout. The socket timeout is an integral part of the test but can be very large. Set to the maximum allowable value. PR-URL: #10224 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
test-http-client-timeout-option-listeners is flaky due to depending on completing operations before a 100ms socket timeout. The socket timeout is an integral part of the test but can be very large. Set to the maximum allowable value. PR-URL: #10224 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test http
Description of change
test-http-client-timeout-option-listeners is flaky due to depending on
completing operations before a 100ms socket timeout. The socket timeout
is an integral part of the test. Load on the machine can affect the
test, so move it to sequential.