From 323881f60b81d0bb60a81bdfcf4bc36f24363269 Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Sat, 22 Apr 2023 23:32:12 +0300 Subject: [PATCH] test_runner: fix test runner concurrency PR-URL: https://github.com/nodejs/node/pull/47675 Fixes: https://github.com/nodejs/node/issues/47365 Fixes: https://github.com/nodejs/node/issues/47696 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Matteo Collina Reviewed-By: Colin Ihrig --- lib/internal/per_context/primordials.js | 8 +------ lib/internal/test_runner/runner.js | 26 ++++++++++++++------- lib/internal/test_runner/test.js | 2 +- lib/internal/test_runner/tests_stream.js | 4 ++++ test/fixtures/test-runner/concurrency/a.mjs | 13 +++++++++++ test/fixtures/test-runner/concurrency/b.mjs | 13 +++++++++++ test/parallel/test-primordials-promise.js | 4 ++-- test/parallel/test-runner-concurrency.js | 20 +++++++++++++++- 8 files changed, 71 insertions(+), 19 deletions(-) create mode 100644 test/fixtures/test-runner/concurrency/a.mjs create mode 100644 test/fixtures/test-runner/concurrency/b.mjs diff --git a/lib/internal/per_context/primordials.js b/lib/internal/per_context/primordials.js index 59c285cd16d165..d7a846b29e6631 100644 --- a/lib/internal/per_context/primordials.js +++ b/lib/internal/per_context/primordials.js @@ -565,13 +565,7 @@ primordials.SafePromiseAllSettled = (promises, mapFn) => * @returns {Promise} */ primordials.SafePromiseAllSettledReturnVoid = async (promises, mapFn) => { - for (let i = 0; i < promises.length; i++) { - try { - await (mapFn != null ? mapFn(promises[i], i) : promises[i]); - } catch { - // In all settled, we can ignore errors. - } - } + await primordials.SafePromiseAllSettled(promises, mapFn); }; /** diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index 36dc9ac9337b4a..5537c81d7f2258 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -206,10 +206,6 @@ class FileTest extends Test { const testNumber = nesting === 0 ? (this.root.harness.counters.topLevel + 1) : node.id; const method = pass ? 'ok' : 'fail'; this.reporter[method](nesting, this.name, testNumber, node.description, diagnostics, directive); - if (nesting === 0) { - this.failedSubtests ||= !pass; - } - this.#reportedChildren++; countCompletedTest({ name: node.description, finished: true, @@ -236,22 +232,36 @@ class FileTest extends Test { break; } } + #accumulateReportItem({ kind, node, comments, nesting = 0 }) { + if (kind !== TokenKind.TAP_TEST_POINT) { + return; + } + this.#reportedChildren++; + if (nesting === 0 && !node.status.pass) { + this.failedSubtests = true; + } + } + #drainBuffer() { + if (this.#buffer.length > 0) { + ArrayPrototypeForEach(this.#buffer, (ast) => this.#handleReportItem(ast)); + this.#buffer = []; + } + } addToReport(ast) { + this.#accumulateReportItem(ast); if (!this.isClearToSend()) { ArrayPrototypePush(this.#buffer, ast); return; } - this.reportStarted(); + this.#drainBuffer(); this.#handleReportItem(ast); } reportStarted() {} report() { + this.#drainBuffer(); const skipReporting = this.#skipReporting(); if (!skipReporting) { super.reportStarted(); - } - ArrayPrototypeForEach(this.#buffer, (ast) => this.#handleReportItem(ast)); - if (!skipReporting) { super.report(); } } diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index cd2dd2c1bc85ea..9f82bf6b361544 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -658,7 +658,7 @@ class Test extends AsyncResource { this.reporter.coverage(this.nesting, kFilename, coverage); } - this.reporter.push(null); + this.reporter.end(); } } diff --git a/lib/internal/test_runner/tests_stream.js b/lib/internal/test_runner/tests_stream.js index c32d57d3b4cf71..68379fed11dda4 100644 --- a/lib/internal/test_runner/tests_stream.js +++ b/lib/internal/test_runner/tests_stream.js @@ -59,6 +59,10 @@ class TestsStream extends Readable { this.#emit('test:coverage', { __proto__: null, nesting, file, summary }); } + end() { + this.#tryPush(null); + } + #emit(type, data) { this.emit(type, data); this.#tryPush({ type, data }); diff --git a/test/fixtures/test-runner/concurrency/a.mjs b/test/fixtures/test-runner/concurrency/a.mjs new file mode 100644 index 00000000000000..69954461bfbae0 --- /dev/null +++ b/test/fixtures/test-runner/concurrency/a.mjs @@ -0,0 +1,13 @@ +import tmpdir from '../../../common/tmpdir.js'; +import { setTimeout } from 'node:timers/promises'; +import fs from 'node:fs/promises'; +import path from 'node:path'; + +await fs.writeFile(path.resolve(tmpdir.path, 'test-runner-concurrency'), 'a.mjs'); +while (true) { + const file = await fs.readFile(path.resolve(tmpdir.path, 'test-runner-concurrency'), 'utf8'); + if (file === 'b.mjs') { + break; + } + await setTimeout(10); +} diff --git a/test/fixtures/test-runner/concurrency/b.mjs b/test/fixtures/test-runner/concurrency/b.mjs new file mode 100644 index 00000000000000..09af543a2551eb --- /dev/null +++ b/test/fixtures/test-runner/concurrency/b.mjs @@ -0,0 +1,13 @@ +import tmpdir from '../../../common/tmpdir.js'; +import { setTimeout } from 'node:timers/promises'; +import fs from 'node:fs/promises'; +import path from 'node:path'; + +while (true) { + const file = await fs.readFile(path.resolve(tmpdir.path, 'test-runner-concurrency'), 'utf8'); + if (file === 'a.mjs') { + await fs.writeFile(path.resolve(tmpdir.path, 'test-runner-concurrency'), 'b.mjs'); + break; + } + await setTimeout(10); +} diff --git a/test/parallel/test-primordials-promise.js b/test/parallel/test-primordials-promise.js index 58a65d73bf6618..3dbacc1c528486 100644 --- a/test/parallel/test-primordials-promise.js +++ b/test/parallel/test-primordials-promise.js @@ -55,13 +55,11 @@ assertIsPromise(SafePromisePrototypeFinally(test(), common.mustCall())); assertIsPromise(SafePromiseAllReturnArrayLike([test()])); assertIsPromise(SafePromiseAllReturnVoid([test()])); -assertIsPromise(SafePromiseAllSettledReturnVoid([test()])); assertIsPromise(SafePromiseAny([test()])); assertIsPromise(SafePromiseRace([test()])); assertIsPromise(SafePromiseAllReturnArrayLike([])); assertIsPromise(SafePromiseAllReturnVoid([])); -assertIsPromise(SafePromiseAllSettledReturnVoid([])); { const val1 = Symbol(); @@ -108,9 +106,11 @@ Object.defineProperties(Array.prototype, { assertIsPromise(SafePromiseAll([test()])); assertIsPromise(SafePromiseAllSettled([test()])); +assertIsPromise(SafePromiseAllSettledReturnVoid([test()])); assertIsPromise(SafePromiseAll([])); assertIsPromise(SafePromiseAllSettled([])); +assertIsPromise(SafePromiseAllSettledReturnVoid([])); async function test() { const catchFn = common.mustCall(); diff --git a/test/parallel/test-runner-concurrency.js b/test/parallel/test-runner-concurrency.js index 8d756971d68551..4eab5ba16d0140 100644 --- a/test/parallel/test-runner-concurrency.js +++ b/test/parallel/test-runner-concurrency.js @@ -1,7 +1,14 @@ 'use strict'; const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const fixtures = require('../common/fixtures'); const { describe, it, test } = require('node:test'); -const assert = require('assert'); +const assert = require('node:assert'); +const path = require('node:path'); +const fs = require('node:fs/promises'); +const os = require('node:os'); + +tmpdir.refresh(); describe('Concurrency option (boolean) = true ', { concurrency: true }, () => { let isFirstTestOver = false; @@ -62,3 +69,14 @@ describe( it('should run after other suites', expectedTestTree); }); } + +test('--test multiple files', { skip: os.availableParallelism() < 3 }, async () => { + await fs.writeFile(path.resolve(tmpdir.path, 'test-runner-concurrency'), ''); + const { code, stderr } = await common.spawnPromisified(process.execPath, [ + '--test', + fixtures.path('test-runner', 'concurrency', 'a.mjs'), + fixtures.path('test-runner', 'concurrency', 'b.mjs'), + ]); + assert.strictEqual(stderr, ''); + assert.strictEqual(code, 0); +});