test_runner: improve describe.only behavior

PR-URL: #52296
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

doc/api/test.md

@@ -230,15 +230,24 @@ const { describe, it } = require('node:test');
 ## `only` tests
 
 If Node.js is started with the [`--test-only`][] command-line option, 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. The test context's `runOnly()`
+possible to skip all tests except for a selected subset by passing
+the `only` option to the tests that should run. When a test with the `only`
+option is set, all subtests are also run.
+If a suite has the `only` option set, all tests within the suite are run,
+unless it has descendants with the `only` option set, in which case only those
+tests are run.
+
+When using [subtests][] within a `test()`/`it()`, it is required to mark
+all ancestor tests with the `only` option to run only a
+selected subset of tests.
+
+The test context's `runOnly()`
 method can be used to implement the same behavior at the subtest level. Tests
 that are not executed are omitted from the test runner output.
 
 ```js
 // Assume Node.js is run with the --test-only command-line option.
-// The 'only' option is set, so this test is run.
+// The suite's 'only' option is set, so these tests are run.
 test('this test is run', { only: true }, async (t) => {
   // Within this test, all subtests are run by default.
   await t.test('running subtest');
@@ -262,6 +271,29 @@ test('this test is not run', () => {
   // This code is not run.
   throw new Error('fail');
 });
+
+describe('a suite', () => {
+  // The 'only' option is set, so this test is run.
+  it('this test is run', { only: true }, () => {
+    // This code is run.
+  });
+
+  it('this test is not run', () => {
+    // This code is not run.
+    throw new Error('fail');
+  });
+});
+
+describe.only('a suite', () => {
+  // The 'only' option is set, so this test is run.
+  it('this test is run', () => {
+    // This code is run.
+  });
+
+  it('this test is run', () => {
+    // This code is run.
+  });
+});
 ```
 
 ## Filtering tests by name
@@ -3126,6 +3158,7 @@ Can be used to abort test subtasks when the test has been aborted.
 [describe options]: #describename-options-fn
 [it options]: #testname-options-fn
 [stream.compose]: stream.md#streamcomposestreams
+[subtests]: #subtests
 [suite options]: #suitename-options-fn
 [test reporters]: #test-reporters
 [test runner execution model]: #test-runner-execution-model

lib/internal/test_runner/test.js

