e2e/pendingreview_test.go

  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}