e2e/pendingreview_test.go
149 lines · 6299 bytes
1package e2e
2
3import (
4 "encoding/json"
5 "os"
6 "path/filepath"
7 "strings"
8 "testing"
9)
10
11// TestPendingReviewBatch is #111's second stage: a reviewer composes a
12// review and submits it as a unit, instead of every comment landing in
13// the author's inbox the moment it is typed.
14func TestPendingReviewBatch(t *testing.T) {
15 inst := startInstance(t)
16 authorKey := inst.newKey(t, "author")
17 reviewerKey := inst.newKey(t, "reviewer")
18 inst.admin(t, "admin", "user", "create", "author", "--key", authorKey+".pub")
19 inst.admin(t, "admin", "user", "create", "reviewer", "--key", reviewerKey+".pub")
20 if _, errOut, code := inst.ssh(t, authorKey, "", "repo", "create", "author/lib"); code != 0 {
21 t.Fatalf("repo create: %s", errOut)
22 }
23 if _, _, code := inst.ssh(t, authorKey, "", "repo", "access", "grant", "author/lib", "reviewer", "write"); code != 0 {
24 t.Fatal("grant failed")
25 }
26 if _, _, code := inst.ssh(t, authorKey, "", "repo", "settings", "require-resolved", "author/lib", "on"); code != 0 {
27 t.Fatal("require-resolved failed")
28 }
29
30 env := inst.gitEnv(authorKey)
31 work := t.TempDir()
32 mustGit(t, work, env, "clone", inst.sshURL("author/lib"), "w")
33 dir := filepath.Join(work, "w")
34 os.WriteFile(filepath.Join(dir, "a.go"), []byte("package lib\n"), 0o644)
35 mustGit(t, dir, env, "checkout", "-q", "-b", "main")
36 mustGit(t, dir, env, "add", ".")
37 mustGit(t, dir, env, "commit", "-q", "-m", "base")
38 mustGit(t, dir, env, "push", "-q", "origin", "main")
39 mustGit(t, dir, env, "checkout", "-q", "-b", "feat")
40 os.WriteFile(filepath.Join(dir, "a.go"), []byte("package lib\n\nvar A = 1\nvar B = 2\n"), 0o644)
41 mustGit(t, dir, env, "add", ".")
42 mustGit(t, dir, env, "commit", "-q", "-m", "add A and B")
43 mustGit(t, dir, env, "push", "-q", "origin", "feat")
44 if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "create", "author/lib",
45 "--source", "feat", "--target", "main", "--title", "'add vars'"); code != 0 {
46 t.Fatalf("mr create: %s", errOut)
47 }
48
49 // Two comments held back.
50 for _, line := range []string{"3", "4"} {
51 if _, errOut, code := inst.ssh(t, reviewerKey, "", "mr", "diff-comment", "author/lib", "1",
52 "--path", "a.go", "--line", line, "--pending", "--message", "'name it better'"); code != 0 {
53 t.Fatalf("pending comment on line %s: %s", line, errOut)
54 }
55 }
56
57 // The author sees none of it, and their inbox is untouched.
58 if out := threadsFor(t, inst, authorKey, "1"); len(out) != 0 {
59 t.Fatalf("author sees unsubmitted comments: %v", out)
60 }
61 if got := inbox(t, inst, authorKey); strings.Contains(got, "commented on") {
62 t.Fatalf("an unsubmitted comment reached the author's inbox:\n%s", got)
63 }
64 // The reviewer sees their own.
65 if out := threadsFor(t, inst, reviewerKey, "1"); len(out) != 2 {
66 t.Fatalf("reviewer sees %d of their own pending threads, want 2", len(out))
67 }
68
69 // And an unsubmitted thread must not gate the merge: nobody else can
70 // see it, so nobody else could resolve it.
71 if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "1"); code != 0 {
72 t.Fatalf("pending thread blocked a merge: %s", errOut)
73 }
74
75 // Same again on a second MR, this time submitted.
76 mustGit(t, dir, env, "checkout", "-q", "-b", "feat2", "main")
77 os.WriteFile(filepath.Join(dir, "b.go"), []byte("package lib\n\nvar C = 3\n"), 0o644)
78 mustGit(t, dir, env, "add", ".")
79 mustGit(t, dir, env, "commit", "-q", "-m", "add C")
80 mustGit(t, dir, env, "push", "-q", "origin", "feat2")
81 if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "create", "author/lib",
82 "--source", "feat2", "--target", "main", "--title", "'add C'"); code != 0 {
83 t.Fatalf("mr create: %s", errOut)
84 }
85 if _, errOut, code := inst.ssh(t, reviewerKey, "", "mr", "diff-comment", "author/lib", "2",
86 "--path", "b.go", "--line", "3", "--pending", "--message", "'C needs a doc comment'"); code != 0 {
87 t.Fatalf("pending comment: %s", errOut)
88 }
89 out, errOut, code := inst.ssh(t, reviewerKey, "", "mr", "review", "author/lib", "2", "--request-changes", "--json")
90 if code != 0 {
91 t.Fatalf("review: %s", errOut)
92 }
93 if !strings.Contains(out, `"published":1`) {
94 t.Fatalf("review did not publish the batch: %s", out)
95 }
96 if n := len(threadsFor(t, inst, authorKey, "2")); n != 1 {
97 t.Fatalf("author sees %d threads after the review, want 1", n)
98 }
99 // One notification for the review, carrying the count — not one per
100 // comment as they were written.
101 got := inbox(t, inst, authorKey)
102 if !strings.Contains(got, "with 1 comment") {
103 t.Fatalf("review notification does not mention the batch:\n%s", got)
104 }
105 // Now it gates.
106 if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "2"); code != 4 ||
107 !strings.Contains(errOut, "threads resolved") {
108 t.Fatalf("published thread did not gate: exit %d, %s", code, errOut)
109 }
110
111 // Discard throws away only what has not been submitted.
112 if _, errOut, code := inst.ssh(t, reviewerKey, "", "mr", "diff-comment", "author/lib", "2",
113 "--path", "b.go", "--line", "1", "--pending", "--message", "'never mind'"); code != 0 {
114 t.Fatalf("pending comment: %s", errOut)
115 }
116 out, errOut, code = inst.ssh(t, reviewerKey, "", "mr", "review", "author/lib", "2", "--discard", "--json")
117 if code != 0 || !strings.Contains(out, `"discarded":1`) {
118 t.Fatalf("discard: exit %d, %s, %s", code, errOut, out)
119 }
120 if n := len(threadsFor(t, inst, reviewerKey, "2")); n != 1 {
121 t.Fatalf("discard removed a published comment: %d threads remain", n)
122 }
123 // A verdict and a discard together is a usage error, not a guess.
124 if _, _, code := inst.ssh(t, reviewerKey, "", "mr", "review", "author/lib", "2", "--approve", "--discard"); code != 2 {
125 t.Fatalf("--approve --discard exit %d, want 2", code)
126 }
127}
128
129// threadsFor returns the diff-comment thread ids this account can see on
130// one merge request. Pending threads belong to their author alone, so who
131// asks changes the answer — which is the whole point.
132func threadsFor(t *testing.T, inst *instance, key, n string) []int64 {
133 t.Helper()
134 out, errOut, code := inst.ssh(t, key, "", "mr", "threads", "author/lib", n, "--json")
135 if code != 0 {
136 t.Fatalf("mr threads: %s", errOut)
137 }
138 var env struct {
139 Data []struct {
140 ID int64 `json:"id"`
141 } `json:"data"`
142 }
143 json.Unmarshal([]byte(out), &env)
144 var ids []int64
145 for _, d := range env.Data {
146 ids = append(ids, d.ID)
147 }
148 return ids
149}