Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: reduce string concatenations #12735

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
2 changes: 1 addition & 1 deletion test/addons/repl-domain-abort/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ process.on('exit', function() {

const lines = [
// This line shouldn't cause an assertion error.
'require(\'' + buildPath + '\')' +
`require('${buildPath}')` +
// Log output to double check callback ran.
'.method(function() { console.log(\'cb_ran\'); });',
];
Expand Down
20 changes: 9 additions & 11 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,8 @@ exports.isLinux = process.platform === 'linux';
exports.isOSX = process.platform === 'darwin';

exports.enoughTestMem = os.totalmem() > 0x40000000; /* 1 Gb */
exports.bufferMaxSizeMsg = new RegExp('^RangeError: "size" argument' +
' must not be larger than ' +
buffer.kMaxLength + '$');
exports.bufferMaxSizeMsg = new RegExp(
`^RangeError: "size" argument must not be larger than ${buffer.kMaxLength}$`);
const cpus = os.cpus();
exports.enoughTestCpu = Array.isArray(cpus) &&
(cpus.length > 1 || cpus[0].speed > 999);
Expand Down Expand Up @@ -118,7 +117,7 @@ exports.refreshTmpDir = function() {

if (process.env.TEST_THREAD_ID) {
exports.PORT += process.env.TEST_THREAD_ID * 100;
exports.tmpDirName += '.' + process.env.TEST_THREAD_ID;
exports.tmpDirName += `.${process.env.TEST_THREAD_ID}`;
}
exports.tmpDir = path.join(testRoot, exports.tmpDirName);

Expand Down Expand Up @@ -217,10 +216,10 @@ 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;
exports.PIPE += `.${process.env.TEST_THREAD_ID}`;
}
} else {
exports.PIPE = exports.tmpDir + '/test.sock';
exports.PIPE = `${exports.tmpDir}/test.sock`;
}

const ifaces = os.networkInterfaces();
Expand Down Expand Up @@ -256,10 +255,9 @@ exports.childShouldThrowAndAbort = function() {
exports.ddCommand = function(filename, kilobytes) {
if (exports.isWindows) {
const p = path.resolve(exports.fixturesDir, 'create-file.js');
return '"' + process.argv[0] + '" "' + p + '" "' +
filename + '" ' + (kilobytes * 1024);
return `"${process.argv[0]}" "${p}" "${filename}" ${kilobytes * 1024}`;
} else {
return 'dd if=/dev/zero of="' + filename + '" bs=1024 count=' + kilobytes;
return `dd if=/dev/zero of="${filename}" bs=1024 count=${kilobytes}`;
}
};

Expand Down Expand Up @@ -495,7 +493,7 @@ exports.canCreateSymLink = function() {
let output = '';

try {
output = execSync(whoamiPath + ' /priv', { timout: 1000 });
output = execSync(`${whoamiPath} /priv`, { timout: 1000 });
} catch (e) {
err = true;
} finally {
Expand All @@ -522,7 +520,7 @@ exports.skip = function(msg) {
function ArrayStream() {
this.run = function(data) {
data.forEach((line) => {
this.emit('data', line + '\n');
this.emit('data', `${line}\n`);
});
};
}
Expand Down
16 changes: 8 additions & 8 deletions test/debugger/helper-debugger-repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ let quit;

function startDebugger(scriptToDebug) {
scriptToDebug = process.env.NODE_DEBUGGER_TEST_SCRIPT ||
common.fixturesDir + '/' + scriptToDebug;
`${common.fixturesDir}/${scriptToDebug}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be path.join

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are many such things in the tests. Should we address them in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Separate PR is likely safer.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think a path.join is important here, node modules always use / (so it's not an OS compatibility concern). The main advantage of path.join is that it abstracts joining complex paths, for concatenating two strings / should be fine.

That said, if this view is contested - we can just leave it as is in the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO if we're touching this, so might as well "fix" it. There are multiple path concatenations done as strings, I feel they should all be changed, as path.join is semantically different, even if string concat works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be ~300 re-editions in the PR...

Copy link
Contributor

@refack refack Apr 29, 2017

Choose a reason for hiding this comment

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

Your call @vsemozhetbyt if you have the energy do it. I'll review it.
If you say not this PR I'll retract those comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am in doubt if there is a minimum consensus for this. If there are 2-3 +1 and no strong -1, I can try to refactor with path.join().

Copy link
Member

Choose a reason for hiding this comment

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

Like James and Benjamin, I’m fine with leaving this as-is. Leaving path.join() refactors for a later PR sounds good to me. (If there’s consensus they should happen.)

Copy link
Member

Choose a reason for hiding this comment

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

Yep, seems like a job for a separate PR.


child = spawn(process.execPath, ['debug', '--port=' + port, scriptToDebug]);
child = spawn(process.execPath, ['debug', `--port=${port}`, scriptToDebug]);

console.error('./node', 'debug', '--port=' + port, scriptToDebug);
console.error('./node', 'debug', `--port=${port}`, scriptToDebug);

child.stdout.setEncoding('utf-8');
child.stdout.on('data', function(data) {
Expand All @@ -53,10 +53,10 @@ function startDebugger(scriptToDebug) {
child.on('line', function(line) {
line = line.replace(/^(debug> *)+/, '');
console.log(line);
assert.ok(expected.length > 0, 'Got unexpected line: ' + line);
assert.ok(expected.length > 0, `Got unexpected line: ${line}`);

const expectedLine = expected[0].lines.shift();
assert.ok(line.match(expectedLine) !== null, line + ' != ' + expectedLine);
assert.ok(line.match(expectedLine) !== null, `${line} != ${expectedLine}`);

if (expected[0].lines.length === 0) {
const callback = expected[0].callback;
Expand All @@ -83,7 +83,7 @@ function startDebugger(scriptToDebug) {
console.error('dying badly buffer=%j', buffer);
let err = 'Timeout';
if (expected.length > 0 && expected[0].lines) {
err = err + '. Expected: ' + expected[0].lines.shift();
err = `${err}. Expected: ${expected[0].lines.shift()}`;
}

child.on('close', function() {
Expand Down Expand Up @@ -112,8 +112,8 @@ function startDebugger(scriptToDebug) {
function addTest(input, output) {
function next() {
if (expected.length > 0) {
console.log('debug> ' + expected[0].input);
child.stdin.write(expected[0].input + '\n');
console.log(`debug> ${expected[0].input}`);
child.stdin.write(`${expected[0].input}\n`);

if (!expected[0].lines) {
const callback = expected[0].callback;
Expand Down
2 changes: 1 addition & 1 deletion test/debugger/test-debugger-repl-utf8.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

'use strict';
const common = require('../common');
const script = common.fixturesDir + '/breakpoints_utf8.js';
const script = `${common.fixturesDir}/breakpoints_utf8.js`;
process.env.NODE_DEBUGGER_TEST_SCRIPT = script;

require('./test-debugger-repl.js');
2 changes: 1 addition & 1 deletion test/gc/test-http-client-connaborted.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ let done = 0;
let count = 0;
let countGC = 0;

console.log('We should do ' + todo + ' requests');
console.log(`We should do ${todo} requests`);

const server = http.createServer(serverHandler);
server.listen(0, getall);
Expand Down
2 changes: 1 addition & 1 deletion test/gc/test-http-client-onerror.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ let done = 0;
let count = 0;
let countGC = 0;

console.log('We should do ' + todo + ' requests');
console.log(`We should do ${todo} requests`);

const server = http.createServer(serverHandler);
server.listen(0, runTest);
Expand Down
2 changes: 1 addition & 1 deletion test/gc/test-http-client-timeout.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ let done = 0;
let count = 0;
let countGC = 0;

console.log('We should do ' + todo + ' requests');
console.log(`We should do ${todo} requests`);

const server = http.createServer(serverHandler);
server.listen(0, getall);
Expand Down
2 changes: 1 addition & 1 deletion test/gc/test-http-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ let done = 0;
let count = 0;
let countGC = 0;

console.log('We should do ' + todo + ' requests');
console.log(`We should do ${todo} requests`);

const server = http.createServer(serverHandler);
server.listen(0, getall);
Expand Down
2 changes: 1 addition & 1 deletion test/gc/test-net-timeout.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ let done = 0;
let count = 0;
let countGC = 0;

console.log('We should do ' + todo + ' requests');
console.log(`We should do ${todo} requests`);

const server = net.createServer(serverHandler);
server.listen(0, getall);
Expand Down
22 changes: 10 additions & 12 deletions test/inspector/inspector-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,15 @@ TestSession.prototype.processMessage_ = function(message) {
assert.strictEqual(id, this.expectedId_);
this.expectedId_++;
if (this.responseCheckers_[id]) {
assert(message['result'], JSON.stringify(message) + ' (response to ' +
JSON.stringify(this.messages_[id]) + ')');
const messageJSON = JSON.stringify(message);
const idJSON = JSON.stringify(this.messages_[id]);
assert(message['result'], `${messageJSON} (response to ${idJSON})`);
this.responseCheckers_[id](message['result']);
delete this.responseCheckers_[id];
}
assert(!message['error'], JSON.stringify(message) + ' (replying to ' +
JSON.stringify(this.messages_[id]) + ')');
const messageJSON = JSON.stringify(message);
const idJSON = JSON.stringify(this.messages_[id]);
assert(!message['error'], `${messageJSON} (replying to ${idJSON})`);
delete this.messages_[id];
if (id === this.lastId_) {
this.lastMessageResponseCallback_ && this.lastMessageResponseCallback_();
Expand Down Expand Up @@ -213,12 +215,8 @@ TestSession.prototype.sendInspectorCommands = function(commands) {
};
this.sendAll_(commands, () => {
timeoutId = setTimeout(() => {
let s = '';
for (const id in this.messages_) {
s += id + ', ';
}
assert.fail('Messages without response: ' +
s.substring(0, s.length - 2));
assert.fail(`Messages without response: ${
Object.keys(this.messages_).join(', ')}`);
}, TIMEOUT);
});
});
Expand All @@ -241,7 +239,7 @@ TestSession.prototype.expectMessages = function(expects) {
if (!(expects instanceof Array)) expects = [ expects ];

const callback = this.createCallbackWithTimeout_(
'Matching response was not received:\n' + expects[0]);
`Matching response was not received:\n${expects[0]}`);
this.messagefilter_ = (message) => {
if (expects[0](message))
expects.shift();
Expand All @@ -256,7 +254,7 @@ TestSession.prototype.expectMessages = function(expects) {
TestSession.prototype.expectStderrOutput = function(regexp) {
this.harness_.addStderrFilter(
regexp,
this.createCallbackWithTimeout_('Timed out waiting for ' + regexp));
this.createCallbackWithTimeout_(`Timed out waiting for ${regexp}`));
return this;
};

Expand Down
6 changes: 3 additions & 3 deletions test/inspector/test-inspector.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ function checkVersion(err, response) {
assert.ifError(err);
assert.ok(response);
const expected = {
'Browser': 'node.js/' + process.version,
'Browser': `node.js/${process.version}`,
'Protocol-Version': '1.1',
};
assert.strictEqual(JSON.stringify(response),
Expand All @@ -36,7 +36,7 @@ function expectMainScriptSource(result) {
const expected = helper.mainScriptSource();
const source = result['scriptSource'];
assert(source && (source.includes(expected)),
'Script source is wrong: ' + source);
`Script source is wrong: ${source}`);
}

function setupExpectBreakOnLine(line, url, session, scopeIdCallback) {
Expand Down Expand Up @@ -187,7 +187,7 @@ function testI18NCharacters(session) {
{
'method': 'Debugger.evaluateOnCallFrame', 'params': {
'callFrameId': '{"ordinal":0,"injectedScriptId":1}',
'expression': 'console.log("' + chars + '")',
'expression': `console.log("${chars}")`,
'objectGroup': 'console',
'includeCommandLineAPI': true,
'silent': false,
Expand Down
2 changes: 1 addition & 1 deletion test/internet/test-dns-cares-domains.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ methods.forEach(function(method) {
const d = domain.create();
d.run(function() {
dns[method]('google.com', function() {
assert.strictEqual(process.domain, d, method + ' retains domain');
assert.strictEqual(process.domain, d, `${method} retains domain`);
});
});
});
2 changes: 1 addition & 1 deletion test/internet/test-dns-ipv6.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ TEST(function test_lookup_all_ipv6(done) {

ips.forEach((ip) => {
assert.ok(isIPv6(ip.address),
'Invalid IPv6: ' + ip.address.toString());
`Invalid IPv6: ${ip.address.toString()}`);
assert.strictEqual(ip.family, 6);
});

Expand Down
2 changes: 1 addition & 1 deletion test/internet/test-dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,7 @@ req.oncomplete = function(err, domains) {
};

process.on('exit', function() {
console.log(completed + ' tests completed');
console.log(`${completed} tests completed`);
assert.strictEqual(running, false);
assert.strictEqual(expected, completed);
assert.ok(getaddrinfoCallbackCalled);
Expand Down
2 changes: 1 addition & 1 deletion test/internet/test-tls-add-ca-cert.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const fs = require('fs');
const tls = require('tls');

function filenamePEM(n) {
return require('path').join(common.fixturesDir, 'keys', n + '.pem');
return require('path').join(common.fixturesDir, 'keys', `${n}.pem`);
}

function loadPEM(n) {
Expand Down
2 changes: 1 addition & 1 deletion test/known_issues/test-cwd-enoent-file.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ if (process.argv[2] === 'child') {
// Do nothing.
} else {
common.refreshTmpDir();
const dir = fs.mkdtempSync(common.tmpDir + '/');
const dir = fs.mkdtempSync(`${common.tmpDir}/`);
process.chdir(dir);
fs.rmdirSync(dir);
assert.throws(process.cwd,
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ assert.doesNotThrow(makeBlock(a.deepEqual, a1, a2));

// having an identical prototype property
const nbRoot = {
toString: function() { return this.first + ' ' + this.last; }
toString: function() { return `${this.first} ${this.last}`; }
};

function nameBuilder(first, last) {
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-async-wrap-check-providers.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ process.on('SIGINT', () => process.exit());
// Run from closed net server above.
function checkTLS() {
const options = {
key: fs.readFileSync(common.fixturesDir + '/keys/ec-key.pem'),
cert: fs.readFileSync(common.fixturesDir + '/keys/ec-cert.pem')
key: fs.readFileSync(`${common.fixturesDir}/keys/ec-key.pem`),
cert: fs.readFileSync(`${common.fixturesDir}/keys/ec-cert.pem`)
};
const server = tls.createServer(options, common.noop)
.listen(0, function() {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-buffer-badhex.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,6 @@ const Buffer = require('buffer').Buffer;
const hex = buf.toString('hex');
assert.deepStrictEqual(Buffer.from(hex, 'hex'), buf);

const badHex = hex.slice(0, 256) + 'xx' + hex.slice(256, 510);
const badHex = `${hex.slice(0, 256)}xx${hex.slice(256, 510)}`;
assert.deepStrictEqual(Buffer.from(badHex, 'hex'), buf.slice(0, 128));
}
2 changes: 1 addition & 1 deletion test/parallel/test-buffer-includes.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ const longBufferString = Buffer.from(longString);
let pattern = 'ABACABADABACABA';
for (let i = 0; i < longBufferString.length - pattern.length; i += 7) {
const includes = longBufferString.includes(pattern, i);
assert(includes, 'Long ABACABA...-string at index ' + i);
assert(includes, `Long ABACABA...-string at index ${i}`);
}
assert(longBufferString.includes('AJABACA'), 'Long AJABACA, First J');
assert(longBufferString.includes('AJABACA', 511), 'Long AJABACA, Second J');
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-buffer-indexof.js
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ let pattern = 'ABACABADABACABA';
for (let i = 0; i < longBufferString.length - pattern.length; i += 7) {
const index = longBufferString.indexOf(pattern, i);
assert.strictEqual((i + 15) & ~0xf, index,
'Long ABACABA...-string at index ' + i);
`Long ABACABA...-string at index ${i}`);
}
assert.strictEqual(510, longBufferString.indexOf('AJABACA'),
'Long AJABACA, First J');
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-child-process-buffering.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ function pwd(callback) {

child.stdout.setEncoding('utf8');
child.stdout.on('data', function(s) {
console.log('stdout: ' + JSON.stringify(s));
console.log(`stdout: ${JSON.stringify(s)}`);
output += s;
});

child.on('exit', common.mustCall(function(c) {
console.log('exit: ' + c);
console.log(`exit: ${c}`);
assert.strictEqual(0, c);
}));

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-child-process-default-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ let response = '';
child.stdout.setEncoding('utf8');

child.stdout.on('data', function(chunk) {
console.log('stdout: ' + chunk);
console.log(`stdout: ${chunk}`);
response += chunk;
});

Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-child-process-double-pipe.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ if (common.isWindows) {

// pipe echo | grep
echo.stdout.on('data', function(data) {
console.error('grep stdin write ' + data.length);
console.error(`grep stdin write ${data.length}`);
if (!grep.stdin.write(data)) {
echo.stdout.pause();
}
Expand Down Expand Up @@ -86,7 +86,7 @@ sed.on('exit', function() {

// pipe grep | sed
grep.stdout.on('data', function(data) {
console.error('grep stdout ' + data.length);
console.error(`grep stdout ${data.length}`);
if (!sed.stdin.write(data)) {
grep.stdout.pause();
}
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-child-process-env.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ let response = '';
child.stdout.setEncoding('utf8');

child.stdout.on('data', function(chunk) {
console.log('stdout: ' + chunk);
console.log(`stdout: ${chunk}`);
response += chunk;
});

Expand Down
Loading