-
Notifications
You must be signed in to change notification settings - Fork 30.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test_runner: introduce NODE_TEST_WORKER_ID for improved concurrent te… #56091
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -358,14 +358,14 @@ class FileTest extends Test { | |
} | ||
} | ||
|
||
function runTestFile(path, filesWatcher, opts) { | ||
function runTestFile(path, filesWatcher, opts, workerId = 1) { | ||
const watchMode = filesWatcher != null; | ||
const testPath = path === kIsolatedProcessName ? '' : path; | ||
const testOpts = { __proto__: null, signal: opts.signal }; | ||
const subtest = opts.root.createSubtest(FileTest, testPath, testOpts, async (t) => { | ||
const args = getRunArgs(path, opts); | ||
const stdio = ['pipe', 'pipe', 'pipe']; | ||
const env = { __proto__: null, ...process.env, NODE_TEST_CONTEXT: 'child-v8' }; | ||
const env = { __proto__: null, ...process.env, NODE_TEST_CONTEXT: 'child-v8', NODE_TEST_WORKER_ID: workerId }; | ||
if (watchMode) { | ||
stdio.push('ipc'); | ||
env.WATCH_REPORT_DEPENDENCIES = '1'; | ||
|
@@ -724,8 +724,10 @@ function run(options = kEmptyObject) { | |
runFiles = () => { | ||
root.harness.bootstrapPromise = null; | ||
root.harness.buildPromise = null; | ||
let workerId = 1; | ||
return SafePromiseAllSettledReturnVoid(testFiles, (path) => { | ||
const subtest = runTestFile(path, filesWatcher, opts); | ||
const subtest = runTestFile(path, filesWatcher, opts, workerId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if I have 8 cpus and 16 tests, I would expect each worker to be reused twice, but this implementation will not reuse worker ids, I think that is misleading |
||
workerId++; | ||
filesWatcher?.runningSubtests.set(path, subtest); | ||
return subtest; | ||
}); | ||
|
@@ -766,6 +768,7 @@ function run(options = kEmptyObject) { | |
|
||
root.entryFile = resolve(testFile); | ||
debug('loading test file:', fileURL.href); | ||
process.env.NODE_TEST_WORKER_ID = 1; | ||
try { | ||
await cascadedLoader.import(fileURL, parent, { __proto__: null }); | ||
} catch (err) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
const test = require('node:test'); | ||
|
||
test('test1', t => { | ||
console.log('NODE_TEST_WORKER_ID', process.env.NODE_TEST_WORKER_ID) | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
const test = require('node:test'); | ||
|
||
test('test2', t => { | ||
console.log('NODE_TEST_WORKER_ID', process.env.NODE_TEST_WORKER_ID) | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
const test = require('node:test'); | ||
|
||
test('test3', t => { | ||
console.log('NODE_TEST_WORKER_ID', process.env.NODE_TEST_WORKER_ID) | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
'use strict'; | ||
|
||
require('../common'); | ||
const assert = require('assert'); | ||
const test = require('node:test'); | ||
const { spawnSync, spawn } = require('child_process'); | ||
const { join } = require('path'); | ||
const fixtures = require('../common/fixtures'); | ||
const testFixtures = fixtures.path('test-runner'); | ||
|
||
test('testing with isolation enabled', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I suggest being more expressive about what we are testing. For example, |
||
const args = ['--test', '--experimental-test-isolation=process']; | ||
const child = spawnSync(process.execPath, args, { cwd: join(testFixtures, 'worker-id') }); | ||
|
||
assert.strictEqual(child.stderr.toString(), ''); | ||
const stdout = child.stdout.toString(); | ||
|
||
assert.match(stdout, /NODE_TEST_WORKER_ID 1/); | ||
assert.match(stdout, /NODE_TEST_WORKER_ID 2/); | ||
assert.match(stdout, /NODE_TEST_WORKER_ID 3/); | ||
|
||
assert.strictEqual(child.status, 0); | ||
}); | ||
|
||
test('testing with isolation disabled', () => { | ||
const args = ['--test', '--experimental-test-isolation=none']; | ||
const child = spawnSync(process.execPath, args, { cwd: join(testFixtures, 'worker-id') }); | ||
|
||
assert.strictEqual(child.stderr.toString(), ''); | ||
const stdout = child.stdout.toString(); | ||
const regex = /NODE_TEST_WORKER_ID 1/g; | ||
const result = stdout.match(regex); | ||
|
||
assert.strictEqual(result.length, 3); | ||
assert.strictEqual(child.status, 0); | ||
}); | ||
|
||
|
||
test('testing with isolation enabled in watch mode', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The watch mode tests added here have at least two problems:
I think these need to be refactored so that:
|
||
const args = ['--watch', '--test', '--experimental-test-isolation=process']; | ||
|
||
const child = spawn(process.execPath, args, { cwd: join(testFixtures, 'worker-id') }); | ||
|
||
let outputData = ''; | ||
let errorData = ''; | ||
|
||
child.stdout.on('data', (data) => { | ||
outputData += data.toString(); | ||
}); | ||
|
||
child.stderr.on('data', (data) => { | ||
errorData += data.toString(); | ||
}); | ||
|
||
setTimeout(() => { | ||
child.kill(); | ||
|
||
assert.strictEqual(errorData, ''); | ||
assert.match(outputData, /NODE_TEST_WORKER_ID 1/); | ||
assert.match(outputData, /NODE_TEST_WORKER_ID 2/); | ||
assert.match(outputData, /NODE_TEST_WORKER_ID 3/); | ||
|
||
if (child.exitCode !== null) { | ||
assert.strictEqual(child.exitCode, null); // Since we killed it manually | ||
} | ||
}, 1000); | ||
|
||
}); | ||
|
||
test('testing with isolation disabled in watch mode', async () => { | ||
const args = ['--watch', '--test', '--experimental-test-isolation=none']; | ||
|
||
const child = spawn(process.execPath, args, { cwd: join(testFixtures, 'worker-id') }); | ||
|
||
let outputData = ''; | ||
let errorData = ''; | ||
|
||
child.stdout.on('data', (data) => { | ||
outputData += data.toString(); | ||
}); | ||
|
||
child.stderr.on('data', (data) => { | ||
errorData += data.toString(); | ||
}); | ||
|
||
setTimeout(() => { | ||
child.kill(); | ||
|
||
assert.strictEqual(errorData, ''); | ||
const regex = /NODE_TEST_WORKER_ID 1/g; | ||
const result = outputData.match(regex); | ||
|
||
assert.strictEqual(result.length, 3); | ||
|
||
if (child.exitCode !== null) { | ||
assert.strictEqual(child.exitCode, null); | ||
} | ||
}, 1000); | ||
|
||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is the correct place for this. this is a list of env vars node accepts, not env vars it exposes