test_runner: fix #50665 .skip, .todo and .only missing in subtests

[rluvaton]

Fix #50665

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/55582/

[ljharb]

wouldn't it be more performant to leave this as a prototype method, and instead do a public class field test = FunctionPrototypeBind(this.test, this);, so that the "meat" of the method can be shared and optimized across all instances?

separately, couldn't the skip/todo/only methods be assigned to the prototype method once, in a static block, instead of a new function created on every instance?

[rluvaton]

If you mean with Object.defineProperty, then without changing the #test to [kTest] it can't as it cannot access private method outside the class.
if you mean adding the skip/todo/any in the constructor but keeping the test function as is, than for some reason it did not work for me

The skip/todo/any methods are needed to assign on the test function, not on the TestContext

[ljharb]

Ah, true, the 3 expandos need to be done in the constructor every time - altho the meat could be shared private methods and the per-instance thing could just be a bound function.

[MoLow]

why not keep as prototype methods?

[rluvaton]

for some reason it did not work for me adding skip/todo/etc to the test function

[bakkot]

The docs for only say

it is possible to skip all top level tests except for a selected subset by passing the only option to the tests that should be run. When a test with the only option set is run, all subtests are also run.

How is that supposed to map to use of only in subtests? Consider test('a', t => { t.test.only('b', () => {}); }) - i.e., a top-level test which is not marked as only with a subtest which is. I can imagine at least two possibilities:

• Since the top-level test does not have only, running in --test-only skips the entire top-level test. You'd have to use test.only('a', t => { t.test.only('b', () => {}); }) for this to work.
• It somehow figures out that the top-level test contains a t.only, and enables the top-level test, but skips subtests of that test.

Presumably this implements the first one (though I don't see any tests for that scenario). Either way the docs should be updated, because right now they imply that only is specific to top-level tests. Maybe something like:

When a test with the only option set is run, all subtests are also run, unless some of the subtests of that test also have the only option set, in which case only those subtests are run. Using only for a subtest has no effect unless its parent test also has only set.

[rluvaton]

@bakkot this is a known issue and unrelated to this PR or the linked issue

If I'm correct #48932 is the one to fix that issue

[aduh95]

Removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. as this PR has no approvals

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[github-actions]

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.