Skip to content

Commit ccbc78c

Browse files
Trottevanlucas
authored andcommitted
test: remove common.getServiceName()
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
1 parent 8c634d7 commit ccbc78c

File tree

3 files changed

+4
-87
lines changed

3 files changed

+4
-87
lines changed

test/common.js

Lines changed: 1 addition & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ Object.defineProperty(exports, 'opensslCli', {get: function() {
160160
opensslCli = false;
161161
}
162162
return opensslCli;
163-
}, enumerable: true });
163+
}, enumerable: true});
164164

165165
Object.defineProperty(exports, 'hasCrypto', {
166166
get: function() {
@@ -395,55 +395,6 @@ exports.mustCall = function(fn, expected) {
395395
};
396396
};
397397

398-
var etcServicesFileName = path.join('/etc', 'services');
399-
if (exports.isWindows) {
400-
etcServicesFileName = path.join(process.env.SystemRoot, 'System32', 'drivers',
401-
'etc', 'services');
402-
}
403-
404-
/*
405-
* Returns a string that represents the service name associated
406-
* to the service bound to port "port" and using protocol "protocol".
407-
*
408-
* If the service is not defined in the services file, it returns
409-
* the port number as a string.
410-
*
411-
* Returns undefined if /etc/services (or its equivalent on non-UNIX
412-
* platforms) can't be read.
413-
*/
414-
exports.getServiceName = function getServiceName(port, protocol) {
415-
if (port == null) {
416-
throw new Error('Missing port number');
417-
}
418-
419-
if (typeof protocol !== 'string') {
420-
throw new Error('Protocol must be a string');
421-
}
422-
423-
/*
424-
* By default, if a service can't be found in /etc/services,
425-
* its name is considered to be its port number.
426-
*/
427-
var serviceName = port.toString();
428-
429-
try {
430-
var servicesContent = fs.readFileSync(etcServicesFileName,
431-
{ encoding: 'utf8'});
432-
var regexp = `^(\\w+)\\s+\\s${port}/${protocol}\\s`;
433-
var re = new RegExp(regexp, 'm');
434-
435-
var matches = re.exec(servicesContent);
436-
if (matches && matches.length > 1) {
437-
serviceName = matches[1];
438-
}
439-
} catch (e) {
440-
console.error('Cannot read file: ', etcServicesFileName);
441-
return undefined;
442-
}
443-
444-
return serviceName;
445-
};
446-
447398
exports.hasMultiLocalhost = function hasMultiLocalhost() {
448399
var TCP = process.binding('tcp_wrap').TCP;
449400
var t = new TCP();

test/internet/test-dns-ipv4.js

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
'use strict';
2-
const common = require('../common');
2+
require('../common');
33
const assert = require('assert');
44
const dns = require('dns');
55
const net = require('net');
@@ -173,24 +173,7 @@ TEST(function test_lookupservice_ip_ipv4(done) {
173173
if (err) throw err;
174174
assert.equal(typeof host, 'string');
175175
assert(host);
176-
177-
/*
178-
* Retrieve the actual HTTP service name as setup on the host currently
179-
* running the test by reading it from /etc/services. This is not ideal,
180-
* as the service name lookup could use another mechanism (e.g nscd), but
181-
* it's already better than hardcoding it.
182-
*/
183-
var httpServiceName = common.getServiceName(80, 'tcp');
184-
if (!httpServiceName) {
185-
/*
186-
* Couldn't find service name, reverting to the most sensible default
187-
* for port 80.
188-
*/
189-
httpServiceName = 'http';
190-
}
191-
192-
assert.strictEqual(service, httpServiceName);
193-
176+
assert(['http', 'www', '80'].includes(service));
194177
done();
195178
});
196179

test/internet/test-dns-ipv6.js

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -182,24 +182,7 @@ TEST(function test_lookupservice_ip_ipv6(done) {
182182
if (err) throw err;
183183
assert.equal(typeof host, 'string');
184184
assert(host);
185-
186-
/*
187-
* Retrieve the actual HTTP service name as setup on the host currently
188-
* running the test by reading it from /etc/services. This is not ideal,
189-
* as the service name lookup could use another mechanism (e.g nscd), but
190-
* it's already better than hardcoding it.
191-
*/
192-
var httpServiceName = common.getServiceName(80, 'tcp');
193-
if (!httpServiceName) {
194-
/*
195-
* Couldn't find service name, reverting to the most sensible default
196-
* for port 80.
197-
*/
198-
httpServiceName = 'http';
199-
}
200-
201-
assert.strictEqual(service, httpServiceName);
202-
185+
assert(['http', 'www', '80'].includes(service));
203186
done();
204187
});
205188

0 commit comments

Comments
 (0)