From 14af24be49365fb8e4e2ffd270e86bababa3820e Mon Sep 17 00:00:00 2001 From: Jithil Date: Thu, 27 Oct 2022 20:26:49 +1100 Subject: [PATCH 1/5] test_runner: fix afterEach not running on test failures test_runner: fix afterEach not running on test failures --- lib/internal/test_runner/test.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 75a3a1558924b1..24dbc75a2abb1b 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -482,8 +482,9 @@ class Test extends AsyncResource { return; } + const { args, ctx } = this.getRunArgs(); + try { - const { args, ctx } = this.getRunArgs(); if (this.parent?.hooks.beforeEach.length > 0) { await this.parent[kRunHook]('beforeEach', { args, ctx }); } @@ -518,10 +519,6 @@ class Test extends AsyncResource { return; } - if (this.parent?.hooks.afterEach.length > 0) { - await this.parent[kRunHook]('afterEach', { args, ctx }); - } - this.pass(); } catch (err) { if (isTestFailureError(err)) { @@ -533,6 +530,12 @@ class Test extends AsyncResource { } else { this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure)); } + } finally { + const afterEach = this.parent?.hooks?.afterEach || []; + const shouldRunAfterEach = afterEach.length > 0; + if (shouldRunAfterEach) { + await this.parent[kRunHook]('afterEach', { args, ctx }); + } } // Clean up the test. Then, try to report the results and execute any From 30268cde9df3939ffec70cfb71334f85ed8bfe33 Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Sat, 5 Nov 2022 20:46:53 +0200 Subject: [PATCH 2/5] fix --- lib/internal/test_runner/test.js | 27 ++++---- lib/internal/util.js | 2 +- test/message/test_runner_describe_it.out | 2 - test/message/test_runner_hooks.js | 15 ++++- test/message/test_runner_hooks.out | 78 ++++++++++++++++++++++-- test/message/test_runner_output.out | 2 - 6 files changed, 103 insertions(+), 23 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 24dbc75a2abb1b..ac595efc9e94d6 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 { @@ -483,6 +484,11 @@ class Test extends AsyncResource { } const { args, ctx } = this.getRunArgs(); + const afterEach = runOnce(async () => { + if (this.parent?.hooks.afterEach.length > 0) { + await this.parent[kRunHook]('afterEach', { args, ctx }); + } + }); try { if (this.parent?.hooks.beforeEach.length > 0) { @@ -519,8 +525,10 @@ class Test extends AsyncResource { return; } + await afterEach(); this.pass(); } catch (err) { + await afterEach(); if (isTestFailureError(err)) { if (err.failureType === kTestTimeoutFailure) { this.cancel(err); @@ -530,12 +538,6 @@ class Test extends AsyncResource { } else { this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure)); } - } finally { - const afterEach = this.parent?.hooks?.afterEach || []; - const shouldRunAfterEach = afterEach.length > 0; - if (shouldRunAfterEach) { - await this.parent[kRunHook]('afterEach', { args, ctx }); - } } // Clean up the test. Then, try to report the results and execute any @@ -731,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; @@ -742,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); @@ -756,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) { + await afterEach(); if (isTestFailureError(err)) { this.fail(err); } else { diff --git a/lib/internal/util.js b/lib/internal/util.js index 76d3fd9122383e..4f74f912936611 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -453,7 +453,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..32e1cda1a4f626 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,12 @@ describe('afterEach throws', () => { it('2', () => {}); }); +describe('afterEach when test fails', () => { + afterEach(common.mustCall(2)); + 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 +117,10 @@ 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', () => {}); +}); diff --git a/test/message/test_runner_hooks.out b/test/message/test_runner_hooks.out index 57008a44bbb141..a0eda2665861dc 100644 --- a/test/message/test_runner_hooks.out +++ b/test/message/test_runner_hooks.out @@ -189,6 +189,39 @@ 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: test hooks # Subtest: 1 ok 1 - 1 @@ -217,7 +250,7 @@ not ok 5 - afterEach throws duration_ms: * ... 1..3 -ok 6 - test hooks +ok 7 - test hooks --- duration_ms: * ... @@ -261,7 +294,7 @@ ok 6 - test hooks * ... 1..2 -not ok 7 - t.beforeEach throws +not ok 8 - t.beforeEach throws --- duration_ms: * failureType: 'subtestsFailed' @@ -308,17 +341,50 @@ not ok 7 - t.beforeEach throws * ... 1..2 -not ok 8 - t.afterEach throws +not ok 9 - t.afterEach throws --- duration_ms: * failureType: 'subtestsFailed' error: '2 subtests failed' code: 'ERR_TEST_FAILURE' ... -1..8 -# tests 8 +# 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 10 - afterEach when test fails + --- + duration_ms: * + failureType: 'subtestsFailed' + error: '1 subtest failed' + code: 'ERR_TEST_FAILURE' + ... +1..10 +# tests 10 # pass 2 -# fail 6 +# fail 8 # 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 From c17bb82dfe655ffd3ac7a93e4cf4cdbe91ab5bf1 Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Sat, 5 Nov 2022 23:42:08 +0200 Subject: [PATCH 3/5] CR --- lib/internal/test_runner/test.js | 4 +- test/message/test_runner_hooks.js | 12 ++++ test/message/test_runner_hooks.out | 108 +++++++++++++++++++++++++++-- 3 files changed, 115 insertions(+), 9 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index ac595efc9e94d6..68a1ded1c52b81 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -528,7 +528,7 @@ class Test extends AsyncResource { await afterEach(); this.pass(); } catch (err) { - await afterEach(); + await PromisePrototypeThen(PromiseResolve(afterEach()), noop, noop); if (isTestFailureError(err)) { if (err.failureType === kTestTimeoutFailure) { this.cancel(err); @@ -767,7 +767,7 @@ class Suite extends Test { this.pass(); } catch (err) { - await afterEach(); + await PromisePrototypeThen(PromiseResolve(afterEach()), noop, noop); if (isTestFailureError(err)) { this.fail(err); } else { diff --git a/test/message/test_runner_hooks.js b/test/message/test_runner_hooks.js index 32e1cda1a4f626..e5e6bdc9d17ffa 100644 --- a/test/message/test_runner_hooks.js +++ b/test/message/test_runner_hooks.js @@ -82,6 +82,12 @@ describe('afterEach when test fails', () => { 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)); @@ -124,3 +130,9 @@ test('afterEach when test fails', async (t) => { 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 a0eda2665861dc..e0b3e91b8c1376 100644 --- a/test/message/test_runner_hooks.out +++ b/test/message/test_runner_hooks.out @@ -222,6 +222,53 @@ not ok 6 - afterEach when test fails 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 @@ -250,7 +297,7 @@ not ok 6 - afterEach when test fails duration_ms: * ... 1..3 -ok 7 - test hooks +ok 8 - test hooks --- duration_ms: * ... @@ -294,7 +341,7 @@ ok 7 - test hooks * ... 1..2 -not ok 8 - t.beforeEach throws +not ok 9 - t.beforeEach throws --- duration_ms: * failureType: 'subtestsFailed' @@ -341,7 +388,7 @@ not ok 8 - t.beforeEach throws * ... 1..2 -not ok 9 - t.afterEach throws +not ok 10 - t.afterEach throws --- duration_ms: * failureType: 'subtestsFailed' @@ -374,17 +421,64 @@ not ok 9 - t.afterEach throws duration_ms: * ... 1..2 -not ok 10 - afterEach when test fails +not ok 11 - afterEach when test fails --- duration_ms: * failureType: 'subtestsFailed' error: '1 subtest failed' code: 'ERR_TEST_FAILURE' ... -1..10 -# tests 10 +# 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..12 +# tests 12 # pass 2 -# fail 8 +# fail 10 # cancelled 0 # skipped 0 # todo 0 From a9defa08c8302ccd2d0872159238b90060b5a665 Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Sun, 6 Nov 2022 08:44:10 +0200 Subject: [PATCH 4/5] CR --- lib/internal/test_runner/test.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 68a1ded1c52b81..4109c39903cf88 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -528,7 +528,8 @@ class Test extends AsyncResource { await afterEach(); this.pass(); } catch (err) { - await PromisePrototypeThen(PromiseResolve(afterEach()), noop, noop); + // eslint-disable-next-line no-empty + try { await afterEach(); } catch {} if (isTestFailureError(err)) { if (err.failureType === kTestTimeoutFailure) { this.cancel(err); @@ -767,7 +768,8 @@ class Suite extends Test { this.pass(); } catch (err) { - await PromisePrototypeThen(PromiseResolve(afterEach()), noop, noop); + // eslint-disable-next-line no-empty + try { await afterEach(); } catch {} if (isTestFailureError(err)) { this.fail(err); } else { From 5880f19d59b85c74945929c04954e6b6c770210a Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Sun, 6 Nov 2022 22:29:52 +0200 Subject: [PATCH 5/5] CR --- lib/internal/test_runner/test.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 4109c39903cf88..6d30f3aaa6686a 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -528,8 +528,7 @@ class Test extends AsyncResource { await afterEach(); this.pass(); } catch (err) { - // eslint-disable-next-line no-empty - try { await afterEach(); } catch {} + try { await afterEach(); } catch { /* test is already failing, let's the error */ } if (isTestFailureError(err)) { if (err.failureType === kTestTimeoutFailure) { this.cancel(err); @@ -768,8 +767,7 @@ class Suite extends Test { this.pass(); } catch (err) { - // eslint-disable-next-line no-empty - try { await afterEach(); } catch {} + try { await afterEach(); } catch { /* test is already failing, let's the error */ } if (isTestFailureError(err)) { this.fail(err); } else {