From 23b0baf13b2bba9a60b71ccc2a7f1d26e7898dec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Sat, 15 Aug 2020 11:02:13 +0200 Subject: [PATCH] test: move execution of WPT to worker threads Running outside of the main Node.js context prevents us from upgrading the WPT harness because new versions more aggressively check the identity of globals like error constructors. Instead of exposing globals used by the tests on vm sandboxes, use worker threads to run everything. PR-URL: https://github.com/nodejs/node/pull/34796 Reviewed-By: Matteo Collina Reviewed-By: Joyee Cheung Reviewed-By: James M Snell --- test/common/README.md | 2 +- test/common/wpt.js | 146 +++++++++++------------------ test/common/wpt/worker.js | 55 +++++++++++ test/wpt/README.md | 27 +++--- test/wpt/test-console.js | 3 - test/wpt/test-encoding.js | 13 +-- test/wpt/test-hr-time.js | 23 +---- test/wpt/test-microtask-queuing.js | 3 - test/wpt/test-timers.js | 8 -- test/wpt/test-url.js | 22 ++--- 10 files changed, 141 insertions(+), 161 deletions(-) create mode 100644 test/common/wpt/worker.js diff --git a/test/common/README.md b/test/common/README.md index b6b3089b7b4bbe..34342b46c1e147 100644 --- a/test/common/README.md +++ b/test/common/README.md @@ -962,7 +962,7 @@ the original WPT harness, see [the WPT tests README][]. ### Class: WPTRunner -A driver class for running WPT with the WPT harness in a vm. +A driver class for running WPT with the WPT harness in a worker thread. See [the WPT tests README][] for details. diff --git a/test/common/wpt.js b/test/common/wpt.js index e79bd66b370735..60891f1ca338fe 100644 --- a/test/common/wpt.js +++ b/test/common/wpt.js @@ -6,8 +6,8 @@ const fixtures = require('../common/fixtures'); const fs = require('fs'); const fsPromises = fs.promises; const path = require('path'); -const vm = require('vm'); const { inspect } = require('util'); +const { Worker } = require('worker_threads'); // https://github.com/w3c/testharness.js/blob/master/testharness.js // TODO: get rid of this half-baked harness in favor of the one @@ -222,7 +222,6 @@ class IntlRequirement { const intlRequirements = new IntlRequirement(); - class StatusLoader { /** * @param {string} path relative path of the WPT subset @@ -287,10 +286,9 @@ class WPTRunner { constructor(path) { this.path = path; this.resource = new ResourceLoader(path); - this.sandbox = null; - this.context = null; - this.globals = new Map(); + this.flags = []; + this.initScript = null; this.status = new StatusLoader(path); this.status.load(); @@ -304,28 +302,19 @@ class WPTRunner { } /** - * Specify that certain global descriptors from the object - * should be defined in the vm - * @param {object} obj - * @param {string[]} names + * Sets the Node.js flags passed to the worker. + * @param {Array} flags */ - copyGlobalsFromObject(obj, names) { - for (const name of names) { - const desc = Object.getOwnPropertyDescriptor(obj, name); - if (!desc) { - assert.fail(`${name} does not exist on the object`); - } - this.globals.set(name, desc); - } + setFlags(flags) { + this.flags = flags; } /** - * Specify that certain global descriptors should be defined in the vm - * @param {string} name - * @param {object} descriptor + * Sets a script to be run in the worker before executing the tests. + * @param {string} script */ - defineGlobal(name, descriptor) { - this.globals.set(name, descriptor); + setInitScript(script) { + this.initScript = script; } // TODO(joyeecheung): work with the upstream to port more tests in .html @@ -353,8 +342,8 @@ class WPTRunner { const meta = spec.title = this.getMeta(content); const absolutePath = spec.getAbsolutePath(); - const context = this.generateContext(spec); const relativePath = spec.getRelativePath(); + const harnessPath = fixtures.path('wpt', 'resources', 'testharness.js'); const scriptsToRun = []; // Scripts specified with the `// META: script=` header if (meta.script) { @@ -371,24 +360,46 @@ class WPTRunner { filename: absolutePath }); - for (const { code, filename } of scriptsToRun) { - try { - vm.runInContext(code, context, { filename }); - } catch (err) { - this.fail( - testFileName, - { - status: NODE_UNCAUGHT, - name: 'evaluation in WPTRunner.runJsTests()', - message: err.message, - stack: inspect(err) - }, - kUncaught - ); - this.inProgress.delete(filename); - break; + const workerPath = path.join(__dirname, 'wpt/worker.js'); + const worker = new Worker(workerPath, { + execArgv: this.flags, + workerData: { + filename: testFileName, + wptRunner: __filename, + wptPath: this.path, + initScript: this.initScript, + harness: { + code: fs.readFileSync(harnessPath, 'utf8'), + filename: harnessPath, + }, + scriptsToRun, + }, + }); + + worker.on('message', (message) => { + switch (message.type) { + case 'result': + return this.resultCallback(testFileName, message.result); + case 'completion': + return this.completionCallback(testFileName, message.status); + default: + throw new Error(`Unexpected message from worker: ${message.type}`); } - } + }); + + worker.on('error', (err) => { + this.fail( + testFileName, + { + status: NODE_UNCAUGHT, + name: 'evaluation in WPTRunner.runJsTests()', + message: err.message, + stack: inspect(err) + }, + kUncaught + ); + this.inProgress.delete(testFileName); + }); } process.on('exit', () => { @@ -430,56 +441,6 @@ class WPTRunner { }); } - mock(testfile) { - const resource = this.resource; - const result = { - // This is a mock, because at the moment fetch is not implemented - // in Node.js, but some tests and harness depend on this to pull - // resources. - fetch(file) { - return resource.read(testfile, file, true); - }, - GLOBAL: { - isWindow() { return false; } - }, - Object - }; - - return result; - } - - // Note: this is how our global space for the WPT test should look like - getSandbox(filename) { - const result = this.mock(filename); - for (const [name, desc] of this.globals) { - Object.defineProperty(result, name, desc); - } - return result; - } - - generateContext(test) { - const filename = test.filename; - const sandbox = this.sandbox = this.getSandbox(test.getRelativePath()); - const context = this.context = vm.createContext(sandbox); - - const harnessPath = fixtures.path('wpt', 'resources', 'testharness.js'); - const harness = fs.readFileSync(harnessPath, 'utf8'); - vm.runInContext(harness, context, { - filename: harnessPath - }); - - sandbox.add_result_callback( - this.resultCallback.bind(this, filename) - ); - sandbox.add_completion_callback( - this.completionCallback.bind(this, filename) - ); - sandbox.self = sandbox; - // TODO(joyeecheung): we are not a window - work with the upstream to - // add a new scope for us. - return context; - } - getTestTitle(filename) { const spec = this.specMap.get(filename); const title = spec.meta && spec.meta.title; @@ -524,9 +485,9 @@ class WPTRunner { * Report the status of each WPT test (one per file) * * @param {string} filename - * @param {Test[]} test The Test objects returned by WPT harness + * @param {object} harnessStatus - The status object returned by WPT harness. */ - completionCallback(filename, tests, harnessStatus) { + completionCallback(filename, harnessStatus) { // Treat it like a test case failure if (harnessStatus.status === 2) { const title = this.getTestTitle(filename); @@ -644,5 +605,6 @@ class WPTRunner { module.exports = { harness: harnessMock, + ResourceLoader, WPTRunner }; diff --git a/test/common/wpt/worker.js b/test/common/wpt/worker.js new file mode 100644 index 00000000000000..f0faeeba9e8062 --- /dev/null +++ b/test/common/wpt/worker.js @@ -0,0 +1,55 @@ +/* eslint-disable node-core/required-modules,node-core/require-common-first */ + +'use strict'; + +const { runInThisContext } = require('vm'); +const { parentPort, workerData } = require('worker_threads'); + +const { ResourceLoader } = require(workerData.wptRunner); +const resource = new ResourceLoader(workerData.wptPath); + +global.self = global; +global.GLOBAL = { + isWindow() { return false; } +}; +global.require = require; + +// This is a mock, because at the moment fetch is not implemented +// in Node.js, but some tests and harness depend on this to pull +// resources. +global.fetch = function fetch(file) { + return resource.read(workerData.filename, file, true); +}; + +if (workerData.initScript) { + runInThisContext(workerData.initScript); +} + +runInThisContext(workerData.harness.code, { + filename: workerData.harness.filename +}); + +// eslint-disable-next-line no-undef +add_result_callback((result) => { + parentPort.postMessage({ + type: 'result', + result: { + status: result.status, + name: result.name, + message: result.message, + stack: result.stack, + }, + }); +}); + +// eslint-disable-next-line no-undef +add_completion_callback((_, status) => { + parentPort.postMessage({ + type: 'completion', + status, + }); +}); + +for (const scriptToRun of workerData.scriptsToRun) { + runInThisContext(scriptToRun.code, { filename: scriptToRun.filename }); +} diff --git a/test/wpt/README.md b/test/wpt/README.md index d15a56745be090..d2a47737b0367b 100644 --- a/test/wpt/README.md +++ b/test/wpt/README.md @@ -45,23 +45,20 @@ For example, for the URL tests, add a file `test/wpt/test-url.js`: ```js 'use strict'; -// This flag is required by the WPT Runner to patch the internals -// for the tests to run in a vm. -// Flags: --expose-internals - require('../common'); const { WPTRunner } = require('../common/wpt'); const runner = new WPTRunner('url'); -// Copy global descriptors from the global object -runner.copyGlobalsFromObject(global, ['URL', 'URLSearchParams']); -// Define any additional globals with descriptors -runner.defineGlobal('DOMException', { - get() { - return require('internal/domexception'); - } -}); +// Set Node.js flags required for the tests. +runner.setFlags(['--expose-internals']); + +// Set a script that will be executed in the worker before running the tests. +runner.setInitScript(` + const { internalBinding } = require('internal/test/binding'); + const { DOMException } = internalBinding('messaging'); + global.DOMException = DOMException; +`); runner.runJsTests(); ``` @@ -82,7 +79,7 @@ To run a specific test in WPT, for example, `url/url-searchparams.any.js`, pass the file name as argument to the corresponding test driver: ```text -node --expose-internals test/wpt/test-url.js url-searchparams.any.js +node test/wpt/test-url.js url-searchparams.any.js ``` If there are any failures, update the corresponding status file @@ -138,9 +135,9 @@ loads: * Status file (for example, `test/wpt/status/url.json` for `url`) * The WPT harness -Then, for each test, it creates a vm with the globals and mocks, +Then, for each test, it creates a worker thread with the globals and mocks, sets up the harness result hooks, loads the metadata in the test (including -loading extra resources), and runs all the tests in that vm, +loading extra resources), and runs all the tests in that worker thread, skipping tests that cannot be run because of lack of dependency or expected failures. diff --git a/test/wpt/test-console.js b/test/wpt/test-console.js index ae0e3385479bae..e66726431fba6e 100644 --- a/test/wpt/test-console.js +++ b/test/wpt/test-console.js @@ -4,7 +4,4 @@ const { WPTRunner } = require('../common/wpt'); const runner = new WPTRunner('console'); -// Copy global descriptors from the global object -runner.copyGlobalsFromObject(global, ['console']); - runner.runJsTests(); diff --git a/test/wpt/test-encoding.js b/test/wpt/test-encoding.js index 8cd0d5e04e0b7b..b297648d9cb7ee 100644 --- a/test/wpt/test-encoding.js +++ b/test/wpt/test-encoding.js @@ -1,16 +1,11 @@ 'use strict'; require('../common'); -const { MessageChannel } = require('worker_threads'); const { WPTRunner } = require('../common/wpt'); const runner = new WPTRunner('encoding'); -// Copy global descriptors from the global object -runner.copyGlobalsFromObject(global, ['TextDecoder', 'TextEncoder']); - -runner.defineGlobal('MessageChannel', { - get() { - return MessageChannel; - } -}); +runner.setInitScript(` + const { MessageChannel } = require('worker_threads'); + global.MessageChannel = MessageChannel; +`); runner.runJsTests(); diff --git a/test/wpt/test-hr-time.js b/test/wpt/test-hr-time.js index eb9c68797dfcbf..d77ac0bbc6d9c0 100644 --- a/test/wpt/test-hr-time.js +++ b/test/wpt/test-hr-time.js @@ -1,27 +1,14 @@ 'use strict'; -// Flags: --expose-internals - require('../common'); const { WPTRunner } = require('../common/wpt'); -const { performance, PerformanceObserver } = require('perf_hooks'); const runner = new WPTRunner('hr-time'); -runner.copyGlobalsFromObject(global, [ - 'setInterval', - 'clearInterval', - 'setTimeout', - 'clearTimeout' -]); - -runner.defineGlobal('performance', { - get() { - return performance; - } -}); -runner.defineGlobal('PerformanceObserver', { - value: PerformanceObserver -}); +runner.setInitScript(` + const { performance, PerformanceObserver } = require('perf_hooks'); + global.performance = performance; + global.PerformanceObserver = PerformanceObserver; +`); runner.runJsTests(); diff --git a/test/wpt/test-microtask-queuing.js b/test/wpt/test-microtask-queuing.js index 84f29ac2b994b0..3a83fd33ba051e 100644 --- a/test/wpt/test-microtask-queuing.js +++ b/test/wpt/test-microtask-queuing.js @@ -5,7 +5,4 @@ const { WPTRunner } = require('../common/wpt'); const runner = new WPTRunner('html/webappapis/microtask-queuing'); -// Copy global descriptors from the global object -runner.copyGlobalsFromObject(global, ['queueMicrotask']); - runner.runJsTests(); diff --git a/test/wpt/test-timers.js b/test/wpt/test-timers.js index 4d342305400215..7736d540a2aa28 100644 --- a/test/wpt/test-timers.js +++ b/test/wpt/test-timers.js @@ -5,12 +5,4 @@ const { WPTRunner } = require('../common/wpt'); const runner = new WPTRunner('html/webappapis/timers'); -// Copy global descriptors from the global object -runner.copyGlobalsFromObject(global, [ - 'setInterval', - 'clearInterval', - 'setTimeout', - 'clearTimeout' -]); - runner.runJsTests(); diff --git a/test/wpt/test-url.js b/test/wpt/test-url.js index 5d5240ce181393..4652bfd880cd76 100644 --- a/test/wpt/test-url.js +++ b/test/wpt/test-url.js @@ -1,20 +1,18 @@ 'use strict'; -// Flags: --expose-internals - require('../common'); const { WPTRunner } = require('../common/wpt'); -const { internalBinding } = require('internal/test/binding'); -const { DOMException } = internalBinding('messaging'); + const runner = new WPTRunner('url'); -// Copy global descriptors from the global object -runner.copyGlobalsFromObject(global, ['URL', 'URLSearchParams']); -// Needed by urlsearchparams-constructor.any.js -runner.defineGlobal('DOMException', { - get() { - return DOMException; - } -}); +// Needed to access to DOMException. +runner.setFlags(['--expose-internals']); + +// DOMException is needed by urlsearchparams-constructor.any.js +runner.setInitScript(` + const { internalBinding } = require('internal/test/binding'); + const { DOMException } = internalBinding('messaging'); + global.DOMException = DOMException; +`); runner.runJsTests();