test_runner: fix nested hooks

PR-URL: #47648 Fixes: #47643 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
nodejs · Jul 6, 2023 · 0ea6371 · 0ea6371
1 parent ffcff68
commit 0ea6371
Showing 3 changed files with 22 additions and 27 deletions.
diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
@@ -192,6 +192,13 @@ class Test extends AsyncResource {
       this.testNumber = 0;
       this.timeout = kDefaultTimeout;
       this.root = this;
+      this.hooks = {
+        __proto__: null,
+        before: [],
+        after: [],
+        beforeEach: [],
+        afterEach: [],
+      };
     } else {
       const nesting = parent.parent === null ? parent.nesting :
         parent.nesting + 1;
@@ -204,6 +211,13 @@ class Test extends AsyncResource {
       this.testNumber = parent.subtests.length + 1;
       this.timeout = parent.timeout;
       this.root = parent.root;
+      this.hooks = {
+        __proto__: null,
+        before: [],
+        after: [],
+        beforeEach: ArrayPrototypeSlice(parent.hooks.beforeEach),
+        afterEach: ArrayPrototypeSlice(parent.hooks.afterEach),
+      };
     }
 
     switch (typeof concurrency) {
@@ -277,12 +291,6 @@ class Test extends AsyncResource {
     this.pendingSubtests = [];
     this.readySubtests = new SafeMap();
     this.subtests = [];
-    this.hooks = {
-      before: [],
-      after: [],
-      beforeEach: [],
-      afterEach: [],
-    };
     this.waitingOn = 0;
     this.finished = false;
 
@@ -772,11 +780,6 @@ class Suite extends Test {
 
   async run() {
     const hookArgs = this.getRunArgs();
-    const afterEach = runOnce(async () => {
-      if (this.parent?.hooks.afterEach.length > 0) {
-        await this.parent.runHook('afterEach', hookArgs);
-      }
-    });
 
     try {
       this.parent.activeSubtests++;
@@ -789,10 +792,6 @@ class Suite extends Test {
         return;
       }
 
-      if (this.parent?.hooks.beforeEach.length > 0) {
-        await this.parent.runHook('beforeEach', hookArgs);
-      }
-
       await this.runHook('before', hookArgs);
 
       const stopPromise = stopTest(this.timeout, this.signal);
@@ -801,11 +800,9 @@ class Suite extends Test {
 
       await SafePromiseRace([promise, stopPromise]);
       await this.runHook('after', hookArgs);
-      await afterEach();
 
       this.pass();
     } catch (err) {
-      try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ }
       if (isTestFailureError(err)) {
         this.fail(err);
       } else {

diff --git a/test/fixtures/test-runner/output/hooks.js b/test/fixtures/test-runner/output/hooks.js
@@ -17,12 +17,10 @@ describe('describe hooks', () => {
       'before describe hooks',
       'beforeEach 1', '1', 'afterEach 1',
       'beforeEach 2', '2', 'afterEach 2',
-      'beforeEach nested',
       'before nested',
-      'beforeEach nested 1', 'nested 1', 'afterEach nested 1',
-      'beforeEach nested 2', 'nested 2', 'afterEach nested 2',
+      'beforeEach nested 1', '+beforeEach nested 1', 'nested 1', 'afterEach nested 1', '+afterEach nested 1',
+      'beforeEach nested 2', '+beforeEach nested 2', 'nested 2', 'afterEach nested 2', '+afterEach nested 2',
       'after nested',
-      'afterEach nested',
       'after describe hooks',
     ]);
   });
@@ -44,10 +42,10 @@ describe('describe hooks', () => {
       testArr.push('after ' + this.name);
     });
     beforeEach(function() {
-      testArr.push('beforeEach ' + this.name);
+      testArr.push('+beforeEach ' + this.name);
     });
     afterEach(function() {
-      testArr.push('afterEach ' + this.name);
+      testArr.push('+afterEach ' + this.name);
     });
     it('nested 1', () => testArr.push('nested 1'));
     test('nested 2', () => testArr.push('nested 2'));
@@ -111,8 +109,8 @@ test('test hooks', async (t) => {
     'beforeEach 1', 'before test hooks', '1', 'afterEach 1',
     'beforeEach 2', '2', 'afterEach 2',
     'beforeEach nested',
-    'nested beforeEach nested 1', 'nested1', 'nested afterEach nested 1',
-    'nested beforeEach nested 2', 'nested 2', 'nested afterEach nested 2',
+    'beforeEach nested 1', 'nested beforeEach nested 1', 'nested1', 'afterEach nested 1', 'nested afterEach nested 1',
+    'beforeEach nested 2', 'nested beforeEach nested 2', 'nested 2', 'afterEach nested 2', 'nested afterEach nested 2',
     'afterEach nested',
   ]);
 });

diff --git a/test/fixtures/test-runner/output/name_pattern.js b/test/fixtures/test-runner/output/name_pattern.js
@@ -34,8 +34,8 @@ test('top level test enabled', common.mustCall(async (t) => {
 
 describe('top level describe enabled', () => {
   before(common.mustCall());
-  beforeEach(common.mustCall(4));
-  afterEach(common.mustCall(4));
+  beforeEach(common.mustCall(3));
+  afterEach(common.mustCall(3));
   after(common.mustCall());
 
   it('nested it disabled', common.mustNotCall());