test_runner: refactor and simplify internals

cjihrig · targos · commit f45edb4b5e22 · 2024-07-28T09:51:33.000+02:00
This commit refactors some of the internals of the test runner. PR-URL: #53921 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
@@ -208,7 +208,7 @@ function setup(root) {
       };
     },
     counters: null,
-    shouldColorizeTestFiles: false,
+    shouldColorizeTestFiles: shouldColorizeTestFiles(globalOptions.destinations),
     teardown: exitHandler,
     snapshotManager: null,
   };
@@ -218,48 +218,46 @@ function setup(root) {
 }
 
 let globalRoot;
-let reportersSetup;
-function getGlobalRoot() {
+let asyncBootstrap;
+function lazyBootstrapRoot() {
   if (!globalRoot) {
     globalRoot = createTestTree({ __proto__: null, entryFile: process.argv?.[1] });
     globalRoot.reporter.on('test:fail', (data) => {
       if (data.todo === undefined || data.todo === false) {
         process.exitCode = kGenericUserError;
       }
     });
-    reportersSetup = setupTestReporters(globalRoot.reporter);
-    globalRoot.harness.shouldColorizeTestFiles ||= shouldColorizeTestFiles(globalRoot);
+    asyncBootstrap = setupTestReporters(globalRoot.reporter);
   }
   return globalRoot;
 }
 
 async function startSubtest(subtest) {
-  if (reportersSetup) {
+  if (asyncBootstrap) {
     // Only incur the overhead of awaiting the Promise once.
-    await reportersSetup;
-    reportersSetup = undefined;
-  }
-
-  const root = getGlobalRoot();
-  if (!root.harness.bootstrapComplete) {
-    root.harness.bootstrapComplete = true;
-    queueMicrotask(() => {
-      root.harness.allowTestsToRun = true;
-      root.processPendingSubtests();
-    });
+    await asyncBootstrap;
+    asyncBootstrap = undefined;
+    if (!subtest.root.harness.bootstrapComplete) {
+      subtest.root.harness.bootstrapComplete = true;
+      queueMicrotask(() => {
+        subtest.root.harness.allowTestsToRun = true;
+        subtest.root.processPendingSubtests();
+      });
+    }
   }
 
   await subtest.start();
 }
 
 function runInParentContext(Factory) {
   function run(name, options, fn, overrides) {
-    const parent = testResources.get(executionAsyncId()) || getGlobalRoot();
+    const parent = testResources.get(executionAsyncId()) || lazyBootstrapRoot();
     const subtest = parent.createSubtest(Factory, name, options, fn, overrides);
-    if (!(parent instanceof Suite)) {
-      return startSubtest(subtest);
+    if (parent instanceof Suite) {
+      return PromiseResolve();
     }
-    return PromiseResolve();
+
+    return startSubtest(subtest);
   }
 
   const test = (name, options, fn) => {
@@ -286,7 +284,7 @@ function runInParentContext(Factory) {
 
 function hook(hook) {
   return (fn, options) => {
-    const parent = testResources.get(executionAsyncId()) || getGlobalRoot();
+    const parent = testResources.get(executionAsyncId()) || lazyBootstrapRoot();
     parent.createHook(hook, fn, {
       __proto__: null,
       ...options,
diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js
@@ -70,7 +70,6 @@ const {
   convertStringToRegExp,
   countCompletedTest,
   kDefaultPattern,
-  shouldColorizeTestFiles,
 } = require('internal/test_runner/utils');
 const { Glob } = require('internal/fs/glob');
 const { once } = require('events');
@@ -552,7 +551,6 @@ function run(options = kEmptyObject) {
   }
 
   const root = createTestTree({ __proto__: null, concurrency, timeout, signal });
-  root.harness.shouldColorizeTestFiles ||= shouldColorizeTestFiles(root);
 
   if (process.env.NODE_TEST_CONTEXT !== undefined) {
     process.emitWarning('node:test run() is being called recursively within a test file. skipping running files.');
diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
@@ -75,7 +75,6 @@ const kHookFailure = 'hookFailed';
 const kDefaultTimeout = null;
 const noop = FunctionPrototype;
 const kShouldAbort = Symbol('kShouldAbort');
-const kFilename = process.argv?.[1];
 const kHookNames = ObjectSeal(['before', 'after', 'beforeEach', 'afterEach']);
 const kUnwrapErrors = new SafeSet()
   .add(kTestCodeFailure).add(kHookFailure)
@@ -508,7 +507,7 @@ class Test extends AsyncResource {
       this.diagnostic(warning);
     }
 
-    if (loc === undefined || kFilename === undefined) {
+    if (loc === undefined) {
       this.loc = undefined;
     } else {
       this.loc = {
diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js
@@ -131,10 +131,9 @@ function tryBuiltinReporter(name) {
   return require(builtinPath);
 }
 
-function shouldColorizeTestFiles(rootTest) {
+function shouldColorizeTestFiles(destinations) {
   // This function assumes only built-in destinations (stdout/stderr) supports coloring
-  const { reporters, destinations } = parseCommandLine();
-  return ArrayPrototypeSome(reporters, (_, index) => {
+  return ArrayPrototypeSome(destinations, (_, index) => {
     const destination = kBuiltinDestinations.get(destinations[index]);
     return destination && shouldColorize(destination);
   });
