🧪 [Test] Add unit tests for core TUI UI components#490
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| it("adds and renders images", () => { | ||
| const component = new AssistantMessageComponent(); | ||
|
|
||
| component.updateContent("Look at this"); | ||
| component.finalize(); | ||
|
|
||
| component.addImage({ | ||
| base64: "dGVzdA==", | ||
| mimeType: "image/png", | ||
| filename: "test.png" | ||
| }); | ||
|
|
||
| expect(imageInstances.length).toBe(1); | ||
|
|
||
| const rendered = component.render(100); | ||
| expect(rendered).toEqual([ | ||
| "", | ||
| "Look at this", | ||
| "", // spacer | ||
| "[Image]" | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
The test for image addition does not verify the component's behavior when invalid or incomplete image data is provided. This could mask bugs in error handling logic. Consider adding test cases that pass malformed or missing fields in the image object to ensure the component handles such cases gracefully and does not throw uncaught exceptions.
| it("invalidates all sub-components", () => { | ||
| const component = new AssistantMessageComponent(true); | ||
| component.updateThinking("Hmm"); | ||
| component.addImage({ base64: "dGVzdA==", mimeType: "image/png" }); | ||
|
|
||
| component.invalidate(); | ||
|
|
||
| // Check that at least some invalidates were called | ||
| const someMarkdownInvalidated = markdownInstances.some(md => md.invalidate.mock.calls.length > 0); | ||
| expect(someMarkdownInvalidated).toBe(true); | ||
| expect(imageInstances[0].invalidate).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
The invalidation test only checks that at least one markdown instance is invalidated (someMarkdownInvalidated), but does not verify that all markdown and image instances are invalidated as expected. This could allow partial invalidation bugs to go undetected. Consider asserting that every markdown and image instance's invalidate method is called to ensure complete invalidation.
![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 introduces unit tests for the AssistantMessageComponent, ToolExecutionComponent, and UserMessageComponent to verify their rendering and state logic. The reviewer suggested strengthening assertions for component invalidation, improving the mocking of the Box component to allow for instance verification, and removing an unused require statement.
| const someMarkdownInvalidated = markdownInstances.some(md => md.invalidate.mock.calls.length > 0); | ||
| expect(someMarkdownInvalidated).toBe(true); |
There was a problem hiding this comment.
The assertion using .some() is relatively weak as it only verifies that at least one markdown instance was invalidated. Since the component is expected to invalidate all its active sub-components (both the main markdown and the thinking markdown), it's better to verify each one specifically.
| const someMarkdownInvalidated = markdownInstances.some(md => md.invalidate.mock.calls.length > 0); | |
| expect(someMarkdownInvalidated).toBe(true); | |
| expect(markdownInstances[0].invalidate).toHaveBeenCalled(); | |
| expect(markdownInstances[markdownInstances.length - 1].invalidate).toHaveBeenCalled(); |
| let boxChildren: any[] = []; | ||
| let boxBgFn: Function | null = null; | ||
| let loaderInstance: any = null; |
There was a problem hiding this comment.
Consider adding a boxInstance variable to capture the Box instance created by the component, allowing you to verify its methods (like invalidate) in tests.
| let boxChildren: any[] = []; | |
| let boxBgFn: Function | null = null; | |
| let loaderInstance: any = null; | |
| let boxChildren: any[] = []; | |
| let boxBgFn: Function | null = null; | |
| let boxInstance: any = null; | |
| let loaderInstance: any = null; |
| constructor() { | ||
| boxChildren = []; | ||
| boxBgFn = null; | ||
| } |
There was a problem hiding this comment.
Capture the instance here to enable verification in tests.
| constructor() { | |
| boxChildren = []; | |
| boxBgFn = null; | |
| } | |
| constructor() { | |
| boxInstance = this; | |
| boxChildren = []; | |
| boxBgFn = null; | |
| } |
| boxBgFn = null; | ||
| loaderInstance = null; |
There was a problem hiding this comment.
Reset the boxInstance to ensure test isolation.
| boxBgFn = null; | |
| loaderInstance = null; | |
| boxBgFn = null; | |
| boxInstance = null; | |
| loaderInstance = null; |
| // Grab the mock instance from our mock setup | ||
| const { Box } = require("@mariozechner/pi-tui"); |
There was a problem hiding this comment.
This require statement is unused and inconsistent with the ESM import syntax used throughout the project. It appears to be leftover code and should be removed.
1 participant
🎯 What: Added dedicated, high-quality test suites for previously untested
ToolExecutionComponent,AssistantMessageComponent, andUserMessageComponentinsrc/tui/components/.📊 Coverage: Added validation of component initialization, Markdown theme bindings, component-specific behavior like loader management, image attachment processing, thinking stream generation, long argument parsing and collapse mechanisms, and final UI layout construction.
✨ Result: Verified reliable layout rules and state progression logic locally across tested TUI UI components and successfully bumped root unit coverage levels without modifying any production logic.
PR created automatically by Jules for task 14764441107623730950 started by @Dexploarer