tests: review and fix ineffective error tests

[ChALkeR]

See #26078 (review) 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:

node/test/parallel/test-stream-readable-async-iterators.js

Lines 355 to 361 in 453ed05

	readable.destroy(err);
	try {
	await readable[Symbol.asyncIterator]().next();
	} catch (e) {
	assert.strictEqual(e, err);
	}
	}

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:

node/test/parallel/test-vm-module-errors.js

Lines 266 to 272 in 453ed05

	try {
	await evaluatePromise;
	} catch (err) {
	assert.strictEqual(m.error, err);
	return;
	}
	assert.fail('Missing expected exception');

[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:

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+ actual\u001b[39m \u001b[31m- expected\u001b[39m' +
	' \u001b[34m...\u001b[39m Lines skipped\n\n' +
	' [\n' +
	'\u001b[32m+\u001b[39m 1,\n' +
	'\u001b[31m-\u001b[39m 2,\n' +
	' 2,\n' +
	'\u001b[34m...\u001b[39m\n' +
	' 2\n' +
	' ]';
	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 - expected\n' +
	'\n' +
	'+ {}\n' +
	'- {\n' +
	'- foo: \'bar\'\n' +
	'- }';
	assert.strictEqual(error.message, expected);
	}

[ChALkeR]

node/test/sequential/test-module-loading.js

Lines 192 to 212 in 453ed05

	try {
	require(`${loadOrder}file3`);
	} catch (e) {
	// Not a real .node module, but we know we require'd the right thing.
	if (common.isOpenBSD) // OpenBSD errors with non-ELF object error
	assert.ok(/File not an ELF object/.test(e.message.replace(backslash, '/')));
	else
	assert.ok(/file3\.node/.test(e.message.replace(backslash, '/')));
	}
	assert.strictEqual(require(`${loadOrder}file4`).file4, 'file4.reg');
	assert.strictEqual(require(`${loadOrder}file5`).file5, 'file5.reg2');
	assert.strictEqual(require(`${loadOrder}file6`).file6, 'file6/index.js');
	try {
	require(`${loadOrder}file7`);
	} catch (e) {
	if (common.isOpenBSD)
	assert.ok(/File not an ELF object/.test(e.message.replace(backslash, '/')));
	else
	assert.ok(/file7\/index\.node/.test(e.message.replace(backslash, '/')));
	}

Speculative:

node/test/es-module/test-esm-error-cache.js

Lines 21 to 25 in 453ed05

	try {
	await import(file);
	} catch (e) {
	assert.strictEqual(error, e);
	}

[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+ actual\u001b[39m \u001b[31m- expected\u001b[39m' +
' \u001b[34m...\u001b[39m Lines skipped\n\n' +
' [\n' +
'\u001b[32m+\u001b[39m 1,\n' +
'\u001b[31m-\u001b[39m 2,\n' +
' 2,\n' +
'\u001b[34m...\u001b[39m\n' +
' 2\n' +
' ]';
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 - expected\n' +
'\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!