Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

test/internet/test-dns.js' service name test breaks on some machines #8047

Closed
misterdjules opened this issue Aug 1, 2014 · 2 comments
Closed

Comments

@misterdjules
Copy link

In this test:

TEST(function test_lookupservice_ip_ipv6(done) {
  var req = dns.lookupService('::1', 80, function(err, host, service) {
    if (err) throw err;
    /*
     * On some systems, ::1 can be set to "localhost", on others it
     * can be set to "ip6-localhost". There does not seem to be
     * a consensus on that. Ultimately, it could be set to anything
     * else just by changing /etc/hosts for instance, but it seems
     * that most sane platforms use either one of these two by default.
     */
    assert(host === 'localhost' || host === 'ip6-localhost');
    assert.strictEqual(service, 'http');

    done();
  });

  checkWrap(req);
});

The following assert:

assert.strictEqual(service, 'http');

fails on some of our jenkins nodes because the service name for port 80 can be different than "http". Possible names include "80" and "www". This is usually read by getnameinfo from /etc/services (strace/dtrace confirm this) but I think it may also be read from nscd depending on the system's configuration.

We could work around this issue by hard coding the most common values in the test, but I think we have an opportunity to make the test more robust.

There are several solutions, and I'd like to get your feedback and your suggestions:

  1. LD_PRELOAD uv_getnameinfo and return specific values when resolving specific services' names. For instance, we could run the test with this command:
LD_PRELOAD=fake_uv_getnameinfo UV_GETNAMEINFO_OVERRIDE="80:http" node test/internet/test-dns.js
  1. Run a command like following:
grep -v '^#' /etc/services | grep '\b80/tcp\b' | tr "\t" " " | cut -d ' ' -f 1

before running the test so that we know the actual service name as it is setup on the system that runs it. The problem with this approach is that as mentioned earlier, the service name could be read from nscd and not /etc/services. I'm not sure about that though, it would need some confirmation, I'm still looking into that.

  1. Run the tests in well defined containers of some form. This is just thinking out loud, and would obviously require a large amount of refactoring, so that's probably not going to happen anytime soon.

What do you guys think?

@tjfontaine
Copy link

Long term being able to mock out the apis and return specific results for these tests I think are attractive, but it's also not unreasonable for us to have requirements that slaves that run these tests have a well known configuration, that includes reading from /etc/services for a useful value to lookup

So I'm going to suggest we go with option 2 for now, and I'll make sure each slave has at least something defined in there

misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 6, 2014
Instead of hard-coding http service name in test-dns, retrieve it from
/etc/services. This is not ideal, but it's still better than hard-coding
it.

Fixes nodejs#8047.
misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 6, 2014
Instead of hard-coding http service name in test-dns, retrieve it from
/etc/services. This is not ideal, but it's still better than hard-coding
it.

Fixes nodejs#8047.
@misterdjules
Copy link
Author

Don't pay attention to the current changes, I'm currently setting up a Windows dev environment to make this work on Windows too. Even if we don't run tests on windows with the nodejs-master job, I'd rather have them work right now than find out later.

misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 9, 2014
Instead of hard-coding http service name in test-dns, retrieve it from
/etc/services. This is not ideal, but it's still better than hard-coding
it.

Fixes nodejs#8047.
misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 9, 2014
Instead of hard-coding http service name in test-dns, retrieve it from
/etc/services. This is not ideal, but it's still better than hard-coding
it.

Fixes nodejs#8047.
misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 12, 2014
Instead of hard-coding http service name in test-dns, retrieve it from
/etc/services. This is not ideal, but it's still better than hard-coding
it.

Fixes nodejs#8047.
tjfontaine pushed a commit that referenced this issue Aug 12, 2014
Instead of hard-coding http service name in test-dns, retrieve it from
/etc/services. This is not ideal, but it's still better than hard-coding
it.

Fixes #8047.
misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 12, 2014
Instead of hard-coding http service name in test-dns, retrieve it from
/etc/services. This is not ideal, but it's still better than hard-coding
it.

Fixes nodejs#8047.
misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 12, 2014
Instead of hard-coding http service name in test-dns, retrieve it from
/etc/services. This is not ideal, but it's still better than hard-coding
it.

Fixes nodejs#8047.
misterdjules pushed a commit to misterdjules/node that referenced this issue Aug 12, 2014
Instead of hard-coding http service name in test-dns, retrieve it from
/etc/services. This is not ideal, but it's still better than hard-coding
it.

Fixes nodejs#8047.
tjfontaine pushed a commit that referenced this issue Aug 13, 2014
Instead of hard-coding http service name in test-dns, retrieve it from
/etc/services. This is not ideal, but it's still better than hard-coding
it.

Fixes #8047.

Signed-off-by: Timothy J Fontaine <tjfontaine@gmail.com>
fiveisprime pushed a commit to fiveisprime/node that referenced this issue Aug 17, 2014
Instead of hard-coding http service name in test-dns, retrieve it from
/etc/services. This is not ideal, but it's still better than hard-coding
it.

Fixes nodejs#8047.

Signed-off-by: Timothy J Fontaine <tjfontaine@gmail.com>
Trott added a commit to Trott/io.js that referenced this issue May 12, 2016
Replace lightly-used services file parsing in favor of
confirming one of a small number of allowable values in service name
lookup tests.

In nodejs/node-v0.x-archive#8047, it was
decided that this sort of service file parsing was superior to
hardcoding acceptable values, but I'm not convinced:

* No guarantee that the host uses /etc/services before, e.g., nscd.
* Increases complexity of tests without guaranteeing robustness.

I think that simply checking against a small set of expected values
may be a better solution. Ideally, there would also be a unit test that
used a test double for the appropriate `cares` function and confirms
that it is called with the correct parameters, but now we're getting way
ahead of ourselves.
Trott added a commit to Trott/io.js that referenced this issue May 14, 2016
Replace lightly-used services file parsing in favor of
confirming one of a small number of allowable values in service name
lookup tests.

In nodejs/node-v0.x-archive#8047, it was
decided that this sort of service file parsing was superior to
hardcoding acceptable values, but I'm not convinced:

* No guarantee that the host uses /etc/services before, e.g., nscd.
* Increases complexity of tests without guaranteeing robustness.

I think that simply checking against a small set of expected values
may be a better solution. Ideally, there would also be a unit test that
used a test double for the appropriate `cares` function and confirms
that it is called with the correct parameters, but now we're getting way
ahead of ourselves.

PR-URL: nodejs#6709
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
evanlucas pushed a commit to nodejs/node that referenced this issue May 17, 2016
Replace lightly-used services file parsing in favor of
confirming one of a small number of allowable values in service name
lookup tests.

In nodejs/node-v0.x-archive#8047, it was
decided that this sort of service file parsing was superior to
hardcoding acceptable values, but I'm not convinced:

* No guarantee that the host uses /etc/services before, e.g., nscd.
* Increases complexity of tests without guaranteeing robustness.

I think that simply checking against a small set of expected values
may be a better solution. Ideally, there would also be a unit test that
used a test double for the appropriate `cares` function and confirms
that it is called with the correct parameters, but now we're getting way
ahead of ourselves.

PR-URL: #6709
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants