fix(tests): sync test files to proto rename and API changes#5
Merged
Conversation
…ead of full task list Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Update all test files broken by the .qwen→.proto migration and related code changes — no production source files changed. - Replace all `.qwen`/`Qwen` path/header refs with `.proto`/`Proto` - Add missing `getTelemetryLogPromptsEnabled` mock in anthropic tests - Update `returnDisplay` assertions to match new `Skill loaded: name — desc` format - Fix skill-manager base dir count (3→4, new .proto/skills dir) - Fix hook assertions to include `toolInput` in context object - Fix subagent count (3→4, new coordinator builtin) - Fix gemini generator test to use `toBeDefined()` over reference equality Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughAdds a TaskUpdate diff result type and UI renderer, updates TaskUpdate tool to compute field diffs, and applies widespread test expectation updates (branding path Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant TaskUpdate as TaskUpdateTool
participant Store as TaskStore
participant Renderer as UI/Display
Client->>TaskUpdate: createInvocation(updatePayload)
TaskUpdate->>Store: fetch existing task (before)
Store-->>TaskUpdate: return before
TaskUpdate->>Store: update task with new values
Store-->>TaskUpdate: return after
TaskUpdate->>TaskUpdate: compare before vs after (status,title,priority,description)
TaskUpdate->>Renderer: produce TaskUpdateDiffDisplay {taskId, title, changes}
Renderer-->>Client: return/display task_update_diff
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/ui/hooks/useGeminiStream.test.tsx (1)
2069-2089:⚠️ Potential issue | 🟡 MinorMissing new
drainQueuedMessagesargument on oneuseGeminiStreaminvocation.Line 2089 call still omits the final callback parameter, unlike the other updated call sites. This leaves that test path out of sync with the new API contract.
🔧 Proposed fix
useGeminiStream( mockConfig.getGeminiClient() as GeminiClient, [], mockAddItem, mockConfig, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, false, // shellModeActive vi.fn(), // getPreferredEditor vi.fn(), // onAuthError vi.fn(), // performMemoryRefresh false, // modelSwitched vi.fn(), // setModelSwitched vi.fn(), // onEditorClose vi.fn(), // onCancelSubmit vi.fn(), // setShellInputFocused 80, // terminalWidth 24, // terminalHeight + () => [], ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/hooks/useGeminiStream.test.tsx` around lines 2069 - 2089, The test invocation of useGeminiStream is missing the new final drainQueuedMessages callback argument; update the renderHook call that constructs useGeminiStream (the one passing mockConfig.getGeminiClient(), mockAddItem, mockConfig, mockLoadedSettings, etc.) to include a mock drainQueuedMessages function (e.g., vi.fn() or the existing mockDrainQueuedMessages) as the last parameter so the call site matches the updated useGeminiStream signature.
🧹 Nitpick comments (5)
packages/core/src/core/geminiContentGenerator/geminiContentGenerator.test.ts (1)
122-122: Strengthen the stream assertion to validate behavior, not just existenceOn Line 122,
toBeDefined()is too permissive and can miss regressions (e.g., wrong object shape). Since the method now returns a wrapped stream, assert that it still behaves as an async generator and yields the expected chunk.Proposed test adjustment
- expect(stream).toBeDefined(); + expect(stream).toBeDefined(); + const firstChunk = await stream.next(); + expect(firstChunk.done).toBe(false); + expect(firstChunk.value).toEqual({ responseId: '1' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/core/geminiContentGenerator/geminiContentGenerator.test.ts` at line 122, Replace the weak existence assertion on the returned "stream" with a behavioral check that it is an async generator and yields the expected chunk: verify that stream[Symbol.asyncIterator] is a function (or use for await... to consume) and assert the first yielded value matches the expected chunk content (and that done is false), e.g., call stream[Symbol.asyncIterator]().next() and expect the returned { value, done } to have done === false and value containing the expected string/marker; update the test around the "stream" variable accordingly to validate behavior rather than just presence.packages/core/src/ide/ide-client.test.ts (1)
134-134: Consider deriving expected paths dynamically for consistency.For improved resilience to future directory name changes, consider deriving the expected lock file paths using
Storage.getGlobalIdeDir()similar to howlogger.test.tsnow usesStorage.getRuntimeBaseDir(). This would make the test self-updating if the directory constant changes again.Example:
expect(fs.promises.readFile).toHaveBeenCalledWith( path.join(Storage.getGlobalIdeDir(), '8080.lock'), 'utf8', );This is a low-priority improvement since the current hardcoded approach is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ide/ide-client.test.ts` at line 134, Update the test to derive the expected lock file path using Storage.getGlobalIdeDir() instead of a hardcoded '/home/test/.proto/ide' path; locate the assertion in ide-client.test.ts that checks fs.promises.readFile (the call that currently expects path.join('/home/test', '.proto', 'ide', '8080.lock')) and replace the hardcoded path with path.join(Storage.getGlobalIdeDir(), '8080.lock') so the test follows the same runtime directory constant as the rest of the suite.packages/core/src/extension/extensionManager.test.ts (1)
103-103: Consider importingDEFAULT_CONTEXT_FILENAMEfor maintainability.The test hardcodes
'PROTO.md'in multiple places (lines 103, 195, 231, 240). While this matches the current production code, importingDEFAULT_CONTEXT_FILENAMEfrommemoryTool.tswould make the test more resilient to future filename changes.♻️ Optional: Import and use the constant
import { INSTALL_METADATA_FILENAME, EXTENSIONS_CONFIG_FILENAME, } from './variables.js'; import { QWEN_DIR } from '../config/storage.js'; +import { DEFAULT_CONTEXT_FILENAME } from '../tools/memoryTool.js';Then replace hardcoded
'PROTO.md'strings withDEFAULT_CONTEXT_FILENAME.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/extension/extensionManager.test.ts` at line 103, Import DEFAULT_CONTEXT_FILENAME from the module that defines it (memoryTool.ts) and replace hardcoded 'PROTO.md' usages in the test with that constant (e.g., change fs.writeFileSync(path.join(extDir, 'PROTO.md'), 'context') to use DEFAULT_CONTEXT_FILENAME and update the other occurrences in the same test file), ensuring tests reference DEFAULT_CONTEXT_FILENAME consistently so future filename changes in memoryTool.ts won't break tests.packages/core/src/tools/memoryTool.test.ts (1)
34-40: Mock setup is correct; consider importingMEMORY_SECTION_HEADER.The
vi.hoistedpattern for mockingcreateMemoryis correctly implemented. However,MEMORY_SECTION_HEADERis hardcoded here while it's already exported frommemoryTool.ts. Importing it would ensure consistency with the production code.♻️ Optional: Import the constant
import { MemoryTool, setGeminiMdFilename, getCurrentGeminiMdFilename, getAllGeminiMdFilenames, DEFAULT_CONTEXT_FILENAME, + MEMORY_SECTION_HEADER, } from './memoryTool.js'; // ... -const MEMORY_SECTION_HEADER = '## Proto Added Memories';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/tools/memoryTool.test.ts` around lines 34 - 40, The test hardcodes the MEMORY_SECTION_HEADER string instead of reusing the exported constant from the implementation; update the test to import MEMORY_SECTION_HEADER from memoryTool.ts and replace the hardcoded '## Proto Added Memories' value with the imported constant so the test stays consistent with the production code (keep the existing mockCreateMemory/vi.hoisted setup for createMemory from memoryStore.js).packages/core/src/tools/task-update.test.ts (1)
37-37: Unused mock method.The
listmock is set up but never used in any test, since theTaskUpdateToolimplementation no longer callsstore.list()after the refactor totask_update_diff. This is harmless but could be removed for clarity.♻️ Optional: Remove unused mock
function makeConfig(before: Task | null, after: Task | null) { const store = { get: vi.fn().mockReturnValue(before ?? undefined), update: vi.fn().mockReturnValue(after ?? undefined), - list: vi.fn().mockReturnValue(after ? [after] : []), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/tools/task-update.test.ts` at line 37, The test includes an unused mock method `list` on the mocked store (see the `list: vi.fn().mockReturnValue(after ? [after] : [])` line) which is no longer called by `TaskUpdateTool` after the `task_update_diff` refactor; remove that `list` mock from the mock object in packages/core/src/tools/task-update.test.ts to clean up the test setup and avoid dead mocks (keep other mocks for methods still used by `TaskUpdateTool`).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/cli/src/ui/hooks/useGeminiStream.test.tsx`:
- Around line 2069-2089: The test invocation of useGeminiStream is missing the
new final drainQueuedMessages callback argument; update the renderHook call that
constructs useGeminiStream (the one passing mockConfig.getGeminiClient(),
mockAddItem, mockConfig, mockLoadedSettings, etc.) to include a mock
drainQueuedMessages function (e.g., vi.fn() or the existing
mockDrainQueuedMessages) as the last parameter so the call site matches the
updated useGeminiStream signature.
---
Nitpick comments:
In
`@packages/core/src/core/geminiContentGenerator/geminiContentGenerator.test.ts`:
- Line 122: Replace the weak existence assertion on the returned "stream" with a
behavioral check that it is an async generator and yields the expected chunk:
verify that stream[Symbol.asyncIterator] is a function (or use for await... to
consume) and assert the first yielded value matches the expected chunk content
(and that done is false), e.g., call stream[Symbol.asyncIterator]().next() and
expect the returned { value, done } to have done === false and value containing
the expected string/marker; update the test around the "stream" variable
accordingly to validate behavior rather than just presence.
In `@packages/core/src/extension/extensionManager.test.ts`:
- Line 103: Import DEFAULT_CONTEXT_FILENAME from the module that defines it
(memoryTool.ts) and replace hardcoded 'PROTO.md' usages in the test with that
constant (e.g., change fs.writeFileSync(path.join(extDir, 'PROTO.md'),
'context') to use DEFAULT_CONTEXT_FILENAME and update the other occurrences in
the same test file), ensuring tests reference DEFAULT_CONTEXT_FILENAME
consistently so future filename changes in memoryTool.ts won't break tests.
In `@packages/core/src/ide/ide-client.test.ts`:
- Line 134: Update the test to derive the expected lock file path using
Storage.getGlobalIdeDir() instead of a hardcoded '/home/test/.proto/ide' path;
locate the assertion in ide-client.test.ts that checks fs.promises.readFile (the
call that currently expects path.join('/home/test', '.proto', 'ide',
'8080.lock')) and replace the hardcoded path with
path.join(Storage.getGlobalIdeDir(), '8080.lock') so the test follows the same
runtime directory constant as the rest of the suite.
In `@packages/core/src/tools/memoryTool.test.ts`:
- Around line 34-40: The test hardcodes the MEMORY_SECTION_HEADER string instead
of reusing the exported constant from the implementation; update the test to
import MEMORY_SECTION_HEADER from memoryTool.ts and replace the hardcoded '##
Proto Added Memories' value with the imported constant so the test stays
consistent with the production code (keep the existing
mockCreateMemory/vi.hoisted setup for createMemory from memoryStore.js).
In `@packages/core/src/tools/task-update.test.ts`:
- Line 37: The test includes an unused mock method `list` on the mocked store
(see the `list: vi.fn().mockReturnValue(after ? [after] : [])` line) which is no
longer called by `TaskUpdateTool` after the `task_update_diff` refactor; remove
that `list` mock from the mock object in
packages/core/src/tools/task-update.test.ts to clean up the test setup and avoid
dead mocks (keep other mocks for methods still used by `TaskUpdateTool`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ab6e17f-2eec-4a03-8fbb-aca89d235692
📒 Files selected for processing (23)
packages/cli/src/commands/auth/status.test.tspackages/cli/src/gemini.test.tsxpackages/cli/src/ui/AppContainer.test.tsxpackages/cli/src/ui/components/Header.test.tsxpackages/cli/src/ui/components/messages/ToolMessage.tsxpackages/cli/src/ui/hooks/useFocus.test.tspackages/cli/src/ui/hooks/useGeminiStream.test.tsxpackages/core/src/config/storage.test.tspackages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.tspackages/core/src/core/geminiContentGenerator/geminiContentGenerator.test.tspackages/core/src/core/logger.test.tspackages/core/src/extension/extensionManager.test.tspackages/core/src/hooks/hookEventHandler.test.tspackages/core/src/ide/ide-client.test.tspackages/core/src/skills/skill-manager.test.tspackages/core/src/subagents/subagent-manager.test.tspackages/core/src/tools/memoryTool.test.tspackages/core/src/tools/skill.test.tspackages/core/src/tools/task-update.test.tspackages/core/src/tools/task-update.tspackages/core/src/tools/tools.tspackages/core/src/utils/installationManager.test.tspackages/core/src/utils/memoryDiscovery.test.ts
…r commit Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 tasks
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/ui/components/TaskUpdateDiffDisplay.tsx (1)
36-55: Consider using a more robust key if the data contract may change.Using
fieldas the React key relies on the assumption that each field appears at most once in thechangesarray. Based on the current implementation intask-update.ts, this is true sincecheckFieldis called once per field type. However, if the contract ever changes to allow multiple updates to the same field, this would cause duplicate key warnings.A safer alternative would be to use the index or a composite key:
- return ( - <Box key={field} flexDirection="row"> + return ( + <Box key={`${field}-${index}`} flexDirection="row">This is a minor defensive coding suggestion—current behavior is correct given the existing data contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/components/TaskUpdateDiffDisplay.tsx` around lines 36 - 55, The map in TaskUpdateDiffDisplay.tsx uses field as the React key in data.changes.map (rendering the Box with key={field}), which can break if the contract ever allows duplicate fields; change the key to a more robust identifier (e.g., use the map index or a composite like `${field}-${index}`) when returning the Box to avoid duplicate key warnings while preserving current rendering logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cli/src/ui/components/TaskUpdateDiffDisplay.tsx`:
- Around line 36-55: The map in TaskUpdateDiffDisplay.tsx uses field as the
React key in data.changes.map (rendering the Box with key={field}), which can
break if the contract ever allows duplicate fields; change the key to a more
robust identifier (e.g., use the map index or a composite like
`${field}-${index}`) when returning the Box to avoid duplicate key warnings
while preserving current rendering logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcf52f45-ba52-4877-bc5d-b495303a51be
📒 Files selected for processing (1)
packages/cli/src/ui/components/TaskUpdateDiffDisplay.tsx
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.qwen/Qwenpath and header references with.proto/Protoacross 20 test filesgetTelemetryLogPromptsEnabledmock in anthropic content generator testsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests