refactor(tests): split DiscoveryProgress tests into focused files - #256
Conversation
WalkthroughAdds seven focused test suites and a shared testutils module for DiscoveryProgress, covering core flows, answer UI, progress, PRD lifecycle, WebSocket events, error recovery, timers, and task-generation interactions. Changes
Sequence Diagram(s)(omitted — changes are test additions and testing utilities; no new runtime control flow introduced) Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-25T19:08:37.203ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
Comment |
Split DiscoveryProgress tests into focused suites and update Jest to ignore
|
Code Review SummaryThis is a well-executed refactoring that significantly improves test maintainability. The split from a monolithic 3713-line file into 7 focused test files (4019 lines total including testutils) is a solid improvement. ✅ StrengthsTest Organization
Shared Utilities (testutils.tsx)
Test Quality
Code Style
|
| File | Tests | Focus |
|---|---|---|
| core.test.tsx | 24 | Data fetching, phases, discovery states, auto-refresh |
| answer.test.tsx | 13 | Answer UI, validation, keyboard shortcuts |
| progress.test.tsx | 7 | Progress bar, question counter, completion flow |
| prd.test.tsx | 6 | PRD generation, View PRD button |
| websocket.test.tsx | 8 | All WebSocket event types |
| error.test.tsx | 7 | Stuck state, restart/retry flows |
| advanced.test.tsx | 30 | Minimized view, task generation |
Total: 95 tests ✅ (parity verified)
🔍 Specific Observations
testutils.tsx
- Line 56-61: Good use of data-testid for icon mocking
- Line 115-126: Excellent cleanup function that clears all mocks and timers
- Fixture functions (createIdleDiscovery, etc.) are well-designed
core.test.tsx
- Line 488-543: Test using real timers for complex async - good choice
- Line 546-666: Auto-refresh tests are comprehensive
websocket.test.tsx
- Line 311-340: Excellent test for project_id filtering
- Good coverage of all event types (discovery_starting, prd_generation_progress, etc.)
advanced.test.tsx
- Line 745-764: Good test for task state initialization on mount
- Line 812-843: Idempotent backend response handling is well-tested
🚀 Performance & Security
- Performance: Fake timers properly used and cleaned up - no issues
- Security: authFetch properly mocked; no security concerns
- Memory: cleanupMocks() clears timers and mock handlers properly
✅ Recommendations
- Delete original file after confirming all tests pass (as noted in checklist)
- Extract
mockPlanningPhaseDatato testutils.tsx to reduce duplication - Add JSDoc comments to fixture functions explaining when to use each
- Complete or document the incomplete test at answer.test.tsx:214
🎯 Overall Assessment
LGTM with minor improvements recommended
This is a high-quality refactoring that significantly improves maintainability while preserving 100% test coverage. The testutils extraction is particularly well-executed and will benefit future test development.
No blockers found. The code follows project conventions and is production-ready after addressing the minor issues above.
Code Review SummaryThis is a well-executed refactoring that significantly improves test maintainability. The split from a monolithic 3713-line file into 7 focused test files (4019 lines total including testutils) is a solid improvement. ✅ StrengthsTest Organization
Shared Utilities (testutils.tsx)
Test Quality
Code Style
|
| File | Tests | Focus |
|---|---|---|
| core.test.tsx | 24 | Data fetching, phases, discovery states, auto-refresh |
| answer.test.tsx | 13 | Answer UI, validation, keyboard shortcuts |
| progress.test.tsx | 7 | Progress bar, question counter, completion flow |
| prd.test.tsx | 6 | PRD generation, View PRD button |
| websocket.test.tsx | 8 | All WebSocket event types |
| error.test.tsx | 7 | Stuck state, restart/retry flows |
| advanced.test.tsx | 30 | Minimized view, task generation |
Total: 95 tests ✅ (parity verified)
🔍 Specific Observations
testutils.tsx
- Line 56-61: Good use of data-testid for icon mocking
- Line 115-126: Excellent cleanup function that clears all mocks and timers
- Fixture functions (createIdleDiscovery, etc.) are well-designed
core.test.tsx
- Line 488-543: Test using real timers for complex async - good choice
- Line 546-666: Auto-refresh tests are comprehensive
websocket.test.tsx
- Line 311-340: Excellent test for project_id filtering
- Good coverage of all event types (discovery_starting, prd_generation_progress, etc.)
advanced.test.tsx
- Line 745-764: Good test for task state initialization on mount
- Line 812-843: Idempotent backend response handling is well-tested
🚀 Performance & Security
- Performance: Fake timers properly used and cleaned up - no issues
- Security: authFetch properly mocked; no security concerns
- Memory: cleanupMocks() clears timers and mock handlers properly
✅ Recommendations
- Delete original file after confirming all tests pass (as noted in checklist)
- Extract mockPlanningPhaseData to testutils.tsx to reduce duplication
- Add JSDoc comments to fixture functions explaining when to use each
- Complete or document incomplete test at answer.test.tsx:214
🎯 Overall Assessment
LGTM with minor improvements recommended
This is a high-quality refactoring that significantly improves maintainability while preserving 100% test coverage. The testutils extraction is particularly well-executed and will benefit future test development.
No blockers found. The code follows project conventions and is production-ready after addressing the minor issues above.
Split the monolithic DiscoveryProgress.test.tsx (2992 lines, 95 tests) into 7 focused test files organized by feature area: - core.test.tsx: Data fetching, loading states, phases (24 tests) - answer.test.tsx: Answer UI - textarea, submit, validation (13 tests) - progress.test.tsx: Progress bar, question counter, completion (7 tests) - prd.test.tsx: PRD generation, View PRD button, minimize (6 tests) - websocket.test.tsx: WebSocket message handlers (8 tests) - error.test.tsx: Stuck state detection, restart/retry flows (7 tests) - advanced.test.tsx: Minimized view, task generation, auto-minimize (30 tests) Shared utilities extracted to testutils.tsx with mock factories, setup/cleanup functions, and WebSocket simulation helpers. All 95 tests pass. Test count parity verified with original file.
|
Code review posted successfully to PR #256. Summary: LGTM with minor improvements recommended The refactoring is well-executed - splitting 3713 lines into 7 focused test files with 95 tests preserved. Key strengths include excellent organization, clean testutils extraction, and comprehensive coverage. Minor recommendations include cleaning up the original file, reducing mock data duplication, and documenting incomplete tests. No blockers found. |
5b2d789 to
2bbc740
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
web-ui/__tests__/components/DiscoveryProgress.websocket.test.tsx (1)
77-85: Consider documenting why the 500ms delay is needed.The
jest.advanceTimersByTime(500)after simulating the WebSocket message appears to be waiting for some debounce or internal timing mechanism. If this is an implementation detail of the component, consider adding a brief comment explaining why this delay is necessary to prevent the test from becoming brittle if that timing changes.// Simulate WebSocket discovery_starting message simulateWsMessage({ type: 'discovery_starting', project_id: 1 }); - // Should trigger a refresh + // Should trigger a debounced refresh (component debounces WS-triggered refreshes by 500ms) jest.advanceTimersByTime(500);web-ui/__tests__/components/DiscoveryProgress.core.test.tsx (1)
487-543: Timer mode switching may cause test isolation issues.Switching from fake to real timers mid-test with
jest.useRealTimers()and then restoringjest.useFakeTimers()at the end can cause issues:
- If the test fails before line 542, fake timers won't be restored, potentially affecting subsequent tests.
- The
cleanupMocks()inafterEachcallsjest.useRealTimers(), so the manual restoration may be unnecessary.Consider restructuring to avoid mid-test timer switching, or use a try-finally pattern:
♻️ Suggested improvement
it('should transition to discovering state after successful start', async () => { - // Use real timers for this test since it involves complex async interactions - jest.useRealTimers(); + // This test requires real timers - cleanupMocks() will restore them + jest.runOnlyPendingTimers(); + jest.useRealTimers(); // ... test body ... - }, { timeout: 3000 }); - - // Restore fake timers for other tests - jest.useFakeTimers(); + }, { timeout: 3000 }); + // Note: cleanupMocks() in afterEach will call useRealTimers(), + // and the next test's setupMocks() will call useFakeTimers() });Alternatively, consider extracting this test to a separate describe block with its own timer setup.
web-ui/__tests__/components/DiscoveryProgress.answer.test.tsx (2)
64-68: Consider testing behavior over implementation details.Testing for specific CSS classes like
resize-noneandw-fullcouples the tests to implementation details. If the styling approach changes (e.g., using different utility classes or inline styles), these tests would break even if the behavior is correct.Consider whether these assertions are testing a requirement (textarea should not be resizable and should be full-width) or implementation details. If the former, testing the computed styles might be more robust, though the current approach is acceptable for a component test suite.
527-562: Creative workaround for bypassing maxLength, but note the fragility.Using
Object.definePropertyto set a value beyondmaxLengthis a clever way to test validation logic that the browser would normally prevent. However, this approach:
- Tests a scenario that can't happen via normal user interaction (browser enforces maxLength)
- May behave differently across testing environments
This is acceptable if the goal is to test server-side validation handling for malicious inputs, but consider adding a comment explaining the purpose.
📝 Suggested comment
// Type answer that's too long (textarea has maxLength=5000, so we need to simulate programmatically) const textarea = screen.getByPlaceholderText(/type your answer here/i) as HTMLTextAreaElement; const longAnswer = 'a'.repeat(5001); - // Manually set value to bypass maxLength (to test validation logic) + // Manually set value to bypass browser's maxLength enforcement. + // This tests the component's client-side validation as a defense-in-depth + // against malformed requests (e.g., API calls not going through the form). Object.defineProperty(textarea, 'value', {web-ui/__tests__/components/DiscoveryProgress.testutils.tsx (1)
64-76: Inconsistent mock pattern forgetDiscoveryProgress.
getDiscoveryProgressat line 66 is mocked inline asjest.fn()without an exported reference, while other methods (startProject,restartDiscovery, etc.) have corresponding exported mock functions (mockStartProject,mockRestartDiscovery). This creates an inconsistent test authoring experience:// Other mocks - clean: mockStartProject.mockResolvedValue(...); // getDiscoveryProgress - requires casting: (projectsApi.getDiscoveryProgress as jest.Mock).mockResolvedValue(...);🔧 Optional: Add exported mock for consistency
export const mockStartProject = jest.fn(); export const mockRestartDiscovery = jest.fn(); export const mockRetryPrdGeneration = jest.fn(); export const mockGenerateTasks = jest.fn(); export const mockGetPRD = jest.fn(); export const mockTasksList = jest.fn(); +export const mockGetDiscoveryProgress = jest.fn(); // ... jest.mock('@/lib/api', () => ({ projectsApi: { - getDiscoveryProgress: jest.fn(), + getDiscoveryProgress: (...args: unknown[]) => mockGetDiscoveryProgress(...args), startProject: (...args: unknown[]) => mockStartProject(...args),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
web-ui/__tests__/components/DiscoveryProgress.advanced.test.tsxweb-ui/__tests__/components/DiscoveryProgress.answer.test.tsxweb-ui/__tests__/components/DiscoveryProgress.core.test.tsxweb-ui/__tests__/components/DiscoveryProgress.error.test.tsxweb-ui/__tests__/components/DiscoveryProgress.prd.test.tsxweb-ui/__tests__/components/DiscoveryProgress.progress.test.tsxweb-ui/__tests__/components/DiscoveryProgress.testutils.tsxweb-ui/__tests__/components/DiscoveryProgress.websocket.test.tsx
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/lib/**/*.{ts,tsx} : Implement WebSocket automatic reconnection with exponential backoff (1s → 30s) and full state resync
Applied to files:
web-ui/__tests__/components/DiscoveryProgress.websocket.test.tsxweb-ui/__tests__/components/DiscoveryProgress.advanced.test.tsxweb-ui/__tests__/components/DiscoveryProgress.error.test.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Include auth token as query parameter in WebSocket connections (?token=TOKEN)
Applied to files:
web-ui/__tests__/components/DiscoveryProgress.websocket.test.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/**/__tests__/**/*.test.{ts,tsx} : Create JavaScript test files colocated or in __tests__/ as *.test.ts
Applied to files:
web-ui/__tests__/components/DiscoveryProgress.websocket.test.tsxweb-ui/__tests__/components/DiscoveryProgress.progress.test.tsxweb-ui/__tests__/components/DiscoveryProgress.advanced.test.tsxweb-ui/__tests__/components/DiscoveryProgress.core.test.tsxweb-ui/__tests__/components/DiscoveryProgress.prd.test.tsxweb-ui/__tests__/components/DiscoveryProgress.answer.test.tsxweb-ui/__tests__/components/DiscoveryProgress.testutils.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Use TEST_PROJECT_IDS.ACTIVE for tests requiring pre-seeded active phase with agents
Applied to files:
web-ui/__tests__/components/DiscoveryProgress.websocket.test.tsxweb-ui/__tests__/components/DiscoveryProgress.progress.test.tsxweb-ui/__tests__/components/DiscoveryProgress.advanced.test.tsxweb-ui/__tests__/components/DiscoveryProgress.core.test.tsxweb-ui/__tests__/components/DiscoveryProgress.error.test.tsxweb-ui/__tests__/components/DiscoveryProgress.prd.test.tsxweb-ui/__tests__/components/DiscoveryProgress.testutils.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Use TEST_PROJECT_IDS.PLANNING for tests requiring pre-seeded planning phase tasks
Applied to files:
web-ui/__tests__/components/DiscoveryProgress.advanced.test.tsxweb-ui/__tests__/components/DiscoveryProgress.core.test.tsxweb-ui/__tests__/components/DiscoveryProgress.error.test.tsxweb-ui/__tests__/components/DiscoveryProgress.prd.test.tsxweb-ui/__tests__/components/DiscoveryProgress.testutils.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/**/*.{ts,tsx,test.ts} : Never use test.skip() inside test logic; skip at describe level or use separate test projects for different states
Applied to files:
web-ui/__tests__/components/DiscoveryProgress.advanced.test.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/components/**/*.{ts,tsx} : Wrap AgentStateProvider with ErrorBoundary component for graceful error handling
Applied to files:
web-ui/__tests__/components/DiscoveryProgress.error.test.tsx
🧬 Code graph analysis (7)
web-ui/__tests__/components/DiscoveryProgress.websocket.test.tsx (1)
web-ui/__tests__/components/DiscoveryProgress.testutils.tsx (3)
setupMocks(115-126)cleanupMocks(131-134)simulateWsMessage(48-50)
web-ui/__tests__/components/DiscoveryProgress.advanced.test.tsx (1)
web-ui/__tests__/components/DiscoveryProgress.testutils.tsx (7)
setupMocks(115-126)cleanupMocks(131-134)DiscoveryProgressResponse(14-14)simulateWsMessage(48-50)mockGetPRD(24-24)mockTasksList(25-25)mockGenerateTasks(23-23)
web-ui/__tests__/components/DiscoveryProgress.core.test.tsx (1)
web-ui/__tests__/components/DiscoveryProgress.testutils.tsx (7)
setupMocks(115-126)cleanupMocks(131-134)DiscoveryProgressResponse(14-14)projectsApi(13-13)render(11-11)waitFor(11-11)mockStartProject(20-20)
web-ui/__tests__/components/DiscoveryProgress.error.test.tsx (1)
web-ui/__tests__/components/DiscoveryProgress.testutils.tsx (9)
setupMocks(115-126)cleanupMocks(131-134)DiscoveryProgressResponse(14-14)projectsApi(13-13)render(11-11)waitFor(11-11)mockRestartDiscovery(21-21)mockRetryPrdGeneration(22-22)simulateWsMessage(48-50)
web-ui/__tests__/components/DiscoveryProgress.prd.test.tsx (1)
web-ui/__tests__/components/DiscoveryProgress.testutils.tsx (6)
setupMocks(115-126)cleanupMocks(131-134)DiscoveryProgressResponse(14-14)simulateWsMessage(48-50)mockGetPRD(24-24)mockTasksList(25-25)
web-ui/__tests__/components/DiscoveryProgress.answer.test.tsx (1)
web-ui/__tests__/components/DiscoveryProgress.testutils.tsx (9)
setupMocks(115-126)cleanupMocks(131-134)DiscoveryProgressResponse(14-14)projectsApi(13-13)render(11-11)waitFor(11-11)screen(11-11)fireEvent(11-11)mockAuthFetch(88-88)
web-ui/__tests__/components/DiscoveryProgress.testutils.tsx (1)
web-ui/src/lib/api-client.ts (1)
authFetch(106-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: opencode-review
🔇 Additional comments (27)
web-ui/__tests__/components/DiscoveryProgress.websocket.test.tsx (2)
1-23: LGTM - Clean imports and well-documented file header.The file header clearly documents the test coverage areas, and imports are properly consolidated from the shared test utilities module.
311-340: Good pattern for verifying message filtering.Capturing the initial call count before sending the message for a different project ID and then asserting it remains unchanged is a solid approach for verifying the filtering logic.
web-ui/__tests__/components/DiscoveryProgress.prd.test.tsx (2)
1-25: LGTM - Well-organized imports and clear documentation.The file header accurately describes the test coverage, and imports are properly consolidated.
186-258: Comprehensive multi-stage progress tracking test.This test thoroughly validates the PRD generation lifecycle by simulating sequential WebSocket progress events (started → 10% → 30% → 80% → completed). This provides good coverage of the state machine transitions.
web-ui/__tests__/components/DiscoveryProgress.core.test.tsx (2)
1-29: LGTM - Comprehensive core functionality test suite.Well-documented header with clear coverage areas. Imports include
actwhich is necessary for the complex async interactions tested here.
639-666: Good test for timer cleanup on unmount.This test properly verifies that the component cleans up its polling interval when unmounted, preventing memory leaks and unnecessary API calls.
web-ui/__tests__/components/DiscoveryProgress.error.test.tsx (3)
1-27: LGTM - Well-structured error handling test suite.Clear documentation of test coverage areas and proper imports from shared utilities.
164-211: Good pattern for testing slow-resolving promises, but consider safer variable handling.The
let resolveRestart: () => voidpattern with deferred assignment is valid, but using the non-null assertion (resolveRestart!()) could be risky if the mock implementation changes. The current usage is safe since themockImplementationcallback is synchronous.This is a minor observation - the pattern works correctly as written.
301-359: Thorough test for loading state transitions during retry.This test properly validates that:
- The retry button triggers the API call
- The error state is cleared and loading state appears
- The promise is cleaned up to prevent hanging tests
web-ui/__tests__/components/DiscoveryProgress.progress.test.tsx (3)
1-24: LGTM - Well-organized progress tracking tests.Clear documentation referencing specific user stories (US8, US9, US10) and test IDs (T066, T072, etc.) provides excellent traceability to requirements.
39-129: Comprehensive progress update test with good assertions.This test thoroughly validates the progress bar update flow:
- Initial state verification (2/20, 10%)
- Answer submission via mocked API
- Timer advancement for state refresh
- Updated state verification (3/20, 15%)
- Previous question removal and textarea clearing
The assertions cover both the happy path and state transitions.
447-519: Good completion flow test covering UI cleanup.This test verifies that upon discovery completion:
- The final answer can be submitted
- The "Discovery Complete" message appears
- The answer textarea is removed
- The submit button is hidden
This ensures the UI properly transitions to the post-discovery state.
web-ui/__tests__/components/DiscoveryProgress.advanced.test.tsx (5)
1-30: LGTM - Comprehensive advanced UI test suite.Well-documented coverage areas including minimized view, task generation, and WebSocket event handling. Imports are properly consolidated from shared utilities.
259-269: Good use of shared mock data constant.Extracting
mockPlanningPhaseDataas a constant reduces duplication across the Task Generation Button tests and ensures consistency.
376-379: Clever use of minimal timer advancement.Using
jest.advanceTimersByTime(100)after WebSocket simulation allows React state updates to propagate without triggering the 3-second auto-minimize timer. This prevents test interference while still testing the intended behavior.
796-809: Excellent fail-open resilience test.Testing that a tasks fetch failure doesn't block the UI (showing the generate button anyway) demonstrates good resilience thinking. This ensures users can still proceed even if the tasks check fails.
812-873: Good idempotent backend response handling tests.These tests verify that when the backend returns
tasks_already_exist: true, the UI correctly transitions to the "tasks ready" state without showing errors. This ensures graceful handling of race conditions where tasks may have been generated by another process.web-ui/__tests__/components/DiscoveryProgress.answer.test.tsx (3)
1-24: LGTM - Well-organized answer UI test suite.Clear documentation referencing specific user stories and test coverage areas. Proper imports from shared utilities.
121-128: Good boundary test for character counter warning.Testing the 4500-character threshold where the counter changes to destructive styling validates an important UX requirement. The assertions for class changes (
text-destructivevstext-muted-foreground) are appropriate here since they're testing user-visible feedback.
673-717: Good duplicate submission prevention test.Testing that rapid button clicks only result in a single API call is an important UX and data integrity test. The use of a never-resolving promise keeps the component in the submitting state to verify the guard.
web-ui/__tests__/components/DiscoveryProgress.testutils.tsx (7)
1-14: Well-organized test utility module.Good pattern for centralizing re-exports. This reduces import boilerplate across the seven test files.
20-25: LGTM.Standard mock function declarations for API operations.
31-50: Well-designed WebSocket testing utilities.The handler registration/unregistration pattern and
simulateWsMessagehelper provide clean abstractions for WebSocket integration tests.
91-106: LGTM.Child component mocks provide minimal implementations with test IDs for assertions.
115-134: Proper setup and cleanup patterns.Good use of
jest.runOnlyPendingTimers()beforejest.useRealTimers()to avoid leaving dangling timers. The combination ofclearAllMocks()plus individualmockReset()calls ensures both call history and mock implementations are reset.
140-152: Good fixture factory pattern.The spread at line 151 performs a shallow merge, so overriding nested
discoveryproperties requires spreading the full object. The specialized fixtures (createIdleDiscovery,createDiscoveringWithQuestion, etc.) effectively address this by providing complete pre-configured states.
154-201: Comprehensive fixture coverage.The fixtures cover the key discovery states: idle, discovering (with/without question), and completed. Note that
createCompletedDiscoverycorrectly transitions tophase: 'planning', which aligns with the component's lifecycle.
Code Review SummaryThis is an excellent refactoring that significantly improves test maintainability. The split from a monolithic file into 7 focused test files with a shared testutils module is well-executed. ✅ Strengths
|
Add pattern to testPathIgnorePatterns to prevent Jest from treating *.testutils.tsx utility files as test suites requiring tests.
Code Review SummaryThis is an excellent refactoring that significantly improves test maintainability. The split from a 2992-line monolithic file into 7 focused test files is well-executed and follows best practices. ✅ Overall Assessment: LGTM with minor suggestionsStrengthsTest Organization
Test Utilities
Quality
Minor Suggestions (complementing previous reviews)1. Extract 2. Original file cleanup 3. Follow-up commit was necessary Files Changed
ConclusionProduction-ready refactoring that achieves its goals of improved maintainability and test organization. The minor suggestions above are optional improvements that can be addressed in follow-ups. No blockers - this PR can be merged as-is. |
|
Code review posted to PR #256: #256 (comment) Summary: LGTM - Excellent refactoring Split a 2992-line monolithic test file into 7 focused test files (95 tests preserved) with shared test utilities. Key strengths include logical feature-based organization, clean mock/factory extraction, proper timer cleanup, and comprehensive edge case coverage. Minor suggestions included extracting duplicate mock data and cleaning up the original file. No blockers. |
1 participant
Summary
DiscoveryProgress.test.tsx(2992 lines, 95 tests) into 7 focused test filestestutils.tsxwith mock factories and helpersTest Files Created
core.test.tsxanswer.test.tsxprogress.test.tsxprd.test.tsxwebsocket.test.tsxerror.test.tsxadvanced.test.tsxShared Utilities (
testutils.tsx)mockStartProject,mockRestartDiscovery,mockAuthFetch, etc.createIdleDiscovery(),createCompletedDiscovery(), etc.setupMocks(),cleanupMocks()simulateWsMessage()Test plan
npm test -- --testPathPatterns="DiscoveryProgress")DiscoveryProgress.test.tsxcan be removed after merge verificationSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.