Skip to content

Commit

Permalink
test: avoid deep comparisons with literals
Browse files Browse the repository at this point in the history
Comparing any value to any non-RegExp literal or undefined using
strictEqual (or notStrictEqual) passes if and only if deepStrictEqual
(or notDeepStrictEqual, respectively) passes.

Unnecessarily using deep comparisons adds confusion.

This patch adds an ESLint rule that forbids the use of deepStrictEqual
and notDeepStrictEqual when the expected value (i.e., the second
argument) is a non-RegExp literal or undefined.

For reference, an ESTree literal is defined as follows.

    extend interface Literal <: Expression {
        type: "Literal";
        value: string | boolean | null | number | RegExp | bigint;
    }

The value `undefined` is an `Identifier` with `name: 'undefined'`.

PR-URL: nodejs#40634
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Voltrex <mohammadkeyvanzade94@gmail.com>
  • Loading branch information
tniessen committed Feb 16, 2022
1 parent 74c0464 commit d23b259
Show file tree
Hide file tree
Showing 42 changed files with 111 additions and 107 deletions.
4 changes: 4 additions & 0 deletions test/.eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ rules:
no-restricted-syntax:
# Config copied from .eslintrc.js
- error
- selector: "CallExpression:matches([callee.name='deepStrictEqual'], [callee.property.name='deepStrictEqual']):matches([arguments.1.type='Literal']:not([arguments.1.regex]), [arguments.1.type='Identifier'][arguments.1.name='undefined'])"
message: "Use strictEqual instead of deepStrictEqual for literals or undefined."
- selector: "CallExpression:matches([callee.name='notDeepStrictEqual'], [callee.property.name='notDeepStrictEqual']):matches([arguments.1.type='Literal']:not([arguments.1.regex]), [arguments.1.type='Identifier'][arguments.1.name='undefined'])"
message: "Use notStrictEqual instead of notDeepStrictEqual for literals or undefined."
- selector: "CallExpression:matches([callee.name='deepStrictEqual'], [callee.property.name='deepStrictEqual'])[arguments.2.type='Literal']"
message: "Do not use a literal for the third argument of assert.deepStrictEqual()"
- selector: "CallExpression:matches([callee.name='doesNotThrow'], [callee.property.name='doesNotThrow'])"
Expand Down
18 changes: 9 additions & 9 deletions test/es-module/test-esm-data-urls.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ function createBase64URL(mime, body) {
const plainESMURL = createURL('text/javascript', body);
const ns = await import(plainESMURL);
assert.deepStrictEqual(Object.keys(ns), ['default']);
assert.deepStrictEqual(ns.default.a, 'aaa');
assert.strictEqual(ns.default.a, 'aaa');
const importerOfURL = createURL(
'text/javascript',
`export {default as default} from ${JSON.stringify(plainESMURL)}`
Expand All @@ -35,41 +35,41 @@ function createBase64URL(mime, body) {
const plainESMURL = createURL('text/javascript', body);
const ns = await import(plainESMURL);
assert.deepStrictEqual(Object.keys(ns), ['default']);
assert.deepStrictEqual(ns.default, plainESMURL);
assert.strictEqual(ns.default, plainESMURL);
}
{
const body = 'export default import.meta.url;';
const plainESMURL = createURL('text/javascript;charset=UTF-8', body);
const ns = await import(plainESMURL);
assert.deepStrictEqual(Object.keys(ns), ['default']);
assert.deepStrictEqual(ns.default, plainESMURL);
assert.strictEqual(ns.default, plainESMURL);
}
{
const body = 'export default import.meta.url;';
const plainESMURL = createURL('text/javascript;charset="UTF-8"', body);
const ns = await import(plainESMURL);
assert.deepStrictEqual(Object.keys(ns), ['default']);
assert.deepStrictEqual(ns.default, plainESMURL);
assert.strictEqual(ns.default, plainESMURL);
}
{
const body = 'export default import.meta.url;';
const plainESMURL = createURL('text/javascript;;a=a;b=b;;', body);
const ns = await import(plainESMURL);
assert.deepStrictEqual(Object.keys(ns), ['default']);
assert.deepStrictEqual(ns.default, plainESMURL);
assert.strictEqual(ns.default, plainESMURL);
}
{
const ns = await import('data:application/json;foo="test,"this"',
{ assert: { type: 'json' } });
assert.deepStrictEqual(Object.keys(ns), ['default']);
assert.deepStrictEqual(ns.default, 'this');
assert.strictEqual(ns.default, 'this');
}
{
const ns = await import(`data:application/json;foo=${
encodeURIComponent('test,')
},0`, { assert: { type: 'json' } });
assert.deepStrictEqual(Object.keys(ns), ['default']);
assert.deepStrictEqual(ns.default, 0);
assert.strictEqual(ns.default, 0);
}
{
await assert.rejects(async () =>
Expand All @@ -84,14 +84,14 @@ function createBase64URL(mime, body) {
const plainESMURL = createURL('application/json', body);
const ns = await import(plainESMURL, { assert: { type: 'json' } });
assert.deepStrictEqual(Object.keys(ns), ['default']);
assert.deepStrictEqual(ns.default.x, 1);
assert.strictEqual(ns.default.x, 1);
}
{
const body = '{"default": 2}';
const plainESMURL = createURL('application/json', body);
const ns = await import(plainESMURL, { assert: { type: 'json' } });
assert.deepStrictEqual(Object.keys(ns), ['default']);
assert.deepStrictEqual(ns.default.default, 2);
assert.strictEqual(ns.default.default, 2);
}
{
const body = 'null';
Expand Down
8 changes: 4 additions & 4 deletions test/js-native-api/test_conversions/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,10 @@ assert.deepStrictEqual(new String(''), test.toObject(''));
assert.deepStrictEqual(new Number(0), test.toObject(0));
assert.deepStrictEqual(new Number(Number.NaN), test.toObject(Number.NaN));
assert.deepStrictEqual(new Object(testSym), test.toObject(testSym));
assert.notDeepStrictEqual(test.toObject(false), false);
assert.notDeepStrictEqual(test.toObject(true), true);
assert.notDeepStrictEqual(test.toObject(''), '');
assert.notDeepStrictEqual(test.toObject(0), 0);
assert.notStrictEqual(test.toObject(false), false);
assert.notStrictEqual(test.toObject(true), true);
assert.notStrictEqual(test.toObject(''), '');
assert.notStrictEqual(test.toObject(0), 0);
assert.ok(!Number.isNaN(test.toObject(Number.NaN)));

assert.strictEqual(test.toString(''), '');
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-assert-deep.js
Original file line number Diff line number Diff line change
Expand Up @@ -858,11 +858,13 @@ assert.throws(
}

assert.throws(
// eslint-disable-next-line no-restricted-syntax
() => assert.deepStrictEqual(4, '4'),
{ message: `${defaultMsgStart}\n4 !== '4'\n` }
);

assert.throws(
// eslint-disable-next-line no-restricted-syntax
() => assert.deepStrictEqual(true, 1),
{ message: `${defaultMsgStart}\ntrue !== 1\n` }
);
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -1243,6 +1243,7 @@ assert.throws(
{
let threw = false;
try {
// eslint-disable-next-line no-restricted-syntax
assert.deepStrictEqual(Array(100).fill(1), 'foobar');
} catch (err) {
threw = true;
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-crypto-hmac.js
Original file line number Diff line number Diff line change
Expand Up @@ -422,8 +422,8 @@ assert.strictEqual(
}
{
const h = crypto.createHmac('sha1', 'key').update('data');
assert.deepStrictEqual(h.digest('latin1'), expected);
assert.deepStrictEqual(h.digest('latin1'), '');
assert.strictEqual(h.digest('latin1'), expected);
assert.strictEqual(h.digest('latin1'), '');
}
}

Expand All @@ -440,8 +440,8 @@ assert.strictEqual(
}
{
const h = crypto.createHmac('sha1', 'key');
assert.deepStrictEqual(h.digest('latin1'), expected);
assert.deepStrictEqual(h.digest('latin1'), '');
assert.strictEqual(h.digest('latin1'), expected);
assert.strictEqual(h.digest('latin1'), '');
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-dns-lookup.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ dns.lookup('127.0.0.1', {
family: 4,
all: false
}, common.mustSucceed((result, addressType) => {
assert.deepStrictEqual(result, '127.0.0.1');
assert.strictEqual(result, '127.0.0.1');
assert.strictEqual(addressType, 4);
}));

Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -337,10 +337,10 @@ assert.throws(() => {

{
dns.resolveMx('foo.onion', function(err) {
assert.deepStrictEqual(err.code, 'ENOTFOUND');
assert.deepStrictEqual(err.syscall, 'queryMx');
assert.deepStrictEqual(err.hostname, 'foo.onion');
assert.deepStrictEqual(err.message, 'queryMx ENOTFOUND foo.onion');
assert.strictEqual(err.code, 'ENOTFOUND');
assert.strictEqual(err.syscall, 'queryMx');
assert.strictEqual(err.hostname, 'foo.onion');
assert.strictEqual(err.message, 'queryMx ENOTFOUND foo.onion');
});
}

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-error-serdes.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,5 +64,5 @@ assert.strictEqual(cycle(Function), '[Function: Function]');
}

serializeError(new DynamicError());
assert.deepStrictEqual(called, true);
assert.strictEqual(called, true);
}
2 changes: 1 addition & 1 deletion test/parallel/test-fs-promises-file-handle-chmod.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ async function validateFilePermission() {
const fileHandle = await open(filePath, 'w+', 0o444);
// File created with r--r--r-- 444
const statsBeforeMod = fs.statSync(filePath);
assert.deepStrictEqual(statsBeforeMod.mode & 0o444, 0o444);
assert.strictEqual(statsBeforeMod.mode & 0o444, 0o444);

let expectedAccess;
const newPermissions = 0o765;
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-fs-promises-file-handle-truncate.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ async function validateTruncate() {
const buffer = Buffer.from(text, 'utf8');
await fileHandle.write(buffer, 0, buffer.length);

assert.deepStrictEqual((await readFile(filename)).toString(), text);
assert.strictEqual((await readFile(filename)).toString(), text);

await fileHandle.truncate(5);
assert.deepStrictEqual((await readFile(filename)).toString(), 'Hello');
assert.strictEqual((await readFile(filename)).toString(), 'Hello');

await fileHandle.close();
}
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-fs-promises-readfile-with-fd.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ async function readFileTest() {
const buf = Buffer.alloc(5);
const { bytesRead } = await handle.read(buf, 0, 5, null);
assert.strictEqual(bytesRead, 5);
assert.deepStrictEqual(buf.toString(), 'Hello');
assert.strictEqual(buf.toString(), 'Hello');

/* readFile() should read from position five, instead of zero. */
assert.deepStrictEqual((await handle.readFile()).toString(), ' World');
assert.strictEqual((await handle.readFile()).toString(), ' World');

await handle.close();
}
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-fs-promises-writefile-with-fd.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ async function writeFileTest() {
await handle.writeFile('World');

/* New content should be written at position five, instead of zero. */
assert.deepStrictEqual(readFileSync(fn).toString(), 'HelloWorld');
assert.strictEqual(readFileSync(fn).toString(), 'HelloWorld');

await handle.close();
}
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-fs-promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ async function getHandle(dest) {
assert.strictEqual(ret.bytesRead, bufLen);
assert.deepStrictEqual(ret.buffer, buf);
await truncate(dest, 5);
assert.deepStrictEqual((await readFile(dest)).toString(), 'hello');
assert.strictEqual((await readFile(dest)).toString(), 'hello');
await handle.close();
}

Expand Down
10 changes: 5 additions & 5 deletions test/parallel/test-fs-readfile-fd.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,11 @@ function tempFdSync(callback) {

// Read only five bytes, so that the position moves to five.
const buf = Buffer.alloc(5);
assert.deepStrictEqual(fs.readSync(fd, buf, 0, 5), 5);
assert.deepStrictEqual(buf.toString(), 'Hello');
assert.strictEqual(fs.readSync(fd, buf, 0, 5), 5);
assert.strictEqual(buf.toString(), 'Hello');

// readFileSync() should read from position five, instead of zero.
assert.deepStrictEqual(fs.readFileSync(fd).toString(), ' World');
assert.strictEqual(fs.readFileSync(fd).toString(), ' World');

fs.closeSync(fd);
}
Expand All @@ -81,11 +81,11 @@ function tempFdSync(callback) {
// Read only five bytes, so that the position moves to five.
fs.read(fd, buf, 0, 5, null, common.mustSucceed((bytes) => {
assert.strictEqual(bytes, 5);
assert.deepStrictEqual(buf.toString(), 'Hello');
assert.strictEqual(buf.toString(), 'Hello');

fs.readFile(fd, common.mustSucceed((data) => {
// readFile() should read from position five, instead of zero.
assert.deepStrictEqual(data.toString(), ' World');
assert.strictEqual(data.toString(), ' World');

fs.closeSync(fd);
}));
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-fs-readv-promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ const allocateEmptyBuffers = (combinedLength) => {

let { bytesRead, buffers } = await handle.readv([Buffer.from('')],
null);
assert.deepStrictEqual(bytesRead, 0);
assert.strictEqual(bytesRead, 0);
assert.deepStrictEqual(buffers, [Buffer.from('')]);

({ bytesRead, buffers } = await handle.readv(bufferArr, null));
assert.deepStrictEqual(bytesRead, expectedLength);
assert.strictEqual(bytesRead, expectedLength);
assert.deepStrictEqual(buffers, bufferArr);
assert(Buffer.concat(bufferArr).equals(await fs.readFile(filename)));
handle.close();
Expand All @@ -54,11 +54,11 @@ const allocateEmptyBuffers = (combinedLength) => {
const expectedLength = exptectedBuff.length;

let { bytesRead, buffers } = await handle.readv([Buffer.from('')]);
assert.deepStrictEqual(bytesRead, 0);
assert.strictEqual(bytesRead, 0);
assert.deepStrictEqual(buffers, [Buffer.from('')]);

({ bytesRead, buffers } = await handle.readv(bufferArr));
assert.deepStrictEqual(bytesRead, expectedLength);
assert.strictEqual(bytesRead, expectedLength);
assert.deepStrictEqual(buffers, bufferArr);
assert(Buffer.concat(bufferArr).equals(await fs.readFile(filename)));
handle.close();
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-fs-readv-sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ const allocateEmptyBuffers = (combinedLength) => {
const bufferArr = allocateEmptyBuffers(exptectedBuff.length);

let read = fs.readvSync(fd, [Buffer.from('')], 0);
assert.deepStrictEqual(read, 0);
assert.strictEqual(read, 0);

read = fs.readvSync(fd, bufferArr, 0);
assert.deepStrictEqual(read, expectedLength);
assert.strictEqual(read, expectedLength);

fs.closeSync(fd);

Expand All @@ -49,10 +49,10 @@ const allocateEmptyBuffers = (combinedLength) => {
const bufferArr = allocateEmptyBuffers(exptectedBuff.length);

let read = fs.readvSync(fd, [Buffer.from('')]);
assert.deepStrictEqual(read, 0);
assert.strictEqual(read, 0);

read = fs.readvSync(fd, bufferArr);
assert.deepStrictEqual(read, expectedLength);
assert.strictEqual(read, expectedLength);

fs.closeSync(fd);

Expand Down
10 changes: 5 additions & 5 deletions test/parallel/test-fs-writefile-with-fd.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ tmpdir.refresh();
const fd = fs.openSync(filename, 'w');
try {
/* Write only five characters, so that the position moves to five. */
assert.deepStrictEqual(fs.writeSync(fd, 'Hello'), 5);
assert.deepStrictEqual(fs.readFileSync(filename).toString(), 'Hello');
assert.strictEqual(fs.writeSync(fd, 'Hello'), 5);
assert.strictEqual(fs.readFileSync(filename).toString(), 'Hello');

/* Write some more with writeFileSync(). */
fs.writeFileSync(fd, 'World');

/* New content should be written at position five, instead of zero. */
assert.deepStrictEqual(fs.readFileSync(filename).toString(), 'HelloWorld');
assert.strictEqual(fs.readFileSync(filename).toString(), 'HelloWorld');
} finally {
fs.closeSync(fd);
}
Expand All @@ -54,12 +54,12 @@ process.on('beforeExit', common.mustCall(() => {
/* Write only five characters, so that the position moves to five. */
fs.write(fd, 'Hello', common.mustSucceed((bytes) => {
assert.strictEqual(bytes, 5);
assert.deepStrictEqual(fs.readFileSync(file).toString(), 'Hello');
assert.strictEqual(fs.readFileSync(file).toString(), 'Hello');

/* Write some more with writeFile(). */
fs.writeFile(fd, 'World', common.mustSucceed(() => {
/* New content should be written at position five, instead of zero. */
assert.deepStrictEqual(fs.readFileSync(file).toString(), 'HelloWorld');
assert.strictEqual(fs.readFileSync(file).toString(), 'HelloWorld');
}));
}));
}));
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-fs-writev-promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ tmpdir.refresh();
const expectedLength = bufferArr.length * buffer.byteLength;
let { bytesWritten, buffers } = await handle.writev([Buffer.from('')],
null);
assert.deepStrictEqual(bytesWritten, 0);
assert.strictEqual(bytesWritten, 0);
assert.deepStrictEqual(buffers, [Buffer.from('')]);
({ bytesWritten, buffers } = await handle.writev(bufferArr, null));
assert.deepStrictEqual(bytesWritten, expectedLength);
Expand All @@ -39,7 +39,7 @@ tmpdir.refresh();
const bufferArr = [buffer, buffer, buffer];
const expectedLength = bufferArr.length * buffer.byteLength;
let { bytesWritten, buffers } = await handle.writev([Buffer.from('')]);
assert.deepStrictEqual(bytesWritten, 0);
assert.strictEqual(bytesWritten, 0);
assert.deepStrictEqual(buffers, [Buffer.from('')]);
({ bytesWritten, buffers } = await handle.writev(bufferArr));
assert.deepStrictEqual(bytesWritten, expectedLength);
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-fs-writev-sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ const getFileName = (i) => path.join(tmpdir.path, `writev_sync_${i}.txt`);
const expectedLength = bufferArr.length * buffer.byteLength;

let written = fs.writevSync(fd, [Buffer.from('')], null);
assert.deepStrictEqual(written, 0);
assert.strictEqual(written, 0);

written = fs.writevSync(fd, bufferArr, null);
assert.deepStrictEqual(written, expectedLength);
assert.strictEqual(written, expectedLength);

fs.closeSync(fd);

Expand All @@ -46,10 +46,10 @@ const getFileName = (i) => path.join(tmpdir.path, `writev_sync_${i}.txt`);
const expectedLength = bufferArr.length * buffer.byteLength;

let written = fs.writevSync(fd, [Buffer.from('')]);
assert.deepStrictEqual(written, 0);
assert.strictEqual(written, 0);

written = fs.writevSync(fd, bufferArr);
assert.deepStrictEqual(written, expectedLength);
assert.strictEqual(written, expectedLength);

fs.closeSync(fd);

Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-http-agent-keepalive.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ function second() {
function remoteClose() {
// Mock remote server close the socket
const req = get('/remote_close', common.mustCall((res) => {
assert.deepStrictEqual(req.reusedSocket, true);
assert.deepStrictEqual(res.statusCode, 200);
assert.strictEqual(req.reusedSocket, true);
assert.strictEqual(res.statusCode, 200);
res.on('data', checkDataAndSockets);
res.on('end', common.mustCall(() => {
assert.strictEqual(agent.sockets[name].length, 1);
Expand Down
Loading

0 comments on commit d23b259

Please sign in to comment.