mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-21 15:55:43 +00:00
test(desktop): extract hiddenWindowsChildOptions + stopBackendChild for real unit tests
windows-child-process.test.ts regexed main.ts and bootstrap-runner.ts source text to check that spawn/execFileSync call sites wrapped their options with hiddenWindowsChildOptions(), and that backend teardown chose the right kill strategy. Extract both into dependency-free sibling modules: - windows-child-options.ts: hiddenWindowsChildOptions(options, isWindows) now takes isWindows as an injectable param (defaults to the real platform check). main.ts and bootstrap-runner.ts both import the same implementation instead of each defining their own copy. - backend-child.ts: stopBackendChild(child, deps) with forceKillProcessTree and isWindows injected, so the SIGTERM-vs-tree-kill branching is directly testable with a fake child + a spy. windows-child-options.test.ts replaces windows-child-process.test.ts, calling the real functions with fake spawn/execFileSync-shaped objects and asserting on the actual returned options / kill call.
This commit is contained in:
parent
5265b3002c
commit
4527943e91
6 changed files with 231 additions and 31 deletions
55
apps/desktop/electron/backend-child.ts
Normal file
55
apps/desktop/electron/backend-child.ts
Normal file
|
|
@ -0,0 +1,55 @@
|
|||
/**
|
||||
* backend-child.ts
|
||||
*
|
||||
* Windows-aware teardown for the desktop's managed backend child process.
|
||||
*
|
||||
* Node's `child.kill()` only signals the direct child. On Windows a backend
|
||||
* that spawned its own grandchildren (a `hermes` REPL, a pty terminal
|
||||
* session, the gateway) survives a plain SIGTERM and keeps files (e.g. the
|
||||
* venv shim) locked. So on Windows we tree-kill via `forceKillProcessTree`;
|
||||
* everywhere else a plain SIGTERM is correct and sufficient (POSIX has no
|
||||
* mandatory locks, and the backend is not spawned detached so there's no
|
||||
* process-group to negative-pid-kill).
|
||||
*
|
||||
* Extracted into its own dependency-free module (no electron import) so the
|
||||
* SIGTERM-vs-tree-kill branching can be asserted directly with a fake child
|
||||
* object and a spy `forceKillProcessTree`, instead of grepping main.ts source
|
||||
* text for the function body.
|
||||
*/
|
||||
|
||||
export interface StopBackendChildDeps {
|
||||
/** Defaults to the real platform check; injectable for tests. */
|
||||
isWindows?: boolean
|
||||
/** Windows tree-kill implementation (real: taskkill /T /F via execFileSync). */
|
||||
forceKillProcessTree: (pid: number) => void
|
||||
}
|
||||
|
||||
export interface KillableChild {
|
||||
pid?: number | null
|
||||
killed?: boolean
|
||||
kill: (signal: string) => void
|
||||
}
|
||||
|
||||
/**
|
||||
* Stop a managed child process, choosing the right strategy for the platform.
|
||||
* No-ops silently if `child` is falsy, already killed, or the kill attempt
|
||||
* throws (the process may already be gone) -- mirrors the original inline
|
||||
* best-effort semantics in main.ts.
|
||||
*/
|
||||
export function stopBackendChild(child: KillableChild | null | undefined, deps: StopBackendChildDeps) {
|
||||
if (!child || child.killed) {
|
||||
return
|
||||
}
|
||||
|
||||
const isWindows = deps.isWindows ?? process.platform === 'win32'
|
||||
|
||||
try {
|
||||
if (isWindows && Number.isInteger(child.pid)) {
|
||||
deps.forceKillProcessTree(child.pid as number)
|
||||
} else {
|
||||
child.kill('SIGTERM')
|
||||
}
|
||||
} catch {
|
||||
// Already gone.
|
||||
}
|
||||
}
|
||||
|
|
@ -38,15 +38,10 @@ import fsp from 'node:fs/promises'
|
|||
import https from 'node:https'
|
||||
import path from 'node:path'
|
||||
|
||||
import { hiddenWindowsChildOptions } from './windows-child-options'
|
||||
|
||||
const IS_WINDOWS = process.platform === 'win32'
|
||||
|
||||
function hiddenWindowsChildOptions(options = {}) {
|
||||
if (!IS_WINDOWS || Object.prototype.hasOwnProperty.call(options, 'windowsHide')) {
|
||||
return options
|
||||
}
|
||||
|
||||
return { ...options, windowsHide: true }
|
||||
}
|
||||
|
||||
const STAMP_COMMIT_RE = /^[0-9a-f]{7,40}$/i
|
||||
|
||||
|
|
|
|||
|
|
@ -1,5 +1,4 @@
|
|||
type Method = string
|
||||
import type { ExecFileSyncOptionsWithStringEncoding } from 'node:child_process'
|
||||
|
||||
import { execFile, execFileSync, spawn } from 'node:child_process'
|
||||
import crypto from 'node:crypto'
|
||||
import fs from 'node:fs'
|
||||
|
|
@ -30,6 +29,7 @@ import {
|
|||
} from 'electron'
|
||||
import nodePty from 'node-pty'
|
||||
|
||||
import { stopBackendChild as stopBackendChildImpl } from './backend-child'
|
||||
import { dashboardFallbackArgs, sourceDeclaresServe } from './backend-command'
|
||||
import { buildDesktopBackendEnv, normalizeHermesHomeRoot } from './backend-env'
|
||||
import { canImportHermesCli, verifyHermesCli } from './backend-probes'
|
||||
|
|
@ -125,6 +125,7 @@ import {
|
|||
MIN_HEIGHT as WINDOW_MIN_HEIGHT,
|
||||
MIN_WIDTH as WINDOW_MIN_WIDTH
|
||||
} from './window-state'
|
||||
import { hiddenWindowsChildOptions } from './windows-child-options'
|
||||
import { readWindowsUserEnvVar } from './windows-user-env'
|
||||
import { isPackagedInstallPath as isPackagedInstallPathUnderRoots } from './workspace-cwd'
|
||||
import { readWslWindowsClipboardImage } from './wsl-clipboard-image'
|
||||
|
|
@ -152,13 +153,6 @@ const APP_ROOT = app.getAppPath()
|
|||
// Dev (`npm run dev`) and prod both load the esbuild output from dist/.
|
||||
const PRELOAD_PATH = path.join(APP_ROOT, 'dist', 'electron-preload.js')
|
||||
|
||||
function hiddenWindowsChildOptions(options: any = {}): ExecFileSyncOptionsWithStringEncoding {
|
||||
if (!IS_WINDOWS || Object.prototype.hasOwnProperty.call(options, 'windowsHide')) {
|
||||
return options as any
|
||||
}
|
||||
|
||||
return { ...options, windowsHide: true } as any
|
||||
}
|
||||
|
||||
// Remote displays (SSH X11 forwarding, VNC, RDP) make Chromium's GPU
|
||||
// compositor flicker — accelerated layers can't be presented cleanly over the
|
||||
|
|
@ -6169,7 +6163,7 @@ async function fetchJsonForProfile(profile, path) {
|
|||
}
|
||||
|
||||
// Issue an arbitrary method against a profile's resolved backend, parsed JSON.
|
||||
async function requestJsonForProfile(profile: string, path: string, method: Method, body?: string) {
|
||||
async function requestJsonForProfile(profile: string, path: string, method: string, body?: string) {
|
||||
const conn = await ensureBackend(profile)
|
||||
const url = `${conn.baseUrl}${path}`
|
||||
const opts = { method, body, timeoutMs: DEFAULT_FETCH_TIMEOUT_MS }
|
||||
|
|
@ -6322,19 +6316,7 @@ function resetBootProgressForReconnect() {
|
|||
}
|
||||
|
||||
function stopBackendChild(child) {
|
||||
if (!child || child.killed) {
|
||||
return
|
||||
}
|
||||
|
||||
try {
|
||||
if (IS_WINDOWS && Number.isInteger(child.pid)) {
|
||||
forceKillProcessTree(child.pid)
|
||||
} else {
|
||||
child.kill('SIGTERM')
|
||||
}
|
||||
} catch {
|
||||
// Already gone.
|
||||
}
|
||||
stopBackendChildImpl(child, { forceKillProcessTree, isWindows: IS_WINDOWS })
|
||||
}
|
||||
|
||||
// Soft gateway-mode apply: tear down the primary without resetting boot UI or
|
||||
|
|
|
|||
131
apps/desktop/electron/windows-child-options.test.ts
Normal file
131
apps/desktop/electron/windows-child-options.test.ts
Normal file
|
|
@ -0,0 +1,131 @@
|
|||
import assert from 'node:assert/strict'
|
||||
import test from 'node:test'
|
||||
|
||||
import { stopBackendChild } from './backend-child'
|
||||
import { hiddenWindowsChildOptions } from './windows-child-options'
|
||||
|
||||
test('hiddenWindowsChildOptions adds windowsHide:true on Windows when unset', () => {
|
||||
assert.deepEqual(hiddenWindowsChildOptions({}, true), { windowsHide: true })
|
||||
})
|
||||
|
||||
test('hiddenWindowsChildOptions preserves an existing windowsHide:false on Windows', () => {
|
||||
assert.deepEqual(hiddenWindowsChildOptions({ windowsHide: false }, true), { windowsHide: false })
|
||||
})
|
||||
|
||||
test('hiddenWindowsChildOptions preserves an existing windowsHide:true on Windows', () => {
|
||||
assert.deepEqual(hiddenWindowsChildOptions({ windowsHide: true }, true), { windowsHide: true })
|
||||
})
|
||||
|
||||
test('hiddenWindowsChildOptions leaves options unchanged off Windows', () => {
|
||||
assert.deepEqual(hiddenWindowsChildOptions({}, false), {})
|
||||
assert.deepEqual(hiddenWindowsChildOptions({ stdio: 'ignore' }, false), { stdio: 'ignore' })
|
||||
})
|
||||
|
||||
test('hiddenWindowsChildOptions merges windowsHide alongside other options on Windows', () => {
|
||||
assert.deepEqual(hiddenWindowsChildOptions({ encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }, true), {
|
||||
encoding: 'utf8',
|
||||
stdio: ['ignore', 'pipe', 'ignore'],
|
||||
windowsHide: true
|
||||
})
|
||||
})
|
||||
|
||||
test('hiddenWindowsChildOptions defaults isWindows from process.platform when omitted', () => {
|
||||
const result = hiddenWindowsChildOptions({})
|
||||
const expectedHide = process.platform === 'win32'
|
||||
|
||||
assert.equal(Boolean(result.windowsHide), expectedHide)
|
||||
})
|
||||
|
||||
function makeChild(overrides: Partial<{ pid: number | null; killed: boolean }> = {}) {
|
||||
const calls: string[] = []
|
||||
|
||||
return {
|
||||
calls,
|
||||
child: {
|
||||
kill: (signal: string) => {
|
||||
calls.push(signal)
|
||||
},
|
||||
killed: overrides.killed ?? false,
|
||||
pid: 'pid' in overrides ? overrides.pid : 1234
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
test('stopBackendChild tree-kills on Windows when the child has a pid', () => {
|
||||
const { child, calls } = makeChild({ pid: 4242 })
|
||||
const treeKillCalls: number[] = []
|
||||
|
||||
stopBackendChild(child, {
|
||||
forceKillProcessTree: (pid: number) => treeKillCalls.push(pid),
|
||||
isWindows: true
|
||||
})
|
||||
|
||||
assert.deepEqual(treeKillCalls, [4242])
|
||||
assert.deepEqual(calls, [], 'SIGTERM must not be sent when the Windows tree-kill path is taken')
|
||||
})
|
||||
|
||||
test('stopBackendChild sends SIGTERM on non-Windows platforms', () => {
|
||||
const { child, calls } = makeChild({ pid: 4242 })
|
||||
const treeKillCalls: number[] = []
|
||||
|
||||
stopBackendChild(child, {
|
||||
forceKillProcessTree: (pid: number) => treeKillCalls.push(pid),
|
||||
isWindows: false
|
||||
})
|
||||
|
||||
assert.deepEqual(calls, ['SIGTERM'])
|
||||
assert.deepEqual(treeKillCalls, [], 'tree-kill must not run off Windows')
|
||||
})
|
||||
|
||||
test('stopBackendChild falls back to SIGTERM on Windows when the pid is not an integer', () => {
|
||||
const { child, calls } = makeChild({ pid: null })
|
||||
const treeKillCalls: number[] = []
|
||||
|
||||
stopBackendChild(child, {
|
||||
forceKillProcessTree: (pid: number) => treeKillCalls.push(pid),
|
||||
isWindows: true
|
||||
})
|
||||
|
||||
assert.deepEqual(calls, ['SIGTERM'])
|
||||
assert.deepEqual(treeKillCalls, [])
|
||||
})
|
||||
|
||||
test('stopBackendChild is a no-op for an already-killed child', () => {
|
||||
const { child, calls } = makeChild({ killed: true })
|
||||
const treeKillCalls: number[] = []
|
||||
|
||||
stopBackendChild(child, {
|
||||
forceKillProcessTree: (pid: number) => treeKillCalls.push(pid),
|
||||
isWindows: true
|
||||
})
|
||||
|
||||
assert.deepEqual(calls, [])
|
||||
assert.deepEqual(treeKillCalls, [])
|
||||
})
|
||||
|
||||
test('stopBackendChild is a no-op for a null/undefined child', () => {
|
||||
const treeKillCalls: number[] = []
|
||||
|
||||
assert.doesNotThrow(() => {
|
||||
stopBackendChild(null, { forceKillProcessTree: (pid: number) => treeKillCalls.push(pid), isWindows: true })
|
||||
stopBackendChild(undefined, { forceKillProcessTree: (pid: number) => treeKillCalls.push(pid), isWindows: true })
|
||||
})
|
||||
assert.deepEqual(treeKillCalls, [])
|
||||
})
|
||||
|
||||
test('stopBackendChild swallows errors thrown by the kill strategy', () => {
|
||||
const child = {
|
||||
kill: () => {
|
||||
throw new Error('ESRCH: no such process')
|
||||
},
|
||||
killed: false,
|
||||
pid: 99
|
||||
}
|
||||
|
||||
assert.doesNotThrow(() => {
|
||||
stopBackendChild(child, {
|
||||
forceKillProcessTree: () => {},
|
||||
isWindows: false
|
||||
})
|
||||
})
|
||||
})
|
||||
37
apps/desktop/electron/windows-child-options.ts
Normal file
37
apps/desktop/electron/windows-child-options.ts
Normal file
|
|
@ -0,0 +1,37 @@
|
|||
/**
|
||||
* windows-child-options.ts
|
||||
*
|
||||
* Shared helper for opting Windows child processes (spawn/execFileSync) into
|
||||
* a hidden console. Windows spawns a visible console window per child by
|
||||
* default; every desktop-launched helper process (git, curl, taskkill, the
|
||||
* backend itself, the bootstrap PowerShell runner, ...) needs `windowsHide:
|
||||
* true` so the user doesn't see consoles flashing on screen.
|
||||
*
|
||||
* Extracted into its own dependency-free module (no electron import) so it
|
||||
* can be unit-tested directly for both platforms without reading source
|
||||
* text, and so main.ts and bootstrap-runner.ts share exactly one
|
||||
* implementation instead of each defining their own copy.
|
||||
*/
|
||||
|
||||
import type { ExecFileSyncOptionsWithStringEncoding } from 'node:child_process'
|
||||
|
||||
/**
|
||||
* Merge `windowsHide: true` into `options` when running on Windows, unless
|
||||
* the caller already specified a `windowsHide` value (which is preserved
|
||||
* as-is, including an explicit `false` for cases that intentionally want a
|
||||
* visible/interactive console). No-op on non-Windows platforms.
|
||||
*
|
||||
* @param options - spawn/execFileSync options to (possibly) augment.
|
||||
* @param isWindows - defaults to the real platform check; injectable for
|
||||
* tests so both branches can be exercised without mocking process.platform.
|
||||
*/
|
||||
export function hiddenWindowsChildOptions(
|
||||
options: any = {},
|
||||
isWindows: boolean = process.platform === 'win32'
|
||||
): ExecFileSyncOptionsWithStringEncoding {
|
||||
if (!isWindows || Object.prototype.hasOwnProperty.call(options, 'windowsHide')) {
|
||||
return options as any
|
||||
}
|
||||
|
||||
return { ...options, windowsHide: true } as any
|
||||
}
|
||||
|
|
@ -38,7 +38,7 @@
|
|||
"test:desktop:nsis": "node scripts/test-desktop.mjs nsis",
|
||||
"test:desktop:existing": "node scripts/test-desktop.mjs existing",
|
||||
"test:desktop:fresh": "node scripts/test-desktop.mjs fresh",
|
||||
"test:desktop:platforms": "node --test electron/bootstrap-platform.test.ts electron/hardening.test.ts electron/backend-env.test.ts electron/backend-probes.test.ts electron/backend-ready.test.ts electron/bootstrap-runner.test.ts electron/connection-config.test.ts electron/dashboard-token.test.ts electron/gateway-ws-probe.test.ts electron/oauth-net-request.test.ts electron/desktop-uninstall.test.ts electron/session-windows.test.ts electron/link-title-window.test.ts electron/workspace-cwd.test.ts electron/fs-read-dir.test.ts electron/wsl-path-bridge.test.ts electron/git-root.test.ts electron/git-worktree-ops.test.ts electron/windows-child-process.test.ts electron/update-remote.test.ts electron/update-count.test.ts electron/update-rebuild.test.ts electron/update-marker.test.ts electron/update-relaunch.test.ts electron/windows-user-env.test.ts electron/wsl-clipboard-image.test.ts electron/titlebar-overlay-width.test.ts electron/window-state.test.ts electron/zoom.test.ts electron/windows-hermes-resolution.test.ts electron/oauth-session-request.test.ts",
|
||||
"test:desktop:platforms": "node --test electron/bootstrap-platform.test.ts electron/hardening.test.ts electron/backend-env.test.ts electron/backend-probes.test.ts electron/backend-ready.test.ts electron/bootstrap-runner.test.ts electron/connection-config.test.ts electron/dashboard-token.test.ts electron/gateway-ws-probe.test.ts electron/oauth-net-request.test.ts electron/desktop-uninstall.test.ts electron/session-windows.test.ts electron/link-title-window.test.ts electron/workspace-cwd.test.ts electron/fs-read-dir.test.ts electron/git-root.test.ts electron/git-worktree-ops.test.ts electron/windows-child-options.test.ts electron/update-remote.test.ts electron/update-count.test.ts electron/update-rebuild.test.ts electron/update-marker.test.ts electron/update-relaunch.test.ts electron/windows-user-env.test.ts electron/wsl-clipboard-image.test.ts electron/titlebar-overlay-width.test.ts electron/window-state.test.ts electron/zoom.test.ts electron/windows-hermes-resolution.test.ts electron/oauth-session-request.test.ts",
|
||||
"typecheck": "tsc -p . --noEmit && tsc -p tsconfig.electron.json --noEmit",
|
||||
"lint": "eslint src/ electron/",
|
||||
"lint:fix": "eslint src/ electron/ --fix",
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue