From bd3e1def7a1932907c4c53ff52ce926292d35795 Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Thu, 28 Dec 2023 09:52:24 -0500 Subject: [PATCH 1/3] Apply url.fileURLToPath to main.test.js Should've been part of commit 34922071c738f2b71ecbca3beba348064fcab930. This fixes the problem of not finding the `jsdoc` stub on Windows, but now the tests in main.test.js are failing on Windows with: Serialized Error: { errno: -4094, code: 'UNKNOWN', syscall: 'spawn' } Which is odd, because runJsdoc() calls spawn() and runJsdoc.test.js passes just fine. Investigating... --- test/main.test.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/main.test.js b/test/main.test.js index 5ee8099..9ba94bb 100644 --- a/test/main.test.js +++ b/test/main.test.js @@ -11,19 +11,20 @@ import DestDirHelper from './DestDirHelper' import { afterEach, describe, expect, test } from 'vitest' import { spawn } from 'node:child_process' import path from 'node:path' +import { fileURLToPath } from 'node:url' describe('jsdoc-cli-wrapper', () => { const root = fixturePath('fakeJsdoc') const destDirHelper = new DestDirHelper() + const mainPath = fileURLToPath(new URL('../index.js', import.meta.url)) afterEach(async () => await destDirHelper.cleanup()) const spawnMain = async(testEnvPath, ...argv) => { - const origPath = process.env.PATH + const origEnvPath = process.env.PATH process.env.PATH = testEnvPath const result = await new Promise(resolve => { - const mainPath = new URL('../index.js', import.meta.url).pathname const wrapper = spawn(mainPath, argv) let stdout = '' let stderr = '' @@ -38,7 +39,7 @@ describe('jsdoc-cli-wrapper', () => { }) }) - process.env.PATH = origPath + process.env.PATH = origEnvPath return result } From 8697dab77e20b5868a3aed401a283a290ff0ebfc Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Thu, 28 Dec 2023 12:21:31 -0500 Subject: [PATCH 2/3] Fix remaining Windows portability issues There were a few problems here. First, I added an error handler to the process spawned within spawnMain() to make the tests fail faster instead of timing out after five seconds. This made iterating on the following problems much faster. Today I learned that on Windows, we need to use process.env.Path, not process.env.PATH. So now the new PATH_KEY constant is used in place of raw env.PATH accesses and `{ PATH: "..." }` object literals. This was the biggest of the problems. Next, the jsdoc.CMD invocation kept echoing its commands to standard output, causing tests to fail. The solution was to add `@echo off` as the first line of the jsdoc.CMD script. In runMainWithoutJsdoc from main.test.js, `jsdocDir` contained unescaped backslashes on Windows, so the regular expression created from it didn't match the jsdoc path. The fix was to replace all `\` in `jsdocDir` with `\\` first. Finally, spawnMain() from main.test.js now runs the mainPath script by passing process.execPath (the path to the Node interpreter) as the first argument. This is because Windows can't interpret the shebang at the top of the mainPath script. This is also portable to macOS and Linux. In the process (ha!), I also stopped manipulating process.env.PATH in spawnMain() after remembering I could just define a new env object. This also enabled returning the Promise directly from the function, rather than using await internally to reset process.env.PATH afterwards. --- lib/index.js | 15 +++++++++++---- test/fixtures/fakeJsdoc/jsdoc.CMD | 1 + test/fixtures/index.js | 3 ++- test/getPath.test.js | 4 ++-- test/main.test.js | 22 +++++++++------------- test/runJsdoc.test.js | 6 +++--- 6 files changed, 28 insertions(+), 23 deletions(-) diff --git a/lib/index.js b/lib/index.js index 8a95672..72a784b 100644 --- a/lib/index.js +++ b/lib/index.js @@ -53,16 +53,23 @@ export async function runJsdoc(argv, env, platform) { return {exitCode} } +/** + * The key for the command search path within process.env. + * + * On every system other than Windows, this will be "PATH". On Windows, this + * will be "Path". + */ +export const PATH_KEY = process.platform !== 'win32' ? 'PATH' : 'Path' + /** * Returns the full path to the specified command - * @param {string} cmdName - command to find in env.PATH + * @param {string} cmdName - command to find in env[PATH_KEY] * @param {object} env - environment variables, presumably process.env - * @param {string} env.PATH - the PATH environment variable * @param {string} platform - the process.platform string * @returns {Promise} - path to the command */ export async function getPath(cmdName, env, platform) { - for (const p of env.PATH.split(path.delimiter)) { + for (const p of env[PATH_KEY].split(path.delimiter)) { // pnpm will install both the original script and versions ending with .CMD // and .ps1. We'll just default to .CMD. const extension = (platform === 'win32') ? '.CMD' : '' @@ -73,7 +80,7 @@ export async function getPath(cmdName, env, platform) { return candidate } catch { /* try next candidate */ } } - return Promise.reject(`${cmdName} not found in PATH`) + return Promise.reject(`${cmdName} not found in ${PATH_KEY}`) } /** diff --git a/test/fixtures/fakeJsdoc/jsdoc.CMD b/test/fixtures/fakeJsdoc/jsdoc.CMD index b917d08..d5e0a72 100644 --- a/test/fixtures/fakeJsdoc/jsdoc.CMD +++ b/test/fixtures/fakeJsdoc/jsdoc.CMD @@ -6,4 +6,5 @@ :: https://stackoverflow.com/questions/5034076/what-does-dp0-mean-and-how-does-it-work :: https://ss64.com/nt/ :: https://htipe.wordpress.com/2008/10/09/the-dp0-variable/ +@echo off node "%~dp0\jsdoc" %* diff --git a/test/fixtures/index.js b/test/fixtures/index.js index 2532796..6793369 100644 --- a/test/fixtures/index.js +++ b/test/fixtures/index.js @@ -36,5 +36,6 @@ export function fixtureEnv(fixtureName) { const paths = [ path.join('usr', 'local', 'bin'), path.join('usr', 'bin'), 'bin' ] - return { PATH: paths.map(p => path.join(root, p)).join(path.delimiter) } + const pathKey = process.platform !== 'win32' ? 'PATH' : 'Path' + return {[pathKey]: paths.map(p => path.join(root, p)).join(path.delimiter)} } diff --git a/test/getPath.test.js b/test/getPath.test.js index 61b3b7e..aab376c 100644 --- a/test/getPath.test.js +++ b/test/getPath.test.js @@ -5,7 +5,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -import { getPath } from '../lib' +import { getPath, PATH_KEY } from '../lib' import { fixturePath, fixtureEnv } from './fixtures' import { describe, expect, test } from 'vitest' import path from 'node:path' @@ -26,6 +26,6 @@ describe('getPath', () => { test('rejects when command isn\'t found', async () => { await expect(getPath('nonexistent', env, process.platform)).rejects - .toBe('nonexistent not found in PATH') + .toBe(`nonexistent not found in ${PATH_KEY}`) }) }) diff --git a/test/main.test.js b/test/main.test.js index 9ba94bb..312ff61 100644 --- a/test/main.test.js +++ b/test/main.test.js @@ -5,7 +5,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -import { getPath } from '../lib' +import { getPath, PATH_KEY } from '../lib' import { fixturePath } from './fixtures' import DestDirHelper from './DestDirHelper' import { afterEach, describe, expect, test } from 'vitest' @@ -20,12 +20,10 @@ describe('jsdoc-cli-wrapper', () => { afterEach(async () => await destDirHelper.cleanup()) - const spawnMain = async(testEnvPath, ...argv) => { - const origEnvPath = process.env.PATH - process.env.PATH = testEnvPath - - const result = await new Promise(resolve => { - const wrapper = spawn(mainPath, argv) + const spawnMain = (testEnvPath, ...argv) => { + return new Promise((resolve, reject) => { + const env = {...process.env, [PATH_KEY]: testEnvPath} + const wrapper = spawn(process.execPath, [mainPath, ...argv], {env}) let stdout = '' let stderr = '' @@ -37,23 +35,21 @@ describe('jsdoc-cli-wrapper', () => { if (stderr) result.stderr = stderr resolve(result) }) + wrapper.on('error', (err) => reject(err)) }) - - process.env.PATH = origEnvPath - return result } const runMain = async (...argv) => { - const testEnvPath = [root, process.env.PATH].join(path.delimiter) + const testEnvPath = [root, process.env[PATH_KEY]].join(path.delimiter) return spawnMain(testEnvPath, ...argv) } const runMainWithoutJsdoc = async (...argv) => { - let testEnvPath = process.env.PATH + let testEnvPath = process.env[PATH_KEY] try { const jsdocPath = await getPath('jsdoc', process.env, process.platform) - const jsdocDir = path.dirname(jsdocPath) + const jsdocDir = path.dirname(jsdocPath).replaceAll('\\', '\\\\') const pat = new RegExp(`${path.delimiter}?${jsdocDir}${path.delimiter}?`) testEnvPath = testEnvPath.replace(pat, '') } catch { /* It's OK if it's not actually installed. */ } diff --git a/test/runJsdoc.test.js b/test/runJsdoc.test.js index 5aab4f1..8302568 100644 --- a/test/runJsdoc.test.js +++ b/test/runJsdoc.test.js @@ -5,7 +5,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -import { runJsdoc } from '../lib' +import { runJsdoc, PATH_KEY } from '../lib' import { fixturePath } from './fixtures' import DestDirHelper from './DestDirHelper' import { afterEach, beforeEach, describe, expect, test } from 'vitest' @@ -13,7 +13,7 @@ import path from 'node:path' describe('runJsdoc', () => { const root = fixturePath('fakeJsdoc') - const env = { PATH: root } + const env = { [PATH_KEY]: root } const platform = process.platform const destDirHelper = new DestDirHelper() @@ -35,7 +35,7 @@ describe('runJsdoc', () => { test('emits error if jsdoc not found', async () => { const bogusPath = path.join(root, 'nonexistent') - await expect(runJsdoc(argv, {PATH: bogusPath}, platform)) + await expect(runJsdoc(argv, {[PATH_KEY]: bogusPath}, platform)) .rejects.toContain('npm add -g jsdoc') await expect(readIndexHtml()).resolves.toStrictEqual({ actualPath: origIndexPath, content: 'Old and Busted' From 407c5ae09608c922b7a1517f0a6b9ba0ac26d2cc Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Thu, 28 Dec 2023 12:41:47 -0500 Subject: [PATCH 3/3] Rename fakeJsdoc test fixture to jsdocStub This doesn't change any functionality, but better reflects the nature of the jsdoc test double scripts. "Fake" would imply that the doubles are fully functional, but merely scaled down implementations. These are really more "stubs," incomplete implementations to simulate specific operations and responses. --- test/fixtures/{fakeJsdoc => jsdocStub}/jsdoc | 0 test/fixtures/{fakeJsdoc => jsdocStub}/jsdoc.CMD | 0 test/main.test.js | 2 +- test/runJsdoc.test.js | 2 +- 4 files changed, 2 insertions(+), 2 deletions(-) rename test/fixtures/{fakeJsdoc => jsdocStub}/jsdoc (100%) rename test/fixtures/{fakeJsdoc => jsdocStub}/jsdoc.CMD (100%) diff --git a/test/fixtures/fakeJsdoc/jsdoc b/test/fixtures/jsdocStub/jsdoc similarity index 100% rename from test/fixtures/fakeJsdoc/jsdoc rename to test/fixtures/jsdocStub/jsdoc diff --git a/test/fixtures/fakeJsdoc/jsdoc.CMD b/test/fixtures/jsdocStub/jsdoc.CMD similarity index 100% rename from test/fixtures/fakeJsdoc/jsdoc.CMD rename to test/fixtures/jsdocStub/jsdoc.CMD diff --git a/test/main.test.js b/test/main.test.js index 312ff61..087cdd3 100644 --- a/test/main.test.js +++ b/test/main.test.js @@ -14,7 +14,7 @@ import path from 'node:path' import { fileURLToPath } from 'node:url' describe('jsdoc-cli-wrapper', () => { - const root = fixturePath('fakeJsdoc') + const root = fixturePath('jsdocStub') const destDirHelper = new DestDirHelper() const mainPath = fileURLToPath(new URL('../index.js', import.meta.url)) diff --git a/test/runJsdoc.test.js b/test/runJsdoc.test.js index 8302568..dd0eb95 100644 --- a/test/runJsdoc.test.js +++ b/test/runJsdoc.test.js @@ -12,7 +12,7 @@ import { afterEach, beforeEach, describe, expect, test } from 'vitest' import path from 'node:path' describe('runJsdoc', () => { - const root = fixturePath('fakeJsdoc') + const root = fixturePath('jsdocStub') const env = { [PATH_KEY]: root } const platform = process.platform const destDirHelper = new DestDirHelper()