test_runner: run before hook immediately if test started

PR-URL: #52003 Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
nodejs · Mar 9, 2024 · 5d13419 · 5d13419
1 parent f0e6acd
commit 5d13419
Show file tree

Hide file tree

Showing 8 changed files with 431 additions and 38 deletions.
diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
@@ -151,7 +151,11 @@ function setup(root) {
   const rejectionHandler =
     createProcessEventHandler('unhandledRejection', root);
   const coverage = configureCoverage(root, globalOptions);
-  const exitHandler = () => {
+  const exitHandler = async () => {
+    if (root.subtests.length === 0 && (root.hooks.before.length > 0 || root.hooks.after.length > 0)) {
+      // Run global before/after hooks in case there are no tests
+      await root.run();
+    }
     root.postRun(new ERR_TEST_FAILURE(
       'Promise resolution is still pending but the event loop has already resolved',
       kCancelledByParent));

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
@@ -177,19 +177,23 @@ class TestContext {
   }
 
   before(fn, options) {
-    this.#test.createHook('before', fn, options);
+    this.#test
+      .createHook('before', fn, { __proto__: null, ...options, hookType: 'before', loc: getCallerLocation() });
   }
 
   after(fn, options) {
-    this.#test.createHook('after', fn, options);
+    this.#test
+      .createHook('after', fn, { __proto__: null, ...options, hookType: 'after', loc: getCallerLocation() });
   }
 
   beforeEach(fn, options) {
-    this.#test.createHook('beforeEach', fn, options);
+    this.#test
+      .createHook('beforeEach', fn, { __proto__: null, ...options, hookType: 'beforeEach', loc: getCallerLocation() });
   }
 
   afterEach(fn, options) {
-    this.#test.createHook('afterEach', fn, options);
+    this.#test
+      .createHook('afterEach', fn, { __proto__: null, ...options, hookType: 'afterEach', loc: getCallerLocation() });
   }
 }
 
@@ -518,6 +522,14 @@ class Test extends AsyncResource {
     if (name === 'before' || name === 'after') {
       hook.run = runOnce(hook.run);
     }
+    if (name === 'before' && this.startTime !== null) {
+      // Test has already started, run the hook immediately
+      PromisePrototypeThen(hook.run(this.getRunArgs()), () => {
+        if (hook.error != null) {
+          this.fail(hook.error);
+        }
+      });
+    }
     ArrayPrototypePush(this.hooks[name], hook);
     return hook;
   }
@@ -615,26 +627,28 @@ class Test extends AsyncResource {
       return;
     }
 
-    const { args, ctx } = this.getRunArgs();
+    const hookArgs = this.getRunArgs();
+    const { args, ctx } = hookArgs;
     const after = async () => {
       if (this.hooks.after.length > 0) {
-        await this.runHook('after', { __proto__: null, args, ctx });
+        await this.runHook('after', hookArgs);
       }
     };
     const afterEach = runOnce(async () => {
       if (this.parent?.hooks.afterEach.length > 0) {
-        await this.parent.runHook('afterEach', { __proto__: null, args, ctx });
+        await this.parent.runHook('afterEach', hookArgs);
       }
     });
 
     let stopPromise;
 
     try {
       if (this.parent?.hooks.before.length > 0) {
+        // This hook usually runs immediately, we need to wait for it to finish
         await this.parent.runHook('before', this.parent.getRunArgs());
       }
       if (this.parent?.hooks.beforeEach.length > 0) {
-        await this.parent.runHook('beforeEach', { __proto__: null, args, ctx });
+        await this.parent.runHook('beforeEach', hookArgs);
       }
       stopPromise = stopTest(this.timeout, this.signal);
       const runArgs = ArrayPrototypeSlice(args);

diff --git a/test/fixtures/test-runner/output/global-hooks-with-no-tests.js b/test/fixtures/test-runner/output/global-hooks-with-no-tests.js
@@ -0,0 +1,6 @@
+'use strict';
+const common = require('../../../common');
+const { before, after } = require('node:test');
+
+before(common.mustCall(() => console.log('before')));
+after(common.mustCall(() => console.log('after')));
diff --git a/test/fixtures/test-runner/output/global-hooks-with-no-tests.snapshot b/test/fixtures/test-runner/output/global-hooks-with-no-tests.snapshot
@@ -0,0 +1,12 @@
+before
+TAP version 13
+after
+1..0
+# tests 0
+# suites 0
+# pass 0
+# fail 0
+# cancelled 0
+# skipped 0
+# todo 0
+# duration_ms *
diff --git a/test/fixtures/test-runner/output/hooks.js b/test/fixtures/test-runner/output/hooks.js
@@ -11,7 +11,7 @@ describe('describe hooks', () => {
   before(function() {
     testArr.push('before ' + this.name);
   });
-  after(function() {
+  after(common.mustCall(function() {
     testArr.push('after ' + this.name);
     assert.deepStrictEqual(testArr, [
       'before describe hooks',
@@ -23,7 +23,7 @@ describe('describe hooks', () => {
       'after nested',
       'after describe hooks',
     ]);
-  });
+  }));
   beforeEach(function() {
     testArr.push('beforeEach ' + this.name);
   });
@@ -52,18 +52,43 @@ describe('describe hooks', () => {
   });
 });
 
+describe('describe hooks - no subtests', () => {
+  const testArr = [];
+  before(function() {
+    testArr.push('before ' + this.name);
+  });
+  after(common.mustCall(function() {
+    testArr.push('after ' + this.name);
+    assert.deepStrictEqual(testArr, [
+      'before describe hooks - no subtests',
+      'after describe hooks - no subtests',
+    ]);
+  }));
+  beforeEach(common.mustNotCall());
+  afterEach(common.mustNotCall());
+});
+
 describe('before throws', () => {
   before(() => { throw new Error('before'); });
   it('1', () => {});
   test('2', () => {});
 });
 
+describe('before throws - no subtests', () => {
+  before(() => { throw new Error('before'); });
+  after(common.mustCall());
+});
+
 describe('after throws', () => {
   after(() => { throw new Error('after'); });
   it('1', () => {});
   test('2', () => {});
 });
 
+describe('after throws - no subtests', () => {
+  after(() => { throw new Error('after'); });
+});
+
 describe('beforeEach throws', () => {
   beforeEach(() => { throw new Error('beforeEach'); });
   it('1', () => {});
@@ -123,13 +148,48 @@ test('test hooks', async (t) => {
   }));
 });
 
+
+test('test hooks - no subtests', async (t) => {
+  const testArr = [];
+
+  t.before((t) => testArr.push('before ' + t.name));
+  t.after(common.mustCall((t) => testArr.push('after ' + t.name)));
+  t.beforeEach(common.mustNotCall());
+  t.afterEach(common.mustNotCall());
+
+  t.after(common.mustCall(() => {
+    assert.deepStrictEqual(testArr, [
+      'before test hooks - no subtests',
+      'after test hooks - no subtests',
+    ]);
+  }));
+});
+
 test('t.before throws', async (t) => {
   t.after(common.mustCall());
   t.before(() => { throw new Error('before'); });
   await t.test('1', () => {});
   await t.test('2', () => {});
 });
 
+test('t.before throws - no subtests', async (t) => {
+  t.after(common.mustCall());
+  t.before(() => { throw new Error('before'); });
+});
+
+test('t.after throws', async (t) => {
+  t.before(common.mustCall());
+  t.after(() => { throw new Error('after'); });
+  await t.test('1', () => {});
+  await t.test('2', () => {});
+});
+
+test('t.after throws - no subtests', async (t) => {
+  t.before(common.mustCall());
+  t.after(() => { throw new Error('after'); });
+});
+
+
 test('t.beforeEach throws', async (t) => {
   t.after(common.mustCall());
   t.beforeEach(() => { throw new Error('beforeEach'); });