-
-
Notifications
You must be signed in to change notification settings - Fork 266
fix(utils): Replace sync device name resolver calls #376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| const { execFileMock, readFileMock, unlinkMock } = vi.hoisted(() => ({ | ||
| execFileMock: vi.fn(), | ||
| readFileMock: vi.fn(), | ||
| unlinkMock: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock('node:child_process', () => ({ | ||
| execFile: execFileMock, | ||
| })); | ||
|
|
||
| vi.mock('node:fs/promises', () => ({ | ||
| readFile: readFileMock, | ||
| unlink: unlinkMock, | ||
| })); | ||
|
Check warning on line 16 in src/utils/__tests__/device-name-resolver.test.ts
|
||
|
|
||
| async function flushAsyncWork(): Promise<void> { | ||
| await Promise.resolve(); | ||
| await new Promise((resolve) => setTimeout(resolve, 0)); | ||
| } | ||
|
|
||
| describe('device-name-resolver', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| vi.resetModules(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.useRealTimers(); | ||
| }); | ||
|
|
||
| it('loads device names asynchronously and caches resolved names', async () => { | ||
| execFileMock.mockImplementation( | ||
| ( | ||
| _file: string, | ||
| _args: string[], | ||
| _options: unknown, | ||
| callback: (error: Error | null, stdout: string, stderr: string) => void, | ||
| ) => { | ||
| callback(null, '', ''); | ||
| return {}; | ||
| }, | ||
| ); | ||
|
|
||
| readFileMock.mockResolvedValue( | ||
| JSON.stringify({ | ||
| result: { | ||
| devices: [ | ||
| { | ||
| identifier: 'device-1', | ||
| deviceProperties: { name: 'iPhone 15 Pro' }, | ||
| hardwareProperties: { udid: 'udid-1' }, | ||
| }, | ||
| ], | ||
| }, | ||
| }), | ||
| ); | ||
| unlinkMock.mockResolvedValue(undefined); | ||
|
|
||
| const { resolveDeviceName, formatDeviceId } = await import('../device-name-resolver.ts'); | ||
|
|
||
| expect(resolveDeviceName('device-1')).toBeUndefined(); | ||
| expect(execFileMock).toHaveBeenCalledTimes(1); | ||
|
|
||
| await flushAsyncWork(); | ||
|
|
||
| expect(resolveDeviceName('device-1')).toBe('iPhone 15 Pro'); | ||
| expect(resolveDeviceName('udid-1')).toBe('iPhone 15 Pro'); | ||
| expect(formatDeviceId('device-1')).toBe('iPhone 15 Pro (device-1)'); | ||
| expect(readFileMock).toHaveBeenCalledTimes(1); | ||
| expect(unlinkMock).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('does not spawn duplicate refreshes while one is in flight', async () => { | ||
| let callback: ((error: Error | null, stdout: string, stderr: string) => void) | undefined; | ||
|
|
||
| execFileMock.mockImplementation( | ||
| ( | ||
| _file: string, | ||
| _args: string[], | ||
| _options: unknown, | ||
| cb: (error: Error | null, stdout: string, stderr: string) => void, | ||
| ) => { | ||
| callback = cb; | ||
| return {}; | ||
| }, | ||
| ); | ||
|
|
||
| readFileMock.mockResolvedValue( | ||
| JSON.stringify({ | ||
| result: { devices: [{ identifier: 'device-2', deviceProperties: { name: 'iPad' } }] }, | ||
| }), | ||
| ); | ||
| unlinkMock.mockResolvedValue(undefined); | ||
|
|
||
| const { resolveDeviceName } = await import('../device-name-resolver.ts'); | ||
|
|
||
| expect(resolveDeviceName('device-2')).toBeUndefined(); | ||
| expect(resolveDeviceName('device-2')).toBeUndefined(); | ||
| expect(execFileMock).toHaveBeenCalledTimes(1); | ||
|
|
||
| callback?.(null, '', ''); | ||
| await flushAsyncWork(); | ||
|
|
||
| expect(resolveDeviceName('device-2')).toBe('iPad'); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,63 +1,93 @@ | ||
| import { execSync } from 'node:child_process'; | ||
| import { readFileSync, unlinkSync } from 'node:fs'; | ||
| import { execFile } from 'node:child_process'; | ||
| import { readFile, unlink } from 'node:fs/promises'; | ||
| import { tmpdir } from 'node:os'; | ||
| import { join } from 'node:path'; | ||
| import { promisify } from 'node:util'; | ||
|
|
||
| const CACHE_TTL_MS = 30_000; | ||
| const execFileAsync = promisify(execFile); | ||
|
|
||
| let cachedDevices: Map<string, string> | null = null; | ||
| let cacheTimestamp = 0; | ||
| let loadPromise: Promise<void> | null = null; | ||
|
|
||
| interface DeviceCtlEntry { | ||
| identifier: string; | ||
| deviceProperties: { name: string }; | ||
| hardwareProperties?: { udid?: string }; | ||
| } | ||
|
|
||
| function loadDeviceNames(): Map<string, string> { | ||
| if (cachedDevices && Date.now() - cacheTimestamp < CACHE_TTL_MS) { | ||
| return cachedDevices; | ||
| } | ||
| function cacheIsFresh(): boolean { | ||
| return cachedDevices !== null && Date.now() - cacheTimestamp < CACHE_TTL_MS; | ||
| } | ||
|
|
||
| function createDeviceMap(data: { result?: { devices?: DeviceCtlEntry[] } }): Map<string, string> { | ||
| const map = new Map<string, string>(); | ||
| const tmpFile = join(tmpdir(), `devicectl-list-${process.pid}.json`); | ||
|
|
||
| try { | ||
| execSync(`xcrun devicectl list devices --json-output ${tmpFile}`, { | ||
| encoding: 'utf8', | ||
| timeout: 10_000, | ||
| stdio: 'pipe', | ||
| }); | ||
|
|
||
| const data = JSON.parse(readFileSync(tmpFile, 'utf8')) as { | ||
| result?: { devices?: DeviceCtlEntry[] }; | ||
| }; | ||
|
|
||
| for (const device of data.result?.devices ?? []) { | ||
| const name = device.deviceProperties.name; | ||
| map.set(device.identifier, name); | ||
| if (device.hardwareProperties?.udid) { | ||
| map.set(device.hardwareProperties.udid, name); | ||
| } | ||
|
|
||
| for (const device of data.result?.devices ?? []) { | ||
| const name = device.deviceProperties.name; | ||
| map.set(device.identifier, name); | ||
| if (device.hardwareProperties?.udid) { | ||
| map.set(device.hardwareProperties.udid, name); | ||
| } | ||
| } catch { | ||
| // Device list unavailable -- return empty map, will fall back to UUID only | ||
| } finally { | ||
| } | ||
|
|
||
| return map; | ||
| } | ||
|
|
||
| async function refreshDeviceNames(): Promise<void> { | ||
| if (cacheIsFresh()) { | ||
| return; | ||
| } | ||
|
|
||
| if (loadPromise) { | ||
| return loadPromise; | ||
| } | ||
|
|
||
| const tmpFile = join(tmpdir(), `devicectl-list-${process.pid}-${Date.now()}.json`); | ||
|
|
||
| loadPromise = (async () => { | ||
| try { | ||
| unlinkSync(tmpFile); | ||
| await execFileAsync('xcrun', ['devicectl', 'list', 'devices', '--json-output', tmpFile], { | ||
| encoding: 'utf8', | ||
| timeout: 10_000, | ||
| }); | ||
|
|
||
| const data = JSON.parse(await readFile(tmpFile, 'utf8')) as { | ||
| result?: { devices?: DeviceCtlEntry[] }; | ||
| }; | ||
|
|
||
| cachedDevices = createDeviceMap(data); | ||
| cacheTimestamp = Date.now(); | ||
| } catch { | ||
| // ignore | ||
| // Device list unavailable -- keep existing cache and fall back to UUID only | ||
| if (cachedDevices === null) { | ||
| cachedDevices = new Map(); | ||
| cacheTimestamp = Date.now(); | ||
| } | ||
| } finally { | ||
| loadPromise = null; | ||
| try { | ||
| await unlink(tmpFile); | ||
| } catch { | ||
|
Check warning on line 72 in src/utils/device-name-resolver.ts
|
||
| // ignore | ||
| } | ||
| } | ||
|
Check warning on line 75 in src/utils/device-name-resolver.ts
|
||
|
Comment on lines
+68
to
75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Race condition clears loadPromise before unlink completes, allowing duplicate xcrun invocations In refreshDeviceNames, the IIFE sets VerificationTraced the IIFE assigned to loadPromise: the outer Identified by Warden |
||
| } | ||
| })(); | ||
|
Check warning on line 76 in src/utils/device-name-resolver.ts
|
||
|
|
||
| cachedDevices = map; | ||
| cacheTimestamp = Date.now(); | ||
| return map; | ||
| return loadPromise; | ||
| } | ||
|
Check warning on line 79 in src/utils/device-name-resolver.ts
|
||
|
|
||
| function ensureDeviceNamesRefresh(): void { | ||
| void refreshDeviceNames(); | ||
| } | ||
|
|
||
| export function resolveDeviceName(deviceId: string): string | undefined { | ||
| const names = loadDeviceNames(); | ||
| return names.get(deviceId); | ||
| if (!cacheIsFresh()) { | ||
|
Check warning on line 86 in src/utils/device-name-resolver.ts
|
||
|
sentry-warden[bot] marked this conversation as resolved.
|
||
| ensureDeviceNamesRefresh(); | ||
| } | ||
|
|
||
| return cachedDevices?.get(deviceId); | ||
| } | ||
|
|
||
| export function formatDeviceId(deviceId: string): string { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test mocks node:child_process and node:fs/promises directly instead of injecting executors
The test uses
vi.mock('node:child_process')andvi.mock('node:fs/promises')to interceptexecFile,readFile, andunlinkrather than injecting aCommandExecutor/FileSystemExecutoras the project's test guardrails require. A repo-wide grep shows this is the only test file mockingnode:child_processthis way — every other unit test goes throughcreateMockExecutorfromsrc/test-utils/mock-executors.ts. The underlying cause is thatsrc/utils/device-name-resolver.tsimportsexecFile/readFile/unlinkdirectly at module scope and offers no injection seam, so the test cannot follow the standard pattern; this couples the suite to Node internals and bypasses the executor mock helpers the guardrails mandate.Verification
Read src/utils/device-name-resolver.ts and confirmed it directly imports execFile from node:child_process and readFile/unlink from node:fs/promises with no executor parameter. Read src/test-utils/mock-executors.ts and confirmed createMockExecutor / FileSystemExecutor are the project's standard injection helpers. Ran a grep for
vi.mock('node:child_process')across src/ — this new test is the only match, confirming it deviates from the established pattern.Identified by Warden
xcodebuildmcp-test-boundary-review·TZF-HCM