Commit ccbc78c
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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>1 parent 8c634d7 commit ccbc78c
3 files changed
Lines changed: 4 additions & 87 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -160,7 +160,7 @@ Object.defineProperty(exports, 'opensslCli', {get: function() { | |||
| 160 | 160 | opensslCli = false; | |
| 161 | 161 | } | |
| 162 | 162 | return opensslCli; | |
| 163 | - }, enumerable: true }); | ||
| 163 | + }, enumerable: true}); | ||
| 164 | 164 | ||
| 165 | 165 | Object.defineProperty(exports, 'hasCrypto', { | |
| 166 | 166 | get: function() { | |
@@ -395,55 +395,6 @@ exports.mustCall = function(fn, expected) { | |||
| 395 | 395 | }; | |
| 396 | 396 | }; | |
| 397 | 397 | ||
| 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 | - | ||
| 447 | 398 | exports.hasMultiLocalhost = function hasMultiLocalhost() { | |
| 448 | 399 | var TCP = process.binding('tcp_wrap').TCP; | |
| 449 | 400 | var t = new TCP(); | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1,5 +1,5 @@ | |||
| 1 | 1 | 'use strict'; | |
| 2 | - const common = require('../common'); | ||
| 2 | + require('../common'); | ||
| 3 | 3 | const assert = require('assert'); | |
| 4 | 4 | const dns = require('dns'); | |
| 5 | 5 | const net = require('net'); | |
@@ -173,24 +173,7 @@ TEST(function test_lookupservice_ip_ipv4(done) { | |||
| 173 | 173 | if (err) throw err; | |
| 174 | 174 | assert.equal(typeof host, 'string'); | |
| 175 | 175 | 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)); | ||
| 194 | 177 | done(); | |
| 195 | 178 | }); | |
| 196 | 179 | ||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -182,24 +182,7 @@ TEST(function test_lookupservice_ip_ipv6(done) { | |||
| 182 | 182 | if (err) throw err; | |
| 183 | 183 | assert.equal(typeof host, 'string'); | |
| 184 | 184 | 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)); | ||
| 203 | 186 | done(); | |
| 204 | 187 | }); | |
| 205 | 188 | ||
0 commit comments