diff --git a/scripts/lib/platform-launch.js b/scripts/lib/platform-launch.js index ff2e8c0b5..c51da1be8 100644 --- a/scripts/lib/platform-launch.js +++ b/scripts/lib/platform-launch.js @@ -9,10 +9,11 @@ * branch that silently no-op'd on Windows/Linux). This helper: * * 1. Dispatches `open` / `cmd /c start` / `xdg-open` based on process.platform - * 2. Wires the child's 'error' event so ENOENT / EACCES propagate to the caller - * instead of being swallowed by detached spawns - * 3. Returns a structured { opened, reason } result so CLI consumers can - * surface the truth (browser did/did not open) instead of a lying true/false + * 2. Handles the child's 'error' event so a missing launcher does not cause + * an unhandled error after a detached spawn + * 3. Returns a structured { opened, reason } result for the launch request. + * Later asynchronous errors cannot change the returned result; success + * does not prove a browser opened. * * The signature is intentionally small (single function, no class) so callers * can import without picking up the rest of scripts/lib. @@ -39,15 +40,15 @@ function openerCommandFor(platform, url) { /** * Open a URL in the user's default browser, dispatching per-platform. * - * Always returns a structured result so callers can: - * - show a clear error to the agent (no silent failures) - * - keep JSON CLI output truthful when browsers cannot launch + * Returns the synchronous launch-request result. Asynchronous child errors + * are handled, but are not an acknowledgment that a browser opened. * * @param {string} url * @param {NodeJS.Platform} [platform] - injectable for tests; defaults to process.platform + * @param {typeof spawn} [spawnProcess] - injectable process launcher for tests * @returns {{ opened: boolean, reason: string }} */ -function openBrowser(url, platform = process.platform) { +function openBrowser(url, platform = process.platform, spawnProcess = spawn) { if (typeof url !== 'string' || url.length === 0) { return { opened: false, reason: 'invalid-url' }; } @@ -55,7 +56,7 @@ function openBrowser(url, platform = process.platform) { const [cmd, args] = openerCommandFor(platform, url); let child; try { - child = spawn(cmd, args, { + child = spawnProcess(cmd, args, { detached: true, stdio: 'ignore', }); diff --git a/tests/lib/platform-launch.test.js b/tests/lib/platform-launch.test.js index 482aaa006..e54c75cf9 100644 --- a/tests/lib/platform-launch.test.js +++ b/tests/lib/platform-launch.test.js @@ -21,28 +21,65 @@ test('openerCommandFor: unknown falls through to xdg-open', () => { }); test('openBrowser: invalid url returns invalid-url without spawning', () => { - const r1 = openBrowser(''); - assert.equal(r1.opened, false); - assert.equal(r1.reason, 'invalid-url'); - const r2 = openBrowser(null); - assert.equal(r2.opened, false); - assert.equal(r2.reason, 'invalid-url'); + let calls = 0; + const launch = () => { calls += 1; }; + assert.deepEqual(openBrowser('', 'linux', launch), { opened: false, reason: 'invalid-url' }); + assert.deepEqual(openBrowser(null, 'linux', launch), { opened: false, reason: 'invalid-url' }); + assert.equal(calls, 0); }); -test('openBrowser: returns structured { opened, reason }', () => { - // Use a platform + URL that's syntactically valid. We can't easily assert - // whether the browser actually opens in CI, but the structure must match. - const r = openBrowser('http://localhost:0', 'linux'); - assert.equal(typeof r.opened, 'boolean'); - assert.equal(typeof r.reason, 'string'); - assert.ok(r.reason.length > 0); +test('openBrowser: reports synchronous launcher failures', () => { + const withCode = () => { throw Object.assign(new Error('missing launcher'), { code: 'ENOENT' }); }; + const withoutCode = () => { throw new Error('launcher failed'); }; + assert.deepEqual(openBrowser('http://localhost:0', 'linux', withCode), { + opened: false, reason: 'spawn-threw:ENOENT', + }); + assert.deepEqual(openBrowser('http://localhost:0', 'linux', withoutCode), { + opened: false, reason: 'spawn-threw:unknown', + }); }); -test('openBrowser: uses xdg-open on linux', () => { - // Spy by stubbing spawn via require cache (not possible without mocking module). - // Smoke-test: just ensure the function is callable. - const r = openBrowser('http://localhost:0', 'linux'); - // Either opened=true (xdg-open exists on runner) or opened=false with reason - assert.ok(['spawned', 'child-error:ENOENT', 'child-error:EACCES', 'spawn-threw:ENOENT'].includes(r.reason) - || r.opened === true || r.opened === false); +test('openBrowser: installs an error listener and detaches the launcher', () => { + const handlers = new Map(); + let unrefCalls = 0; + let launchCalls = 0; + const child = { + on(event, listener) { + handlers.set(event, listener); + }, + unref() { + assert.equal(typeof handlers.get('error'), 'function', 'listen before detaching'); + unrefCalls += 1; + }, + }; + + const result = openBrowser('http://localhost:0', 'linux', (command, args, options) => { + launchCalls += 1; + assert.equal(command, 'xdg-open'); + assert.deepEqual(args, ['http://localhost:0']); + assert.deepEqual(options, { detached: true, stdio: 'ignore' }); + return child; + }); + + assert.deepEqual(result, { opened: true, reason: 'spawned' }); + assert.equal(launchCalls, 1); + assert.equal(unrefCalls, 1); + assert.equal(typeof handlers.get('error'), 'function'); + assert.doesNotThrow(() => handlers.get('error')({ code: 'ENOENT' })); +}); + +test('openBrowser: a detach failure does not escape after the listener is installed', () => { + let listener; + const result = openBrowser('http://localhost:0', 'linux', () => ({ + on(event, callback) { + assert.equal(event, 'error'); + listener = callback; + }, + unref() { + assert.equal(typeof listener, 'function'); + throw new Error('cannot detach'); + }, + })); + assert.deepEqual(result, { opened: true, reason: 'spawned' }); + assert.doesNotThrow(() => listener({ code: 'EACCES' })); });