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-http-client-timeout-option-with-agent #22403

Closed
wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 19, 2018

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
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 19, 2018
@Trott
Copy link
Member Author

Trott commented Aug 19, 2018

@Trott
Copy link
Member Author

Trott commented Aug 19, 2018

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

@Trott
Copy link
Member Author

Trott commented Aug 19, 2018

@nodejs/testing

// some number of milliseconds, so test it in second units.
assert.strictEqual(duration / 1000 | 0, HTTP_CLIENT_TIMEOUT / 1000);
// some number of milliseconds.
assert.ok(duration >= HTTP_CLIENT_TIMEOUT,
Copy link
Member

@lpinca lpinca Aug 19, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

assert.strictEqual(duration / 1000 | 0, HTTP_CLIENT_TIMEOUT / 1000);
// some number of milliseconds.
assert.ok(duration >= HTTP_CLIENT_TIMEOUT,
`${duration} < ${HTTP_CLIENT_TIMEOUT}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 19, 2018
@Trott Trott force-pushed the time-math branch 2 times, most recently from b832020 to 4475597 Compare August 19, 2018 23:05
@Trott
Copy link
Member Author

Trott commented Aug 19, 2018

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

@refack
Copy link
Contributor

refack commented Aug 19, 2018

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.
@Trott
Copy link
Member Author

Trott commented Aug 19, 2018

@Trott
Copy link
Member Author

Trott commented Aug 20, 2018

@refack
Copy link
Contributor

refack commented Aug 20, 2018

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

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

Trott commented Aug 20, 2018

Please 👍 here to fast-track.

@refack refack added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Aug 20, 2018
Trott added a commit to Trott/io.js that referenced this pull request Aug 21, 2018
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>
Trott added a commit to Trott/io.js that referenced this pull request Aug 21, 2018
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>
Trott added a commit to Trott/io.js that referenced this pull request Aug 21, 2018
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>
@Trott
Copy link
Member Author

Trott commented Aug 21, 2018

Landed in b1e2612...36696cf

@Trott Trott closed this Aug 21, 2018
targos pushed a commit that referenced this pull request Aug 21, 2018
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>
targos pushed a commit that referenced this pull request Aug 21, 2018
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>
targos pushed a commit that referenced this pull request Aug 21, 2018
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>
targos pushed a commit that referenced this pull request Sep 3, 2018
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>
targos pushed a commit that referenced this pull request Sep 3, 2018
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>
targos pushed a commit that referenced this pull request Sep 3, 2018
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>
@Trott Trott deleted the time-math branch January 13, 2022 22:50
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. fast-track PRs that do not need to wait for 48 hours to land. flaky-test Issues and PRs related to the tests with unstable failures on the CI. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants