Skip to content

Commit

Permalink
test: remove common.getServiceName()
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
Trott authored and evanlucas committed May 17, 2016
1 parent 8c634d7 commit ccbc78c
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 87 deletions.
51 changes: 1 addition & 50 deletions test/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ Object.defineProperty(exports, 'opensslCli', {get: function() {
opensslCli = false;
}
return opensslCli;
}, enumerable: true });
}, enumerable: true});

Object.defineProperty(exports, 'hasCrypto', {
get: function() {
Expand Down Expand Up @@ -395,55 +395,6 @@ exports.mustCall = function(fn, expected) {
};
};

var etcServicesFileName = path.join('/etc', 'services');
if (exports.isWindows) {
etcServicesFileName = path.join(process.env.SystemRoot, 'System32', 'drivers',
'etc', 'services');
}

/*
* Returns a string that represents the service name associated
* to the service bound to port "port" and using protocol "protocol".
*
* If the service is not defined in the services file, it returns
* the port number as a string.
*
* Returns undefined if /etc/services (or its equivalent on non-UNIX
* platforms) can't be read.
*/
exports.getServiceName = function getServiceName(port, protocol) {
if (port == null) {
throw new Error('Missing port number');
}

if (typeof protocol !== 'string') {
throw new Error('Protocol must be a string');
}

/*
* By default, if a service can't be found in /etc/services,
* its name is considered to be its port number.
*/
var serviceName = port.toString();

try {
var servicesContent = fs.readFileSync(etcServicesFileName,
{ encoding: 'utf8'});
var regexp = `^(\\w+)\\s+\\s${port}/${protocol}\\s`;
var re = new RegExp(regexp, 'm');

var matches = re.exec(servicesContent);
if (matches && matches.length > 1) {
serviceName = matches[1];
}
} catch (e) {
console.error('Cannot read file: ', etcServicesFileName);
return undefined;
}

return serviceName;
};

exports.hasMultiLocalhost = function hasMultiLocalhost() {
var TCP = process.binding('tcp_wrap').TCP;
var t = new TCP();
Expand Down
21 changes: 2 additions & 19 deletions test/internet/test-dns-ipv4.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
'use strict';
const common = require('../common');
require('../common');
const assert = require('assert');
const dns = require('dns');
const net = require('net');
Expand Down Expand Up @@ -173,24 +173,7 @@ TEST(function test_lookupservice_ip_ipv4(done) {
if (err) throw err;
assert.equal(typeof host, 'string');
assert(host);

/*
* Retrieve the actual HTTP service name as setup on the host currently
* running the test by reading it from /etc/services. This is not ideal,
* as the service name lookup could use another mechanism (e.g nscd), but
* it's already better than hardcoding it.
*/
var httpServiceName = common.getServiceName(80, 'tcp');
if (!httpServiceName) {
/*
* Couldn't find service name, reverting to the most sensible default
* for port 80.
*/
httpServiceName = 'http';
}

assert.strictEqual(service, httpServiceName);

assert(['http', 'www', '80'].includes(service));
done();
});

Expand Down
19 changes: 1 addition & 18 deletions test/internet/test-dns-ipv6.js
Original file line number Diff line number Diff line change
Expand Up @@ -182,24 +182,7 @@ TEST(function test_lookupservice_ip_ipv6(done) {
if (err) throw err;
assert.equal(typeof host, 'string');
assert(host);

/*
* Retrieve the actual HTTP service name as setup on the host currently
* running the test by reading it from /etc/services. This is not ideal,
* as the service name lookup could use another mechanism (e.g nscd), but
* it's already better than hardcoding it.
*/
var httpServiceName = common.getServiceName(80, 'tcp');
if (!httpServiceName) {
/*
* Couldn't find service name, reverting to the most sensible default
* for port 80.
*/
httpServiceName = 'http';
}

assert.strictEqual(service, httpServiceName);

assert(['http', 'www', '80'].includes(service));
done();
});

Expand Down

0 comments on commit ccbc78c

Please sign in to comment.