diff --git a/tests/unit/commentResolutions.test.ts b/tests/unit/commentResolutions.test.ts new file mode 100644 index 000000000..2be0ff7be --- /dev/null +++ b/tests/unit/commentResolutions.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, test } from "vitest"; + +import { resolutionsByComment } from "@app/lib/commentResolutions"; + +import { author, operation } from "./support/cobs"; + +const alice = author("alice"); +const bob = author("bob"); + +function resolve(comment: string) { + return { type: "review.comment.resolve" as const, review: "r", comment }; +} + +function unresolve(comment: string) { + return { type: "review.comment.unresolve" as const, review: "r", comment }; +} + +describe("resolutionsByComment", () => { + test("records who resolved a comment and when", () => { + const resolutions = resolutionsByComment([ + operation(alice, 10, [resolve("c1")]), + ]); + + expect(resolutions.get("c1")).toEqual({ author: alice, timestamp: 10 }); + }); + + test("the latest resolve wins", () => { + const resolutions = resolutionsByComment([ + operation(alice, 10, [resolve("c1")]), + operation(bob, 20, [resolve("c1")]), + ]); + + expect(resolutions.get("c1")).toEqual({ author: bob, timestamp: 20 }); + }); + + test("replays operations in timestamp order, not array order", () => { + const resolutions = resolutionsByComment([ + operation(bob, 20, [unresolve("c1")]), + operation(alice, 10, [resolve("c1")]), + ]); + + expect(resolutions.has("c1")).toBe(false); + }); + + test("unresolving drops the entry", () => { + const resolutions = resolutionsByComment([ + operation(alice, 10, [resolve("c1"), resolve("c2")]), + operation(bob, 20, [unresolve("c1")]), + ]); + + expect([...resolutions.keys()]).toEqual(["c2"]); + }); + + test("ignores other actions", () => { + const resolutions = resolutionsByComment([ + operation(alice, 10, [{ type: "label", labels: ["bug"] }]), + ]); + + expect(resolutions.size).toBe(0); + }); + + test("does not reorder the caller's array", () => { + const activity = [ + operation(bob, 20, [resolve("c1")]), + operation(alice, 10, [resolve("c2")]), + ]; + resolutionsByComment(activity); + + expect(activity.map(op => op.timestamp)).toEqual([20, 10]); + }); +}); diff --git a/tests/unit/commentSources.test.ts b/tests/unit/commentSources.test.ts new file mode 100644 index 000000000..850606f8e --- /dev/null +++ b/tests/unit/commentSources.test.ts @@ -0,0 +1,98 @@ +import { describe, expect, test } from "vitest"; + +import { commentSourcesOf, STANDALONE_COMMENTS } from "@app/lib/commentSources"; + +import { author, comment, location, review, revision } from "./support/cobs"; + +const alice = author("alice"); +const bob = author("bob"); +const carol = author("carol"); +const code = location("src/main.rs", "new", 3); + +describe("commentSourcesOf", () => { + test("lists each review with code comments, counting only roots", () => { + const sources = commentSourcesOf( + revision({ + reviews: [ + review({ + id: "r1", + author: alice, + comments: [ + comment({ id: "a", location: code }), + comment({ id: "b", location: code }), + comment({ id: "reply", location: code, replyTo: "a" }), + ], + }), + ], + }), + ); + + expect(sources).toEqual([ + { id: "r1", name: "alice", count: 2, nids: ["z6Mkalice"] }, + ]); + }); + + test("skips reviews without code comments", () => { + const sources = commentSourcesOf( + revision({ + reviews: [ + review({ id: "r1", comments: [comment({ location: null })] }), + review({ id: "r2", comments: [] }), + ], + }), + ); + + expect(sources).toEqual([]); + }); + + test("names a review author without an alias by a short key", () => { + const [source] = commentSourcesOf( + revision({ + reviews: [ + review({ + author: author("anonymous", null), + comments: [comment({ location: code })], + }), + ], + }), + ); + + expect(source.name).toBe("z6Mkan"); + }); + + test("collects code comments outside reviews into their own bucket", () => { + const sources = commentSourcesOf( + revision({ + discussion: [ + comment({ id: "1", author: bob, location: code }), + comment({ id: "2", author: carol, location: code }), + comment({ id: "3", author: bob, location: code }), + comment({ id: "plain", author: alice, location: null }), + comment({ id: "reply", author: alice, location: code, replyTo: "1" }), + ], + }), + ); + + expect(sources).toEqual([ + { + id: STANDALONE_COMMENTS, + name: "Not part of a review", + count: 3, + nids: ["z6Mkbob", "z6Mkcarol"], + }, + ]); + }); + + test("puts reviews before the standalone bucket", () => { + const sources = commentSourcesOf( + revision({ + reviews: [ + review({ id: "r1", comments: [comment({ location: code })] }), + ], + discussion: [comment({ location: code })], + }), + ); + + expect(sources.map(s => s.id)).toEqual(["r1", STANDALONE_COMMENTS]); + }); +}); diff --git a/tests/unit/draftReviewStorage.test.ts b/tests/unit/draftReviewStorage.test.ts new file mode 100644 index 000000000..5e8e63389 --- /dev/null +++ b/tests/unit/draftReviewStorage.test.ts @@ -0,0 +1,294 @@ +import { beforeEach, describe, expect, test, vi } from "vitest"; + +import { author, location } from "./support/cobs"; + +const invoke = vi.fn(); +vi.mock("@app/lib/invoke", () => ({ invoke })); + +const key = "repo.patches.draftReviews"; +const alice = author("alice"); +const bob = author("bob"); +const aliceNid = "z6Mkalice"; +const bobNid = "z6Mkbob"; + +// The store reads localStorage once, when the module loads. +async function load() { + vi.resetModules(); + return (await import("@app/lib/draftReviewStorage")).draftReviewStorage; +} + +function stored(): Record> { + return JSON.parse(localStorage.getItem(key) ?? "{}"); +} + +beforeEach(() => { + localStorage.clear(); + invoke.mockReset(); + vi.spyOn(console, "error").mockReturnValue(undefined); +}); + +describe("draftReviewStorage", () => { + test("creates an empty accepting draft for its author", async () => { + const drafts = await load(); + const id = drafts.create("rad:repo", "patch", "rev", aliceNid); + + expect(drafts.get(id, alice)).toEqual({ + id, + draft: true, + rid: "rad:repo", + author: alice, + revisionId: "rev", + verdict: "accept", + summary: "", + labels: [], + comments: [], + checkedFiles: [], + }); + expect(stored()[id]).toMatchObject({ nid: aliceNid, patchId: "patch" }); + }); + + test("hides a draft from other identities", async () => { + const drafts = await load(); + const id = drafts.create("rad:repo", "patch", "rev", aliceNid); + + expect(drafts.get(id, bob)).toBeUndefined(); + expect(drafts.getForRevision("rev", bob)).toBeUndefined(); + expect(drafts.hasForRevision("rev", bobNid)).toBe(false); + }); + + test("finds a draft by revision", async () => { + const drafts = await load(); + drafts.create("rad:repo", "patch", "other", aliceNid); + const id = drafts.create("rad:repo", "patch", "rev", aliceNid); + + expect(drafts.getForRevision("rev", alice)?.id).toBe(id); + expect(drafts.hasForRevision("rev", aliceNid)).toBe(true); + expect(drafts.hasForRevision("missing", aliceNid)).toBe(false); + }); + + test("updates summary, verdict and labels", async () => { + const drafts = await load(); + const id = drafts.create("rad:repo", "patch", "rev", aliceNid); + drafts.update(id, { summary: "LGTM", verdict: undefined, labels: ["ok"] }); + + expect(drafts.get(id, alice)).toMatchObject({ + summary: "LGTM", + verdict: undefined, + labels: ["ok"], + }); + }); + + test("toggles checked files", async () => { + const drafts = await load(); + const id = drafts.create("rad:repo", "patch", "rev", aliceNid); + + drafts.toggleCheckedFile(id, "a.ts"); + drafts.toggleCheckedFile(id, "b.ts"); + expect(drafts.isFileChecked(id, "a.ts")).toBe(true); + + drafts.toggleCheckedFile(id, "a.ts"); + expect(drafts.isFileChecked(id, "a.ts")).toBe(false); + expect(drafts.get(id, alice)?.checkedFiles).toEqual(["b.ts"]); + expect(drafts.isFileChecked("missing", "a.ts")).toBe(false); + }); + + describe("comments", () => { + test("adds a comment as the draft author's", async () => { + const drafts = await load(); + const id = drafts.create("rad:repo", "patch", "rev", aliceNid); + const where = location("a.ts", "new", 3); + const commentId = drafts.addComment(id, { body: "nit", location: where }); + + expect(drafts.get(id, alice)?.comments).toEqual([ + { + id: commentId, + author: alice, + edits: [{ author: alice, timestamp: 0, body: "nit" }], + reactions: [], + replyTo: null, + location: where, + resolved: false, + }, + ]); + }); + + test("edits a comment's body", async () => { + const drafts = await load(); + const id = drafts.create("rad:repo", "patch", "rev", aliceNid); + const commentId = drafts.addComment(id, { + body: "nit", + location: location("a.ts", "new", 3), + }); + drafts.updateComment(id, commentId, { body: "fixed" }); + + expect(drafts.get(id, alice)?.comments[0].edits[0].body).toBe("fixed"); + expect(() => drafts.updateComment(id, "missing", { body: "x" })).toThrow( + "Comment missing does not exist", + ); + }); + + test("deletes only the given comment", async () => { + const drafts = await load(); + const id = drafts.create("rad:repo", "patch", "rev", aliceNid); + const where = location("a.ts", "new", 3); + const first = drafts.addComment(id, { body: "one", location: where }); + const second = drafts.addComment(id, { body: "two", location: where }); + + drafts.deleteComment(id, "missing"); + expect(drafts.get(id, alice)?.comments.map(c => c.id)).toEqual([ + first, + second, + ]); + + drafts.deleteComment(id, first); + expect(drafts.get(id, alice)?.comments.map(c => c.id)).toEqual([second]); + }); + + test("editing a draft that doesn't exist throws", async () => { + const drafts = await load(); + + expect(() => + drafts.addComment("missing", { + body: "x", + location: location("a.ts", "new", 1), + }), + ).toThrow("Draft review missing does not exist"); + }); + }); + + test("deletes a draft and returns it", async () => { + const drafts = await load(); + const id = drafts.create("rad:repo", "patch", "rev", aliceNid); + + expect(drafts.delete(id)?.id).toBe(id); + expect(drafts.get(id, alice)).toBeUndefined(); + expect(stored()).toEqual({}); + }); + + test("prunes only this author's drafts on this patch's removed revisions", async () => { + const drafts = await load(); + const gone = drafts.create("rad:repo", "patch", "gone", aliceNid); + const live = drafts.create("rad:repo", "patch", "live", aliceNid); + const otherPatch = drafts.create("rad:repo", "other", "gone", aliceNid); + const otherAuthor = drafts.create("rad:repo", "patch", "gone", bobNid); + + drafts.pruneStale("patch", ["live"], aliceNid); + + expect(Object.keys(stored()).sort()).toEqual( + [live, otherPatch, otherAuthor].sort(), + ); + expect(stored()[gone]).toBeUndefined(); + }); + + describe("publish", () => { + test("publishes the review, then deletes the draft", async () => { + invoke.mockResolvedValue(undefined); + const drafts = await load(); + const id = drafts.create("rad:repo", "patch", "rev", aliceNid); + drafts.update(id, { + summary: "LGTM", + verdict: undefined, + labels: ["ok"], + }); + const where = location("a.ts", "new", 3); + drafts.addComment(id, { body: "nit", location: where }); + + await drafts.publish(id); + + expect(invoke).toHaveBeenCalledWith("create_patch_review", { + args: { + rid: "rad:repo", + revision: "rev", + verdict: null, + summary: "LGTM", + labels: ["ok"], + comments: [{ body: "nit", location: where }], + }, + }); + expect(drafts.get(id, alice)).toBeUndefined(); + }); + + test("publishes a comment without a location as null", async () => { + invoke.mockResolvedValue(undefined); + localStorage.setItem( + key, + JSON.stringify({ + old: { + id: "old", + rid: "rad:repo", + revision: "rev", + labels: [], + comments: [{ id: "c", body: "general remark" }], + }, + }), + ); + const drafts = await load(); + + await drafts.publish("old"); + + expect(invoke.mock.calls[0][1].args.comments).toEqual([ + { body: "general remark", location: null }, + ]); + }); + + test("keeps the draft when publishing fails", async () => { + invoke.mockRejectedValue(new Error("offline")); + const drafts = await load(); + const id = drafts.create("rad:repo", "patch", "rev", aliceNid); + + await expect(drafts.publish(id)).rejects.toThrow("offline"); + expect(drafts.get(id, alice)?.id).toBe(id); + }); + + test("rejects a draft that doesn't exist", async () => { + const drafts = await load(); + + await expect(drafts.publish("missing")).rejects.toThrow( + "Review missing does not exist", + ); + expect(invoke).not.toHaveBeenCalled(); + }); + }); + + describe("persistence", () => { + test("drafts survive a reload", async () => { + const id = (await load()).create("rad:repo", "patch", "rev", aliceNid); + + expect((await load()).get(id, alice)?.revisionId).toBe("rev"); + }); + + test("reads drafts stored before nid, patchId, summary and checkedFiles existed", async () => { + localStorage.setItem( + key, + JSON.stringify({ + old: { + id: "old", + rid: "rad:repo", + revision: "rev", + labels: [], + comments: [{ id: "c", body: "legacy" }], + }, + }), + ); + const drafts = await load(); + + // Without a recorded author, the draft belongs to whoever is signed in. + expect(drafts.get("old", bob)).toMatchObject({ + summary: "", + checkedFiles: [], + comments: [{ id: "c", location: null }], + }); + // Without a patch id it can't be pruned. + drafts.pruneStale("patch", [], bobNid); + expect(drafts.get("old", bob)).toBeDefined(); + }); + + test("starts empty when the stored data is invalid", async () => { + localStorage.setItem(key, "{not json"); + expect((await load()).hasForRevision("rev", aliceNid)).toBe(false); + + localStorage.setItem(key, JSON.stringify({ x: { id: 1 } })); + expect((await load()).get("x", alice)).toBeUndefined(); + }); + }); +}); diff --git a/tests/unit/patchContributions.test.ts b/tests/unit/patchContributions.test.ts new file mode 100644 index 000000000..55f2968ae --- /dev/null +++ b/tests/unit/patchContributions.test.ts @@ -0,0 +1,55 @@ +import { expect, test } from "vitest"; + +import { patchContributions } from "@app/lib/patchContributions"; + +import { author, comment, review, revision } from "./support/cobs"; + +const alice = author("alice"); +const bob = author("bob"); + +const revisions = [ + revision({ + id: "r1", + author: alice, + discussion: [ + comment({ author: alice }), + comment({ author: bob }), + comment({ author: bob }), + ], + reviews: [ + review({ + author: bob, + comments: [comment({ author: bob }), comment({ author: alice })], + }), + ], + }), + revision({ + id: "r2", + author: bob, + reviews: [review({ author: alice }), review({ author: bob })], + }), +]; + +test("counts one person's revisions, comments and reviews", () => { + expect(patchContributions(revisions, bob.did)).toEqual({ + revisionCount: 1, + commentCount: 3, + reviewCount: 2, + }); +}); + +test("counts comments in revision discussions and in reviews", () => { + expect(patchContributions(revisions, alice.did)).toEqual({ + revisionCount: 1, + commentCount: 2, + reviewCount: 1, + }); +}); + +test("counts nothing for someone who took no part", () => { + expect(patchContributions(revisions, author("carol").did)).toEqual({ + revisionCount: 0, + commentCount: 0, + reviewCount: 0, + }); +}); diff --git a/tests/unit/patchSlice.test.ts b/tests/unit/patchSlice.test.ts new file mode 100644 index 000000000..513ce6d68 --- /dev/null +++ b/tests/unit/patchSlice.test.ts @@ -0,0 +1,123 @@ +import { describe, expect, test } from "vitest"; + +import { sliceForRange } from "@app/lib/patchSlice"; + +const preamble = [ + "diff --git a/f.txt b/f.txt", + "index 1111111..2222222 100644", + "--- a/f.txt", + "+++ b/f.txt", +]; + +// Old side: l1..l10 on lines 1-10. New side: l1-l4, L5, L5b, l6-l10 on lines +// 1-11. +const hunk = [ + "@@ -1,10 +1,11 @@ fn main", + " l1", + " l2", + " l3", + " l4", + "-l5", + "+L5", + "+L5b", + " l6", + " l7", + " l8", + " l9", + " l10", +]; + +function patch(...lines: string[]): string { + return [...preamble, ...lines, ""].join("\n"); +} + +describe("sliceForRange", () => { + test("cuts a new-side range down to its context and renumbers the hunk", () => { + expect(sliceForRange(patch(...hunk), "new", 5, 6, 1)).toBe( + patch("@@ -5,1 +5,2 @@", "-l5", "+L5", "+L5b"), + ); + }); + + test("keeps three lines of context by default", () => { + expect(sliceForRange(patch(...hunk), "new", 5, 6)).toBe( + patch( + "@@ -3,5 +3,6 @@", + " l3", + " l4", + "-l5", + "+L5", + "+L5b", + " l6", + " l7", + ), + ); + }); + + test("cuts an old-side range", () => { + expect(sliceForRange(patch(...hunk), "old", 5, 6, 1)).toBe( + patch("@@ -4,2 +4,2 @@", " l4", "-l5", "+L5"), + ); + }); + + test("covers a multi-line range with context on both ends", () => { + expect(sliceForRange(patch(...hunk), "new", 7, 9, 1)).toBe( + patch("@@ -6,3 +6,4 @@", "+L5b", " l6", " l7", " l8"), + ); + }); + + test("keeps the hunk verbatim when the window covers all of it", () => { + expect(sliceForRange(patch(...hunk), "new", 5, 6, 20)).toBe(patch(...hunk)); + }); + + test("writes 0 for a side the slice has no lines on", () => { + expect(sliceForRange(patch(...hunk), "new", 5, 7, 0)).toBe( + patch("@@ -0,0 +5,2 @@", "+L5", "+L5b"), + ); + }); + + test("returns undefined when no hunk covers the range", () => { + expect(sliceForRange(patch(...hunk), "new", 50, 51)).toBeUndefined(); + expect(sliceForRange(patch(...hunk), "old", 11, 12)).toBeUndefined(); + }); + + test("uses the hunk that covers the range and keeps the preamble", () => { + const twoHunks = patch( + "@@ -1,2 +1,2 @@", + "-a", + "+A", + " b", + "@@ -20,3 +20,3 @@ fn second", + " x", + "-y", + "+Y", + " z", + ); + + expect(sliceForRange(twoHunks, "new", 21, 22, 0)).toBe( + patch("@@ -0,0 +21,1 @@", "+Y"), + ); + expect(sliceForRange(twoHunks, "new", 21, 22, 5)).toBe( + patch("@@ -20,3 +20,3 @@ fn second", " x", "-y", "+Y", " z"), + ); + }); + + test("reads a hunk header without counts", () => { + expect(sliceForRange(patch("@@ -3 +3 @@", "-c", "+C"), "new", 3, 4)).toBe( + patch("@@ -3 +3 @@", "-c", "+C"), + ); + }); + + test("a no-newline marker doesn't shift the line numbers", () => { + const noNewline = patch( + "@@ -1,2 +1,2 @@", + "-a", + "\\ No newline at end of file", + "+A", + " b", + ); + + expect(sliceForRange(noNewline, "new", 2, 3, 0)).toBe( + patch("@@ -2,1 +2,1 @@", " b"), + ); + }); +}); diff --git a/tests/unit/pierreComments.test.ts b/tests/unit/pierreComments.test.ts new file mode 100644 index 000000000..e2589bab4 --- /dev/null +++ b/tests/unit/pierreComments.test.ts @@ -0,0 +1,260 @@ +import type { CodeLocation } from "@bindings/cob/thread/CodeLocation"; +import type { CodeRange } from "@bindings/cob/thread/CodeRange"; + +import { describe, expect, test } from "vitest"; + +import type { ComposerTarget } from "@app/lib/pierreComments"; +import { + anchorOf, + commentCountsByPath, + fileAnnotations, + formatAnchor, + formatAnchorLines, + isCommentableStatus, + locationOf, +} from "@app/lib/pierreComments"; + +import { comment, location, thread } from "./support/cobs"; + +function lines(start: number, end: number): CodeRange { + return { type: "lines", range: { start, end } }; +} + +function at( + path: string, + props: { old?: CodeRange; new?: CodeRange }, +): CodeLocation { + return { + commit: "c".repeat(40), + path, + old: props.old ?? null, + new: props.new ?? null, + }; +} + +describe("anchorOf", () => { + test("anchors a new-side range under its last line", () => { + expect(anchorOf(at("a.ts", { new: lines(3, 8) }))).toEqual({ + path: "a.ts", + side: "additions", + line: 7, + }); + }); + + test("anchors an old-side range on the deletions side", () => { + expect(anchorOf(at("a.ts", { old: lines(5, 6) }))).toEqual({ + path: "a.ts", + side: "deletions", + line: 5, + }); + }); + + test("prefers the new side when a location has both", () => { + expect( + anchorOf(at("a.ts", { old: lines(1, 2), new: lines(9, 10) }))?.side, + ).toBe("additions"); + }); + + test("places a chars range on its line", () => { + expect( + anchorOf( + at("a.ts", { + new: { type: "chars", line: 4, range: { start: 0, end: 2 } }, + }), + )?.line, + ).toBe(4); + }); + + test.each([null, undefined, at("a.ts", {})])( + "has no anchor for %j", + value => { + expect(anchorOf(value)).toBeUndefined(); + }, + ); +}); + +describe("locationOf", () => { + const target: ComposerTarget = { + path: "a.ts", + side: "additions", + firstLine: 4, + lastLine: 6, + }; + + test("writes the range on the composer's side", () => { + expect(locationOf("abc", target)).toEqual({ + commit: "abc", + path: "a.ts", + old: null, + new: lines(4, 7), + }); + }); + + test("writes a deletions range on the old side", () => { + expect(locationOf("abc", { ...target, side: "deletions" })).toEqual({ + commit: "abc", + path: "a.ts", + old: lines(4, 7), + new: null, + }); + }); + + test("normalises a range dragged upwards", () => { + expect( + locationOf("abc", { ...target, firstLine: 6, lastLine: 4 }).new, + ).toEqual(lines(4, 7)); + }); + + test("round-trips through anchorOf to the composer's last line", () => { + expect(anchorOf(locationOf("abc", target))).toEqual({ + path: "a.ts", + side: "additions", + line: 6, + }); + }); +}); + +describe("formatAnchorLines", () => { + test.each<[CodeLocation, string | undefined]>([ + [at("a.ts", { new: lines(12, 13) }), "R12"], + [at("a.ts", { old: lines(3, 10) }), "L3-L9"], + [at("a.ts", { new: lines(3, 10) }), "R3-R9"], + [ + at("a.ts", { + old: { type: "chars", line: 7, range: { start: 1, end: 4 } }, + }), + "L7", + ], + [at("a.ts", {}), undefined], + ])("formats %j as %j", (value, expected) => { + expect(formatAnchorLines(value)).toBe(expected); + }); + + test("formatAnchor prefixes the path", () => { + expect(formatAnchor(at("src/a.ts", { old: lines(3, 10) }))).toBe( + "src/a.ts:L3-L9", + ); + expect(formatAnchor(at("src/a.ts", {}))).toBe("src/a.ts"); + }); +}); + +test.each([ + ["added", true], + ["deleted", true], + ["modified", true], + [undefined, true], + ["moved", false], + ["copied", false], +] as const)("isCommentableStatus(%j) is %j", (status, expected) => { + expect(isCommentableStatus(status)).toBe(expected); +}); + +describe("fileAnnotations", () => { + const onLine5 = location("a.ts", "new", 5); + + test("keeps only this file's threads, one annotation per side and line", () => { + const annotations = fileAnnotations( + "a.ts", + [ + thread(comment({ id: "1", location: onLine5 })), + thread(comment({ id: "2", location: location("a.ts", "old", 5) })), + thread(comment({ id: "3", location: location("b.ts", "new", 5) })), + thread(comment({ id: "4", location: null })), + ], + undefined, + ); + + expect( + annotations.map(a => [ + a.side, + a.lineNumber, + a.metadata.threads.map(t => t.root.id), + ]), + ).toEqual([ + ["additions", 5, ["1"]], + ["deletions", 5, ["2"]], + ]); + }); + + test("orders threads on the same line oldest first", () => { + const [annotation] = fileAnnotations( + "a.ts", + [ + thread(comment({ id: "new", location: onLine5, timestamp: 20 })), + thread(comment({ id: "old", location: onLine5, timestamp: 10 })), + ], + undefined, + ); + + expect(annotation.metadata.threads.map(t => t.root.id)).toEqual([ + "old", + "new", + ]); + }); + + test("puts the composer on its last line, sharing a slot with threads", () => { + const composer: ComposerTarget = { + path: "a.ts", + side: "additions", + firstLine: 2, + lastLine: 5, + }; + const annotations = fileAnnotations( + "a.ts", + [thread(comment({ id: "1", location: onLine5 }))], + composer, + ); + + expect(annotations).toHaveLength(1); + expect(annotations[0].metadata.composer).toBe(composer); + expect(annotations[0].metadata.threads).toHaveLength(1); + }); + + test("ignores a composer in another file", () => { + const annotations = fileAnnotations("a.ts", [], { + path: "b.ts", + side: "additions", + firstLine: 1, + lastLine: 1, + }); + + expect(annotations).toEqual([]); + }); +}); + +describe("commentCountsByPath", () => { + test("counts resolved and unresolved threads per file", () => { + const counts = commentCountsByPath( + [ + thread(comment({ id: "1", location: location("a.ts", "new", 1) })), + thread( + comment({ + id: "2", + location: location("a.ts", "new", 2), + resolved: true, + }), + ), + thread(comment({ id: "3", location: location("b.ts", "old", 1) })), + thread(comment({ id: "4", location: null })), + ], + () => true, + ); + + expect(Object.fromEntries(counts)).toEqual({ + "a.ts": { resolved: 1, unresolved: 1 }, + "b.ts": { resolved: 0, unresolved: 1 }, + }); + }); + + test("skips threads that can't be resolved", () => { + const counts = commentCountsByPath( + [ + thread(comment({ id: "draft", location: location("a.ts", "new", 1) })), + thread(comment({ id: "real", location: location("a.ts", "new", 2) })), + ], + id => id !== "draft", + ); + + expect(counts.get("a.ts")).toEqual({ resolved: 0, unresolved: 1 }); + }); +}); diff --git a/tests/unit/pierreParse.test.ts b/tests/unit/pierreParse.test.ts new file mode 100644 index 000000000..87394c503 --- /dev/null +++ b/tests/unit/pierreParse.test.ts @@ -0,0 +1,132 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; + +type Request = { id: number; patch: string; cacheKeyPrefix?: string }; + +// Stands in for the parse worker. Tests decide how each instance behaves. +class FakeWorker { + static instances: FakeWorker[] = []; + static failToStart = false; + + onmessage: ((event: { data: unknown }) => void) | null = null; + onerror: ((event: { message: string }) => void) | null = null; + requests: Request[] = []; + + constructor() { + if (FakeWorker.failToStart) throw new Error("no workers here"); + FakeWorker.instances.push(this); + } + + postMessage(request: Request) { + this.requests.push(request); + } + + reply(data: unknown) { + this.onmessage?.({ data }); + } +} + +const processPatch = vi.fn((patch: string) => ({ + files: [{ name: `main-thread:${patch}` }], +})); + +vi.mock("@app/lib/pierreParse.worker?worker", () => ({ default: FakeWorker })); +vi.mock("@pierre/diffs", () => ({ processPatch })); + +async function load() { + vi.resetModules(); + return (await import("@app/lib/pierreParse")).parsePatch; +} + +// Lets the worker receive the request before the test replies. +async function tick() { + await new Promise(resolve => setTimeout(resolve, 0)); +} + +beforeEach(() => { + FakeWorker.instances = []; + FakeWorker.failToStart = false; + processPatch.mockClear(); + vi.spyOn(console, "error").mockReturnValue(undefined); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("parsePatch", () => { + test("parses in the worker", async () => { + const parsePatch = await load(); + const result = parsePatch("patch", "rev1"); + await tick(); + const [worker] = FakeWorker.instances; + const [request] = worker.requests; + expect(request).toMatchObject({ patch: "patch", cacheKeyPrefix: "rev1" }); + + worker.reply({ id: request.id, files: [{ name: "from-worker" }] }); + + expect(await result).toEqual([{ name: "from-worker" }]); + expect(processPatch).not.toHaveBeenCalled(); + }); + + test("routes out-of-order replies to the right request", async () => { + const parsePatch = await load(); + const first = parsePatch("one"); + const second = parsePatch("two"); + await tick(); + const [worker] = FakeWorker.instances; + const [a, b] = worker.requests; + + worker.reply({ id: b.id, files: [{ name: "two" }] }); + worker.reply({ id: a.id, files: [{ name: "one" }] }); + + expect(await first).toEqual([{ name: "one" }]); + expect(await second).toEqual([{ name: "two" }]); + expect(FakeWorker.instances).toHaveLength(1); + }); + + test("parses on the main thread when the worker can't start", async () => { + FakeWorker.failToStart = true; + const parsePatch = await load(); + + expect(await parsePatch("patch", "rev1")).toEqual([ + { name: "main-thread:patch" }, + ]); + expect(processPatch).toHaveBeenCalledWith("patch", "rev1"); + }); + + test("falls back to the main thread when the worker reports an error", async () => { + const parsePatch = await load(); + const result = parsePatch("patch"); + await tick(); + const [worker] = FakeWorker.instances; + + worker.reply({ id: worker.requests[0].id, error: "bad patch" }); + + expect(await result).toEqual([{ name: "main-thread:patch" }]); + }); + + test("a crashed worker fails over its pending requests and is not reused", async () => { + const parsePatch = await load(); + const pending = parsePatch("one"); + await tick(); + const [worker] = FakeWorker.instances; + + worker.onerror?.({ message: "crashed" }); + + expect(await pending).toEqual([{ name: "main-thread:one" }]); + expect(await parsePatch("two")).toEqual([{ name: "main-thread:two" }]); + expect(FakeWorker.instances).toHaveLength(1); + }); + + test("ignores replies for unknown requests", async () => { + const parsePatch = await load(); + const result = parsePatch("patch"); + await tick(); + const [worker] = FakeWorker.instances; + + worker.reply({ id: 999, files: [{ name: "stray" }] }); + worker.reply({ id: worker.requests[0].id, files: [{ name: "real" }] }); + + expect(await result).toEqual([{ name: "real" }]); + }); +}); diff --git a/tests/unit/reviewSummary.test.ts b/tests/unit/reviewSummary.test.ts index 950f76306..067449819 100644 --- a/tests/unit/reviewSummary.test.ts +++ b/tests/unit/reviewSummary.test.ts @@ -1,7 +1,15 @@ import { describe, expect, test } from "vitest"; import type { ReviewEntry } from "@app/lib/reviewSummary"; -import { reviewSummary } from "@app/lib/reviewSummary"; +import { + entriesFromReviewers, + entriesFromRevisions, + isOutdatedReview, + reviewSummary, + summaryTitle, +} from "@app/lib/reviewSummary"; + +import * as cobs from "./support/cobs"; function review( alias: string, @@ -112,3 +120,93 @@ describe("reviewSummary", () => { expect(state.allAccept).toBe(true); }); }); + +describe("entries", () => { + test("entriesFromRevisions numbers reviews by revision and marks delegates", () => { + const alice = cobs.author("alice"); + const bob = cobs.author("bob"); + const entries = entriesFromRevisions( + [ + cobs.revision({ + id: "rev1", + reviews: [cobs.review({ id: "a", author: alice, verdict: "accept" })], + }), + cobs.revision({ id: "rev2" }), + cobs.revision({ + id: "rev3", + reviews: [cobs.review({ id: "b", author: bob, verdict: "reject" })], + }), + ], + [bob.did], + ); + + expect(entries).toEqual([ + { + author: alice, + verdict: "accept", + revisionNumber: 1, + delegate: false, + reviewId: "a", + }, + { + author: bob, + verdict: "reject", + revisionNumber: 3, + delegate: true, + reviewId: "b", + }, + ]); + }); + + test("entriesFromReviewers keeps the reviewer fields, without a review id", () => { + const reviewer = { + author: cobs.author("alice"), + verdict: "accept" as const, + revisionNumber: 2, + delegate: true, + }; + + expect(entriesFromReviewers([reviewer])).toEqual([reviewer]); + }); +}); + +test.each([ + [2, 3, true], + [3, 3, false], + [1, 0, false], +])( + "isOutdatedReview for revision %j of %j is %j", + (revisionNumber, revisionCount, expected) => { + expect( + isOutdatedReview(review("alice", revisionNumber), revisionCount), + ).toBe(expected); + }, +); + +describe("summaryTitle", () => { + test("counts reviews and reviewers, singular and plural", () => { + expect(summaryTitle(reviewSummary([review("alice", 1)], 1))).toBe( + "1 review by 1 reviewer", + ); + expect( + summaryTitle( + reviewSummary( + [review("alice", 1), review("alice", 2), review("bob", 2)], + 2, + ), + ), + ).toBe("3 reviews by 2 reviewers"); + }); + + test("mentions delegates", () => { + expect( + summaryTitle(reviewSummary([review("alice", 1, undefined, true)], 1)), + ).toBe("1 review by 1 reviewer, 1 a delegate"); + }); + + test("warns when the reviews are outdated", () => { + expect(summaryTitle(reviewSummary([review("alice", 2)], 4))).toBe( + "1 review by 1 reviewer — newest is of revision 2, so these may no longer apply", + ); + }); +}); diff --git a/tests/unit/revisionList.test.ts b/tests/unit/revisionList.test.ts new file mode 100644 index 000000000..faaf426d8 --- /dev/null +++ b/tests/unit/revisionList.test.ts @@ -0,0 +1,95 @@ +import { describe, expect, test } from "vitest"; + +import { orderRevisions, revisionNumbers } from "@app/lib/revisionList"; +import type { RevisionListSettings } from "@app/lib/revisionListSettings"; + +import { author, revision } from "./support/cobs"; + +const alice = author("alice"); +const bob = author("bob"); +const carol = author("carol"); + +// Timeline order: bob and carol both push before the patch author's second +// revision, so grouping has to move revisions around. +const revisions = [ + revision({ id: "a1", author: alice }), + revision({ id: "b1", author: bob }), + revision({ id: "c1", author: carol }), + revision({ id: "a2", author: alice }), + revision({ id: "b2", author: bob }), +]; + +function settings(props: Partial): RevisionListSettings { + return { + sortDesc: false, + groupByAuthor: false, + showNumber: false, + showStats: false, + showReviewers: true, + ...props, + }; +} + +function ids(ordered: { id: string }[]) { + return ordered.map(r => r.id); +} + +test("revisionNumbers numbers revisions by timeline position from 1", () => { + expect(revisionNumbers(revisions)).toEqual({ + a1: 1, + b1: 2, + c1: 3, + a2: 4, + b2: 5, + }); +}); + +describe("orderRevisions", () => { + test("keeps the timeline order by default", () => { + expect(ids(orderRevisions(revisions, alice.did, settings({})))).toEqual([ + "a1", + "b1", + "c1", + "a2", + "b2", + ]); + }); + + test("reverses the timeline when sorting descending", () => { + expect( + ids(orderRevisions(revisions, alice.did, settings({ sortDesc: true }))), + ).toEqual(["b2", "a2", "c1", "b1", "a1"]); + }); + + test("groups by author, patch author first, others by first appearance", () => { + expect( + ids( + orderRevisions(revisions, carol.did, settings({ groupByAuthor: true })), + ), + ).toEqual(["c1", "a1", "a2", "b1", "b2"]); + }); + + test("sorting descending reverses within groups, not the group order", () => { + expect( + ids( + orderRevisions( + revisions, + alice.did, + settings({ groupByAuthor: true, sortDesc: true }), + ), + ), + ).toEqual(["a2", "a1", "b2", "b1", "c1"]); + }); + + test("does not reorder the caller's array", () => { + const input = [...revisions]; + orderRevisions(input, alice.did, settings({ sortDesc: true })); + orderRevisions( + input, + alice.did, + settings({ groupByAuthor: true, sortDesc: true }), + ); + + expect(ids(input)).toEqual(ids(revisions)); + }); +}); diff --git a/tests/unit/support/cobs.ts b/tests/unit/support/cobs.ts new file mode 100644 index 000000000..a74d4c66b --- /dev/null +++ b/tests/unit/support/cobs.ts @@ -0,0 +1,81 @@ +import type { Author } from "@bindings/cob/Author"; +import type { Operation } from "@bindings/cob/Operation"; +import type { Action } from "@bindings/cob/patch/Action"; +import type { Review } from "@bindings/cob/patch/Review"; +import type { Revision } from "@bindings/cob/patch/Revision"; +import type { CodeLocation } from "@bindings/cob/thread/CodeLocation"; +import type { Comment } from "@bindings/cob/thread/Comment"; +import type { Thread } from "@bindings/cob/thread/Thread"; + +// Pass `null` for an author without an alias. +export function author(name: string, alias: string | null = name): Author { + return { did: `did:key:z6Mk${name}`, alias: alias ?? undefined }; +} + +export function location( + path: string, + side: "old" | "new", + start: number, + end = start + 1, +): CodeLocation { + const range = { type: "lines" as const, range: { start, end } }; + return { + commit: "c".repeat(40), + path, + old: side === "old" ? range : null, + new: side === "new" ? range : null, + }; +} + +export function comment( + props: Partial> & { timestamp?: number } = {}, +): Comment { + const { timestamp = 0, ...rest } = props; + const by = rest.author ?? author("alice"); + return { + id: "comment", + author: by, + edits: [{ author: by, timestamp, body: "body" }], + reactions: [], + replyTo: null, + location: null, + resolved: false, + ...rest, + }; +} + +export function thread(root: Comment): Thread { + return { root, replies: [] }; +} + +export function review(props: Partial = {}): Review { + return { + id: "review", + author: author("alice"), + summary: "", + comments: [], + timestamp: 0, + labels: [], + ...props, + }; +} + +export function revision(props: Partial = {}): Revision { + return { + id: "revision", + author: author("alice"), + description: [], + base: "b".repeat(40), + head: "h".repeat(40), + timestamp: 0, + ...props, + }; +} + +export function operation( + by: Author, + timestamp: number, + actions: Action[], +): Operation { + return { id: `op-${timestamp}`, author: by, timestamp, actions }; +}