e2e/reviewloop_test.go
207 lines · 8779 bytes
1package e2e
2
3import (
4 "encoding/json"
5 "fmt"
6 "os"
7 "path/filepath"
8 "strings"
9 "testing"
10)
11
12// TestTwoAccountReviewLoop drives the collaboration features end to end as
13// two distinct accounts: the author never approves their own work, and the
14// reviewer sees each step arrive.
15//
16// This is not the second human #139 asks for, and it is worth being exact
17// about why. The CODEOWNERS bug (#99) was a gate that never fired; a test
18// written from the same understanding as the code would have asserted the
19// same wrong thing. What this does close is the narrower gap that nothing
20// exercised the loop end to end at all — every feature had its own test
21// and none of them met.
22func TestTwoAccountReviewLoop(t *testing.T) {
23 t.Parallel()
24 inst := startInstance(t)
25 authorKey := inst.newKey(t, "author")
26 reviewerKey := inst.newKey(t, "reviewer")
27 // The author merges, and a merge commit carries their identity, so
28 // the account needs a verified address. The first merge below can
29 // fast-forward and would not have needed one; the second cannot.
30 inst.admin(t, "admin", "user", "create", "author", "--key", authorKey+".pub",
31 "--email", "author@example.test", "--verified")
32 inst.admin(t, "admin", "user", "create", "reviewer", "--key", reviewerKey+".pub")
33 // A third account with write access who owns nothing, so an approval
34 // from them satisfies the count and leaves CODEOWNERS unsatisfied.
35 helperKey := inst.newKey(t, "helper")
36 inst.admin(t, "admin", "user", "create", "helper", "--key", helperKey+".pub")
37
38 if _, errOut, code := inst.ssh(t, authorKey, "", "repo", "create", "author/lib"); code != 0 {
39 t.Fatalf("repo create: %s", errOut)
40 }
41 for _, who := range []string{"reviewer", "helper"} {
42 if _, _, code := inst.ssh(t, authorKey, "", "repo", "access", "grant", "author/lib", who, "write"); code != 0 {
43 t.Fatalf("grant %s failed", who)
44 }
45 }
46 // Every gate at once, which is how a repository that means it is set
47 // up, and which no single-feature test covers together.
48 for _, s := range [][]string{
49 {"repo", "settings", "require-approvals", "author/lib", "1"},
50 {"repo", "settings", "require-resolved", "author/lib", "on"},
51 {"repo", "settings", "require-codeowners", "author/lib", "on"},
52 } {
53 if _, errOut, code := inst.ssh(t, authorKey, "", s...); code != 0 {
54 t.Fatalf("%v: %s", s, errOut)
55 }
56 }
57
58 env := inst.gitEnv(authorKey)
59 work := t.TempDir()
60 mustGit(t, work, env, "clone", inst.sshURL("author/lib"), "w")
61 dir := filepath.Join(work, "w")
62 os.WriteFile(filepath.Join(dir, "CODEOWNERS"), []byte("*.go @reviewer\n"), 0o644)
63 os.WriteFile(filepath.Join(dir, "lib.go"), []byte("package lib\n"), 0o644)
64 mustGit(t, dir, env, "checkout", "-q", "-b", "main")
65 mustGit(t, dir, env, "add", ".")
66 mustGit(t, dir, env, "commit", "-q", "-m", "base")
67 mustGit(t, dir, env, "push", "-q", "origin", "main")
68
69 mustGit(t, dir, env, "checkout", "-q", "-b", "feat")
70 os.WriteFile(filepath.Join(dir, "lib.go"), []byte("package lib\n\nvar V = 1\n"), 0o644)
71 mustGit(t, dir, env, "add", ".")
72 mustGit(t, dir, env, "commit", "-q", "-m", "add V")
73 mustGit(t, dir, env, "push", "-q", "origin", "feat")
74
75 // Opened as a draft: the reviewer is not asked yet.
76 if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "create", "author/lib",
77 "--source", "feat", "--target", "main", "--title", "'add V'", "--draft"); code != 0 {
78 t.Fatalf("mr create: %s", errOut)
79 }
80 if q := reviewQueue(t, inst, reviewerKey); len(q) != 0 {
81 t.Fatalf("a draft is waiting on the reviewer: %v", q)
82 }
83
84 // Ready. Now it is theirs, and it reaches their inbox.
85 if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "ready", "author/lib", "1"); code != 0 {
86 t.Fatalf("mr ready: %s", errOut)
87 }
88 if q := reviewQueue(t, inst, reviewerKey); len(q) != 1 || q[0] != 1 {
89 t.Fatalf("review queue = %v, want !1", q)
90 }
91 // The queue is how this reviewer finds out: nobody ran "mr review
92 // request" for them, so being an owner or already in the thread is
93 // the only other way in, and this reviewer is neither. Writing this
94 // test is what surfaced the gap — see #145; TestMRReviewRequest covers
95 // the case where someone has been asked directly.
96 if got := inbox(t, inst, reviewerKey); strings.Contains(got, "ready for review") {
97 t.Fatalf("a reviewer is notified without being asked or involved:\n%s", got)
98 }
99
100 // The author cannot approve their own work past the gate.
101 if _, _, code := inst.ssh(t, authorKey, "", "mr", "review", "author/lib", "1", "--approve"); code != 0 {
102 t.Fatal("self review refused outright; it should be recorded and not counted")
103 }
104 _, errOut, code := inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "1")
105 if code != 4 || !strings.Contains(errOut, "requires 1 fresh approval") {
106 t.Fatalf("author's own approval counted: exit %d, %s", code, errOut)
107 }
108
109 // A review thread from the reviewer blocks on require-resolved.
110 out, errOut, code := inst.ssh(t, reviewerKey, "", "mr", "diff-comment", "author/lib", "1",
111 "--path", "lib.go", "--line", "3", "--message", "'name it better'", "--json")
112 if code != 0 {
113 t.Fatalf("diff-comment: %s", errOut)
114 }
115 var thread struct {
116 Data struct {
117 Thread int64 `json:"thread"`
118 } `json:"data"`
119 }
120 json.Unmarshal([]byte(out), &thread)
121 if !strings.Contains(inbox(t, inst, authorKey), "commented on lib.go") {
122 t.Fatalf("author not told about the review thread:\n%s", inbox(t, inst, authorKey))
123 }
124
125 if _, _, code := inst.ssh(t, reviewerKey, "", "mr", "review", "author/lib", "1", "--approve"); code != 0 {
126 t.Fatal("reviewer approve failed")
127 }
128 _, errOut, code = inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "1")
129 if code != 4 || !strings.Contains(errOut, "threads resolved") {
130 t.Fatalf("open thread did not block the merge: exit %d, %s", code, errOut)
131 }
132
133 // Resolve, and every gate is satisfied at once: an owner approved, the
134 // count is met, the thread is closed.
135 if _, errOut, code := inst.ssh(t, reviewerKey, "", "mr", "resolve", "author/lib", "1",
136 fmt.Sprint(thread.Data.Thread)); code != 0 {
137 t.Fatalf("resolve: %s", errOut)
138 }
139 if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "1"); code != 0 {
140 t.Fatalf("fully reviewed MR refused: %s", errOut)
141 }
142 if !strings.Contains(inbox(t, inst, reviewerKey), "merged !1") {
143 t.Fatalf("reviewer not told about the merge:\n%s", inbox(t, inst, reviewerKey))
144 }
145
146 // A CODEOWNERS gate with no owner approval still refuses, on a second
147 // MR, so the pass above was the approval and not the gate being off.
148 mustGit(t, dir, env, "checkout", "-q", "-b", "feat2", "main")
149 os.WriteFile(filepath.Join(dir, "other.go"), []byte("package lib\n"), 0o644)
150 mustGit(t, dir, env, "add", ".")
151 mustGit(t, dir, env, "commit", "-q", "-m", "another")
152 mustGit(t, dir, env, "push", "-q", "origin", "feat2")
153 if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "create", "author/lib",
154 "--source", "feat2", "--target", "main", "--title", "'another'"); code != 0 {
155 t.Fatalf("mr create: %s", errOut)
156 }
157 // helper's approval meets require-approvals but owns none of the
158 // changed files, so what refuses this merge is CODEOWNERS alone —
159 // the gate that used to be reachable only through the approval count
160 // (#99), now on its own toggle (#142).
161 if _, _, code := inst.ssh(t, helperKey, "", "mr", "review", "author/lib", "2", "--approve"); code != 0 {
162 t.Fatal("helper approve failed")
163 }
164 _, errOut, code = inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "2")
165 if code != 4 || !strings.Contains(errOut, "CODEOWNERS") {
166 t.Fatalf("CODEOWNERS gate did not fire on the second MR: exit %d, %s", code, errOut)
167 }
168 // And the owner's approval clears it.
169 if _, _, code := inst.ssh(t, reviewerKey, "", "mr", "review", "author/lib", "2", "--approve"); code != 0 {
170 t.Fatal("reviewer approve failed")
171 }
172 if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "2"); code != 0 {
173 t.Fatalf("owner-approved MR refused: %s", errOut)
174 }
175}
176
177// reviewQueue returns the MR numbers waiting on this account.
178func reviewQueue(t *testing.T, inst *instance, key string) []int64 {
179 t.Helper()
180 out, errOut, code := inst.ssh(t, key, "", "dashboard", "--json")
181 if code != 0 {
182 t.Fatalf("dashboard: %s", errOut)
183 }
184 var env struct {
185 Data struct {
186 Reviews []struct {
187 Number int64 `json:"number"`
188 } `json:"review_queue"`
189 } `json:"data"`
190 }
191 json.Unmarshal([]byte(out), &env)
192 var ns []int64
193 for _, r := range env.Data.Reviews {
194 ns = append(ns, r.Number)
195 }
196 return ns
197}
198
199// inbox returns this account's notifications as raw JSON.
200func inbox(t *testing.T, inst *instance, key string) string {
201 t.Helper()
202 out, errOut, code := inst.ssh(t, key, "", "notifications", "list", "--all", "--json")
203 if code != 0 {
204 t.Fatalf("notifications list: %s", errOut)
205 }
206 return out
207}