Repository navigation
test(mobile): test PR review diff behavior instead of internals - #8029
Conversation
…vior tests Delete tests that cannot fail or only pin internals: the self-comparing accessibility label test, the parse-call-count WeakMap cache test with its parse-patch mock, the Infinity endLine sentinel assertion, the highlight cache equality test, and the three viewed-files mock pass-through tests. Replace pass-throughs with observable contracts: the file-list retention bound is asserted at the literal documented ceiling and the hook's first-page versus later-page error classification is covered by a new mounted suite; the viewed-files LRU test seeds distinguishable paths and asserts the stored key count; the refetch fan-out test routes its observer options through withInfiniteRetention; the store tests await observable publishes instead of fixed microtask drains; a new mounted suite covers the pagination gate's hold-until-drag and identity reset; the context loader suite asserts the partial, unavailable, and error expansion states.
iscekic
added
drain
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit adds the trailing separator's Files Reviewed (1 file)
Review was static (read-only); the suites were not executed. Previous Review Summaries (3 snapshots, latest commit 874e8bd)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 874e8bd)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe final lint-only commit (making the SecureStore stubs await) introduces no new issues; the sole remaining item is the previously raised, still-open coverage gap for the trailing separator's non-finite Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Review was static (read-only); the suites were not executed. Fix these issues in Kilo Cloud Previous review (commit 31f0851)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe follow-up commit resolves the five prior findings and adds the missing drag-before-switch, idempotent-hold, colliding-provider, and fourth-page retention coverage; the only remaining gap is the deleted assertion on the trailing separator's non-finite Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (13 files)
Review was static (read-only); the suites were not executed. Fix these issues in Kilo Cloud Previous review (commit 6919f61)Status: 6 Issues Found | Recommendation: Address before merge Executive SummaryThe test-only refactor is structurally sound, but the new page-gate identity test leaves the drag-before-switch regression (stale Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (13 files)
Review was static (read-only); the suites were not executed. Reviewed by deepseek-v4.1-flash-1017 · Input: 32.1K · Output: 2.1K · Cached: 280.6K Review guidance: REVIEW.md from base branch |
…t hold, colliding provider fixtures, 4th-page retention cap
iscekic
left a comment
iscekic
left a comment
There was a problem hiding this comment.
bot: Fixed in 31f0851. The reset coverage is now two cases: resets the gate when the PR identity changes drags pr-1, switches to pr-2, and asserts the mount-time end-reach does not fetch (catches a userDragged reset miss); discards a held end-reach from the previous PR on identity change keeps the original held-end-reach-then-switch case. Verified both fail against a gate that only resets heldEndReach and pass on the current code.
iscekic
added
merge:bot
Change
Test-only change under
apps/mobile/src/lib. The PR review diff suites kept several tests that pin implementation internals or cannot fail, and missed behavior coverage on two hooks. This PR swaps each pinned contract for the observable one:parse-patch-accessibility.test.ts: removed the self-comparing label test; the exact-label assertions already prove the parsed type drives the status word.pr-diff-list-builder.test.ts: removed the parse-call-count test with its parse-patch mock; the trailing separator now asserts its unknown end as the expand row's unknown-end branch input, and the separator presence, gap index, and start line stay asserted.highlight.test.ts: removed the cache test whose only assertion (identical output for identical input) holds by determinism.pr-review-file-list-state: the pure builder test now asserts the literal documented 60-page ceiling; a new mounted suite (pr-review-file-list-state.mounted.test.tsx) drivesusePrReviewFileListQueryand asserts the first-page versus later-page error classification (firstPageErrorState,laterPageError, loaded files kept).use-pr-review-viewed-files.mounted.test.tsx: removed the three mock call-argument pass-through tests; extended the mounted behavior coverage with a two-provider independence case (a toggle through a GitLab MR never lights up a same-numbered GitHub PR).viewed-files.test.ts: the LRU cap test seeds distinguishable paths, asserts the surviving entry keeps its own set, and asserts the stored map holds exactly 20 keys after the 21st toggle.infinite-retention.test.ts: the refetch fan-out test now builds its observer options throughwithInfiniteRetention(..., 3), so the module's own helper is exercised end to end.viewed-files-store.test.ts: replaced the fixed two-microtasksettle()drains with a boundedwaitForon observable publishes (snapshot loaded, listener counts, path sets) plus a single macrotask flush where the test asserts nothing published; the held-read fence tests keep their gated promises.use-pr-diff-page-gate.mounted.test.tsx: covers the pagination gate's hold-until-drag contract (end-reach held until a drag, drag alone never fetches, post-drag end-reach fetches directly, no next page / in-flight page fetch blocked) and the identity reset (a held end-reach from the previous PR is discarded; the switched-in PR starts with a fresh gate). Rerenders keep the same root element type so the hook instance survives the identity change.use-pr-diff-context-loader.mounted.test.tsx: added the partial, unavailable (no lines), unavailable (NOT_FOUND), and error expansion states, asserted onexpandedContext.pr-review-file-list-state.tsexports the namedPrReviewFileListQueryResulttype so the mounted test imports a contract name instead of reaching throughReturnType.Verification
vitest run --project mobile-pureoversrc/lib/pr-review/diff/,src/lib/pr-review/viewed-files.test.ts,src/lib/query/infinite-retention.test.ts: 166 passed.vitest run --project mobile-mountedoversrc/lib/pr-review/diff/: 20 passed.maxPagesdropped fromwithInfiniteRetention, three retention tests fail. All restored to production code afterwards.oxlint(repo mobile config) on all changed files: 0 warnings, 0 errors. Mobiletsgotypecheck error count is unchanged frommain(pre-existing tRPC build-artifact errors; none reference the changed files).No runtime behavior changed. The only non-test edits are an exported type alias for the existing hook return type (
PrReviewFileListQueryResult) and two type-only imports that use it.Proxy answers
Answer: Rewrite through withInfiniteRetention
Confidence: 0.60 · Precedent: Do the least work possible with no drop in quality; session 7f3470e0ba6f: "do the least amount of work possible, with no drop in quality". A rewrite keeps the maxPages fan-out invariant and ties it to the module's own code.
Answer: New mounted file (Recommended)
Confidence: 0.70 · Precedent: Make executive decisions yourself and use "is this reasonable?" as the test; session 7f3470e0ba6f. Mounting the hook tests behavior with less mock surface.
Answer: renderer.update with QueryClientProvider (Recommended)
Confidence: 0.70 · Precedent: Keep similar elements consistent across surfaces; session a247f3c19b1e: the thought row should match the tool call row. This option matches the existing mounted-test harness and makes the identity change a real render.
Answer: Bounded waitFor + one-macrotask flush (Recommended)
Confidence: 0.65 · Precedent: Speed and least work: "do the least amount of work possible, with no drop in quality" (session 7f3470e0ba6f). The brief allows a bounded waitFor, and it avoids per-test promise wiring.
Answer: Two-provider independence test (Recommended)
Confidence: 0.70 · Precedent: Speed and least work: "skip work that doesn't need to be done" (session 00eb2a1d3e4b). viewed-files.test.ts already covers instance keying.
Answer: Spot-check one suite (Recommended)
Confidence: 0.75 · Precedent: Proof and verification: "logs and/or screenshots are mandatory proof ... make sure they make sense" (session 55c9c80332a7). A cheap mutation check shows the new tests can fail.