Skip to content

test: fix flaky test-http-dns-error#16534

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.

Sign up for GitHub

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

Jump to bottom
Closed
bengl wants to merge 1 commit into from
Closed

test: fix flaky test-http-dns-error #16534

bengl wants to merge 1 commit into from

Conversation

@bengl
Copy link
Member

@benglbengl commented Oct 26, 2017

Under some conditions, the error received from getaddrinfo might
actually be EAGAIN, meaning the request should be retried. Allowing for
5 retries before erroring out.

This was happening to me about 50% of the time when running make test-parallel, and about 5-10% of the time when running this test file alone. My machine: Linux benglbox 4.12.4-1-ARCH #1 SMP PREEMPT Fri Jul 28 18:54:18 UTC 2017 x86_64 GNU/Linux.

Also replace one-off function with common.mustNotCall().

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@bengl
test: fix flaky test-http-dns-error
182278b
Under some conditions, the error received from getaddrinfo might actually be EAGAIN, meaning the request should be retried. Allowing for 5 retries before erroring out. Also replace one-off function with common.mustNotCall().
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Oct 26, 2017
@bengl
Copy link
MemberAuthor

bengl commented Oct 26, 2017

/cc @nodejs/testing

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

@refack
Copy link
Contributor

refack commented Oct 27, 2017

Besides the fix this should either be moved to /internet/ or made to use ../common/dns like

node/test/parallel/test-dns-resolveany.js

Line 3 in 5118f31

constdnstools=require('../common/dns');

@mscdexmscdex added the http Issues or PRs related to the http subsystem. label Oct 27, 2017
@joyeecheung
Copy link
Member

joyeecheung commented Oct 27, 2017

@refack@bengl Can you reporuduces this offline? If so then it should still be in parallel...

@BridgeAR
Copy link
Member

BridgeAR commented Nov 23, 2017

Ping @bengl

@maclover7maclover7 added the wip Issues and PRs that are still a work in progress. label Dec 21, 2017
@BridgeAR
Copy link
Member

BridgeAR commented Jan 5, 2018

Ping @bengl again

@BridgeAR
Copy link
Member

BridgeAR commented Jan 19, 2018

@refack@joyeecheung do you think it can land as is? Otherwise I would go ahead and close it.

@joyeecheung
Copy link
Member

joyeecheung commented Jan 20, 2018

@BridgeAR I don't think it hurts anything so I am OK with landing this. I am not entirely sure what this test is testing, if it's just to verify that the DNS errors bubble up properly then it should just mock the DNS error instead of working around real errors like this - that indicates the test belongs to test/internet.

@BridgeARBridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Jan 31, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Jan 31, 2018

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

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 1, 2018
@bengl@BridgeAR
test: fix flaky test-http-dns-error
7d4b772
Under some conditions, the error received from getaddrinfo might actually be EAGAIN, meaning the request should be retried. Allowing for 5 retries before erroring out. Also replace one-off function with common.mustNotCall(). PR-URL: nodejs#16534 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

Landed in 7d4b772

@BridgeARBridgeAR closed this Feb 1, 2018
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
@bengl@MylesBorins
test: fix flaky test-http-dns-error
1c9ab28
Under some conditions, the error received from getaddrinfo might actually be EAGAIN, meaning the request should be retried. Allowing for 5 retries before erroring out. Also replace one-off function with common.mustNotCall(). PR-URL: #16534 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@bengl@MylesBorins
test: fix flaky test-http-dns-error
447919a
Under some conditions, the error received from getaddrinfo might actually be EAGAIN, meaning the request should be retried. Allowing for 5 retries before erroring out. Also replace one-off function with common.mustNotCall(). PR-URL: #16534 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@bengl@MylesBorins
test: fix flaky test-http-dns-error
6b74064
Under some conditions, the error received from getaddrinfo might actually be EAGAIN, meaning the request should be retried. Allowing for 5 retries before erroring out. Also replace one-off function with common.mustNotCall(). PR-URL: #16534 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@bengl@MylesBorins
test: fix flaky test-http-dns-error
b2e8cce
Under some conditions, the error received from getaddrinfo might actually be EAGAIN, meaning the request should be retried. Allowing for 5 retries before erroring out. Also replace one-off function with common.mustNotCall(). PR-URL: #16534 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 20, 2018

Didn't land cleanly on 6.x, and opted not to land.

MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@bengl@MylesBorins
test: fix flaky test-http-dns-error
de63954
Under some conditions, the error received from getaddrinfo might actually be EAGAIN, meaning the request should be retried. Allowing for 5 retries before erroring out. Also replace one-off function with common.mustNotCall(). PR-URL: #16534 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@bengl@MylesBorins
test: fix flaky test-http-dns-error
a1fb263
Under some conditions, the error received from getaddrinfo might actually be EAGAIN, meaning the request should be retried. Allowing for 5 retries before erroring out. Also replace one-off function with common.mustNotCall(). PR-URL: #16534 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@bengl@MayaLekova
test: fix flaky test-http-dns-error
f8d4102
Under some conditions, the error received from getaddrinfo might actually be EAGAIN, meaning the request should be retried. Allowing for 5 retries before erroring out. Also replace one-off function with common.mustNotCall(). PR-URL: nodejs#16534 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

+1 more reviewer

@refackrefackrefack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.httpIssues or PRs related to the http subsystem.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@bengl@refack@joyeecheung@BridgeAR@MylesBorins@jasnell@mscdex@maclover7@nodejs-github-bot