Skip to content

test_runner: add skip, todo, only to subtest context#61251

Open
heathdutton wants to merge 1 commit intonodejs:mainfrom
heathdutton:test_runner/fix-subtest-skip-only-todo-50665
Open

test_runner: add skip, todo, only to subtest context#61251
heathdutton wants to merge 1 commit intonodejs:mainfrom
heathdutton:test_runner/fix-subtest-skip-only-todo-50665

Conversation

@heathdutton
Copy link
Copy Markdown
Contributor

The test context passed to subtests was missing the .skip, .todo,
and .only variants on t.test. These were available on the top-level
test function but not on the subtest context.

Move the test function from a class method to an arrow function in
the constructor so that .skip, .todo, and .only properties can
be attached to it.

Fixes: #50665

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jan 2, 2026
@avivkeller
Copy link
Copy Markdown
Member

Can you please add tests?

The test context passed to subtests was missing the `.skip`, `.todo`,
and `.only` variants on `t.test`. These were available on the top-level
`test` function but not on the subtest context.

Move the test function from a class method to an arrow function in
the constructor so that `.skip`, `.todo`, and `.only` properties can
be attached to it.

Fixes: nodejs#50665
@heathdutton heathdutton force-pushed the test_runner/fix-subtest-skip-only-todo-50665 branch from c87fc54 to f8775f9 Compare January 2, 2026 03:48
@heathdutton
Copy link
Copy Markdown
Contributor Author

Added test file: test/parallel/test-runner-subtest-skip-todo-only.js

Copy link
Copy Markdown
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together, and sorry it took a while to get to it.

Comment on lines +259 to +276
this.test = (name, options, fn) => {
const overrides = {
__proto__: null,
loc: getCallerLocation(),
};

const { plan } = this.#test;
if (plan !== null) {
plan.count();
}

const subtest = this.#test.createSubtest(
// eslint-disable-next-line no-use-before-define
Test, name, options, fn, overrides,
);

return subtest.start();
};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's not necessary or desirable to move this into the constructor. (The code is otherwise unchanged)

@@ -1,5 +1,6 @@
'use strict';
const {
ArrayPrototypeForEach,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
ArrayPrototypeForEach,

MathMax,
Number,
NumberPrototypeToFixed,
ObjectSeal,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
ObjectAssign,
ObjectSeal,

Comment on lines +278 to +298
ArrayPrototypeForEach(['skip', 'todo', 'only'], (keyword) => {
this.test[keyword] = (name, options, fn) => {
const overrides = {
__proto__: null,
[keyword]: true,
loc: getCallerLocation(),
};

const { plan } = this.#test;
if (plan !== null) {
plan.count();
}

const subtest = this.#test.createSubtest(
// eslint-disable-next-line no-use-before-define
Test, name, options, fn, overrides,
);

return subtest.start();
};
});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think duplicating the contents of TestContext.test is not necessary?

Suggested change
ArrayPrototypeForEach(['skip', 'todo', 'only'], (keyword) => {
this.test[keyword] = (name, options, fn) => {
const overrides = {
__proto__: null,
[keyword]: true,
loc: getCallerLocation(),
};
const { plan } = this.#test;
if (plan !== null) {
plan.count();
}
const subtest = this.#test.createSubtest(
// eslint-disable-next-line no-use-before-define
Test, name, options, fn, overrides,
);
return subtest.start();
};
});
ObjectAssign(this.test, {
__proto__: null,
'expectFailure': this.test,
'only': this.test,
'skip': this.test,
'todo': this.test,
});

That might create some inception though. If it does, I think cut+paste this.test to a private method like this.#runTest, set this.test = this.#runTest (outside the constructor), and change the this.tests within the ObjectAssign to this.#runTest.

P.S. You forgot about expectFailure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Needs test-cases for expectFailure 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test runner (--test, node:test) subtest test context lack .skip, .only and .todo functions

4 participants