test_runner: make skip/todo/expectFailure use JS truthiness#62346
test_runner: make skip/todo/expectFailure use JS truthiness#62346VaishnavIUpadyaya wants to merge 4 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
JakobJingleheimer
added
request-ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62346 +/- ##
==========================================
+ Coverage 89.65% 89.68% +0.02%
==========================================
Files 676 676
Lines 206555 206694 +139
Branches 39547 39577 +30
==========================================
+ Hits 185195 185377 +182
+ Misses 13495 13446 -49
- Partials 7865 7871 +6
🚀 New features to boost your workflow:
|
| const common = require('../common'); | ||
| const { test } = require('node:test'); | ||
|
|
||
| test('skip option empty string should not skip', { skip: '' }, common.mustCall()); |
There was a problem hiding this comment.
I would fix the docs, not the implementation - it seems for correct
There was a problem hiding this comment.
There was a problem hiding this comment.
I can see why both behaviors would make sense.
Do we know what other test runners do?
There was a problem hiding this comment.
Thanks for the feedback!
I based this change on the docs describing these options as "truthy", so I assumed an empty string ('') should be treated as false.
That said, I see your point — changing the implementation could introduce unintended breaking behavior. Updating the docs to reflect the current behavior sounds reasonable.
I'm happy to update this PR to focus on the docs instead. Let me know what you prefer!
There was a problem hiding this comment.
I think there's a decent chance an empty string is an accident, but the user could be relying on something that outputs a potentially empty string.
I think in general, truthy/falsy makes sense; since that includes an empty string, yeah.
I'm not sure what others do. I'll take a look this evening.
There was a problem hiding this comment.
It looks like jest and mocha do not have this—they expose them only as methods like test.only('…') and test.skip('…').
There was a problem hiding this comment.
tape has this, and uses truthiness.
MoLow
removed
the
commit-queue-squash
github-actions
bot
removed
the
request-ci
| @@ -679,7 +680,7 @@ class Test extends AsyncResource { | |||
| this.expectedAssertions = plan; | |||
| this.cancelled = false; | |||
| this.expectFailure = parseExpectFailure(expectFailure) || this.parent?.expectFailure; | |||
| this.skipped = skip !== undefined && skip !== false; | |||
| this.skipped = Boolean(skip); | |||
There was a problem hiding this comment.
| this.skipped = Boolean(skip); | |
| this.skipped = !!skip; |
nit
7 participants
Fixes #61815
Updates skip, todo, and expectFailure handling in the test runner
to use JavaScript truthiness instead of explicit undefined/false checks.
This ensures:
Adds a test to verify that skip: '' does not skip the test.