Node: tests: review and fix ineffective error tests

[ChALkeR]

See https://github.com/nodejs/node/pull/26078#pullrequestreview-209542148 for context.

After that, I looked up if we already have similar ineffective tests in our codebase, and I think that I found at least one.

E.g. using grep -r ' catch' test/ -A 2 | grep strictEqual, this one popped up half through the list:

https://github.com/nodejs/node/blob/453ed053432e0c1ae34457e5c364b7091af2615e/test/parallel/test-stream-readable-async-iterators.js#L355-L361

The code above doesn't test that the error is thrown (as it likely should), instead it tests that _another_ error _doesn't_ get thrown.

Note that there are a lot of false positives in the said grep (actually most matches are false positives), because e.g. this is perfectly fine:
https://github.com/nodejs/node/blob/453ed053432e0c1ae34457e5c364b7091af2615e/test/parallel/test-vm-module-errors.js#L266-L272

[ChALkeR]

E.g. these are speculative (as the fact that they indeed throw is probably already tested elsewhere), but I would still like these to include a safeguard check:
https://github.com/nodejs/node/blob/453ed053432e0c1ae34457e5c364b7091af2615e/test/pseudo-tty/test-assert-colors.js#L5-L21
https://github.com/nodejs/node/blob/453ed053432e0c1ae34457e5c364b7091af2615e/test/pseudo-tty/test-assert-no-color.js#L7-L19

[ChALkeR]

https://github.com/nodejs/node/blob/453ed053432e0c1ae34457e5c364b7091af2615e/test/sequential/test-module-loading.js#L192-L212

Speculative:
https://github.com/nodejs/node/blob/453ed053432e0c1ae34457e5c364b7091af2615e/test/es-module/test-esm-error-cache.js#L21-L25

[ChALkeR]

I will prepare a gist of potential entries to review.

[shisama]

@JungMinu Are you working for this? I am going to work for this. If you already do, I want to avoid conflict.

[JungMinu]

@shisama go for it

[shisama]

using grep -r ' catch' test/ -A 2 | grep assert | awk '{print $1}' | uniq, I got the file list.

• [ ] test/parallel/test-vm-module-errors.js
• [ ] test/parallel/test-vm-context.js #L59
• [ ] test/parallel/test-util-inspect.js #L548
• [ ] test/parallel/test-fs-promises.js #L306
• [ ] test/parallel/test-listen-fd-detached-inherit.js #L72
• [ ] test/parallel/test-fs-copyfile.js #69
• [ ] test/parallel/test-vm-codegen.js #L16
• [ ] test/parallel/test-readline-interface.js #512
• [ ] test/parallel/test-worker-message-port-move.js #L48
• [ ] test/parallel/test-console.js #279
• [ ] test/parallel/test-stream-readable-async-iterators.js #L358
• [ ] test/parallel/test-repl-require.js #L37
• [ ] test/parallel/test-assert.js #L302 #L312
• [ ] test/parallel/test-vm-module-dynamic-import.js #L18 #L80
• [ ] test/parallel/test-net-pipe-connect-errors.js #L50
• [ ] test/parallel/test-assert-if-error.js #28 #83
• [ ] test/parallel/test-fs-sync-fd-leak.js #67
• [ ] test/parallel/test-fs-open.js #L33
• [ ] test/parallel/test-fs-mkdir.js #L128 #L169
• [ ] test/parallel/test-listen-fd-detached.js #L72
• [ ] test/addons/non-node-context/test-make-buffer.js #L15
• [ ] test/sequential/test-child-process-emfile.js #L52
• [ ] test/sequential/test-module-loading.js #L205 #L218
• [ ] test/sequential/test-child-process-execsync.js #L49
• [ ] test/es-module/test-esm-error-cache.js #L23
• [ ] test/node-api/test_async/test-uncaught.js #L9
• [ ] test/fixtures/es-module-loaders/not-found-assert-loader.mjs #L14

Strikethrough indicates the file I don't think it includes the problem.

[rawad663]

E.g. these are speculative (as the fact that they indeed throw is probably already tested elsewhere), but I would still like these to include a safeguard check:
node/test/pseudo-tty/test-assert-colors.js

Lines 5 to 21 in 453ed05

try {
// Activate colors even if the tty does not support colors.
process.env.COLORTERM = '1';
assert.deepStrictEqual([1, 2, 2, 2], [2, 2, 2, 2]);
} catch (err) {
const expected = 'Expected values to be strictly deep-equal:n' +
'u001b[32m+ actualu001b[39m u001b[31m- expectedu001b[39m' +
' u001b[34m...u001b[39m Lines skippednn' +
' [n' +
'u001b[32m+u001b[39m 1,n' +
'u001b[31m-u001b[39m 2,n' +
' 2,n' +
'u001b[34m...u001b[39mn' +
' 2n' +
' ]';
assert.strictEqual(err.message, expected);
}

node/test/pseudo-tty/test-assert-no-color.js

Lines 7 to 19 in 453ed05

try {
assert.deepStrictEqual({}, { foo: 'bar' });
} catch (error) {
const expected =
'Expected values to be strictly deep-equal:n' +
'+ actual - expectedn' +
'n' +
'+ {}n' +
'- {n' +
'- foo: 'bar'n' +
'- }';
assert.strictEqual(error.message, expected);
}

Hey @ChALkeR I can work on adding a few safeguard checks for these tests!