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