From 0f69b6c26d575e7b00e9c16c084c142614f4b99f Mon Sep 17 00:00:00 2001 From: Jithil P Ponnan Date: Mon, 7 Nov 2022 16:58:39 +1100 Subject: [PATCH] test_runner: fix afterEach not running on test failures test_runner: fix afterEach not running on test failures PR-URL: https://github.com/nodejs/node/pull/45204 Fixes: https://github.com/nodejs/node/issues/45192 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Moshe Atlow --- lib/internal/test_runner/test.js | 28 ++-- lib/internal/util.js | 2 +- test/message/test_runner_describe_it.out | 2 - test/message/test_runner_hooks.js | 27 +++- test/message/test_runner_hooks.out | 172 ++++++++++++++++++++++- test/message/test_runner_output.out | 2 - 6 files changed, 211 insertions(+), 22 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 75a3a1558924b1..6d30f3aaa6686a 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -41,6 +41,7 @@ const { const { createDeferredPromise, kEmptyObject, + once: runOnce, } = require('internal/util'); const { isPromise } = require('internal/util/types'); const { @@ -482,8 +483,14 @@ class Test extends AsyncResource { return; } + const { args, ctx } = this.getRunArgs(); + const afterEach = runOnce(async () => { + if (this.parent?.hooks.afterEach.length > 0) { + await this.parent[kRunHook]('afterEach', { args, ctx }); + } + }); + try { - const { args, ctx } = this.getRunArgs(); if (this.parent?.hooks.beforeEach.length > 0) { await this.parent[kRunHook]('beforeEach', { args, ctx }); } @@ -518,12 +525,10 @@ class Test extends AsyncResource { return; } - if (this.parent?.hooks.afterEach.length > 0) { - await this.parent[kRunHook]('afterEach', { args, ctx }); - } - + await afterEach(); this.pass(); } catch (err) { + try { await afterEach(); } catch { /* test is already failing, let's the error */ } if (isTestFailureError(err)) { if (err.failureType === kTestTimeoutFailure) { this.cancel(err); @@ -728,6 +733,12 @@ class Suite extends Test { } async run() { + const hookArgs = this.getRunArgs(); + const afterEach = runOnce(async () => { + if (this.parent?.hooks.afterEach.length > 0) { + await this.parent[kRunHook]('afterEach', hookArgs); + } + }); try { this.parent.activeSubtests++; await this.buildSuite; @@ -739,7 +750,6 @@ class Suite extends Test { return; } - const hookArgs = this.getRunArgs(); if (this.parent?.hooks.beforeEach.length > 0) { await this.parent[kRunHook]('beforeEach', hookArgs); @@ -753,13 +763,11 @@ class Suite extends Test { await SafePromiseRace([promise, stopPromise]); await this[kRunHook]('after', hookArgs); - - if (this.parent?.hooks.afterEach.length > 0) { - await this.parent[kRunHook]('afterEach', hookArgs); - } + await afterEach(); this.pass(); } catch (err) { + try { await afterEach(); } catch { /* test is already failing, let's the error */ } if (isTestFailureError(err)) { this.fail(err); } else { diff --git a/lib/internal/util.js b/lib/internal/util.js index e250a798af7153..e284a1c37a5487 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -450,7 +450,7 @@ function once(callback) { return function(...args) { if (called) return; called = true; - ReflectApply(callback, this, args); + return ReflectApply(callback, this, args); }; } diff --git a/test/message/test_runner_describe_it.out b/test/message/test_runner_describe_it.out index 7ca922397a52b3..199e834d6f65ae 100644 --- a/test/message/test_runner_describe_it.out +++ b/test/message/test_runner_describe_it.out @@ -42,8 +42,6 @@ not ok 4 - sync fail todo with message # TODO this is a failing todo * * * - * - * ... # Subtest: sync skip pass ok 5 - sync skip pass # SKIP diff --git a/test/message/test_runner_hooks.js b/test/message/test_runner_hooks.js index 4820b01470d0fe..e5e6bdc9d17ffa 100644 --- a/test/message/test_runner_hooks.js +++ b/test/message/test_runner_hooks.js @@ -1,6 +1,6 @@ // Flags: --no-warnings 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); const { test, describe, it, before, after, beforeEach, afterEach } = require('node:test'); @@ -76,6 +76,18 @@ describe('afterEach throws', () => { it('2', () => {}); }); +describe('afterEach when test fails', () => { + afterEach(common.mustCall(2)); + it('1', () => { throw new Error('test'); }); + it('2', () => {}); +}); + +describe('afterEach throws and test fails', () => { + afterEach(() => { throw new Error('afterEach'); }); + it('1', () => { throw new Error('test'); }); + it('2', () => {}); +}); + test('test hooks', async (t) => { const testArr = []; t.beforeEach((t) => testArr.push('beforeEach ' + t.name)); @@ -111,3 +123,16 @@ test('t.afterEach throws', async (t) => { await t.test('1', () => {}); await t.test('2', () => {}); }); + + +test('afterEach when test fails', async (t) => { + t.afterEach(common.mustCall(2)); + await t.test('1', () => { throw new Error('test'); }); + await t.test('2', () => {}); +}); + +test('afterEach throws and test fails', async (t) => { + afterEach(() => { throw new Error('afterEach'); }); + await t.test('1', () => { throw new Error('test'); }); + await t.test('2', () => {}); +}); diff --git a/test/message/test_runner_hooks.out b/test/message/test_runner_hooks.out index 57008a44bbb141..e0b3e91b8c1376 100644 --- a/test/message/test_runner_hooks.out +++ b/test/message/test_runner_hooks.out @@ -189,6 +189,86 @@ not ok 5 - afterEach throws error: '2 subtests failed' code: 'ERR_TEST_FAILURE' ... +# Subtest: afterEach when test fails + # Subtest: 1 + not ok 1 - 1 + --- + duration_ms: * + failureType: 'testCodeFailure' + error: 'test' + code: 'ERR_TEST_FAILURE' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + # Subtest: 2 + ok 2 - 2 + --- + duration_ms: * + ... + 1..2 +not ok 6 - afterEach when test fails + --- + duration_ms: * + failureType: 'subtestsFailed' + error: '1 subtest failed' + code: 'ERR_TEST_FAILURE' + ... +# Subtest: afterEach throws and test fails + # Subtest: 1 + not ok 1 - 1 + --- + duration_ms: * + failureType: 'testCodeFailure' + error: 'test' + code: 'ERR_TEST_FAILURE' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + # Subtest: 2 + not ok 2 - 2 + --- + duration_ms: * + failureType: 'hookFailed' + error: 'failed running afterEach hook' + code: 'ERR_TEST_FAILURE' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + 1..2 +not ok 7 - afterEach throws and test fails + --- + duration_ms: * + failureType: 'subtestsFailed' + error: '2 subtests failed' + code: 'ERR_TEST_FAILURE' + ... # Subtest: test hooks # Subtest: 1 ok 1 - 1 @@ -217,7 +297,7 @@ not ok 5 - afterEach throws duration_ms: * ... 1..3 -ok 6 - test hooks +ok 8 - test hooks --- duration_ms: * ... @@ -261,7 +341,7 @@ ok 6 - test hooks * ... 1..2 -not ok 7 - t.beforeEach throws +not ok 9 - t.beforeEach throws --- duration_ms: * failureType: 'subtestsFailed' @@ -308,17 +388,97 @@ not ok 7 - t.beforeEach throws * ... 1..2 -not ok 8 - t.afterEach throws +not ok 10 - t.afterEach throws + --- + duration_ms: * + failureType: 'subtestsFailed' + error: '2 subtests failed' + code: 'ERR_TEST_FAILURE' + ... +# Subtest: afterEach when test fails + # Subtest: 1 + not ok 1 - 1 + --- + duration_ms: * + failureType: 'testCodeFailure' + error: 'test' + code: 'ERR_TEST_FAILURE' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + # Subtest: 2 + ok 2 - 2 + --- + duration_ms: * + ... + 1..2 +not ok 11 - afterEach when test fails + --- + duration_ms: * + failureType: 'subtestsFailed' + error: '1 subtest failed' + code: 'ERR_TEST_FAILURE' + ... +# Subtest: afterEach throws and test fails + # Subtest: 1 + not ok 1 - 1 + --- + duration_ms: * + failureType: 'testCodeFailure' + error: 'test' + code: 'ERR_TEST_FAILURE' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + # Subtest: 2 + not ok 2 - 2 + --- + duration_ms: * + failureType: 'hookFailed' + error: 'failed running afterEach hook' + code: 'ERR_TEST_FAILURE' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + 1..2 +not ok 12 - afterEach throws and test fails --- duration_ms: * failureType: 'subtestsFailed' error: '2 subtests failed' code: 'ERR_TEST_FAILURE' ... -1..8 -# tests 8 +1..12 +# tests 12 # pass 2 -# fail 6 +# fail 10 # cancelled 0 # skipped 0 # todo 0 diff --git a/test/message/test_runner_output.out b/test/message/test_runner_output.out index 938989b1b34146..96d977b21c5b1a 100644 --- a/test/message/test_runner_output.out +++ b/test/message/test_runner_output.out @@ -42,8 +42,6 @@ not ok 4 - sync fail todo with message # TODO this is a failing todo * * * - * - * ... # Subtest: sync skip pass ok 5 - sync skip pass # SKIP