From 673551bdd5c3cb52ee894801c2090204096526cc Mon Sep 17 00:00:00 2001 From: Sergey Chernyshev Date: Thu, 25 Apr 2024 01:55:06 +0200 Subject: [PATCH] test: fix some assumptions in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some tests are assuming they will be run from a directory that do not contain any quote or special character in its path. That assumption is not necessary, using `JSON.stringify` or `pathToFileURL` ensures the test can be run whatever the path looks like. PR-URL: https://github.com/nodejs/node/pull/48958 Backport-PR-URL: https://github.com/nodejs/node/pull/50669 Reviewed-By: Moshe Atlow Reviewed-By: Michaël Zasso --- .../test/es-module/test-esm-dynamic-import.js | 8 ++++---- .../es-module/test-esm-loader-spawn-promisified.mjs | 10 +++++----- graal-nodejs/test/parallel/test-fs-mkdtemp.js | 3 ++- .../test-internal-util-decorate-error-stack.js | 2 +- .../test/parallel/test-repl-require-context.js | 2 +- .../test/parallel/test-stdio-pipe-stderr.js | 2 +- .../test-trace-events-worker-metadata-with-name.js | 2 +- .../parallel/test-trace-events-worker-metadata.js | 2 +- graal-nodejs/test/sequential/test-watch-mode.mjs | 13 +++++++------ 9 files changed, 23 insertions(+), 21 deletions(-) diff --git a/graal-nodejs/test/es-module/test-esm-dynamic-import.js b/graal-nodejs/test/es-module/test-esm-dynamic-import.js index ac6b35ebc1b..d246841c2a6 100644 --- a/graal-nodejs/test/es-module/test-esm-dynamic-import.js +++ b/graal-nodejs/test/es-module/test-esm-dynamic-import.js @@ -1,11 +1,11 @@ 'use strict'; const common = require('../common'); +const { pathToFileURL } = require('url'); const assert = require('assert'); const relativePath = '../fixtures/es-modules/test-esm-ok.mjs'; -const absolutePath = require.resolve('../fixtures/es-modules/test-esm-ok.mjs'); -const targetURL = new URL('file:///'); -targetURL.pathname = absolutePath; +const absolutePath = require.resolve(relativePath); +const targetURL = pathToFileURL(absolutePath); function expectModuleError(result, code, message) { Promise.resolve(result).catch(common.mustCall((error) => { @@ -41,7 +41,7 @@ function expectFsNamespace(result) { // expectOkNamespace(import(relativePath)); expectOkNamespace(eval(`import("${relativePath}")`)); expectOkNamespace(eval(`import("${relativePath}")`)); - expectOkNamespace(eval(`import("${targetURL}")`)); + expectOkNamespace(eval(`import(${JSON.stringify(targetURL)})`)); // Importing a built-in, both direct & via eval expectFsNamespace(import('fs')); diff --git a/graal-nodejs/test/es-module/test-esm-loader-spawn-promisified.mjs b/graal-nodejs/test/es-module/test-esm-loader-spawn-promisified.mjs index 3a107ea6481..162316ade41 100644 --- a/graal-nodejs/test/es-module/test-esm-loader-spawn-promisified.mjs +++ b/graal-nodejs/test/es-module/test-esm-loader-spawn-promisified.mjs @@ -28,7 +28,7 @@ describe('Loader hooks throwing errors', { concurrency: true }, () => { fixtures.fileURL('/es-module-loaders/hooks-custom.mjs'), '--input-type=module', '--eval', - `import '${fixtures.fileURL('/es-modules/file.unknown')}'`, + `import ${JSON.stringify(fixtures.fileURL('/es-modules/file.unknown'))}`, ]); assert.match(stderr, /ERR_UNKNOWN_FILE_EXTENSION/); @@ -142,7 +142,7 @@ describe('Loader hooks throwing errors', { concurrency: true }, () => { `import assert from 'node:assert'; await Promise.allSettled([ import('nonexistent/file.mjs'), - import('${fixtures.fileURL('/es-modules/file.unknown')}'), + import(${JSON.stringify(fixtures.fileURL('/es-modules/file.unknown'))}), import('esmHook/badReturnVal.mjs'), import('esmHook/format.false'), import('esmHook/format.true'), @@ -170,7 +170,7 @@ describe('Loader hooks parsing modules', { concurrency: true }, () => { '--input-type=module', '--eval', `import assert from 'node:assert'; - await import('${fixtures.fileURL('/es-module-loaders/js-as-esm.js')}') + await import(${JSON.stringify(fixtures.fileURL('/es-module-loaders/js-as-esm.js'))}) .then((parsedModule) => { assert.strictEqual(typeof parsedModule, 'object'); assert.strictEqual(parsedModule.namedExport, 'named-export'); @@ -191,7 +191,7 @@ describe('Loader hooks parsing modules', { concurrency: true }, () => { '--input-type=module', '--eval', `import assert from 'node:assert'; - await import('${fixtures.fileURL('/es-modules/file.ext')}') + await import(${JSON.stringify(fixtures.fileURL('/es-modules/file.ext'))}) .then((parsedModule) => { assert.strictEqual(typeof parsedModule, 'object'); const { default: defaultExport } = parsedModule; @@ -258,7 +258,7 @@ describe('Loader hooks parsing modules', { concurrency: true }, () => { '--input-type=module', '--eval', `import assert from 'node:assert'; - await import('${fixtures.fileURL('/es-modules/stateful.mjs')}') + await import(${JSON.stringify(fixtures.fileURL('/es-modules/stateful.mjs'))}) .then(({ default: count }) => { assert.strictEqual(count(), 1); });`, diff --git a/graal-nodejs/test/parallel/test-fs-mkdtemp.js b/graal-nodejs/test/parallel/test-fs-mkdtemp.js index b7bb0bbafe0..9ebf8880d23 100644 --- a/graal-nodejs/test/parallel/test-fs-mkdtemp.js +++ b/graal-nodejs/test/parallel/test-fs-mkdtemp.js @@ -4,6 +4,7 @@ const common = require('../common'); const assert = require('assert'); const fs = require('fs'); const path = require('path'); +const { pathToFileURL } = require('url'); const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); @@ -40,7 +41,7 @@ function handler(err, folder) { // Test with URL object { - tmpdir.url = new URL(`file://${tmpdir.path}`); + tmpdir.url = pathToFileURL(tmpdir.path); const urljoin = (base, path) => new URL(path, base); const tmpFolder = fs.mkdtempSync(urljoin(tmpdir.url, 'foo.')); diff --git a/graal-nodejs/test/parallel/test-internal-util-decorate-error-stack.js b/graal-nodejs/test/parallel/test-internal-util-decorate-error-stack.js index 3566d9375fb..f3034fbb05a 100644 --- a/graal-nodejs/test/parallel/test-internal-util-decorate-error-stack.js +++ b/graal-nodejs/test/parallel/test-internal-util-decorate-error-stack.js @@ -58,7 +58,7 @@ checkStack(err.stack); // Verify that the stack is only decorated once for uncaught exceptions. const args = [ '-e', - `require('${badSyntaxPath}')`, + `require(${JSON.stringify(badSyntaxPath)})`, ]; const result = spawnSync(process.argv[0], args, { encoding: 'utf8' }); checkStack(result.stderr); diff --git a/graal-nodejs/test/parallel/test-repl-require-context.js b/graal-nodejs/test/parallel/test-repl-require-context.js index 750235818b8..af09249c2de 100644 --- a/graal-nodejs/test/parallel/test-repl-require-context.js +++ b/graal-nodejs/test/parallel/test-repl-require-context.js @@ -19,6 +19,6 @@ child.on('exit', common.mustCall(() => { child.stdin.write('const isObject = (obj) => obj.constructor === Object;\n'); child.stdin.write('isObject({});\n'); -child.stdin.write(`require('${fixture}').isObject({});\n`); +child.stdin.write(`require(${JSON.stringify(fixture)}).isObject({});\n`); child.stdin.write('.exit'); child.stdin.end(); diff --git a/graal-nodejs/test/parallel/test-stdio-pipe-stderr.js b/graal-nodejs/test/parallel/test-stdio-pipe-stderr.js index 9ec41b4159f..1737424bb04 100644 --- a/graal-nodejs/test/parallel/test-stdio-pipe-stderr.js +++ b/graal-nodejs/test/parallel/test-stdio-pipe-stderr.js @@ -22,7 +22,7 @@ fs.writeFileSync(fakeModulePath, '', 'utf8'); stream.on('open', () => { spawnSync(process.execPath, { - input: `require("${fakeModulePath.replace(/\\/g, '/')}")`, + input: `require(${JSON.stringify(fakeModulePath)})`, stdio: ['pipe', 'pipe', stream] }); const stderr = fs.readFileSync(stderrOutputPath, 'utf8').trim(); diff --git a/graal-nodejs/test/parallel/test-trace-events-worker-metadata-with-name.js b/graal-nodejs/test/parallel/test-trace-events-worker-metadata-with-name.js index 6c3a44f9566..bf6e1005aa4 100644 --- a/graal-nodejs/test/parallel/test-trace-events-worker-metadata-with-name.js +++ b/graal-nodejs/test/parallel/test-trace-events-worker-metadata-with-name.js @@ -7,7 +7,7 @@ const { isMainThread } = require('worker_threads'); if (isMainThread) { const CODE = 'const { Worker } = require(\'worker_threads\'); ' + - `new Worker('${__filename.replace(/\\/g, '/')}', { name: 'foo' })`; + `new Worker(${JSON.stringify(__filename)}, { name: 'foo' })`; const FILE_NAME = 'node_trace.1.log'; const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); diff --git a/graal-nodejs/test/parallel/test-trace-events-worker-metadata.js b/graal-nodejs/test/parallel/test-trace-events-worker-metadata.js index 8b4d0be9c60..6a8702ccadb 100644 --- a/graal-nodejs/test/parallel/test-trace-events-worker-metadata.js +++ b/graal-nodejs/test/parallel/test-trace-events-worker-metadata.js @@ -7,7 +7,7 @@ const { isMainThread } = require('worker_threads'); if (isMainThread) { const CODE = 'const { Worker } = require(\'worker_threads\'); ' + - `new Worker('${__filename.replace(/\\/g, '/')}')`; + `new Worker(${JSON.stringify(__filename)})`; const FILE_NAME = 'node_trace.1.log'; const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); diff --git a/graal-nodejs/test/sequential/test-watch-mode.mjs b/graal-nodejs/test/sequential/test-watch-mode.mjs index 610391863e6..383e15e73dd 100644 --- a/graal-nodejs/test/sequential/test-watch-mode.mjs +++ b/graal-nodejs/test/sequential/test-watch-mode.mjs @@ -7,6 +7,7 @@ import { describe, it } from 'node:test'; import { spawn } from 'node:child_process'; import { writeFileSync, readFileSync, mkdirSync } from 'node:fs'; import { inspect } from 'node:util'; +import { pathToFileURL } from 'node:url'; import { createInterface } from 'node:readline'; if (common.isIBMi) @@ -188,7 +189,7 @@ console.log("don't show me");`); it('should watch changes to dependencies - cjs', async () => { const dependency = createTmpFile('module.exports = {};'); const file = createTmpFile(` -const dependency = require('${dependency.replace(/\\/g, '/')}'); +const dependency = require(${JSON.stringify(dependency)}); console.log(dependency); `); const { stderr, stdout } = await runWriteSucceed({ file, watchedFile: dependency }); @@ -206,7 +207,7 @@ console.log(dependency); it('should watch changes to dependencies - esm', async () => { const dependency = createTmpFile('module.exports = {};'); const file = createTmpFile(` -import dependency from 'file://${dependency.replace(/\\/g, '/')}'; +import dependency from ${JSON.stringify(pathToFileURL(dependency))}; console.log(dependency); `, '.mjs'); const { stderr, stdout } = await runWriteSucceed({ file, watchedFile: dependency }); @@ -278,7 +279,7 @@ console.log(values.random); skip: 'enable once --import is backported', }, async () => { const file = createTmpFile(); - const imported = `file://${createTmpFile('setImmediate(() => process.exit(0));')}`; + const imported = pathToFileURL(createTmpFile('setImmediate(() => process.exit(0));')); const args = ['--import', imported, file]; const { stderr, stdout } = await runWriteSucceed({ file, watchedFile: file, args }); @@ -320,9 +321,9 @@ console.log(values.random); it('should watch changes to previously missing ESM dependency', { skip: !supportsRecursive }, async () => { - const dependency = path.join(tmpdir.path, `${tmpFiles++}.mjs`); - const relativeDependencyPath = `./${path.basename(dependency)}`; - const dependant = createTmpFile(`import '${relativeDependencyPath}'`, '.mjs'); + const relativeDependencyPath = `./${tmpFiles++}.mjs`; + const dependency = path.join(tmpdir.path, relativeDependencyPath); + const dependant = createTmpFile(`import ${JSON.stringify(relativeDependencyPath)}`, '.mjs'); await failWriteSucceed({ file: dependant, watchedFile: dependency }); });