From e5065e340b1e5a79792af4648225db5ad251f21b Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Thu, 14 Jul 2022 15:05:41 +0300 Subject: [PATCH] test_runner: recieve and pass AbortSignal --- doc/api/test.md | 39 +++- lib/internal/main/test_runner.js | 86 ++++---- lib/internal/test_runner/test.js | 111 +++++++--- test/message/test_runner_abort.js | 47 +++++ test/message/test_runner_abort.out | 249 +++++++++++++++++++++++ test/message/test_runner_abort_suite.js | 27 +++ test/message/test_runner_abort_suite.out | 99 +++++++++ test/message/test_runner_describe_it.js | 6 +- test/message/test_runner_describe_it.out | 2 + 9 files changed, 585 insertions(+), 81 deletions(-) create mode 100644 test/message/test_runner_abort.js create mode 100644 test/message/test_runner_abort.out create mode 100644 test/message/test_runner_abort_suite.js create mode 100644 test/message/test_runner_abort_suite.out diff --git a/doc/api/test.md b/doc/api/test.md index 5970106e11da3e..1b08fa9326c072 100644 --- a/doc/api/test.md +++ b/doc/api/test.md @@ -337,6 +337,7 @@ changes: * `only` {boolean} If truthy, and the test context is configured to run `only` tests, then this test will be run. Otherwise, the test is skipped. **Default:** `false`. + * `signal` {AbortSignal} allows aborting an in-progress test * `skip` {boolean|string} If truthy, the test is skipped. If a string is provided, that string is displayed in the test results as the reason for skipping the test. **Default:** `false`. @@ -385,8 +386,9 @@ test('top level test', async (t) => { does not have a name. * `options` {Object} Configuration options for the suite. supports the same options as `test([name][, options][, fn])` -* `fn` {Function} The function under suite. - a synchronous function declaring all subtests and subsuites. +* `fn` {Function|AsyncFunction} The function under suite + declaring all subtests and subsuites. + The first argument to this function is a [`SuiteContext`][] object. **Default:** A no-op function. * Returns: `undefined`. @@ -483,6 +485,20 @@ test('top level test', (t) => { }); ``` +### `context.signal` + + + +this is an used to signal when the test has been aborted. + +```js +test('top level test', async (t) => { + await fetch('some/uri', { signal: t.signal }); +}); +``` + ### `context.skip([message])` + +An instance of `SuiteContext` is passed to each suite function in order to +interact with the test runner. However, the `SuiteContext` constructor is not +exposed as part of the API. + +### `context.signal` + + + +this is an used to signal when the test has been aborted. + [TAP]: https://testanything.org/ [`--test-only`]: cli.md#--test-only [`--test`]: cli.md#--test +[`SuiteContext`]: #class-suitecontext [`TestContext`]: #class-testcontext [`test()`]: #testname-options-fn [describe options]: #describename-options-fn diff --git a/lib/internal/main/test_runner.js b/lib/internal/main/test_runner.js index ccd869d90968e2..ba3826101c4f39 100644 --- a/lib/internal/main/test_runner.js +++ b/lib/internal/main/test_runner.js @@ -6,9 +6,7 @@ const { ArrayPrototypePush, ArrayPrototypeSlice, ArrayPrototypeSort, - Promise, - PromiseAll, - SafeArrayIterator, + SafePromiseAll, SafeSet, } = primordials; const { @@ -16,7 +14,6 @@ const { } = require('internal/bootstrap/pre_execution'); const { spawn } = require('child_process'); const { readdirSync, statSync } = require('fs'); -const { finished } = require('internal/streams/end-of-stream'); const console = require('internal/console/global'); const { codes: { @@ -30,6 +27,7 @@ const { doesPathMatchFilter, } = require('internal/test_runner/utils'); const { basename, join, resolve } = require('path'); +const { once } = require('events'); const kFilterArgs = ['--test']; prepareMainThreadExecution(false); @@ -102,53 +100,41 @@ function filterExecArgv(arg) { } function runTestFile(path) { - return test(path, () => { - return new Promise((resolve, reject) => { - const args = ArrayPrototypeFilter(process.execArgv, filterExecArgv); - ArrayPrototypePush(args, path); - - const child = spawn(process.execPath, args); - // TODO(cjihrig): Implement a TAP parser to read the child's stdout - // instead of just displaying it all if the child fails. - let stdout = ''; - let stderr = ''; - let err; - - child.on('error', (error) => { - err = error; - }); - - child.stdout.setEncoding('utf8'); - child.stderr.setEncoding('utf8'); - - child.stdout.on('data', (chunk) => { - stdout += chunk; - }); - - child.stderr.on('data', (chunk) => { - stderr += chunk; - }); - - child.once('exit', async (code, signal) => { - if (code !== 0 || signal !== null) { - if (!err) { - await PromiseAll(new SafeArrayIterator([finished(child.stderr), finished(child.stdout)])); - err = new ERR_TEST_FAILURE('test failed', kSubtestsFailed); - err.exitCode = code; - err.signal = signal; - err.stdout = stdout; - err.stderr = stderr; - // The stack will not be useful since the failures came from tests - // in a child process. - err.stack = undefined; - } - - return reject(err); - } - - resolve(); - }); + return test(path, async (t) => { + const args = ArrayPrototypeFilter(process.execArgv, filterExecArgv); + ArrayPrototypePush(args, path); + + const child = spawn(process.execPath, args, { signal: t.signal }); + // TODO(cjihrig): Implement a TAP parser to read the child's stdout + // instead of just displaying it all if the child fails. + let err; + + child.on('error', (error) => { + err = error; }); + + child.stdout.setEncoding('utf8'); + child.stderr.setEncoding('utf8'); + const { 0: { code, signal }, 1: stdout, 2: stderr } = await SafePromiseAll([ + once(child, 'exit', { signal: t.signal }), + child.stdout.toArray({ signal: t.signal }), + child.stderr.toArray({ signal: t.signal }), + ]); + + if (code !== 0 || signal !== null) { + if (!err) { + err = new ERR_TEST_FAILURE('test failed', kSubtestsFailed); + err.exitCode = code; + err.signal = signal; + err.stdout = stdout.join(''); + err.stderr = stderr.join(''); + // The stack will not be useful since the failures came from tests + // in a child process. + err.stack = undefined; + } + + throw err; + } }); } diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 2e1ea1d7a3da5b..0d4a339443064c 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -7,15 +7,19 @@ const { Number, ReflectApply, SafeMap, - PromiseRace, SafePromiseAll, + SafePromiseRace, + Symbol, } = primordials; const { AsyncResource } = require('async_hooks'); +const { once } = require('events'); +const { AbortController } = require('internal/abort_controller'); const { codes: { ERR_TEST_FAILURE, }, kIsNodeError, + AbortError, } = require('internal/errors'); const { getOptionValue } = require('internal/options'); const { TapStream } = require('internal/test_runner/tap_stream'); @@ -25,7 +29,7 @@ const { kEmptyObject, } = require('internal/util'); const { isPromise } = require('internal/util/types'); -const { isUint32 } = require('internal/validators'); +const { isUint32, validateAbortSignal } = require('internal/validators'); const { setTimeout } = require('timers/promises'); const { cpus } = require('os'); const { bigint: hrtime } = process.hrtime; @@ -43,20 +47,19 @@ const testOnlyFlag = !isTestRunner && getOptionValue('--test-only'); // TODO(cjihrig): Use uv_available_parallelism() once it lands. const rootConcurrency = isTestRunner ? cpus().length : 1; +const kShouldAbort = Symbol('kShouldAbort'); -function testTimeout(promise, timeout) { + +function stopTest(timeout, signal) { if (timeout === kDefaultTimeout) { - return promise; - } - return PromiseRace([ - promise, - setTimeout(timeout, null, { ref: false }).then(() => { - throw new ERR_TEST_FAILURE( - `test timed out after ${timeout}ms`, - kTestTimeoutFailure - ); - }), - ]); + return once(signal, 'abort'); + } + return setTimeout(timeout, null, { ref: false, signal }).then(() => { + throw new ERR_TEST_FAILURE( + `test timed out after ${timeout}ms`, + kTestTimeoutFailure + ); + }); } class TestContext { @@ -66,6 +69,10 @@ class TestContext { this.#test = test; } + get signal() { + return this.#test.signal; + } + diagnostic(message) { this.#test.diagnostic(message); } @@ -91,11 +98,14 @@ class TestContext { } class Test extends AsyncResource { + #abortController; + #outerSignal; + constructor(options) { super('Test'); let { fn, name, parent, skip } = options; - const { concurrency, only, timeout, todo } = options; + const { concurrency, only, timeout, todo, signal } = options; if (typeof fn !== 'function') { fn = noop; @@ -148,6 +158,14 @@ class Test extends AsyncResource { fn = noop; } + this.#abortController = new AbortController(); + this.#outerSignal = signal; + this.signal = this.#abortController.signal; + + validateAbortSignal(signal, 'options.signal'); + this.#outerSignal?.addEventListener('abort', this.#abortHandler); + + this.fn = fn; this.name = name; this.parent = parent; @@ -241,7 +259,8 @@ class Test extends AsyncResource { // If this test has already ended, attach this test to the root test so // that the error can be properly reported. - if (this.finished) { + const preventAddingSubtests = this.finished || this.buildPhaseFinished; + if (preventAddingSubtests) { while (parent.parent !== null) { parent = parent.parent; } @@ -253,7 +272,7 @@ class Test extends AsyncResource { parent.waitingOn = test.testNumber; } - if (this.finished) { + if (preventAddingSubtests) { test.startTime = test.startTime || hrtime(); test.fail( new ERR_TEST_FAILURE( @@ -267,18 +286,23 @@ class Test extends AsyncResource { return test; } - cancel() { + #abortHandler = () => { + this.cancel(this.#outerSignal?.reason || new AbortError('The test was aborted')); + }; + + cancel(error) { if (this.endTime !== null) { return; } - this.fail( + this.fail(error || new ERR_TEST_FAILURE( 'test did not finish before its parent and was cancelled', kCancelledByParent ) ); this.cancelled = true; + this.#abortController.abort(); } fail(err) { @@ -329,6 +353,16 @@ class Test extends AsyncResource { return this.run(); } + [kShouldAbort]() { + if (this.signal.aborted) { + return true; + } + if (this.#outerSignal?.aborted) { + this.cancel(this.#outerSignal.reason || new AbortError('The test was aborted')); + return true; + } + } + getRunArgs() { const ctx = new TestContext(this); return { ctx, args: [ctx] }; @@ -338,7 +372,13 @@ class Test extends AsyncResource { this.parent.activeSubtests++; this.startTime = hrtime(); + if (this[kShouldAbort]()) { + this.postRun(); + return; + } + try { + const stopPromise = stopTest(this.timeout, this.signal); const { args, ctx } = this.getRunArgs(); ArrayPrototypeUnshift(args, this.fn, ctx); // Note that if it's not OK to mutate args, we need to first clone it. @@ -354,13 +394,19 @@ class Test extends AsyncResource { 'passed a callback but also returned a Promise', kCallbackAndPromisePresent )); - await testTimeout(ret, this.timeout); + await SafePromiseRace([ret, stopPromise]); } else { - await testTimeout(promise, this.timeout); + await SafePromiseRace([PromiseResolve(promise), stopPromise]); } } else { // This test is synchronous or using Promises. - await testTimeout(ReflectApply(this.runInAsyncScope, this, args), this.timeout); + const promise = ReflectApply(this.runInAsyncScope, this, args); + await SafePromiseRace([PromiseResolve(promise), stopPromise]); + } + + if (this[kShouldAbort]()) { + this.postRun(); + return; } this.pass(); @@ -409,6 +455,8 @@ class Test extends AsyncResource { this.fail(new ERR_TEST_FAILURE(msg, kSubtestsFailed)); } + this.#outerSignal?.removeEventListener('abort', this.#abortHandler); + if (this.parent !== null) { this.parent.activeSubtests--; this.parent.addReadySubtest(this); @@ -476,7 +524,7 @@ class Test extends AsyncResource { class ItTest extends Test { constructor(opt) { super(opt); } // eslint-disable-line no-useless-constructor getRunArgs() { - return { ctx: {}, args: [] }; + return { ctx: { signal: this.signal }, args: [] }; } } class Suite extends Test { @@ -484,12 +532,13 @@ class Suite extends Test { super(options); try { - this.buildSuite = this.runInAsyncScope(this.fn); + const context = { signal: this.signal }; + this.buildSuite = this.runInAsyncScope(this.fn, context, [context]); } catch (err) { this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure)); } this.fn = () => {}; - this.finished = true; // Forbid adding subtests to this suite + this.buildPhaseFinished = true; } start() { @@ -504,8 +553,18 @@ class Suite extends Test { } this.parent.activeSubtests++; this.startTime = hrtime(); + + if (this[kShouldAbort]()) { + this.subtests = []; + this.postRun(); + return; + } + + const stopPromise = stopTest(this.timeout, this.signal); const subtests = this.skipped || this.error ? [] : this.subtests; - await SafePromiseAll(subtests, (subtests) => subtests.start()); + const promise = SafePromiseAll(subtests, (subtests) => subtests.start()); + + await SafePromiseRace([promise, stopPromise]); this.pass(); this.postRun(); } diff --git a/test/message/test_runner_abort.js b/test/message/test_runner_abort.js new file mode 100644 index 00000000000000..adf1e72435d41c --- /dev/null +++ b/test/message/test_runner_abort.js @@ -0,0 +1,47 @@ +// Flags: --no-warnings +'use strict'; +require('../common'); +const test = require('node:test'); + +test('promise timeout signal', { signal: AbortSignal.timeout(1) }, async (t) => { + await Promise.all([ + t.test('ok 1', async () => {}), + t.test('ok 2', () => {}), + t.test('ok 3', { signal: t.signal }, async () => {}), + t.test('ok 4', { signal: t.signal }, () => {}), + t.test('not ok 1', () => new Promise(() => {})), + t.test('not ok 2', (t, done) => {}), + t.test('not ok 3', { signal: t.signal }, () => new Promise(() => {})), + t.test('not ok 4', { signal: t.signal }, (t, done) => {}), + t.test('not ok 5', { signal: t.signal }, (t, done) => { + t.signal.addEventListener('abort', done); + }), + ]); +}); + +test('promise abort signal', { signal: AbortSignal.abort() }, async (t) => { + t.test('should not appear', () => {}); +}); + +test('callback timeout signal', { signal: AbortSignal.timeout(1) }, (t, done) => { + t.test('ok 1', async () => {}); + t.test('ok 2', () => {}); + t.test('ok 3', { signal: t.signal }, async () => {}); + t.test('ok 4', { signal: t.signal }, () => {}); + t.test('not ok 1', () => new Promise(() => {})); + t.test('not ok 2', (t, done) => {}); + t.test('not ok 3', { signal: t.signal }, () => new Promise(() => {})); + t.test('not ok 4', { signal: t.signal }, (t, done) => {}); + t.test('not ok 5', { signal: t.signal }, (t, done) => { + t.signal.addEventListener('abort', done); + }); +}); + +test('callback abort signal', { signal: AbortSignal.abort() }, (t, done) => { + t.test('should not appear', () => {}); +}); + +// AbortSignal.timeout(1) doesn't prevent process from closing +// thus we have to keep the process open to prevent cancelation +// of the entire test tree +setTimeout(() => {}, 1000); diff --git a/test/message/test_runner_abort.out b/test/message/test_runner_abort.out new file mode 100644 index 00000000000000..478ec81c33614e --- /dev/null +++ b/test/message/test_runner_abort.out @@ -0,0 +1,249 @@ +TAP version 13 +# Subtest: promise timeout signal + # Subtest: ok 1 + ok 1 - ok 1 + --- + duration_ms: * + ... + # Subtest: ok 2 + ok 2 - ok 2 + --- + duration_ms: * + ... + # Subtest: ok 3 + ok 3 - ok 3 + --- + duration_ms: * + ... + # Subtest: ok 4 + ok 4 - ok 4 + --- + duration_ms: * + ... + # Subtest: not ok 1 + not ok 5 - not ok 1 + --- + duration_ms: * + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' + ... + # Subtest: not ok 2 + not ok 6 - not ok 2 + --- + duration_ms: * + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' + ... + # Subtest: not ok 3 + not ok 7 - not ok 3 + --- + duration_ms: * + error: 'This operation was aborted' + code: 20 + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + # Subtest: not ok 4 + not ok 8 - not ok 4 + --- + duration_ms: * + error: 'This operation was aborted' + code: 20 + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + # Subtest: not ok 5 + not ok 9 - not ok 5 + --- + duration_ms: * + error: 'This operation was aborted' + code: 20 + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + 1..9 +not ok 1 - promise timeout signal + --- + duration_ms: * + error: 'The operation was aborted due to timeout' + code: 23 + stack: |- + * + * + * + * + ... +# Subtest: promise abort signal +not ok 2 - promise abort signal + --- + duration_ms: * + error: 'This operation was aborted' + code: 20 + stack: |- + * + * + * + * + * + * + * + * + * + ... +# Subtest: callback timeout signal + # Subtest: ok 1 + ok 1 - ok 1 + --- + duration_ms: * + ... + # Subtest: ok 2 + ok 2 - ok 2 + --- + duration_ms: * + ... + # Subtest: ok 3 + ok 3 - ok 3 + --- + duration_ms: * + ... + # Subtest: ok 4 + ok 4 - ok 4 + --- + duration_ms: * + ... + # Subtest: not ok 1 + not ok 5 - not ok 1 + --- + duration_ms: * + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' + ... + # Subtest: not ok 2 + not ok 6 - not ok 2 + --- + duration_ms: * + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' + ... + # Subtest: not ok 3 + not ok 7 - not ok 3 + --- + duration_ms: * + error: 'This operation was aborted' + code: 20 + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + # Subtest: not ok 4 + not ok 8 - not ok 4 + --- + duration_ms: * + error: 'This operation was aborted' + code: 20 + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + # Subtest: not ok 5 + not ok 9 - not ok 5 + --- + duration_ms: * + error: 'This operation was aborted' + code: 20 + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + 1..9 +not ok 3 - callback timeout signal + --- + duration_ms: * + error: 'The operation was aborted due to timeout' + code: 23 + stack: |- + * + * + * + * + ... +# Subtest: callback abort signal +not ok 4 - callback abort signal + --- + duration_ms: * + error: 'This operation was aborted' + code: 20 + stack: |- + * + * + * + * + * + * + * + * + * + ... +1..4 +# tests 4 +# pass 0 +# fail 0 +# cancelled 4 +# skipped 0 +# todo 0 +# duration_ms * \ No newline at end of file diff --git a/test/message/test_runner_abort_suite.js b/test/message/test_runner_abort_suite.js new file mode 100644 index 00000000000000..61415c5cca93f7 --- /dev/null +++ b/test/message/test_runner_abort_suite.js @@ -0,0 +1,27 @@ +// Flags: --no-warnings +'use strict'; +require('../common'); +const { describe, it } = require('node:test'); + +describe('describe timeout signal', { signal: AbortSignal.timeout(1) }, (t) => { + it('ok 1', async () => {}); + it('ok 2', () => {}); + it('ok 3', { signal: t.signal }, async () => {}); + it('ok 4', { signal: t.signal }, () => {}); + it('not ok 1', () => new Promise(() => {})); + it('not ok 2', (done) => {}); + it('not ok 3', { signal: t.signal }, () => new Promise(() => {})); + it('not ok 4', { signal: t.signal }, (done) => {}); + it('not ok 5', { signal: t.signal }, function(done) { + this.signal.addEventListener('abort', done); + }); +}); + +describe('describe abort signal', { signal: AbortSignal.abort() }, () => { + it('should not appear', () => {}); +}); + +// AbortSignal.timeout(1) doesn't prevent process from closing +// thus we have to keep the process open to prevent cancelation +// of the entire test tree +setTimeout(() => {}, 1000); diff --git a/test/message/test_runner_abort_suite.out b/test/message/test_runner_abort_suite.out new file mode 100644 index 00000000000000..38669978406b4f --- /dev/null +++ b/test/message/test_runner_abort_suite.out @@ -0,0 +1,99 @@ +TAP version 13 +# Subtest: describe timeout signal + # Subtest: ok 1 + ok 1 - ok 1 + --- + duration_ms: * + ... + # Subtest: ok 2 + ok 2 - ok 2 + --- + duration_ms: * + ... + # Subtest: ok 3 + ok 3 - ok 3 + --- + duration_ms: * + ... + # Subtest: ok 4 + ok 4 - ok 4 + --- + duration_ms: * + ... + # Subtest: not ok 1 + not ok 5 - not ok 1 + --- + duration_ms: * + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' + ... + # Subtest: not ok 2 + not ok 6 - not ok 2 + --- + duration_ms: * + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' + ... + # Subtest: not ok 3 + not ok 7 - not ok 3 + --- + duration_ms: * + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' + ... + # Subtest: not ok 4 + not ok 8 - not ok 4 + --- + duration_ms: * + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' + ... + # Subtest: not ok 5 + not ok 9 - not ok 5 + --- + duration_ms: * + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' + ... + 1..9 +not ok 1 - describe timeout signal + --- + duration_ms: * + error: 'The operation was aborted due to timeout' + code: 23 + stack: |- + * + * + * + * + ... +# Subtest: describe abort signal +not ok 2 - describe abort signal + --- + duration_ms: * + error: 'This operation was aborted' + code: 20 + stack: |- + * + * + * + * + * + * + * + * + * + ... +1..2 +# tests 2 +# pass 0 +# fail 0 +# cancelled 2 +# skipped 0 +# todo 0 +# duration_ms * diff --git a/test/message/test_runner_describe_it.js b/test/message/test_runner_describe_it.js index 156fecddaf6401..c272fb38a749f6 100644 --- a/test/message/test_runner_describe_it.js +++ b/test/message/test_runner_describe_it.js @@ -225,15 +225,15 @@ it('callback fail', (done) => { }); it('sync t is this in test', function() { - assert.deepStrictEqual(this, {}); + assert.deepStrictEqual(this, { signal: this.signal }); }); it('async t is this in test', async function() { - assert.deepStrictEqual(this, {}); + assert.deepStrictEqual(this, { signal: this.signal }); }); it('callback t is this in test', function(done) { - assert.deepStrictEqual(this, {}); + assert.deepStrictEqual(this, { signal: this.signal }); done(); }); diff --git a/test/message/test_runner_describe_it.out b/test/message/test_runner_describe_it.out index 4913038dbb4db9..7961345b976f73 100644 --- a/test/message/test_runner_describe_it.out +++ b/test/message/test_runner_describe_it.out @@ -23,6 +23,7 @@ not ok 3 - sync fail todo # TODO * * * + * ... # Subtest: sync fail todo with message not ok 4 - sync fail todo with message # TODO this is a failing todo @@ -41,6 +42,7 @@ not ok 4 - sync fail todo with message # TODO this is a failing todo * * * + * ... # Subtest: sync skip pass ok 5 - sync skip pass # SKIP