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: implement common.skipIfNoIpv6Localhost #16248

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

Refs: #15936 (comment)

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 17, 2017
@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

Fixed lint errors. New CI: https://ci.nodejs.org/job/node-test-pull-request/10779/

cc @nodejs/testing

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@maclover7
Copy link
Contributor

Nit: should this change to be a skipIf function like skipIf32Bits and skipIfInspectorDisabled? Mostly just to fit to the same style.

@joyeecheung
Copy link
Member Author

@maclover7 Yeah that's what I originally named this..I think at that point I thought the semantics of this did not align with other skipIf* functions but now looking at it again I don't find any problems? Anyway I'll rebase and rename it.

@joyeecheung joyeecheung force-pushed the skip-localhost-ipv6 branch 2 times, most recently from f0726ae to bbeab1a Compare November 6, 2017 13:02
@joyeecheung joyeecheung changed the title test: implement common.runIfHasIpv6Localhost test: implement common.skipIfNoIpv6Localhost Nov 6, 2017
@joyeecheung
Copy link
Member Author

@mhdawson @maclover7 Rebased and renamed , PTAL.

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

@maclover7
Copy link
Contributor

LGTM

@joyeecheung
Copy link
Member Author

This is failing on raspberry pis since they don't have ipv6 support and the tests mustCall the callbacks. I'll fix it.

@joyeecheung joyeecheung added the wip Issues and PRs that are still a work in progress. label Nov 8, 2017
@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 11, 2017

Removed the common.mustCall since uh...the point of this PR is to skip the callbacks when ipv6 localhost is not available...¯\(ツ)

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

@joyeecheung joyeecheung removed the wip Issues and PRs that are still a work in progress. label Nov 11, 2017
Which run the callback with a local host that can be
resolved to ::1 if available, otherwise skip the test.
@joyeecheung
Copy link
Member Author

@@ -20,7 +19,7 @@ function runTest() {
res.end();
})).listen(0, '::1', common.mustCall(function() {
const options = {
host: 'localhost',
host: ipv6Host,
Copy link
Contributor

Choose a reason for hiding this comment

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

given that // This test that the family option of https.get is honored. would it be correct to replace this with '::1' since this what the server bind to?

Copy link
Member Author

@joyeecheung joyeecheung Nov 12, 2017

Choose a reason for hiding this comment

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

@refack I think replacing it with a ip would skip the family option handling altogether. The family option is used when performing a lookup, so if the connection doesn't even need to lookup then it won't be tested. In net.js:

  // If host is an IP, skip performing a lookup
  var addressType = cares.isIP(host);
  if (addressType) {
    nextTick(self[async_id_symbol], function() {
      if (self.connecting)
        internalConnect(self, host, port, addressType, localAddress, localPort);
    });
    return;
  }

....

  var dnsopts = {
    family: options.family,
    hints: options.hints || 0
  };

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Let's think is this is not overkill?

@refack
Copy link
Contributor

refack commented Nov 12, 2017

Let's think is this is not overkill?

AFAICT those two test can skip the DNS calls, so if they work with '::1' I's rather have that.
(the working assumption is that the test harness should be K.I.S.S. as much as possible.)

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 12, 2017

@refack

AFAICT those two test can skip the DNS calls, so if they work with '::1' I's rather have that.
(the working assumption is that the test harness should be K.I.S.S. as much as possible.)

Uh..those tests are testing DNS calls, family is passed to the lookup function.

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 12, 2017

Ah, forgot to run the linter...also the arm compilation seems to be having a outage?

@refack
Copy link
Contributor

refack commented Nov 12, 2017

Uh..those tests are testing DNS calls, family is passed to the lookup function.

I want to dig into that. let's see if we can refactor something around this.

@refack refack mentioned this pull request Nov 12, 2017
3 tasks
@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 15, 2017

Closing in favor of #16976

refack added a commit to refack/node that referenced this pull request Nov 21, 2017
PR-URL: nodejs#16976
Refs: nodejs#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 pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #16976
Refs: #16248
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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>
rvagg pushed a commit that referenced this pull request Aug 16, 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
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants