test(phase2): rigorous coverage for importDetails + 2-hop + watcher queue#53
test(phase2): rigorous coverage for importDetails + 2-hop + watcher queue#53PatrickSys merged 7 commits intomasterfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a9af7a472
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const controller = createAutoRefreshController(); | ||
| expect(controller.onFileChange(true)).toBe(false); | ||
| expect(controller.consumeQueuedRefresh('indexing')).toBe(false); | ||
| expect(controller.consumeQueuedRefresh('ready')).toBe(true); |
There was a problem hiding this comment.
Remove unreachable ready expectation after queue consumption
This new test case is internally inconsistent and will fail every run: createAutoRefreshController.consumeQueuedRefresh clears the queued flag on every call (see src/core/auto-refresh.ts), so after calling it with 'indexing' on the previous line, the follow-up expectation that 'ready' returns true cannot be satisfied. In CI environments where tests execute, this blocks the commit despite no product-code path changing this behavior.
Useful? React with 👍 / 👎.
Greptile SummaryCompletes Phase 2 test coverage with rigorous unit and integration tests for auto-refresh queueing, 2-hop impact tracking, and importDetails persistence. The refactoring extracts auto-refresh logic into a dedicated controller module, improving separation of concerns.
Issue: Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| src/core/auto-refresh.ts | New controller module for managing auto-refresh queueing logic |
| src/index.ts | Refactored auto-refresh logic to use new AutoRefreshController |
| tests/auto-refresh-controller.test.ts | Unit tests for AutoRefreshController - contains logic bug in "queues when indexing" test |
Last reviewed commit: 7a9af7a
| it('queues when indexing and runs after ready', () => { | ||
| const controller = createAutoRefreshController(); | ||
| expect(controller.onFileChange(true)).toBe(false); | ||
| expect(controller.consumeQueuedRefresh('indexing')).toBe(false); |
There was a problem hiding this comment.
calling consumeQueuedRefresh clears the queue regardless of status, so line 14 will return false (not true as expected). Remove this line - you should only call consumeQueuedRefresh once after indexing completes.
| expect(controller.consumeQueuedRefresh('indexing')).toBe(false); | |
| expect(controller.consumeQueuedRefresh('ready')).toBe(true); |
|
Fixed the failing test by making consumeQueuedRefresh('indexing') not clear the queue (defensive behavior). This preserves the queued refresh until an indexing run completes with status Pushed in commit 2f0b319. |
Completes Phase 2 (2a–2d) rigor work (2e excluded):
Notes: