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:
|
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:
|
try { |
|
await evaluatePromise; |
|
} catch (err) { |
|
assert.strictEqual(m.error, err); |
|
return; |
|
} |
|
assert.fail('Missing expected exception'); |
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
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