Uh oh!
There was an error while loading. Please reload this page.
test_runner: make skip/todo/expectFailure use JS truthiness - #62346
test_runner: make skip/todo/expectFailure use JS truthiness#62346VaishnavIUpadyaya wants to merge 6 commits into
Conversation
nodejs-github-bot
commented
Mar 20, 2026
Review requested:
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #62346 +/- ##
==========================================
- Coverage 89.70% 89.69% -0.01%
==========================================
Files 678 691 +13 Lines 207195 213953 +6758 Branches 39730 41039 +1309 ==========================================
+ Hits 185862 191904 +6042 - Misses 13438 14146 +708 - Partials 7895 7903 +8
🚀 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.
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('…').
nodejs-github-bot
commented
Mar 26, 2026
Uh oh!
There was an error while loading. Please reload this page.
753f50c to
6014f0aCompareUh oh!
There was an error while loading. Please reload this page.
vassudanagunta
commented
Jun 7, 2026
Is this hung up on test failures? Or on the bug in code vs docs question? |

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.