@@ -238,8 +238,8 @@ class Test extends AsyncResource {
   constructor(options) {
     super('Test');
 
-    let { fn, name, parent, skip } = options;
-    const { concurrency, loc, only, timeout, todo, signal } = options;
+    let { fn, name, parent } = options;
+    const { concurrency, loc, only, timeout, todo, skip, signal } = options;
 
     if (typeof fn !== 'function') {
       fn = noop;
@@ -300,10 +300,13 @@ class Test extends AsyncResource {
 
       if ((testNamePatterns !== null && !this.matchesTestNamePatterns()) ||
           (testOnlyFlag && !this.only)) {
-        skip = true;
         this.filtered = true;
         this.parent.filteredSubtestCount++;
       }
+
+      if (testOnlyFlag && only === false) {
+        fn = noop;
+      }
     }
 
     switch (typeof concurrency) {
@@ -583,8 +586,12 @@ class Test extends AsyncResource {
     ArrayPrototypePush(this.diagnostics, message);
   }
 
+  get shouldFilter() {
+    return this.filtered && this.parent?.filteredSubtestCount > 0;
+  }
+
   start() {
-    if (this.filtered) {
+    if (this.shouldFilter) {
       noopTestStream ??= new TestsStream();
       this.reporter = noopTestStream;
       this.run = this.filteredRun;
@@ -792,7 +799,7 @@ class Test extends AsyncResource {
     this.mock?.reset();
 
     if (this.parent !== null) {
-      if (!this.filtered) {
+      if (!this.shouldFilter) {
         const report = this.getReportDetails();
         report.details.passed = this.passed;
         this.testNumber ||= ++this.parent.outputSubtestCount;
@@ -1005,23 +1012,27 @@ class Suite extends Test {
           (err) => {
             this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure));
           }),
-        () => {
-          this.buildPhaseFinished = true;
-
-          // A suite can transition from filtered to unfiltered based on the
-          // tests that it contains.
-          if (this.filtered && this.filteredSubtestCount !== this.subtests.length) {
-            this.filtered = false;
-            this.parent.filteredSubtestCount--;
-          }
-        },
+        () => this.postBuild(),
       );
     } catch (err) {
       this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure));
 
       this.buildPhaseFinished = true;
     }
-    this.fn = () => {};
+    this.fn = noop;
+  }
+
+  postBuild() {
+    this.buildPhaseFinished = true;
+    if (this.filtered && this.filteredSubtestCount !== this.subtests.length) {
+      // A suite can transition from filtered to unfiltered based on the
+      // tests that it contains - in case of children matching patterns.
+      this.filtered = false;
+      this.parent.filteredSubtestCount--;
+    } else if (testOnlyFlag && testNamePatterns == null && this.filteredSubtestCount === this.subtests.length) {
+      // If no subtests are marked as "only", run them all
+      this.filteredSubtestCount = 0;
+    }
   }
 
   getRunArgs() {

test/fixtures/test-runner/output/only_tests.js

@@ -1,100 +1,121 @@
 // Flags: --test-only
 'use strict';
-require('../../../common');
+const common = require('../../../common');
 const { test, describe, it } = require('node:test');
 
 // These tests should be skipped based on the 'only' option.
-test('only = undefined');
-test('only = undefined, skip = string', { skip: 'skip message' });
-test('only = undefined, skip = true', { skip: true });
-test('only = undefined, skip = false', { skip: false });
-test('only = false', { only: false });
-test('only = false, skip = string', { only: false, skip: 'skip message' });
-test('only = false, skip = true', { only: false, skip: true });
-test('only = false, skip = false', { only: false, skip: false });
+test('only = undefined', common.mustNotCall());
+test('only = undefined, skip = string', { skip: 'skip message' }, common.mustNotCall());
+test('only = undefined, skip = true', { skip: true }, common.mustNotCall());
+test('only = undefined, skip = false', { skip: false }, common.mustNotCall());
+test('only = false', { only: false }, common.mustNotCall());
+test('only = false, skip = string', { only: false, skip: 'skip message' }, common.mustNotCall());
+test('only = false, skip = true', { only: false, skip: true }, common.mustNotCall());
+test('only = false, skip = false', { only: false, skip: false }, common.mustNotCall());
 
 // These tests should be skipped based on the 'skip' option.
-test('only = true, skip = string', { only: true, skip: 'skip message' });
-test('only = true, skip = true', { only: true, skip: true });
+test('only = true, skip = string', { only: true, skip: 'skip message' }, common.mustNotCall());
+test('only = true, skip = true', { only: true, skip: true }, common.mustNotCall());
 
 // An 'only' test with subtests.
-test('only = true, with subtests', { only: true }, async (t) => {
+test('only = true, with subtests', { only: true }, common.mustCall(async (t) => {
   // These subtests should run.
-  await t.test('running subtest 1');
-  await t.test('running subtest 2');
+  await t.test('running subtest 1', common.mustCall());
+  await t.test('running subtest 2', common.mustCall());
 
   // Switch the context to only execute 'only' tests.
   t.runOnly(true);
-  await t.test('skipped subtest 1');
-  await t.test('skipped subtest 2');
-  await t.test('running subtest 3', { only: true });
+  await t.test('skipped subtest 1', common.mustNotCall());
+  await t.test('skipped subtest 2'), common.mustNotCall();
+  await t.test('running subtest 3', { only: true }, common.mustCall());
 
   // Switch the context back to execute all tests.
   t.runOnly(false);
-  await t.test('running subtest 4', async (t) => {
+  await t.test('running subtest 4', common.mustCall(async (t) => {
     // These subtests should run.
-    await t.test('running sub-subtest 1');
-    await t.test('running sub-subtest 2');
+    await t.test('running sub-subtest 1', common.mustCall());
+    await t.test('running sub-subtest 2', common.mustCall());
 
     // Switch the context to only execute 'only' tests.
     t.runOnly(true);
-    await t.test('skipped sub-subtest 1');
-    await t.test('skipped sub-subtest 2');
-  });
+    await t.test('skipped sub-subtest 1', common.mustNotCall());
+    await t.test('skipped sub-subtest 2', common.mustNotCall());
+  }));
 
   // Explicitly do not run these tests.
-  await t.test('skipped subtest 3', { only: false });
-  await t.test('skipped subtest 4', { skip: true });
-});
+  await t.test('skipped subtest 3', { only: false }, common.mustNotCall());
+  await t.test('skipped subtest 4', { skip: true }, common.mustNotCall());
+}));
 
-describe.only('describe only = true, with subtests', () => {
-  it.only('`it` subtest 1 should run', () => {});
+describe.only('describe only = true, with subtests', common.mustCall(() => {
+  it.only('`it` subtest 1 should run', common.mustCall());
 
-  it('`it` subtest 2 should not run', async () => {});
-});
+  it('`it` subtest 2 should not run', common.mustNotCall());
+}));
 
-describe.only('describe only = true, with a mixture of subtests', () => {
-  it.only('`it` subtest 1', () => {});
+describe.only('describe only = true, with a mixture of subtests', common.mustCall(() => {
+  it.only('`it` subtest 1', common.mustCall());
 
-  it.only('`it` async subtest 1', async () => {});
+  it.only('`it` async subtest 1', common.mustCall(async () => {}));
 
-  it('`it` subtest 2 only=true', { only: true });
+  it('`it` subtest 2 only=true', { only: true }, common.mustCall());
 
-  it('`it` subtest 2 only=false', { only: false }, () => {
-    throw new Error('This should not run');
-  });
+  it('`it` subtest 2 only=false', { only: false }, common.mustNotCall());
 
-  it.skip('`it` subtest 3 skip', () => {
-    throw new Error('This should not run');
-  });
+  it.skip('`it` subtest 3 skip', common.mustNotCall());
 
-  it.todo('`it` subtest 4 todo', { only: false }, () => {
-    throw new Error('This should not run');
-  });
+  it.todo('`it` subtest 4 todo', { only: false }, common.mustNotCall());
 
-  test.only('`test` subtest 1', () => {});
+  test.only('`test` subtest 1', common.mustCall());
 
-  test.only('`test` async subtest 1', async () => {});
+  test.only('`test` async subtest 1', common.mustCall(async () => {}));
 
-  test('`test` subtest 2 only=true', { only: true });
+  test('`test` subtest 2 only=true', { only: true }, common.mustCall());
 
-  test('`test` subtest 2 only=false', { only: false }, () => {
-    throw new Error('This should not run');
-  });
+  test('`test` subtest 2 only=false', { only: false }, common.mustNotCall());
 
-  test.skip('`test` subtest 3 skip', () => {
-    throw new Error('This should not run');
-  });
+  test.skip('`test` subtest 3 skip', common.mustNotCall());
 
-  test.todo('`test` subtest 4 todo', { only: false }, () => {
-    throw new Error('This should not run');
-  });
-});
+  test.todo('`test` subtest 4 todo', { only: false }, common.mustNotCall());
+}));
 
-describe.only('describe only = true, with subtests', () => {
-  test.only('subtest should run', () => {});
+describe.only('describe only = true, with subtests', common.mustCall(() => {
+  test.only('subtest should run', common.mustCall());
 
-  test('async subtest should not run', async () => {});
+  test('async subtest should not run', common.mustNotCall());
 
-  test('subtest should be skipped', { only: false }, () => {});
-});
+  test('subtest should be skipped', { only: false }, common.mustNotCall());
+}));
+
+
+describe('describe only = undefined, with nested only subtest', common.mustCall(() => {
+  test('subtest should not run', common.mustNotCall());
+  describe('nested describe', common.mustCall(() => {
+    test('subtest should not run', common.mustNotCall());
+    test.only('nested test should run', common.mustCall());
+  }));
+}));
+
+
+describe('describe only = undefined, with subtests', common.mustCall(() => {
+  test('async subtest should not run', common.mustNotCall());
+}));
+
+describe('describe only = false, with subtests', { only: false }, common.mustNotCall(() => {
+  test('async subtest should not run', common.mustNotCall());
+}));
+
+
+describe.only('describe only = true, with nested subtests', common.mustCall(() => {
+  test('async subtest should run', common.mustCall());
+  describe('nested describe', common.mustCall(() => {
+    test('nested test should run', common.mustCall());
+  }));
+}));
+
+describe('describe only = false, with nested only subtests', { only: false }, common.mustNotCall(() => {
+  test('async subtest should not run', common.mustNotCall());
+  describe('nested describe', common.mustNotCall(() => {
+    test.only('nested test should run', common.mustNotCall());
+  }));
+}));

test/fixtures/test-runner/output/only_tests.snapshot

@@ -112,10 +112,53 @@ ok 6 - describe only = true, with subtests
   duration_ms: *
   type: 'suite'
   ...
-1..6
-# tests 18
-# suites 3
-# pass 15
+# Subtest: describe only = undefined, with nested only subtest
+    # Subtest: nested describe
+        # Subtest: nested test should run
+        ok 1 - nested test should run
+          ---
+          duration_ms: *
+          ...
+        1..1
+    ok 1 - nested describe
+      ---
+      duration_ms: *
+      type: 'suite'
+      ...
+    1..1
+ok 7 - describe only = undefined, with nested only subtest
+  ---
+  duration_ms: *
+  type: 'suite'
+  ...
+# Subtest: describe only = true, with nested subtests
+    # Subtest: async subtest should run
+    ok 1 - async subtest should run
+      ---
+      duration_ms: *
+      ...
+    # Subtest: nested describe
+        # Subtest: nested test should run
+        ok 1 - nested test should run
+          ---
+          duration_ms: *
+          ...
+        1..1
+    ok 2 - nested describe
+      ---
+      duration_ms: *
+      type: 'suite'
+      ...
+    1..2
+ok 8 - describe only = true, with nested subtests
+  ---
+  duration_ms: *
+  type: 'suite'
+  ...
+1..8
+# tests 21
+# suites 7
+# pass 18
 # fail 0
 # cancelled 0
 # skipped 3