Skip to content

[test] fix ipv6 test#1246

Merged
lpinca merged 1 commit intowebsockets:masterfrom
dcharbonnier:feature/ipv6-test-fix
Dec 1, 2017
Merged

[test] fix ipv6 test#1246
lpinca merged 1 commit intowebsockets:masterfrom
dcharbonnier:feature/ipv6-test-fix

Conversation

@dcharbonnier
Copy link
Contributor

@dcharbonnier dcharbonnier commented Dec 1, 2017

Seams a bit complicated but it's more consistent than simply skipping silently a test.
If the test is skipped it will be mentioned on the result (maybe we should raise an error during ci if a test is skipped) and the test will pass on more machine trying a list of potential alias for the loop-back.

with const hostsCandidates = ['localhost'];

  WebSocket
    options
      - accepts the family option

with const hostsCandidates = ['localhost', 'ip6-loopback'];

  WebSocket
    options
      ✓ accepts the family option

with const hostsCandidates = ['localhost', 'ip6-allnodes'];

  WebSocket
    options
      1) accepts the family option


  0 passing (42ms)
  1 failing

  1) WebSocket
       options
         accepts the family option:
     Error: connect ENETUNREACH ff02::1:39758 - Local (:::0)

@dcharbonnier dcharbonnier changed the title fix ipv6 test [test] fix ipv6 test Dec 1, 2017
@lpinca
Copy link
Member

lpinca commented Dec 1, 2017

What do you think about this?

diff --git a/test/WebSocket.test.js b/test/WebSocket.test.js
index 178dbfd..6e8318e 100644
--- a/test/WebSocket.test.js
+++ b/test/WebSocket.test.js
@@ -7,7 +7,9 @@ const assert = require('assert');
 const crypto = require('crypto');
 const https = require('https');
 const http = require('http');
+const dns = require('dns');
 const net = require('net');
+const os = require('os');
 const fs = require('fs');
 
 const constants = require('../lib/Constants');
@@ -95,20 +97,35 @@ describe('WebSocket', function () {
     });
 
     it('accepts the family option', function (done) {
-      const wss = new WebSocket.Server({ host: '::1', port: 0 }, () => {
-        const port = wss._server.address().port;
-        const ws = new WebSocket(`ws://localhost:${port}`, { family: 6 });
+      const re = process.platform === 'win32' ? /Loopback Pseudo-Interface/ : /lo/;
+      const ifaces = os.networkInterfaces();
+      const hasIPv6 = Object.keys(ifaces).some((name) => {
+        return re.test(name) && ifaces[name].some((info) => info.family === 'IPv6');
       });
 
-      wss.on('error', (err) => {
-        // Skip this test on machines where IPv6 is not supported.
-        if (err.code === 'EADDRNOTAVAIL') err = undefined;
-        wss.close(() => done(err));
-      });
+      // Skip this test on machines where IPv6 is not supported.
+      if (!hasIPv6) return done();
 
-      wss.on('connection', (ws, req) => {
-        assert.strictEqual(req.connection.remoteAddress, '::1');
-        wss.close(done);
+      dns.lookup('localhost', { family: 6, all: true }, (err, addresses) => {
+        // Skip this test if localhost does not resolve to ::1.
+        if (err) {
+          if (err.code === 'ENOTFOUND' || err.code === 'EAI_AGAIN') {
+            err = undefined;
+          }
+          return done(err);
+        }
+
+        if (!addresses.some((val) => val.address === '::1')) return done();
+
+        const wss = new WebSocket.Server({ host: '::1', port: 0 }, () => {
+          const port = wss._server.address().port;
+          const ws = new WebSocket(`ws://localhost:${port}`, { family: 6 });
+        });
+
+        wss.on('connection', (ws, req) => {
+          assert.strictEqual(req.connection.remoteAddress, '::1');
+          wss.close(done);
+        });
       });
     });
   });

It's a little better as it doesn't run the test multiple times if both localhost and ip6-loopback resolves to ::1.

@lpinca
Copy link
Member

lpinca commented Dec 1, 2017

We can use skip() as long as we are consistent with other skipped tests, but I'm not very happy with trying multiple aliases. If IPv6 is enabled on a particular machine and localhost does not resolve to ::1, I think that it is something that needs to be fixed on that particular machine.

@dcharbonnier
Copy link
Contributor Author

I would replace if (!hasIPv6) return done(); with if (!hasIPv6) return this.skip(); because the test don't pass it's just skipped.

And with this change the test will be skipped (or succeed without testing anything). My /etc/hosts is standard, there is nothing strange, just a regular linux machine.
I agree that testing random domain names is not a good idea and passing the test twice is useless, we can add an other variable to skip the second test if the first one was a success, I wanted to keep it has simple as possible but it's not "nice". With await it would be wait more easy to make it "look" good.

Btw the test is probably skipped on the CI and it's invisible ;-)

@dcharbonnier
Copy link
Contributor Author

For example Red Hat Enterprise Linux 5

127.0.0.1        localhost.localdomain localhost
::1              localhost6.localdomain6 localhost6

@lpinca
Copy link
Member

lpinca commented Dec 1, 2017

@dcharbonnier yes same is on fedora but if you do

ping6 localhost

or try to resolve with dns.lookup('localhost', { family: 6 }, cb) it works. Same is for plain Ubuntu, macOS, etc.

@dcharbonnier
Copy link
Contributor Author

➜  ~ salt "*"  cmd.run "ping6 -c 1 localhost"|grep Cannot|wc -l
35
➜  ~ salt "*"  cmd.run "ping6 -c 1 localhost"|grep received|wc -l
65
➜  ~ salt "*"  cmd.run "test -f /proc/net/if_inet6 && echo 'IPv6 supported'"|grep supported|wc -l
100

but ok, let's change it with your proposal will see

@lpinca
Copy link
Member

lpinca commented Dec 1, 2017

You can update this PR if you want.

@lpinca lpinca reopened this Dec 1, 2017
@dcharbonnier
Copy link
Contributor Author

dcharbonnier commented Dec 1, 2017

easier to push your branch :-)

@lpinca lpinca force-pushed the feature/ipv6-test-fix branch from e34f383 to e90159c Compare December 1, 2017 09:41
@lpinca lpinca merged commit a166af4 into websockets:master Dec 1, 2017
@lpinca
Copy link
Member

lpinca commented Dec 1, 2017

Thank you.

@dcharbonnier
Copy link
Contributor Author

Thank you, so now we know this test never pass in the CI ;-)

@lpinca
Copy link
Member

lpinca commented Dec 1, 2017

It passes on AppVeyor and it used to pass on Travis CI when it was possible to bind the server on ::1, see https://travis-ci.org/websockets/ws/jobs/295237782.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants