From 21a9e6d4ee853d888270c3c90801c01fe91eaccf Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Tue, 18 Jul 2023 17:21:41 +0300 Subject: [PATCH 01/12] test_runner: call abort on test finish --- lib/internal/test_runner/test.js | 10 ++ .../aborts/failed-test-still-call-abort.js | 25 ++++ .../successful-test-still-call-abort.js | 23 ++++ test/parallel/test-runner-run.mjs | 116 ++++++++++++++---- 4 files changed, 148 insertions(+), 26 deletions(-) create mode 100644 test/fixtures/test-runner/aborts/failed-test-still-call-abort.js create mode 100644 test/fixtures/test-runner/aborts/successful-test-still-call-abort.js diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index f5cc0fb98f6271..cc56eec948bd85 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -586,10 +586,13 @@ class Test extends AsyncResource { return; } + this.#abortController.abort(); + await afterEach(); await after(); this.pass(); } catch (err) { + this.#abortController.abort(); try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ } try { await after(); } catch { /* Ignore error. */ } if (isTestFailureError(err)) { @@ -743,6 +746,11 @@ class Test extends AsyncResource { this.parent.reportStarted(); this.reporter.start(this.nesting, kFilename, this.name); } + + recreateAbortController() { + this.#abortController = new AbortController(); + this.signal = this.#abortController.signal; + } } class TestHook extends Test { @@ -765,6 +773,8 @@ class TestHook extends Test { return true; } postRun() { + // Need to recreate the abort controller because we abort each time in the end + super.recreateAbortController(); } } diff --git a/test/fixtures/test-runner/aborts/failed-test-still-call-abort.js b/test/fixtures/test-runner/aborts/failed-test-still-call-abort.js new file mode 100644 index 00000000000000..2b23f27d94556f --- /dev/null +++ b/test/fixtures/test-runner/aborts/failed-test-still-call-abort.js @@ -0,0 +1,25 @@ +const {test, afterEach} = require('node:test'); +const assert = require('node:assert'); + +let testCount = 0; +let signal; + +afterEach(() => { + testCount++; + assert.equal(signal.aborted, true); + + console.log(`abort called for test ${testCount}`) +}); + +test("sync", (t) => { + signal = t.signal; + assert.equal(signal.aborted, false); + throw new Error('failing the sync test'); +}); + +test("async", async (t) => { + await null; + signal = t.signal; + assert.equal(signal.aborted, false); + throw new Error('failing the async test'); +}); \ No newline at end of file diff --git a/test/fixtures/test-runner/aborts/successful-test-still-call-abort.js b/test/fixtures/test-runner/aborts/successful-test-still-call-abort.js new file mode 100644 index 00000000000000..58a509e7a278c0 --- /dev/null +++ b/test/fixtures/test-runner/aborts/successful-test-still-call-abort.js @@ -0,0 +1,23 @@ +const {test, afterEach} = require('node:test'); +const assert = require('node:assert'); + +let testCount = 0; +let signal; + +afterEach(() => { + testCount++; + assert.equal(signal.aborted, true); + + console.log(`abort called for test ${testCount}`) +}); + +test("sync", (t) => { + signal = t.signal; + assert.equal(signal.aborted, false); +}); + +test("async", async (t) => { + await null; + signal = t.signal; + assert.equal(signal.aborted, false); +}); \ No newline at end of file diff --git a/test/parallel/test-runner-run.mjs b/test/parallel/test-runner-run.mjs index b5c41b8ae925de..59b1f3e1ee478f 100644 --- a/test/parallel/test-runner-run.mjs +++ b/test/parallel/test-runner-run.mjs @@ -14,7 +14,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { stream.on('test:fail', common.mustNotCall()); stream.on('test:pass', common.mustNotCall()); // eslint-disable-next-line no-unused-vars - for await (const _ of stream); + for await (const _ of stream) ; }); it('should fail with non existing file', async () => { @@ -22,7 +22,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { stream.on('test:fail', common.mustCall(1)); stream.on('test:pass', common.mustNotCall()); // eslint-disable-next-line no-unused-vars - for await (const _ of stream); + for await (const _ of stream) ; }); it('should succeed with a file', async () => { @@ -30,7 +30,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { stream.on('test:fail', common.mustNotCall()); stream.on('test:pass', common.mustCall(1)); // eslint-disable-next-line no-unused-vars - for await (const _ of stream); + for await (const _ of stream) ; }); it('should run same file twice', async () => { @@ -38,7 +38,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { stream.on('test:fail', common.mustNotCall()); stream.on('test:pass', common.mustCall(2)); // eslint-disable-next-line no-unused-vars - for await (const _ of stream); + for await (const _ of stream) ; }); it('should run a failed test', async () => { @@ -46,22 +46,25 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { stream.on('test:fail', common.mustCall(1)); stream.on('test:pass', common.mustNotCall()); // eslint-disable-next-line no-unused-vars - for await (const _ of stream); + for await (const _ of stream) ; }); it('should support timeout', async () => { - const stream = run({ timeout: 50, files: [ - fixtures.path('test-runner', 'never_ending_sync.js'), - fixtures.path('test-runner', 'never_ending_async.js'), - ] }); + const stream = run({ + timeout: 50, files: [ + fixtures.path('test-runner', 'never_ending_sync.js'), + fixtures.path('test-runner', 'never_ending_async.js'), + ] + }); stream.on('test:fail', common.mustCall(2)); stream.on('test:pass', common.mustNotCall()); // eslint-disable-next-line no-unused-vars - for await (const _ of stream); + for await (const _ of stream) ; }); it('should validate files', async () => { - [Symbol(), {}, () => {}, 0, 1, 0n, 1n, '', '1', Promise.resolve([]), true, false] + [Symbol(), {}, () => { + }, 0, 1, 0n, 1n, '', '1', Promise.resolve([]), true, false] .forEach((files) => assert.throws(() => run({ files }), { code: 'ERR_INVALID_ARG_TYPE' })); @@ -118,21 +121,6 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { assert.strictEqual(result[5], 'ok 2 - this should be executed\n'); }); - it('should stop watch mode when abortSignal aborts', async () => { - const controller = new AbortController(); - const result = await run({ files: [join(testFixtures, 'test/random.cjs')], watch: true, signal: controller.signal }) - .compose(async function* (source) { - for await (const chunk of source) { - if (chunk.type === 'test:pass') { - controller.abort(); - yield chunk.data.name; - } - } - }) - .toArray(); - assert.deepStrictEqual(result, ['this should pass']); - }); - it('should emit "test:watch:drained" event on watch mode', async () => { const controller = new AbortController(); await run({ files: [join(testFixtures, 'test/random.cjs')], watch: true, signal: controller.signal }) @@ -143,6 +131,82 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { }); }); + describe('AbortSignal', () => { + it('should stop watch mode when abortSignal aborts', async () => { + const controller = new AbortController(); + const result = await run({ + files: [join(testFixtures, 'test/random.cjs')], + watch: true, + signal: controller.signal + }) + .compose(async function* (source) { + for await (const chunk of source) { + if (chunk.type === 'test:pass') { + controller.abort(); + yield chunk.data.name; + } + } + }) + .toArray(); + assert.deepStrictEqual(result, ['this should pass']); + }); + + it('should abort when test succeeded', async () => { + const stream = run({ + files: [fixtures.path('test-runner', 'aborts', 'successful-test-still-call-abort.js')] + }); + + let passedTestCount = 0; + let failedTestCount = 0; + + let output = ''; + for await (const data of stream) { + if (data.type === 'test:stdout') { + output += data.data.message.toString(); + } + if (data.type === 'test:fail') { + failedTestCount++; + } + if (data.type === 'test:pass') { + passedTestCount++; + } + } + + assert.match(output, /abort called for test 1/); + assert.match(output, /abort called for test 2/); + assert.strictEqual(failedTestCount, 0, new Error('no tests should fail')); + assert.strictEqual(passedTestCount, 2); + }); + + it('should abort when test failed', async () => { + const stream = run({ + files: [fixtures.path('test-runner', 'aborts', 'failed-test-still-call-abort.js')] + }); + + let passedTestCount = 0; + let failedTestCount = 0; + + let output = ''; + for await (const data of stream) { + if (data.type === 'test:stdout') { + output += data.data.message.toString(); + } + if (data.type === 'test:fail') { + failedTestCount++; + } + if (data.type === 'test:pass') { + passedTestCount++; + } + } + + assert.match(output, /abort called for test 1/); + assert.match(output, /abort called for test 2/); + assert.strictEqual(passedTestCount, 0, new Error('no tests should pass')); + assert.strictEqual(failedTestCount, 2); + }); + + }); + describe('sharding', () => { const shardsTestsFixtures = fixtures.path('test-runner', 'shards'); const shardsTestsFiles = [ From 0ed4cb409d17676ba6ae4a07b44fabd3d270d871 Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Tue, 18 Jul 2023 17:22:16 +0300 Subject: [PATCH 02/12] format --- .../fixtures/test-runner/aborts/failed-test-still-call-abort.js | 2 +- .../test-runner/aborts/successful-test-still-call-abort.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/fixtures/test-runner/aborts/failed-test-still-call-abort.js b/test/fixtures/test-runner/aborts/failed-test-still-call-abort.js index 2b23f27d94556f..dc2bd645b319aa 100644 --- a/test/fixtures/test-runner/aborts/failed-test-still-call-abort.js +++ b/test/fixtures/test-runner/aborts/failed-test-still-call-abort.js @@ -22,4 +22,4 @@ test("async", async (t) => { signal = t.signal; assert.equal(signal.aborted, false); throw new Error('failing the async test'); -}); \ No newline at end of file +}); diff --git a/test/fixtures/test-runner/aborts/successful-test-still-call-abort.js b/test/fixtures/test-runner/aborts/successful-test-still-call-abort.js index 58a509e7a278c0..fabafd919d9ac9 100644 --- a/test/fixtures/test-runner/aborts/successful-test-still-call-abort.js +++ b/test/fixtures/test-runner/aborts/successful-test-still-call-abort.js @@ -20,4 +20,4 @@ test("async", async (t) => { await null; signal = t.signal; assert.equal(signal.aborted, false); -}); \ No newline at end of file +}); From c4710a80ecf584396238fab34f416f0aaf8961af Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Tue, 18 Jul 2023 17:23:15 +0300 Subject: [PATCH 03/12] format --- test/parallel/test-runner-run.mjs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/parallel/test-runner-run.mjs b/test/parallel/test-runner-run.mjs index 59b1f3e1ee478f..ea9104034fabb7 100644 --- a/test/parallel/test-runner-run.mjs +++ b/test/parallel/test-runner-run.mjs @@ -14,7 +14,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { stream.on('test:fail', common.mustNotCall()); stream.on('test:pass', common.mustNotCall()); // eslint-disable-next-line no-unused-vars - for await (const _ of stream) ; + for await (const _ of stream); }); it('should fail with non existing file', async () => { @@ -22,7 +22,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { stream.on('test:fail', common.mustCall(1)); stream.on('test:pass', common.mustNotCall()); // eslint-disable-next-line no-unused-vars - for await (const _ of stream) ; + for await (const _ of stream); }); it('should succeed with a file', async () => { @@ -30,7 +30,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { stream.on('test:fail', common.mustNotCall()); stream.on('test:pass', common.mustCall(1)); // eslint-disable-next-line no-unused-vars - for await (const _ of stream) ; + for await (const _ of stream); }); it('should run same file twice', async () => { @@ -38,7 +38,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { stream.on('test:fail', common.mustNotCall()); stream.on('test:pass', common.mustCall(2)); // eslint-disable-next-line no-unused-vars - for await (const _ of stream) ; + for await (const _ of stream); }); it('should run a failed test', async () => { @@ -46,7 +46,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { stream.on('test:fail', common.mustCall(1)); stream.on('test:pass', common.mustNotCall()); // eslint-disable-next-line no-unused-vars - for await (const _ of stream) ; + for await (const _ of stream); }); it('should support timeout', async () => { @@ -59,7 +59,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { stream.on('test:fail', common.mustCall(2)); stream.on('test:pass', common.mustNotCall()); // eslint-disable-next-line no-unused-vars - for await (const _ of stream) ; + for await (const _ of stream); }); it('should validate files', async () => { @@ -320,7 +320,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { executedTestFiles.push(passedTest.file); }); // eslint-disable-next-line no-unused-vars - for await (const _ of stream) ; + for await (const _ of stream); assert.deepStrictEqual(executedTestFiles, [ join(shardsTestsFixtures, 'a.cjs'), @@ -350,7 +350,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { await Promise.all(testStreams.map(async (stream) => { // eslint-disable-next-line no-unused-vars - for await (const _ of stream) ; + for await (const _ of stream); })); assert.deepStrictEqual(executedTestFiles, [...new Set(executedTestFiles)]); @@ -378,7 +378,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { await Promise.all(testStreams.map(async (stream) => { // eslint-disable-next-line no-unused-vars - for await (const _ of stream) ; + for await (const _ of stream); })); assert.deepStrictEqual(executedTestFiles.sort(), [...shardsTestsFiles].sort()); From eeabcf55cad59ad699c76996b3e5befc3cca85ba Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Tue, 18 Jul 2023 18:31:23 +0300 Subject: [PATCH 04/12] do not recreate on each test --- lib/internal/test_runner/test.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index cc56eec948bd85..6c4411ccb54f93 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -586,13 +586,21 @@ class Test extends AsyncResource { return; } - this.#abortController.abort(); + // Do not run for hooks and root test as hooks instance are shared between tests suite so aborting them will + // abort cause them to not run for further tests. + if (this.parent !== null) { + this.#abortController.abort(); + } await afterEach(); await after(); this.pass(); } catch (err) { - this.#abortController.abort(); + // Do not run for hooks and root test as hooks instance are shared between tests suite so aborting them will + // abort cause them to not run for further tests. + if (this.parent !== null) { + this.#abortController.abort(); + } try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ } try { await after(); } catch { /* Ignore error. */ } if (isTestFailureError(err)) { @@ -746,11 +754,6 @@ class Test extends AsyncResource { this.parent.reportStarted(); this.reporter.start(this.nesting, kFilename, this.name); } - - recreateAbortController() { - this.#abortController = new AbortController(); - this.signal = this.#abortController.signal; - } } class TestHook extends Test { @@ -774,7 +777,7 @@ class TestHook extends Test { } postRun() { // Need to recreate the abort controller because we abort each time in the end - super.recreateAbortController(); + // super.recreateAbortController(); } } From f93f38ef89c8dd908afab0884bbdd0e1f54bac36 Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Tue, 18 Jul 2023 18:37:19 +0300 Subject: [PATCH 05/12] cleanup --- lib/internal/test_runner/test.js | 2 -- test/parallel/test-runner-run.mjs | 13 +++++-------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 6c4411ccb54f93..06adf1b6840f61 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -776,8 +776,6 @@ class TestHook extends Test { return true; } postRun() { - // Need to recreate the abort controller because we abort each time in the end - // super.recreateAbortController(); } } diff --git a/test/parallel/test-runner-run.mjs b/test/parallel/test-runner-run.mjs index ea9104034fabb7..08f5da57859a16 100644 --- a/test/parallel/test-runner-run.mjs +++ b/test/parallel/test-runner-run.mjs @@ -50,12 +50,10 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { }); it('should support timeout', async () => { - const stream = run({ - timeout: 50, files: [ - fixtures.path('test-runner', 'never_ending_sync.js'), - fixtures.path('test-runner', 'never_ending_async.js'), - ] - }); + const stream = run({ timeout: 50, files: [ + fixtures.path('test-runner', 'never_ending_sync.js'), + fixtures.path('test-runner', 'never_ending_async.js'), + ] }); stream.on('test:fail', common.mustCall(2)); stream.on('test:pass', common.mustNotCall()); // eslint-disable-next-line no-unused-vars @@ -63,8 +61,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { }); it('should validate files', async () => { - [Symbol(), {}, () => { - }, 0, 1, 0n, 1n, '', '1', Promise.resolve([]), true, false] + [Symbol(), {}, () => {}, 0, 1, 0n, 1n, '', '1', Promise.resolve([]), true, false] .forEach((files) => assert.throws(() => run({ files }), { code: 'ERR_INVALID_ARG_TYPE' })); From 5017fe5e676cb0a07ba275e939a7b206c7dc399d Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Tue, 18 Jul 2023 18:40:39 +0300 Subject: [PATCH 06/12] cleanup --- test/parallel/test-runner-run.mjs | 55 +++++++++++++++++++++++-------- 1 file changed, 42 insertions(+), 13 deletions(-) diff --git a/test/parallel/test-runner-run.mjs b/test/parallel/test-runner-run.mjs index 08f5da57859a16..fb14960e233be0 100644 --- a/test/parallel/test-runner-run.mjs +++ b/test/parallel/test-runner-run.mjs @@ -118,14 +118,32 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { assert.strictEqual(result[5], 'ok 2 - this should be executed\n'); }); - it('should emit "test:watch:drained" event on watch mode', async () => { + it('should stop watch mode when abortSignal aborts', async () => { const controller = new AbortController(); - await run({ files: [join(testFixtures, 'test/random.cjs')], watch: true, signal: controller.signal }) - .on('data', function({ type }) { - if (type === 'test:watch:drained') { - controller.abort(); + const result = await run({ files: [join(testFixtures, 'test/random.cjs')], watch: true, signal: controller.signal }) + .compose(async function* (source) { + for await (const chunk of source) { + if (chunk.type === 'test:pass') { + controller.abort(); + yield chunk.data.name; + } } - }); + }) + .toArray(); + assert.deepStrictEqual(result, ['this should pass']); + }); + + it('should emit "test:watch:drained" event on watch mode', async () => { + const controller = new AbortController(); + await run({ + files: [join(testFixtures, 'test/random.cjs')], + watch: true, + signal: controller.signal, + }).on('data', function({ type }) { + if (type === 'test:watch:drained') { + controller.abort(); + } + }); }); describe('AbortSignal', () => { @@ -134,7 +152,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { const result = await run({ files: [join(testFixtures, 'test/random.cjs')], watch: true, - signal: controller.signal + signal: controller.signal, }) .compose(async function* (source) { for await (const chunk of source) { @@ -150,7 +168,13 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { it('should abort when test succeeded', async () => { const stream = run({ - files: [fixtures.path('test-runner', 'aborts', 'successful-test-still-call-abort.js')] + files: [ + fixtures.path( + 'test-runner', + 'aborts', + 'successful-test-still-call-abort.js' + ), + ], }); let passedTestCount = 0; @@ -177,7 +201,13 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { it('should abort when test failed', async () => { const stream = run({ - files: [fixtures.path('test-runner', 'aborts', 'failed-test-still-call-abort.js')] + files: [ + fixtures.path( + 'test-runner', + 'aborts', + 'failed-test-still-call-abort.js' + ), + ], }); let passedTestCount = 0; @@ -201,7 +231,6 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { assert.strictEqual(passedTestCount, 0, new Error('no tests should pass')); assert.strictEqual(failedTestCount, 2); }); - }); describe('sharding', () => { @@ -317,7 +346,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { executedTestFiles.push(passedTest.file); }); // eslint-disable-next-line no-unused-vars - for await (const _ of stream); + for await (const _ of stream) ; assert.deepStrictEqual(executedTestFiles, [ join(shardsTestsFixtures, 'a.cjs'), @@ -347,7 +376,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { await Promise.all(testStreams.map(async (stream) => { // eslint-disable-next-line no-unused-vars - for await (const _ of stream); + for await (const _ of stream) ; })); assert.deepStrictEqual(executedTestFiles, [...new Set(executedTestFiles)]); @@ -375,7 +404,7 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { await Promise.all(testStreams.map(async (stream) => { // eslint-disable-next-line no-unused-vars - for await (const _ of stream); + for await (const _ of stream) ; })); assert.deepStrictEqual(executedTestFiles.sort(), [...shardsTestsFiles].sort()); From 58da4d2a68a7a911a3f8e6c28bd30db7a88af537 Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Tue, 18 Jul 2023 18:42:49 +0300 Subject: [PATCH 07/12] cleanup --- test/parallel/test-runner-run.mjs | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/test/parallel/test-runner-run.mjs b/test/parallel/test-runner-run.mjs index fb14960e233be0..7e4a8fbe76753a 100644 --- a/test/parallel/test-runner-run.mjs +++ b/test/parallel/test-runner-run.mjs @@ -118,21 +118,6 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { assert.strictEqual(result[5], 'ok 2 - this should be executed\n'); }); - it('should stop watch mode when abortSignal aborts', async () => { - const controller = new AbortController(); - const result = await run({ files: [join(testFixtures, 'test/random.cjs')], watch: true, signal: controller.signal }) - .compose(async function* (source) { - for await (const chunk of source) { - if (chunk.type === 'test:pass') { - controller.abort(); - yield chunk.data.name; - } - } - }) - .toArray(); - assert.deepStrictEqual(result, ['this should pass']); - }); - it('should emit "test:watch:drained" event on watch mode', async () => { const controller = new AbortController(); await run({ From f5f36b601416084cf294bbc9a7f0cd2deaea8201 Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Wed, 19 Jul 2023 14:02:10 +0300 Subject: [PATCH 08/12] abort after all cleanup --- lib/internal/test_runner/test.js | 17 ++++++----------- .../aborts/failed-test-still-call-abort.js | 6 +++--- .../successful-test-still-call-abort.js | 6 +++--- .../aborts/wait-for-abort-helper.js | 19 +++++++++++++++++++ 4 files changed, 31 insertions(+), 17 deletions(-) create mode 100644 test/fixtures/test-runner/aborts/wait-for-abort-helper.js diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 06adf1b6840f61..6fea31b684f07b 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -586,21 +586,10 @@ class Test extends AsyncResource { return; } - // Do not run for hooks and root test as hooks instance are shared between tests suite so aborting them will - // abort cause them to not run for further tests. - if (this.parent !== null) { - this.#abortController.abort(); - } - await afterEach(); await after(); this.pass(); } catch (err) { - // Do not run for hooks and root test as hooks instance are shared between tests suite so aborting them will - // abort cause them to not run for further tests. - if (this.parent !== null) { - this.#abortController.abort(); - } try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ } try { await after(); } catch { /* Ignore error. */ } if (isTestFailureError(err)) { @@ -612,6 +601,12 @@ class Test extends AsyncResource { } else { this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure)); } + } finally { + // Do not run for hooks and root test as hooks instance are shared between tests suite so aborting them will + // abort cause them to not run for further tests. + if (this.parent !== null) { + this.#abortController.abort(); + } } // Clean up the test. Then, try to report the results and execute any diff --git a/test/fixtures/test-runner/aborts/failed-test-still-call-abort.js b/test/fixtures/test-runner/aborts/failed-test-still-call-abort.js index dc2bd645b319aa..496d2331422c00 100644 --- a/test/fixtures/test-runner/aborts/failed-test-still-call-abort.js +++ b/test/fixtures/test-runner/aborts/failed-test-still-call-abort.js @@ -1,14 +1,14 @@ const {test, afterEach} = require('node:test'); const assert = require('node:assert'); +const { waitForAbort } = require('./wait-for-abort-helper'); let testCount = 0; let signal; afterEach(() => { - testCount++; - assert.equal(signal.aborted, true); + assert.equal(signal.aborted, false); - console.log(`abort called for test ${testCount}`) + waitForAbort({ testNumber: ++testCount, signal }); }); test("sync", (t) => { diff --git a/test/fixtures/test-runner/aborts/successful-test-still-call-abort.js b/test/fixtures/test-runner/aborts/successful-test-still-call-abort.js index fabafd919d9ac9..4f3879c624de44 100644 --- a/test/fixtures/test-runner/aborts/successful-test-still-call-abort.js +++ b/test/fixtures/test-runner/aborts/successful-test-still-call-abort.js @@ -1,14 +1,14 @@ const {test, afterEach} = require('node:test'); const assert = require('node:assert'); +const {waitForAbort} = require("./wait-for-abort-helper"); let testCount = 0; let signal; afterEach(() => { - testCount++; - assert.equal(signal.aborted, true); + assert.equal(signal.aborted, false); - console.log(`abort called for test ${testCount}`) + waitForAbort({ testNumber: ++testCount, signal }); }); test("sync", (t) => { diff --git a/test/fixtures/test-runner/aborts/wait-for-abort-helper.js b/test/fixtures/test-runner/aborts/wait-for-abort-helper.js new file mode 100644 index 00000000000000..f513063066a1ff --- /dev/null +++ b/test/fixtures/test-runner/aborts/wait-for-abort-helper.js @@ -0,0 +1,19 @@ +module.exports = { + waitForAbort: function ({ testNumber, signal }) { + let retries = 0; + + const interval = setInterval(() => { + retries++; + if(signal.aborted) { + console.log(`abort called for test ${testNumber}`); + clearInterval(interval); + return; + } + + if(retries > 100) { + clearInterval(interval); + throw new Error(`abort was not called for test ${testNumber}`); + } + }, 10); + } +} \ No newline at end of file From 0c7c812a241b5b3deeeea76f174e9e511146d2dc Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Wed, 19 Jul 2023 14:56:15 +0300 Subject: [PATCH 09/12] Update test/fixtures/test-runner/aborts/wait-for-abort-helper.js Co-authored-by: Moshe Atlow --- test/fixtures/test-runner/aborts/wait-for-abort-helper.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fixtures/test-runner/aborts/wait-for-abort-helper.js b/test/fixtures/test-runner/aborts/wait-for-abort-helper.js index f513063066a1ff..89eda7ed9138ca 100644 --- a/test/fixtures/test-runner/aborts/wait-for-abort-helper.js +++ b/test/fixtures/test-runner/aborts/wait-for-abort-helper.js @@ -16,4 +16,4 @@ module.exports = { } }, 10); } -} \ No newline at end of file +} From 42f7308bfaeda116106aa784caae4933c4b3fa21 Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Thu, 20 Jul 2023 09:15:30 +0300 Subject: [PATCH 10/12] Update lib/internal/test_runner/test.js Co-authored-by: Chemi Atlow --- lib/internal/test_runner/test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 6fea31b684f07b..9a333e1457fbc0 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -602,8 +602,8 @@ class Test extends AsyncResource { this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure)); } } finally { - // Do not run for hooks and root test as hooks instance are shared between tests suite so aborting them will - // abort cause them to not run for further tests. + // Do not abort hooks and the root test as hooks instance are shared between tests suite so aborting them will + // cause them to not run for further tests. if (this.parent !== null) { this.#abortController.abort(); } From 976b24fc7415c46c2d134d25446d06525c825d1e Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Thu, 20 Jul 2023 20:54:11 +0300 Subject: [PATCH 11/12] trigger ci --- .eslintrc.js | 6 + .../set-proto-to-null-in-object.js | 140 ++++++++++++++++++ 2 files changed, 146 insertions(+) create mode 100644 tools/eslint-rules/set-proto-to-null-in-object.js diff --git a/.eslintrc.js b/.eslintrc.js index 43e41026b2007e..405eb7d7862e16 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -111,6 +111,12 @@ module.exports = { }, ] }, }, + { + files: ['lib/**/*.js', 'lib/**/*.cjs', 'lib/**/*.mjs'], + rules: { + 'node-core/set-proto-to-null-in-object': 'error', + }, + }, ], rules: { // ESLint built-in rules diff --git a/tools/eslint-rules/set-proto-to-null-in-object.js b/tools/eslint-rules/set-proto-to-null-in-object.js new file mode 100644 index 00000000000000..e1a6af38c032dc --- /dev/null +++ b/tools/eslint-rules/set-proto-to-null-in-object.js @@ -0,0 +1,140 @@ +'use strict'; + +module.exports = { + meta: { + messages: { + error: 'Add `__proto__: null` to object', + }, + fixable: 'code', + }, + create: function(context) { + return { + ObjectExpression(node) { + // Not adding __proto__ to module.exports as it will break a lot of libraries + if (isModuleExportsObject(node) || isModifiedExports(node)) { + return; + } + + const properties = node.properties; + let hasProto = false; + + for (const property of properties) { + + if (!property.key) { + continue; + } + + if (property.key.type === 'Identifier' && property.key.name === '__proto__') { + hasProto = true; + break; + } + + if (property.key.type === 'Literal' && property.key.value === '__proto__') { + hasProto = true; + break; + } + } + + if (hasProto) { + return; + } + + if (properties.length > 0) { + // If the object has properties but no __proto__ property + context.report({ + node, + message: 'Every object must have __proto__: null', + fix: function(fixer) { + // Generate the fix suggestion to add __proto__: null + const sourceCode = context.getSourceCode(); + const firstProperty = properties[0]; + const firstPropertyToken = sourceCode.getFirstToken(firstProperty); + + + const isMultiLine = properties.length === 1 ? + // If the object has only one property, + // it's multiline if the property is not on the same line as the object parenthesis + properties[0].loc.start.line !== node.loc.start.line : + // If the object has more than one property, + // it's multiline if the first and second properties are not on the same line + properties[0].loc.start.line !== properties[1].loc.start.line; + + const fixText = `__proto__: null,${isMultiLine ? '\n' : ' '}`; + + // Insert the fix suggestion before the first property + return fixer.insertTextBefore(firstPropertyToken, fixText); + }, + }); + } + + if (properties.length === 0) { + // If the object is empty and missing __proto__ + context.report({ + node, + message: 'Every empty object must have __proto__: null', + fix: function(fixer) { + // Generate the fix suggestion to create the object with __proto__: null + const fixText = '{ __proto__: null }'; + + // Replace the empty object with the fix suggestion + return fixer.replaceText(node, fixText); + }, + }); + } + }, + }; + }, +}; + +// Helper function to check if the object is `module.exports` +function isModuleExportsObject(node) { + return ( + node.parent && + node.parent.type === 'AssignmentExpression' && + node.parent.left && + node.parent.left.type === 'MemberExpression' && + node.parent.left.object && + node.parent.left.object.name === 'module' && + node.parent.left.property && + node.parent.left.property.name === 'exports' + ); +} + +function isModifiedExports(node) { + return ( + node.parent && + (isObjectAssignCall(node.parent) || isObjectDefinePropertiesCall(node.parent)) + ); +} + +// Helper function to check if the node represents an ObjectAssign call +function isObjectAssignCall(node) { + return ( + node.type === 'CallExpression' && + node.callee && + node.callee.type === 'Identifier' && + node.callee.name === 'ObjectAssign' && + node.arguments.length > 1 && + node.arguments.some((arg) => + arg.type === 'MemberExpression' && + arg.object.name === 'module' && + arg.property.name === 'exports', + ) + ); +} + +// Helper function to check if the node represents an ObjectDefineProperties call +function isObjectDefinePropertiesCall(node) { + return ( + node.type === 'CallExpression' && + node.callee && + node.callee.type === 'Identifier' && + node.callee.name === 'ObjectDefineProperties' && + node.arguments.length > 1 && + node.arguments.some((arg) => + arg.type === 'MemberExpression' && + arg.object.name === 'module' && + arg.property.name === 'exports', + ) + ); +} From e511f95a9436755c51ae93ea494da5c9869c9daa Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Thu, 20 Jul 2023 20:55:03 +0300 Subject: [PATCH 12/12] Revert "trigger ci" This reverts commit 976b24fc7415c46c2d134d25446d06525c825d1e. --- .eslintrc.js | 6 - .../set-proto-to-null-in-object.js | 140 ------------------ 2 files changed, 146 deletions(-) delete mode 100644 tools/eslint-rules/set-proto-to-null-in-object.js diff --git a/.eslintrc.js b/.eslintrc.js index 405eb7d7862e16..43e41026b2007e 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -111,12 +111,6 @@ module.exports = { }, ] }, }, - { - files: ['lib/**/*.js', 'lib/**/*.cjs', 'lib/**/*.mjs'], - rules: { - 'node-core/set-proto-to-null-in-object': 'error', - }, - }, ], rules: { // ESLint built-in rules diff --git a/tools/eslint-rules/set-proto-to-null-in-object.js b/tools/eslint-rules/set-proto-to-null-in-object.js deleted file mode 100644 index e1a6af38c032dc..00000000000000 --- a/tools/eslint-rules/set-proto-to-null-in-object.js +++ /dev/null @@ -1,140 +0,0 @@ -'use strict'; - -module.exports = { - meta: { - messages: { - error: 'Add `__proto__: null` to object', - }, - fixable: 'code', - }, - create: function(context) { - return { - ObjectExpression(node) { - // Not adding __proto__ to module.exports as it will break a lot of libraries - if (isModuleExportsObject(node) || isModifiedExports(node)) { - return; - } - - const properties = node.properties; - let hasProto = false; - - for (const property of properties) { - - if (!property.key) { - continue; - } - - if (property.key.type === 'Identifier' && property.key.name === '__proto__') { - hasProto = true; - break; - } - - if (property.key.type === 'Literal' && property.key.value === '__proto__') { - hasProto = true; - break; - } - } - - if (hasProto) { - return; - } - - if (properties.length > 0) { - // If the object has properties but no __proto__ property - context.report({ - node, - message: 'Every object must have __proto__: null', - fix: function(fixer) { - // Generate the fix suggestion to add __proto__: null - const sourceCode = context.getSourceCode(); - const firstProperty = properties[0]; - const firstPropertyToken = sourceCode.getFirstToken(firstProperty); - - - const isMultiLine = properties.length === 1 ? - // If the object has only one property, - // it's multiline if the property is not on the same line as the object parenthesis - properties[0].loc.start.line !== node.loc.start.line : - // If the object has more than one property, - // it's multiline if the first and second properties are not on the same line - properties[0].loc.start.line !== properties[1].loc.start.line; - - const fixText = `__proto__: null,${isMultiLine ? '\n' : ' '}`; - - // Insert the fix suggestion before the first property - return fixer.insertTextBefore(firstPropertyToken, fixText); - }, - }); - } - - if (properties.length === 0) { - // If the object is empty and missing __proto__ - context.report({ - node, - message: 'Every empty object must have __proto__: null', - fix: function(fixer) { - // Generate the fix suggestion to create the object with __proto__: null - const fixText = '{ __proto__: null }'; - - // Replace the empty object with the fix suggestion - return fixer.replaceText(node, fixText); - }, - }); - } - }, - }; - }, -}; - -// Helper function to check if the object is `module.exports` -function isModuleExportsObject(node) { - return ( - node.parent && - node.parent.type === 'AssignmentExpression' && - node.parent.left && - node.parent.left.type === 'MemberExpression' && - node.parent.left.object && - node.parent.left.object.name === 'module' && - node.parent.left.property && - node.parent.left.property.name === 'exports' - ); -} - -function isModifiedExports(node) { - return ( - node.parent && - (isObjectAssignCall(node.parent) || isObjectDefinePropertiesCall(node.parent)) - ); -} - -// Helper function to check if the node represents an ObjectAssign call -function isObjectAssignCall(node) { - return ( - node.type === 'CallExpression' && - node.callee && - node.callee.type === 'Identifier' && - node.callee.name === 'ObjectAssign' && - node.arguments.length > 1 && - node.arguments.some((arg) => - arg.type === 'MemberExpression' && - arg.object.name === 'module' && - arg.property.name === 'exports', - ) - ); -} - -// Helper function to check if the node represents an ObjectDefineProperties call -function isObjectDefinePropertiesCall(node) { - return ( - node.type === 'CallExpression' && - node.callee && - node.callee.type === 'Identifier' && - node.callee.name === 'ObjectDefineProperties' && - node.arguments.length > 1 && - node.arguments.some((arg) => - arg.type === 'MemberExpression' && - arg.object.name === 'module' && - arg.property.name === 'exports', - ) - ); -}