e2e/reviewloop_test.go

  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}