test: fix flaky test-http-client-timeout-option-with-agent by Trott · Pull Request #22403 · nodejs/node · GitHub

First commit:

There is no guarantee that a timeout won't be delayed considerably due
to unrelated activity on the host. Instead of checking that the timeout
happens within a certain tolerance, simply check that it did not happen
too soon.

Second commit:

test-http-client-timeout-option-with-agent no longer checks that the
timeout happens within a certain tolerance so it can be moved to the
parallel test suite.

Checklist

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

There is no guarantee that a timeout won't be delayed considerably due to unrelated activity on the host. Instead of checking that the timeout happens within a certain tolerance, simply check that it did not happen too soon. Fixes: nodejs#22041

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/595/pipeline

CI: https://ci.nodejs.org/job/node-test-pull-request/16573/

(Fixes: Investigate flaky test-http-client-timeout-option-with-agent #22041)

@nodejs/testing

I think a timeout can expire a little sooner than the specified delay so I'm not sure this resolves the flakiness.

I think a timeout can expire a little sooner than the specified delay so I'm not sure this resolves the flakiness.

AFAIK it can't, at least not at the millisecond resolution.

It seems this has been fixed in #20555 as per
#10154 (comment).

Maybe actual duration ${duration}ms was less than expected ${HTTP_CLIENT_TIMEOUT}ms.

CI after applying nit from @refack: https://ci.nodejs.org/job/node-test-pull-request/16583/

I think git no like the change-after-rename:

---
 ...t-http-client-timeout-option-with-agent.js | 56 +++++++++++++++++++
 ...t-http-client-timeout-option-with-agent.js |  6 +-
 2 files changed, 60 insertions(+), 2 deletions(-)
 create mode 100644 test/parallel/test-http-client-timeout-option-with-agent.js

diff --git a/test/parallel/test-http-client-timeout-option-with-agent.js b/test/parallel/test-http-client-timeout-option-with-agent.js
new file mode 100644
index 00000000000..63f683975dc
--- /dev/null
+++ b/test/parallel/test-http-client-timeout-option-with-agent.js
@@ -0,0 +1,56 @@
...
diff --git a/test/sequential/test-http-client-timeout-option-with-agent.js b/test/sequential/test-http-client-timeout-option-with-agent.js
index 63f683975dc..26c93ec55bc 100644
--- a/test/sequential/test-http-client-timeout-option-with-agent.js
+++ b/test/sequential/test-http-client-timeout-option-with-agent.js
@@ -43,8 +43,10 @@ function doRequest() {

P.S. if gets hairy, just ignore my nit.

test-http-client-timeout-option-with-agent no longer checks that the timeout happens within a certain tolerance so it can be moved to the parallel test suite.

@refack Yikes! Fixed.

CI: https://ci.nodejs.org/job/node-test-pull-request/16584/

CI: https://ci.nodejs.org/job/node-test-pull-request/16596/

Resume: https://ci.nodejs.org/job/node-test-commit/20741/
(freebsd saw a different flake, linuxone timed out after 5m 🤔 )

Please 👍 here to fast-track.

There is no guarantee that a timeout won't be delayed considerably due to unrelated activity on the host. Instead of checking that the timeout happens within a certain tolerance, simply check that it did not happen too soon. Fixes: nodejs#22041 PR-URL: nodejs#22403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

test-http-client-timeout-option-with-agent no longer checks that the timeout happens within a certain tolerance so it can be moved to the parallel test suite. PR-URL: nodejs#22403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: nodejs#22403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Landed in b1e2612...36696cf

There is no guarantee that a timeout won't be delayed considerably due to unrelated activity on the host. Instead of checking that the timeout happens within a certain tolerance, simply check that it did not happen too soon. Fixes: #22041 PR-URL: #22403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

test-http-client-timeout-option-with-agent no longer checks that the timeout happens within a certain tolerance so it can be moved to the parallel test suite. PR-URL: #22403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: #22403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

There is no guarantee that a timeout won't be delayed considerably due to unrelated activity on the host. Instead of checking that the timeout happens within a certain tolerance, simply check that it did not happen too soon. Fixes: #22041 PR-URL: #22403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

test-http-client-timeout-option-with-agent no longer checks that the timeout happens within a certain tolerance so it can be moved to the parallel test suite. PR-URL: #22403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: #22403 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

test: fix flaky http timeout test

90f3e2f

There is no guarantee that a timeout won't be delayed considerably due to unrelated activity on the host. Instead of checking that the timeout happens within a certain tolerance, simply check that it did not happen too soon. Fixes: nodejs#22041

nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 19, 2018

Trott mentioned this pull request Aug 19, 2018

Investigate flaky test-http-client-timeout-option-with-agent #22041

Closed

BridgeAR approved these changes Aug 19, 2018

View reviewed changes

lpinca reviewed Aug 19, 2018

View reviewed changes

refack reviewed Aug 19, 2018

View reviewed changes

refack approved these changes Aug 19, 2018

View reviewed changes

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 19, 2018

Trott force-pushed the time-math branch 2 times, most recently from b832020 to 4475597 Compare August 19, 2018 23:05

Trott added 2 commits August 19, 2018 16:53

test: move http timeout test to parallel

0cd6873

test-http-client-timeout-option-with-agent no longer checks that the timeout happens within a certain tolerance so it can be moved to the parallel test suite.

test: improve assert message in http timeout test

d194aae

Trott force-pushed the time-math branch from 4475597 to d194aae Compare August 19, 2018 23:56

joyeecheung approved these changes Aug 20, 2018

View reviewed changes

joyeecheung mentioned this pull request Aug 20, 2018

CI failures: 20180820 nodejs/reliability#15

Closed

lpinca approved these changes Aug 20, 2018

View reviewed changes

Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 20, 2018

refack added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Aug 20, 2018

Trott closed this Aug 21, 2018

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

snyk-bot mentioned this pull request Aug 15, 2021

[Snyk] Fix for 1 vulnerabilities ron-a20/node#39

Open

Trott deleted the time-math branch January 13, 2022 22:50

molllyn1 mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities molllyn1/node#106

Open

137717unity mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities 137717unity/node#122

Open

kissofire mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities kissofire/node#124

Open

aliceUnhinged613 mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities aliceUnhinged613/node#123

Open

ron-a20 mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities ron-a20/node#121

Open

wedataintelligence mentioned this pull request Mar 23, 2024

[Snyk] Fix for 2 vulnerabilities wedataintelligence/node#105

Open

test: fix flaky test-http-client-timeout-option-with-agent by Trott · Pull Request #22403 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: fix flaky test-http-client-timeout-option-with-agent - #22403

test: fix flaky test-http-client-timeout-option-with-agent#22403Trott wants to merge 3 commits intonodejs:masternodejs/node:masterfrom Trott:time-mathTrott/io.js:time-mathCopy head branch name to clipboard

Conversation

Trott commented Aug 19, 2018

Checklist

Uh oh!

nodejs-github-bot commented Aug 19, 2018

Uh oh!

Trott commented Aug 19, 2018

Uh oh!

Trott commented Aug 19, 2018

Uh oh!

Trott commented Aug 19, 2018

Uh oh!

lpinca Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack Aug 19, 2018

Choose a reason for hiding this comment

Uh oh!

lpinca Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

refack Aug 19, 2018

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 19, 2018

Uh oh!

refack commented Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Aug 19, 2018

Uh oh!

Trott commented Aug 20, 2018

Uh oh!

refack commented Aug 20, 2018

Uh oh!

Trott commented Aug 20, 2018

Uh oh!

Trott commented Aug 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

test: fix flaky test-http-client-timeout-option-with-agent#22403
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:time-math

lpinca Aug 19, 2018 •
edited
Loading

refack commented Aug 19, 2018 •
edited
Loading