From 5bba15b0384f184686e511e75da8275adb056d8c Mon Sep 17 00:00:00 2001 From: kookyleo Date: Sat, 23 Aug 2025 12:43:03 +0800 Subject: [PATCH] fix(cli): Improve proxy test isolation and sandbox path resolution (#6555) --- packages/cli/src/config/config.test.ts | 141 +++++++++++------- packages/cli/src/config/config.ts | 1 + packages/cli/src/gemini.test.tsx | 97 ++++++++++++ packages/cli/src/gemini.tsx | 69 +++++---- .../src/ui/components/InputPrompt.test.tsx | 12 +- packages/cli/src/utils/sandbox.ts | 1 - 6 files changed, 230 insertions(+), 91 deletions(-) diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 87afca43..2f7c574c 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -328,71 +328,98 @@ describe('loadCliConfig', () => { expect(config.getShowMemoryUsage()).toBe(true); }); - it(`should leave proxy to empty by default`, async () => { - process.argv = ['node', 'script.js']; - const argv = await parseArguments(); - const settings: Settings = {}; - const config = await loadCliConfig(settings, [], 'test-session', argv); - expect(config.getProxy()).toBeFalsy(); - }); + describe('Proxy configuration', () => { + const originalProxyEnv: { [key: string]: string | undefined } = {}; + const proxyEnvVars = [ + 'HTTP_PROXY', + 'HTTPS_PROXY', + 'http_proxy', + 'https_proxy', + ]; - const proxy_url = 'http://localhost:7890'; - const testCases = [ - { - input: { - env_name: 'https_proxy', - proxy_url, - }, - expected: proxy_url, - }, - { - input: { - env_name: 'http_proxy', - proxy_url, - }, - expected: proxy_url, - }, - { - input: { - env_name: 'HTTPS_PROXY', - proxy_url, - }, - expected: proxy_url, - }, - { - input: { - env_name: 'HTTP_PROXY', - proxy_url, - }, - expected: proxy_url, - }, - ]; - testCases.forEach(({ input, expected }) => { - it(`should set proxy to ${expected} according to environment variable [${input.env_name}]`, async () => { - vi.stubEnv(input.env_name, input.proxy_url); + beforeEach(() => { + for (const key of proxyEnvVars) { + originalProxyEnv[key] = process.env[key]; + delete process.env[key]; + } + }); + + afterEach(() => { + for (const key of proxyEnvVars) { + if (originalProxyEnv[key]) { + process.env[key] = originalProxyEnv[key]; + } else { + delete process.env[key]; + } + } + }); + + it(`should leave proxy to empty by default`, async () => { process.argv = ['node', 'script.js']; const argv = await parseArguments(); const settings: Settings = {}; const config = await loadCliConfig(settings, [], 'test-session', argv); - expect(config.getProxy()).toBe(expected); + expect(config.getProxy()).toBeFalsy(); }); - }); - it('should set proxy when --proxy flag is present', async () => { - process.argv = ['node', 'script.js', '--proxy', 'http://localhost:7890']; - const argv = await parseArguments(); - const settings: Settings = {}; - const config = await loadCliConfig(settings, [], 'test-session', argv); - expect(config.getProxy()).toBe('http://localhost:7890'); - }); + const proxy_url = 'http://localhost:7890'; + const testCases = [ + { + input: { + env_name: 'https_proxy', + proxy_url, + }, + expected: proxy_url, + }, + { + input: { + env_name: 'http_proxy', + proxy_url, + }, + expected: proxy_url, + }, + { + input: { + env_name: 'HTTPS_PROXY', + proxy_url, + }, + expected: proxy_url, + }, + { + input: { + env_name: 'HTTP_PROXY', + proxy_url, + }, + expected: proxy_url, + }, + ]; + testCases.forEach(({ input, expected }) => { + it(`should set proxy to ${expected} according to environment variable [${input.env_name}]`, async () => { + vi.stubEnv(input.env_name, input.proxy_url); + process.argv = ['node', 'script.js']; + const argv = await parseArguments(); + const settings: Settings = {}; + const config = await loadCliConfig(settings, [], 'test-session', argv); + expect(config.getProxy()).toBe(expected); + }); + }); - it('should prioritize CLI flag over environment variable for proxy (CLI http://localhost:7890, environment variable http://localhost:7891)', async () => { - vi.stubEnv('http_proxy', 'http://localhost:7891'); - process.argv = ['node', 'script.js', '--proxy', 'http://localhost:7890']; - const argv = await parseArguments(); - const settings: Settings = {}; - const config = await loadCliConfig(settings, [], 'test-session', argv); - expect(config.getProxy()).toBe('http://localhost:7890'); + it('should set proxy when --proxy flag is present', async () => { + process.argv = ['node', 'script.js', '--proxy', 'http://localhost:7890']; + const argv = await parseArguments(); + const settings: Settings = {}; + const config = await loadCliConfig(settings, [], 'test-session', argv); + expect(config.getProxy()).toBe('http://localhost:7890'); + }); + + it('should prioritize CLI flag over environment variable for proxy (CLI http://localhost:7890, environment variable http://localhost:7891)', async () => { + vi.stubEnv('http_proxy', 'http://localhost:7891'); + process.argv = ['node', 'script.js', '--proxy', 'http://localhost:7890']; + const argv = await parseArguments(); + const settings: Settings = {}; + const config = await loadCliConfig(settings, [], 'test-session', argv); + expect(config.getProxy()).toBe('http://localhost:7890'); + }); }); }); diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 498d0255..4b7510f9 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -78,6 +78,7 @@ export interface CliArgs { export async function parseArguments(): Promise { const yargsInstance = yargs(hideBin(process.argv)) + .locale('en') .scriptName('gemini') .usage( 'Usage: gemini [options] [command]\n\nGemini CLI - Launch an interactive CLI, use -p/--prompt for non-interactive mode', diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index a22f1ff9..1b1a7679 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -10,6 +10,7 @@ import { main, setupUnhandledRejectionHandler, validateDnsResolutionOrder, + startInteractiveUI, } from './gemini.js'; import { LoadedSettings, @@ -17,6 +18,7 @@ import { loadSettings, } from './config/settings.js'; import { appEvents, AppEvent } from './utils/events.js'; +import { Config } from '@google/gemini-cli-core'; // Custom error to identify mock process.exit calls class MockProcessExitError extends Error { @@ -251,3 +253,98 @@ describe('validateDnsResolutionOrder', () => { ); }); }); + +describe('startInteractiveUI', () => { + // Mock dependencies + const mockConfig = { + getProjectRoot: () => '/root', + getScreenReader: () => false, + } as Config; + const mockSettings = { + merged: { + hideWindowTitle: false, + }, + } as LoadedSettings; + const mockStartupWarnings = ['warning1']; + const mockWorkspaceRoot = '/root'; + + vi.mock('./utils/version.js', () => ({ + getCliVersion: vi.fn(() => Promise.resolve('1.0.0')), + })); + + vi.mock('./ui/utils/kittyProtocolDetector.js', () => ({ + detectAndEnableKittyProtocol: vi.fn(() => Promise.resolve()), + })); + + vi.mock('./ui/utils/updateCheck.js', () => ({ + checkForUpdates: vi.fn(() => Promise.resolve(null)), + })); + + vi.mock('./utils/cleanup.js', () => ({ + cleanupCheckpoints: vi.fn(() => Promise.resolve()), + registerCleanup: vi.fn(), + })); + + vi.mock('ink', () => ({ + render: vi.fn().mockReturnValue({ unmount: vi.fn() }), + })); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should render the UI with proper React context and exitOnCtrlC disabled', async () => { + const { render } = await import('ink'); + const renderSpy = vi.mocked(render); + + await startInteractiveUI( + mockConfig, + mockSettings, + mockStartupWarnings, + mockWorkspaceRoot, + ); + + // Verify render was called with correct options + expect(renderSpy).toHaveBeenCalledTimes(1); + const [reactElement, options] = renderSpy.mock.calls[0]; + + // Verify render options + expect(options).toEqual({ + exitOnCtrlC: false, + isScreenReaderEnabled: false, + }); + + // Verify React element structure is valid (but don't deep dive into JSX internals) + expect(reactElement).toBeDefined(); + }); + + it('should perform all startup tasks in correct order', async () => { + const { getCliVersion } = await import('./utils/version.js'); + const { detectAndEnableKittyProtocol } = await import( + './ui/utils/kittyProtocolDetector.js' + ); + const { checkForUpdates } = await import('./ui/utils/updateCheck.js'); + const { registerCleanup } = await import('./utils/cleanup.js'); + + await startInteractiveUI( + mockConfig, + mockSettings, + mockStartupWarnings, + mockWorkspaceRoot, + ); + + // Verify all startup tasks were called + expect(getCliVersion).toHaveBeenCalledTimes(1); + expect(detectAndEnableKittyProtocol).toHaveBeenCalledTimes(1); + expect(registerCleanup).toHaveBeenCalledTimes(1); + + // Verify cleanup handler is registered with unmount function + const cleanupFn = vi.mocked(registerCleanup).mock.calls[0][0]; + expect(typeof cleanupFn).toBe('function'); + + // checkForUpdates should be called asynchronously (not waited for) + // We need a small delay to let it execute + await new Promise((resolve) => setTimeout(resolve, 0)); + expect(checkForUpdates).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index f9cacfb7..7e8b10a9 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -132,6 +132,44 @@ ${reason.stack}` }); } +export async function startInteractiveUI( + config: Config, + settings: LoadedSettings, + startupWarnings: string[], + workspaceRoot: string, +) { + const version = await getCliVersion(); + // Detect and enable Kitty keyboard protocol once at startup + await detectAndEnableKittyProtocol(); + setWindowTitle(basename(workspaceRoot), settings); + const instance = render( + + + + + , + { exitOnCtrlC: false, isScreenReaderEnabled: config.getScreenReader() }, + ); + + checkForUpdates() + .then((info) => { + handleAutoUpdate(info, settings, config.getProjectRoot()); + }) + .catch((err) => { + // Silently ignore update check errors. + if (config.getDebugMode()) { + console.error('Update check failed:', err); + } + }); + + registerCleanup(() => instance.unmount()); +} + export async function main() { setupUnhandledRejectionHandler(); const workspaceRoot = process.cwd(); @@ -301,36 +339,7 @@ export async function main() { // Render UI, passing necessary config values. Check that there is no command line question. if (config.isInteractive()) { - const version = await getCliVersion(); - // Detect and enable Kitty keyboard protocol once at startup - await detectAndEnableKittyProtocol(); - setWindowTitle(basename(workspaceRoot), settings); - const instance = render( - - - - - , - { exitOnCtrlC: false, isScreenReaderEnabled: config.getScreenReader() }, - ); - - checkForUpdates() - .then((info) => { - handleAutoUpdate(info, settings, config.getProjectRoot()); - }) - .catch((err) => { - // Silently ignore update check errors. - if (config.getDebugMode()) { - console.error('Update check failed:', err); - } - }); - - registerCleanup(() => instance.unmount()); + await startInteractiveUI(config, settings, startupWarnings, workspaceRoot); return; } // If not a TTY, read from stdin diff --git a/packages/cli/src/ui/components/InputPrompt.test.tsx b/packages/cli/src/ui/components/InputPrompt.test.tsx index c6fba806..608b84d0 100644 --- a/packages/cli/src/ui/components/InputPrompt.test.tsx +++ b/packages/cli/src/ui/components/InputPrompt.test.tsx @@ -1422,11 +1422,17 @@ describe('InputPrompt', () => { ); stdin.write('\x12'); await wait(); + // Verify reverse search is active + expect(stdout.lastFrame()).toContain('(r:)'); + stdin.write('\t'); - await waitFor(() => { - expect(stdout.lastFrame()).not.toContain('(r:)'); - }); + await waitFor( + () => { + expect(stdout.lastFrame()).not.toContain('(r:)'); + }, + { timeout: 5000 }, + ); // Increase timeout expect(props.buffer.setText).toHaveBeenCalledWith('echo hello'); unmount(); diff --git a/packages/cli/src/utils/sandbox.ts b/packages/cli/src/utils/sandbox.ts index d5c2be08..ae8ab4cf 100644 --- a/packages/cli/src/utils/sandbox.ts +++ b/packages/cli/src/utils/sandbox.ts @@ -206,7 +206,6 @@ export async function start_sandbox( let profileFile = fileURLToPath( new URL(`sandbox-macos-${profile}.sb`, import.meta.url), ); - // if profile name is not recognized, then look for file under project settings directory if (!BUILTIN_SEATBELT_PROFILES.includes(profile)) { profileFile = path.join(