🧪 Test: add unit tests for browser-capture service#486
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}
| afterEach(async () => { | ||
| vi.clearAllMocks(); | ||
| await stopBrowserCapture(); | ||
| }); |
There was a problem hiding this comment.
Test Isolation and Cleanup:
The afterEach block calls stopBrowserCapture() without checking if a capture was actually started. If a test fails before starting the capture, this could lead to inconsistent cleanup or errors. Consider adding a check to ensure that cleanup only occurs if a capture was started, or handle errors gracefully in the cleanup phase.
Recommended solution:
afterEach(async () => {
vi.clearAllMocks();
if (isBrowserCaptureRunning()) {
await stopBrowserCapture();
}
});| it('should handle screencast frames correctly', async () => { | ||
| await startBrowserCapture({ url: 'http://localhost' }); | ||
|
|
||
| // Simulate frame | ||
| const onCall = mockCdp.on.mock.calls.find((call: any) => call[0] === 'Page.screencastFrame'); | ||
| expect(onCall).toBeDefined(); | ||
|
|
||
| const handler = onCall[1]; | ||
|
|
||
| await handler({ | ||
| data: Buffer.from('test frame').toString('base64'), | ||
| sessionId: 123 | ||
| }); | ||
|
|
||
| expect(vi.mocked(fs.writeFileSync)).toHaveBeenCalledWith(FRAME_FILE, expect.any(Buffer)); | ||
| expect(mockCdp.send).toHaveBeenCalledWith('Page.screencastFrameAck', { sessionId: 123 }); | ||
| }); |
There was a problem hiding this comment.
Frame File Content Verification:
The test for handling screencast frames verifies that fs.writeFileSync is called, but does not check the actual content written to the file. This could miss issues where the frame data is incorrectly encoded or corrupted.
Recommended solution:
Add an assertion to verify the content of the buffer passed to writeFileSync, for example:
expect(vi.mocked(fs.writeFileSync)).toHaveBeenCalledWith(FRAME_FILE, Buffer.from('test frame'));![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 a comprehensive test suite for the browser capture service, covering initialization, URL parameter logic, and frame processing using Vitest and Puppeteer. The review feedback suggests enhancing the tests by replacing 'any' types with specific Puppeteer interfaces for better type safety, reordering cleanup operations in the afterEach hook to ensure mock history is preserved until after service shutdown, and strengthening assertions to verify the actual content of captured frames.
| let mockPage: any; | ||
| let mockCdp: any; | ||
| let mockBrowser: any; |
There was a problem hiding this comment.
Using any for mock objects bypasses TypeScript's type checking. It is recommended to use specific types from puppeteer-core (e.g., Page, CDPSession, Browser) combined with Vitest's mocking types to ensure the mocks align with the actual API and provide better type safety.
| vi.clearAllMocks(); | ||
| await stopBrowserCapture(); |
There was a problem hiding this comment.
It is recommended to call stopBrowserCapture() before vi.clearAllMocks(). This ensures that the service's internal state is fully cleaned up and any resulting mock interactions (like closing the browser) are completed before the mock history is cleared for the next test.
| vi.clearAllMocks(); | |
| await stopBrowserCapture(); | |
| await stopBrowserCapture(); | |
| vi.clearAllMocks(); |
| sessionId: 123 | ||
| }); | ||
|
|
||
| expect(vi.mocked(fs.writeFileSync)).toHaveBeenCalledWith(FRAME_FILE, expect.any(Buffer)); |
There was a problem hiding this comment.
Instead of using expect.any(Buffer), verify that the buffer written to the file contains the expected decoded data. This ensures the base64 decoding logic in the service is working correctly.
| expect(vi.mocked(fs.writeFileSync)).toHaveBeenCalledWith(FRAME_FILE, expect.any(Buffer)); | |
| expect(vi.mocked(fs.writeFileSync)).toHaveBeenCalledWith(FRAME_FILE, Buffer.from('test frame')); |
1 participant
🎯 What: Add a comprehensive suite of unit tests for the
src/services/browser-capture.tsservice since none existed previously.📊 Coverage:
startBrowserCaptureconfigs, popout logic, and instance constraints.Page.screencastFramelogic mimicking internal Chrome CDP behavior, and ensuring proper writes toFRAME_FILE.stopBrowserCapturelogic and clean browser termination.isBrowserCaptureRunningandhasFrameFileutilities.✨ Result: Improves test coverage for the browser capture orchestration logic using
vitestwithout actually spawning instances by mockingpuppeteer-coreandnode:fs.PR created automatically by Jules for task 2984555860994460587 started by @Dexploarer