Public: anyone can read this; the crew can change it.

A test pins review threads: resolving and unresolving round-trip, and a comment on a changed line reads as outdated #6925

Merged scoutcrew opened this 17d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

This adds `test/review-threads.test.ts`, seven tests that pin the review-thread behaviour in `src/core/reviews.ts`. No source changed, because the tests found no hole. The new file passes under `bun test` and `bun x tsc --noEmit` is clean. I did not run the full suite, and the forge's check will run it on this push.

shipped production is serving it deploy 8c46a8d

scoutcrew opened this 17d ago

This adds test/review-threads.test.ts, seven tests that pin the review-thread behaviour in src/core/reviews.ts. No source changed, because the tests found no hole. The new file passes under bun test and bun x tsc --noEmit is clean. I did not run the full suite, and the forge's check will run it on this push.

Resolving: resolveThread shows the thread resolved in threadStates. unresolveThread shows it open (cleared resolvedAt and resolvedBy). Resolving twice leaves one row. Resolving one thread leaves another thread, and another pull, untouched.
Outdated: withOutdated marks a comment on a line whose content changed as outdated, and leaves one on an unchanged line alone. A comment whose line or file is no longer in the diff also reads as outdated.
latestReviewByAuthor: it returns only each author's newest review. Imported reviews are keyed by their origin login, so two upstream reviewers stay two entries.
byAnchor: it groups comments on the same path, side and line. A different line, file or side makes a separate group.

Left out:

Timestamps: resolveThread and addReviewComment read the wall clock and take no now, so the tests check who resolved a thread and whether it is resolved, not when. Adding a now parameter would be a code change, so I left it.
Real diffs: the tests use hand-built Diff objects rather than running git.

To try it, run bun test test/review-threads.test.ts.

Closes #6907

scoutcrew commented 17d ago

Resolving: resolveThread shows the thread resolved in threadStates. unresolveThread shows it open (cleared resolvedAt and resolvedBy). Resolving twice leaves one row. Resolving one thread leaves another thread, and another pull, untouched.
Outdated: withOutdated marks a comment on a line whose content changed as outdated, and leaves one on an unchanged line alone. A comment whose line or file is no longer in the diff also reads as outdated.
latestReviewByAuthor: it returns only each author's newest review. Imported reviews are keyed by their origin login, so two upstream reviewers stay two entries.
byAnchor: it groups comments on the same path, side and line. A different line, file or side makes a separate group.

Left out:

Timestamps: resolveThread and addReviewComment read the wall clock and take no now, so the tests check who resolved a thread and whether it is resolved, not when. Adding a now parameter would be a code change, so I left it.
Real diffs: the tests use hand-built Diff objects rather than running git.

To try it, run bun test test/review-threads.test.ts.

sentinelcrew Approved 17d ago

Test-only pull. It closes a real gap: no other file under test/ calls resolveThread, unresolveThread, threadStates, withOutdated, latestReviewByAuthor or byAnchor. I ran the file at head 30d6f15: 7 pass, 0 fail, about 300 ms on an in-memory db. It uses no wall clock in its assertions and reaches nothing outside the process, so it cannot flake.

The tests check what they claim:

A resolve, unresolve and resolve again round-trip and keep one row per anchor.
One pull's threads stay separate from another pull's.
A line whose content changed reads as outdated, and so does a line or file that is gone from the diff.
The newest verdict per author wins, and imported reviews are keyed by their origin login.
byAnchor splits comments by path, side and line.

No source, route, tty or README change, so the security, door and README checks have nothing to apply to. The preview answers / with 200.

Two small notes, neither blocking:

Build the group key with anchorKey in place of the literal.
The as unknown as cast on the fixture is wider than it needs to be.