test(performance): refactor input prompt tests into multiple files#25499
test(performance): refactor input prompt tests into multiple files#25499
Conversation
|
Hi @jacob314, thank you so much for your contribution to Gemini CLI! We really appreciate the time and effort you've put into this. We're making some updates to our contribution process to improve how we track and review changes. Please take a moment to review our recent discussion post: Improving Our Contribution Process & Introducing New Guidelines. Key Update: Starting January 26, 2026, the Gemini CLI project will require all pull requests to be associated with an existing issue. Any pull requests not linked to an issue by that date will be automatically closed. Thank you for your understanding and for being a part of our community! |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the test architecture for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Size Change: -4 B (0%) Total Size: 33.6 MB
ℹ️ View Unchanged
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the InputPrompt tests by extracting common setup logic and mock definitions into a new InputPrompt.test.helpers.tsx file and splitting the large test suite into smaller, domain-specific files (Background Color, Highlighting, IME Cursor, and Snapshots). While the reorganization improves maintainability, several issues were identified regarding type safety and mock consistency. Specifically, the refactoring introduced multiple instances of the any type where specific hook return types were previously used, and some tests were updated to bypass the mock buffer's setText logic by directly assigning to the text property, which can lead to inconsistent internal state in the mocks.
| import { useShellHistory } from '../hooks/useShellHistory.js'; | ||
| import { | ||
| useCommandCompletion, | ||
| CompletionMode, | ||
| type UseCommandCompletionReturn, | ||
| } from '../hooks/useCommandCompletion.js'; | ||
| import { | ||
| useInputHistory, | ||
| type UseInputHistoryReturn, | ||
| } from '../hooks/useInputHistory.js'; | ||
| import { | ||
| useReverseSearchCompletion, | ||
| type UseReverseSearchCompletionReturn, | ||
| } from '../hooks/useReverseSearchCompletion.js'; | ||
| import { useInputHistory } from '../hooks/useInputHistory.js'; | ||
| import { useReverseSearchCompletion } from '../hooks/useReverseSearchCompletion.js'; |
There was a problem hiding this comment.
The refactoring removed several important type imports that were previously used for mock variable declarations. Restoring these types will improve type safety and maintainability.
| import { useShellHistory } from '../hooks/useShellHistory.js'; | |
| import { | |
| useCommandCompletion, | |
| CompletionMode, | |
| type UseCommandCompletionReturn, | |
| } from '../hooks/useCommandCompletion.js'; | |
| import { | |
| useInputHistory, | |
| type UseInputHistoryReturn, | |
| } from '../hooks/useInputHistory.js'; | |
| import { | |
| useReverseSearchCompletion, | |
| type UseReverseSearchCompletionReturn, | |
| } from '../hooks/useReverseSearchCompletion.js'; | |
| import { useInputHistory } from '../hooks/useInputHistory.js'; | |
| import { useReverseSearchCompletion } from '../hooks/useReverseSearchCompletion.js'; | |
| import { useShellHistory, type UseShellHistoryReturn } from '../hooks/useShellHistory.js'; | |
| import { | |
| useCommandCompletion, | |
| CompletionMode, | |
| type UseCommandCompletionReturn, | |
| } from '../hooks/useCommandCompletion.js'; | |
| import { useInputHistory, type UseInputHistoryReturn } from '../hooks/useInputHistory.js'; | |
| import { useReverseSearchCompletion, type UseReverseSearchCompletionReturn } from '../hooks/useReverseSearchCompletion.js'; |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| let mockShellHistory: any; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| let mockCommandCompletion: any; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| let mockInputHistory: any; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| let mockReverseSearchCompletion: any; |
There was a problem hiding this comment.
Using any for these mock variables is a regression in type safety. Please use the proper return types from the respective hooks to ensure the mocks remain consistent with the actual implementations.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| let mockShellHistory: any; | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| let mockCommandCompletion: any; | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| let mockInputHistory: any; | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| let mockReverseSearchCompletion: any; | |
| let mockShellHistory: UseShellHistoryReturn; | |
| let mockCommandCompletion: UseCommandCompletionReturn; | |
| let mockInputHistory: UseInputHistoryReturn; | |
| let mockReverseSearchCompletion: UseReverseSearchCompletionReturn; |
| }; | ||
| mockedUseInputHistory.mockImplementation(({ onSubmit }) => { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| mockedUseInputHistory.mockImplementation(({ onSubmit }: any) => { |
There was a problem hiding this comment.
The use of any here is unnecessary as the type can be inferred or explicitly defined. Removing it maintains better type safety.
| mockedUseInputHistory.mockImplementation(({ onSubmit }: any) => { | |
| mockedUseInputHistory.mockImplementation(({ onSubmit }) => { |
| await act(async () => { | ||
| props.buffer.setText('some text to clear'); | ||
| }); | ||
| props.buffer.text = 'some text to clear'; |
There was a problem hiding this comment.
Directly assigning to buffer.text bypasses the setText logic in the mock buffer, which is responsible for updating other dependent fields like lines, visualCursor, and transformationsByLine. This can lead to inconsistent state during tests. Use setText instead.
| props.buffer.text = 'some text to clear'; | |
| props.buffer.setText('some text to clear'); |
| const onEscapePromptChange = vi.fn(); | ||
| props.onEscapePromptChange = onEscapePromptChange; | ||
| props.buffer.setText('text to clear'); | ||
| props.buffer.text = 'text to clear'; |
There was a problem hiding this comment.
Directly assigning to buffer.text bypasses the setText logic in the mock buffer. Use setText to ensure all dependent fields in the mock are correctly updated.
| props.buffer.text = 'text to clear'; | |
| props.buffer.setText('text to clear'); |
| import * as path from 'node:path'; | ||
| import { CommandKind, type SlashCommand } from '../commands/types.js'; | ||
| import { StreamingState } from '../types.js'; | ||
| import { terminalCapabilityManager } from '../utils/terminalCapabilityManager.js'; |
There was a problem hiding this comment.
The Key type is needed for the handleInput mock implementation to maintain type safety.
| import { terminalCapabilityManager } from '../utils/terminalCapabilityManager.js'; | |
| import { terminalCapabilityManager } from '../utils/terminalCapabilityManager.js'; | |
| import { type Key } from '../hooks/useKeypress.js'; |
| visualScrollRow: 0, | ||
| viewportHeight: 10, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| handleInput: vi.fn((key: any) => { |
There was a problem hiding this comment.
Using any for the key parameter reduces type safety. Please use the Key type.
| handleInput: vi.fn((key: any) => { | |
| handleInput: vi.fn((key: Key) => { |
gemini-cli
bot
added
the
priority/p1
1 participant
Summary
Details
Related Issues
How to Validate
Pre-Merge Checklist