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: bypass dns for IPv6 net tests #16976

Merged
merged 1 commit into from
Nov 21, 2017
Merged

Conversation

refack
Copy link
Contributor

@refack refack commented Nov 12, 2017

Refs: #16248

For to test that target https and net mock out the call to dns, but verify all the right parameters are passed

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,net,https,dns

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 12, 2017
@refack
Copy link
Contributor Author

refack commented Nov 12, 2017

@refack refack added dns Issues and PRs related to the dns subsystem. https Issues or PRs related to the https subsystem. net Issues and PRs related to the net subsystem. labels Nov 12, 2017
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

IIUC this is safe to do because the integration test of the family option is done in test/internet/test-dns-ipv*.js...

client.on('error', (err) => {
// ENOTFOUND means we don't have the requested address, so we assume IPv6
// is not supported on the machine and skip the test.
if ((err.syscall === 'getaddrinfo') && (err.code === 'ENOTFOUND')) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this anymore if we are mocking lookup? AFAICT the errors are emitted there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove and stress test.

@refack
Copy link
Contributor Author

refack commented Nov 12, 2017

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2017
@joyeecheung
Copy link
Member

Should be ready to land. New CI: https://ci.nodejs.org/job/node-test-pull-request/11594/

PR-URL: nodejs#16976
Refs: nodejs#16248
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@refack refack merged commit 283b949 into nodejs:master Nov 21, 2017
@refack
Copy link
Contributor Author

refack commented Nov 21, 2017

Thanks. Landed in 283b949

@refack refack deleted the less-ipv6-dns branch November 21, 2017 21:39
@refack refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #16976
Refs: #16248
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #16976
Refs: #16248
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@gibfahn
Copy link
Member

gibfahn commented Dec 19, 2017

@refack mind raising a backport?

BethGriggs pushed a commit to BethGriggs/node that referenced this pull request Jul 10, 2018
PR-URL: nodejs#16976
Refs: nodejs#16248
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2018
Backport-PR-URL: #21736
PR-URL: #16976
Refs: #16248
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem. https Issues or PRs related to the https subsystem. net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants