test: Fix `common.PIPE` to be process unique by refack · Pull Request #14168 · nodejs/node · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions benchmark/http/_chunky_http_client.js
20 changes: 10 additions & 10 deletions benchmark/http/http_server_for_chunky_client.js
Original file line number Diff line number Diff line change
@@ -1,24 +1,22 @@
'use strict';

var assert = require('assert');
var path = require('path');
var http = require('http');
var fs = require('fs');
var fork = require('child_process').fork;
var { fork } = require('child_process');
var common = require('../common.js');
var test = require('../../test/common');
var pep = `${path.dirname(process.argv[1])}/_chunky_http_client.js`;
var PIPE = test.PIPE;
const { PIPE, tmpDir } = require('../../test/common');
process.env.PIPE_NAME = PIPE;

try {
fs.accessSync(test.tmpDir, fs.F_OK);
fs.accessSync(tmpDir, fs.F_OK);
} catch (e) {
fs.mkdirSync(test.tmpDir);
fs.mkdirSync(tmpDir);
}

var server;
try {
fs.unlinkSync(PIPE);
fs.unlinkSync(process.env.PIPE_NAME);
} catch (e) { /* ignore */ }

server = http.createServer(function(req, res) {
Expand All @@ -33,10 +31,12 @@ server = http.createServer(function(req, res) {
server.on('error', function(err) {
throw new Error(`server error: ${err}`);
});

server.listen(PIPE);

var child = fork(pep, process.argv.slice(2));
const child = fork(
`${__dirname}/_chunky_http_client.js`,
process.argv.slice(2)
);
child.on('message', common.sendResult);
child.on('close', function(code) {
server.close();
Expand Down
27 changes: 13 additions & 14 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ Object.defineProperty(exports, 'opensslCli', {get: function() {

Object.defineProperty(exports, 'hasCrypto', {
get: function() {
return process.versions.openssl ? true : false;
return Boolean(process.versions.openssl);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge fan of !! outside of minimal code competitions.
Willing to change if anyone has a strong opinion.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion from me. I think it used to be preferred in hot paths for performance. No idea if that's still the case. Regardless, it's totally sensible to value readable code over performant code in test/common/index.js.

}
});

Expand All @@ -253,22 +253,21 @@ Object.defineProperty(exports, 'hasFipsCrypto', {
}
});

if (exports.isWindows) {
exports.PIPE = '\\\\.\\pipe\\libuv-test';
if (process.env.TEST_THREAD_ID) {
exports.PIPE += `.${process.env.TEST_THREAD_ID}`;
}
} else {
exports.PIPE = `${exports.tmpDir}/test.sock`;
{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is block scoping really needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the const are only used locally and there's a single exported side-effect (exports.PIPE = ...) IMHO it's cleaner.
Similar to the function setupFoo(); setupFoo(); approach in node_bootstap.js
Maybe even V8 will inline the consts ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, easier to see that the consts aren't used except in exports.PIPE.

const pipePrefix = exports.isWindows ? '\\\\.\\pipe\\' : `${exports.tmpDir}/`;
const pipeName = `node-test.${process.pid}.sock`;
exports.PIPE = pipePrefix + pipeName;
}

const ifaces = os.networkInterfaces();
const re = /lo/;
exports.hasIPv6 = Object.keys(ifaces).some(function(name) {
return re.test(name) && ifaces[name].some(function(info) {
return info.family === 'IPv6';
{
const iFaces = os.networkInterfaces();
const re = /lo/;
exports.hasIPv6 = Object.keys(iFaces).some(function(name) {
return re.test(name) && iFaces[name].some(function(info) {
return info.family === 'IPv6';
});
});
});
}

/*
* Check that when running a test with
Expand Down
39 changes: 0 additions & 39 deletions test/fixtures/listen-on-socket-and-exit.js

This file was deleted.

42 changes: 21 additions & 21 deletions test/parallel/test-cluster-eaccess.js