Skip to content

test_runner: make skip/todo/expectFailure use JS truthiness #62346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
VaishnavIUpadyaya wants to merge 6 commits into
base: main
Choose a base branch
from
+8 −2
Open

test_runner: make skip/todo/expectFailure use JS truthiness #62346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0a28ef2
test_runner: make skip/todo/expectFailure use JS truthiness (#61815)
VaishnavIUpadyaya Mar 18, 2026
c96d866
test_runner:make skip/todo/expectFailure use JS truthiness (#61815)
VaishnavIUpadyaya Mar 20, 2026
58be01d
test: use common.mustCall() for skip empty string test
VaishnavIUpadyaya Mar 20, 2026
56ab67a
test: add newline at end of file
VaishnavIUpadyaya Mar 21, 2026
6014f0a
style: use !!skip instead of Boolean(skip)
VaishnavIUpadyaya Mar 27, 2026
814ea72
style: remove unused Boolean from primordials
VaishnavIUpadyaya Mar 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/internal/test_runner/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ function stopTest(timeout, signal) {

disposeFunction = () => {
abortListener[SymbolDispose]();
clearTimeout(timer);
timer[SymbolDispose]();
};
}

Expand Down Expand Up @@ -679,7 +679,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 = !!skip;
this.isTodo = (todo !== undefined && todo !== false) || this.parent?.isTodo;
this.startTime = null;
this.endTime = null;
Expand Down
6 changes: 6 additions & 0 deletions test/parallel/test-runner-skip-empty-string.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
'use strict';

const common = require('../common');
const { test } = require('node:test');

test('skip option empty string should not skip', { skip: '' }, common.mustCall());
Copy link
Copy Markdown
Member

@MoLow MoLow Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not? this seems wrong to me

Copy link
Copy Markdown
Member

@MoLow MoLow Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would fix the docs, not the implementation - it seems for correct

Copy link
Copy Markdown
Member

@MoLow MoLow Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atlowChemi @JakobJingleheimer WDYT?

Copy link
Copy Markdown
Member

@atlowChemi atlowChemi Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see why both behaviors would make sense.
Do we know what other test runners do?

Copy link
Copy Markdown
Author

@VaishnavIUpadyaya VaishnavIUpadyaya Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Member

@JakobJingleheimer JakobJingleheimer Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@JakobJingleheimer JakobJingleheimer Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like jest and mocha do not have this—they expose them only as methods like test.only('…') and test.skip('…').

Copy link
Copy Markdown
Member

@ljharb ljharb Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tape has this, and uses truthiness.