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	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}