-
-
Notifications
You must be signed in to change notification settings - Fork 34.7k
Closed
Labels
good first issueIssues that are suitable for first-time contributors.Issues that are suitable for first-time contributors.help wantedIssues that need assistance from volunteers or PRs that need help to proceed.Issues that need assistance from volunteers or PRs that need help to proceed.testIssues and PRs related to the tests.Issues and PRs related to the tests.
Description
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'); |
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
good first issueIssues that are suitable for first-time contributors.Issues that are suitable for first-time contributors.help wantedIssues that need assistance from volunteers or PRs that need help to proceed.Issues that need assistance from volunteers or PRs that need help to proceed.testIssues and PRs related to the tests.Issues and PRs related to the tests.