Skip to content

Commit a6862c2

Browse files
julianknutsenclaude
julianknutsen
and
claude
committed
Add GitHubPRClient interface for testable GitHub PR operations
Extract GitHubPRClient interface (6 methods) from raw ghPath usage, with ghCLIClient real impl and fakeGitHubPRClient test double. Refactor submitPRReview, prApprovalStatus, closeGitHubPR to accept the interface. Extract mergeApprovalWarning pure function. Add 15 unit tests covering all previously-untestable GitHub code paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent cad4e91 commit a6862c2

File tree

8 files changed

+398
-33
lines changed

8 files changed

+398
-33
lines changed

‎cmd/wl/cmd_approve.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,8 @@ func runApprove(cmd *cobra.Command, stdout, _ io.Writer, branch, comment string)
4848
return fmt.Errorf("gh not found in PATH — install from https://cli.github.com")
4949
}
5050

51-
prURL, err := submitPRReview(ghPath, cfg.GitHubRepo, cfg.ForkOrg, branch, "APPROVE", comment)
51+
client := newGHClient(ghPath)
52+
prURL, err := submitPRReview(client, cfg.GitHubRepo, cfg.ForkOrg, branch, "APPROVE", comment)
5253
if err != nil {
5354
return err
5455
}

‎cmd/wl/cmd_merge.go‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,10 @@ func runMerge(cmd *cobra.Command, stdout, _ io.Writer, branch string, noPush, ke
5757
// Best-effort: check PR approval status before merging.
5858
if cfg.GitHubRepo != "" {
5959
if ghPath, err := exec.LookPath("gh"); err == nil {
60-
hasApproval, hasChangesRequested := prApprovalStatus(ghPath, cfg.GitHubRepo, cfg.ForkOrg, branch)
61-
if hasChangesRequested {
62-
fmt.Fprintf(stdout, " %s PR has outstanding change requests\n", style.Warning.Render("⚠"))
63-
} else if !hasApproval {
64-
fmt.Fprintf(stdout, " %s PR has no approvals\n", style.Warning.Render("⚠"))
60+
client := newGHClient(ghPath)
61+
hasApproval, hasChangesRequested := prApprovalStatus(client, cfg.GitHubRepo, cfg.ForkOrg, branch)
62+
if msg := mergeApprovalWarning(hasApproval, hasChangesRequested); msg != "" {
63+
fmt.Fprintf(stdout, " %s %s\n", style.Warning.Render("⚠"), msg)
6564
}
6665
}
6766
}
@@ -87,9 +86,21 @@ func runMerge(cmd *cobra.Command, stdout, _ io.Writer, branch string, noPush, ke
8786
// Best-effort: auto-close the corresponding GitHub PR shell.
8887
if cfg.GitHubRepo != "" {
8988
if ghPath, err := exec.LookPath("gh"); err == nil {
90-
closeGitHubPR(ghPath, cfg.GitHubRepo, cfg.ForkOrg, cfg.ForkDB, branch, stdout)
89+
closeGitHubPR(newGHClient(ghPath), cfg.GitHubRepo, cfg.ForkOrg, cfg.ForkDB, branch, stdout)
9190
}
9291
}
9392

9493
return nil
9594
}
95+
96+
// mergeApprovalWarning returns a warning message based on PR approval state.
97+
// Returns "" if the PR is approved with no outstanding change requests.
98+
func mergeApprovalWarning(hasApproval, hasChangesRequested bool) string {
99+
if hasChangesRequested {
100+
return "PR has outstanding change requests"
101+
}
102+
if !hasApproval {
103+
return "PR has no approvals"
104+
}
105+
return ""
106+
}

‎cmd/wl/cmd_merge_test.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,40 @@ import (
55
"testing"
66
)
77

8+
func TestMergeApprovalWarning(t *testing.T) {
9+
tests := []struct {
10+
name string
11+
hasApproval bool
12+
hasChangesRequested bool
13+
want string
14+
}{
15+
{
16+
name: "changes requested",
17+
hasChangesRequested: true,
18+
want: "PR has outstanding change requests",
19+
},
20+
{
21+
name: "no approvals",
22+
want: "PR has no approvals",
23+
},
24+
{
25+
name: "approved",
26+
hasApproval: true,
27+
want: "",
28+
},
29+
}
30+
31+
for _, tc := range tests {
32+
t.Run(tc.name, func(t *testing.T) {
33+
got := mergeApprovalWarning(tc.hasApproval, tc.hasChangesRequested)
34+
if got != tc.want {
35+
t.Errorf("mergeApprovalWarning(%v, %v) = %q, want %q",
36+
tc.hasApproval, tc.hasChangesRequested, got, tc.want)
37+
}
38+
})
39+
}
40+
}
41+
842
func TestMergeRequiresArg(t *testing.T) {
943
var stdout, stderr bytes.Buffer
1044
root := newRootCmd(&stdout, &stderr)

‎cmd/wl/cmd_request_changes.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,8 @@ func runRequestChanges(cmd *cobra.Command, stdout, _ io.Writer, branch, comment
4949
return fmt.Errorf("gh not found in PATH — install from https://cli.github.com")
5050
}
5151

52-
prURL, err := submitPRReview(ghPath, cfg.GitHubRepo, cfg.ForkOrg, branch, "REQUEST_CHANGES", comment)
52+
client := newGHClient(ghPath)
53+
prURL, err := submitPRReview(client, cfg.GitHubRepo, cfg.ForkOrg, branch, "REQUEST_CHANGES", comment)
5354
if err != nil {
5455
return err
5556
}

‎cmd/wl/cmd_review.go‎

Lines changed: 11 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -455,19 +455,14 @@ func wantedTitleFromBranch(doltPath, dbDir, branch string) string {
455455

456456
// submitPRReview submits a review on the GitHub PR for the given branch.
457457
// event must be "APPROVE" or "REQUEST_CHANGES".
458-
func submitPRReview(ghPath, upstreamRepo, forkOrg, branch, event, comment string) (string, error) {
458+
func submitPRReview(client GitHubPRClient, upstreamRepo, forkOrg, branch, event, comment string) (string, error) {
459459
head := forkOrg + ":" + branch
460-
prURL, number := findExistingPR(ghPath, upstreamRepo, head)
460+
prURL, number := client.FindPR(upstreamRepo, head)
461461
if number == "" {
462462
return "", fmt.Errorf("no open PR found for branch %s", branch)
463463
}
464464

465-
reviewBody, _ := json.Marshal(map[string]string{
466-
"event": event,
467-
"body": comment,
468-
})
469-
_, err := ghAPICall(ghPath, "POST", fmt.Sprintf("repos/%s/pulls/%s/reviews", upstreamRepo, number), string(reviewBody))
470-
if err != nil {
465+
if err := client.SubmitReview(upstreamRepo, number, event, comment); err != nil {
471466
return "", fmt.Errorf("submitting review: %w", err)
472467
}
473468
return prURL, nil
@@ -505,14 +500,14 @@ func parseReviewStatus(data []byte) (hasApproval, hasChangesRequested bool) {
505500

506501
// prApprovalStatus checks the review status of a GitHub PR. Best-effort.
507502
// Silently returns (false, false) on any error.
508-
func prApprovalStatus(ghPath, upstreamRepo, forkOrg, branch string) (hasApproval, hasChangesRequested bool) {
503+
func prApprovalStatus(client GitHubPRClient, upstreamRepo, forkOrg, branch string) (hasApproval, hasChangesRequested bool) {
509504
head := forkOrg + ":" + branch
510-
_, number := findExistingPR(ghPath, upstreamRepo, head)
505+
_, number := client.FindPR(upstreamRepo, head)
511506
if number == "" {
512507
return false, false
513508
}
514509

515-
data, err := ghAPICall(ghPath, "GET", fmt.Sprintf("repos/%s/pulls/%s/reviews", upstreamRepo, number), "")
510+
data, err := client.ListReviews(upstreamRepo, number)
516511
if err != nil {
517512
return false, false
518513
}
@@ -521,33 +516,24 @@ func prApprovalStatus(ghPath, upstreamRepo, forkOrg, branch string) (hasApproval
521516

522517
// closeGitHubPR finds and closes an open GitHub PR for the given branch.
523518
// Best-effort: failures print warnings but don't block the merge.
524-
func closeGitHubPR(ghPath, upstreamRepo, forkOrg, forkDB, branch string, stdout io.Writer) {
519+
func closeGitHubPR(client GitHubPRClient, upstreamRepo, forkOrg, forkDB, branch string, stdout io.Writer) {
525520
head := forkOrg + ":" + branch
526-
prURL, number := findExistingPR(ghPath, upstreamRepo, head)
521+
prURL, number := client.FindPR(upstreamRepo, head)
527522
if number == "" {
528523
return
529524
}
530525

531-
// Close the PR.
532-
closeBody, _ := json.Marshal(map[string]string{
533-
"state": "closed",
534-
})
535-
_, err := ghAPICall(ghPath, "PATCH", fmt.Sprintf("repos/%s/pulls/%s", upstreamRepo, number), string(closeBody))
536-
if err != nil {
526+
if err := client.ClosePR(upstreamRepo, number); err != nil {
537527
fmt.Fprintf(stdout, " warning: failed to close PR %s: %v\n", prURL, err)
538528
return
539529
}
540530

541531
// Add a closing comment.
542-
commentBody, _ := json.Marshal(map[string]string{
543-
"body": "Merged via `wl merge`.",
544-
})
545-
_, _ = ghAPICall(ghPath, "POST", fmt.Sprintf("repos/%s/issues/%s/comments", upstreamRepo, number), string(commentBody))
532+
_ = client.AddComment(upstreamRepo, number, "Merged via `wl merge`.")
546533

547534
// Delete the branch on the fork.
548535
forkRepo := forkOrg + "/" + forkDB
549-
_, err = ghAPICall(ghPath, "DELETE", fmt.Sprintf("repos/%s/git/refs/heads/%s", forkRepo, branch), "")
550-
if err != nil {
536+
if err := client.DeleteRef(forkRepo, "heads/"+branch); err != nil {
551537
fmt.Fprintf(stdout, " warning: failed to delete GitHub branch %s: %v\n", branch, err)
552538
}
553539

‎cmd/wl/cmd_review_test.go‎

Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package main
22

33
import (
44
"bytes"
5+
"fmt"
56
"strings"
67
"testing"
78
)
@@ -142,6 +143,195 @@ func TestParseReviewStatus(t *testing.T) {
142143
}
143144
}
144145

146+
func TestSubmitPRReview(t *testing.T) {
147+
tests := []struct {
148+
name string
149+
prs map[string]fakePR
150+
submitErr error
151+
event string
152+
wantURL string
153+
wantErr string
154+
}{
155+
{
156+
name: "APPROVE success",
157+
prs: map[string]fakePR{"myfork:wl/rig/w-123": {URL: "https://github.com/org/repo/pull/1", Number: "1"}},
158+
event: "APPROVE",
159+
wantURL: "https://github.com/org/repo/pull/1",
160+
},
161+
{
162+
name: "REQUEST_CHANGES success",
163+
prs: map[string]fakePR{"myfork:wl/rig/w-123": {URL: "https://github.com/org/repo/pull/2", Number: "2"}},
164+
event: "REQUEST_CHANGES",
165+
wantURL: "https://github.com/org/repo/pull/2",
166+
},
167+
{
168+
name: "no PR found",
169+
prs: map[string]fakePR{},
170+
event: "APPROVE",
171+
wantErr: "no open PR",
172+
},
173+
{
174+
name: "SubmitReview fails",
175+
prs: map[string]fakePR{"myfork:wl/rig/w-123": {URL: "https://github.com/org/repo/pull/1", Number: "1"}},
176+
submitErr: fmt.Errorf("API error"),
177+
event: "APPROVE",
178+
wantErr: "submitting review",
179+
},
180+
}
181+
182+
for _, tc := range tests {
183+
t.Run(tc.name, func(t *testing.T) {
184+
client := &fakeGitHubPRClient{
185+
prs: tc.prs,
186+
SubmitReviewErr: tc.submitErr,
187+
}
188+
url, err := submitPRReview(client, "org/repo", "myfork", "wl/rig/w-123", tc.event, "looks good")
189+
if tc.wantErr != "" {
190+
if err == nil {
191+
t.Fatal("expected error")
192+
}
193+
if !strings.Contains(err.Error(), tc.wantErr) {
194+
t.Errorf("error %q should contain %q", err, tc.wantErr)
195+
}
196+
return
197+
}
198+
if err != nil {
199+
t.Fatalf("unexpected error: %v", err)
200+
}
201+
if url != tc.wantURL {
202+
t.Errorf("got URL %q, want %q", url, tc.wantURL)
203+
}
204+
})
205+
}
206+
}
207+
208+
func TestPRApprovalStatus(t *testing.T) {
209+
tests := []struct {
210+
name string
211+
prs map[string]fakePR
212+
reviews map[string][]byte
213+
listReviewsErr error
214+
wantApproval bool
215+
wantChangesReq bool
216+
}{
217+
{
218+
name: "has approval",
219+
prs: map[string]fakePR{"myfork:wl/rig/w-123": {Number: "1"}},
220+
reviews: map[string][]byte{"1": []byte(`[{"user":{"login":"alice"},"state":"APPROVED"}]`)},
221+
wantApproval: true,
222+
},
223+
{
224+
name: "has changes requested",
225+
prs: map[string]fakePR{"myfork:wl/rig/w-123": {Number: "1"}},
226+
reviews: map[string][]byte{"1": []byte(`[{"user":{"login":"alice"},"state":"CHANGES_REQUESTED"}]`)},
227+
wantChangesReq: true,
228+
},
229+
{
230+
name: "no PR found",
231+
prs: map[string]fakePR{},
232+
},
233+
{
234+
name: "ListReviews error",
235+
prs: map[string]fakePR{"myfork:wl/rig/w-123": {Number: "1"}},
236+
listReviewsErr: fmt.Errorf("API error"),
237+
},
238+
}
239+
240+
for _, tc := range tests {
241+
t.Run(tc.name, func(t *testing.T) {
242+
client := &fakeGitHubPRClient{
243+
prs: tc.prs,
244+
reviews: tc.reviews,
245+
ListReviewsErr: tc.listReviewsErr,
246+
}
247+
gotApproval, gotChangesReq := prApprovalStatus(client, "org/repo", "myfork", "wl/rig/w-123")
248+
if gotApproval != tc.wantApproval {
249+
t.Errorf("hasApproval = %v, want %v", gotApproval, tc.wantApproval)
250+
}
251+
if gotChangesReq != tc.wantChangesReq {
252+
t.Errorf("hasChangesRequested = %v, want %v", gotChangesReq, tc.wantChangesReq)
253+
}
254+
})
255+
}
256+
}
257+
258+
func TestCloseGitHubPR(t *testing.T) {
259+
tests := []struct {
260+
name string
261+
prs map[string]fakePR
262+
closeErr error
263+
deleteRefErr error
264+
wantContains []string
265+
wantNotContains []string
266+
wantCloseCalls int
267+
wantCommentCalls int
268+
wantDeleteCalls int
269+
}{
270+
{
271+
name: "full success",
272+
prs: map[string]fakePR{"myfork:wl/rig/w-123": {URL: "https://github.com/org/repo/pull/1", Number: "1"}},
273+
wantContains: []string{"Closed PR"},
274+
wantCloseCalls: 1,
275+
wantCommentCalls: 1,
276+
wantDeleteCalls: 1,
277+
},
278+
{
279+
name: "no PR found",
280+
prs: map[string]fakePR{},
281+
wantNotContains: []string{"Closed PR", "warning"},
282+
},
283+
{
284+
name: "close fails",
285+
prs: map[string]fakePR{"myfork:wl/rig/w-123": {URL: "https://github.com/org/repo/pull/1", Number: "1"}},
286+
closeErr: fmt.Errorf("API error"),
287+
wantContains: []string{"warning"},
288+
wantNotContains: []string{"Closed PR"},
289+
wantCloseCalls: 1,
290+
},
291+
{
292+
name: "deleteRef fails",
293+
prs: map[string]fakePR{"myfork:wl/rig/w-123": {URL: "https://github.com/org/repo/pull/1", Number: "1"}},
294+
deleteRefErr: fmt.Errorf("ref error"),
295+
wantContains: []string{"warning", "Closed PR"},
296+
wantCloseCalls: 1,
297+
wantCommentCalls: 1,
298+
wantDeleteCalls: 1,
299+
},
300+
}
301+
302+
for _, tc := range tests {
303+
t.Run(tc.name, func(t *testing.T) {
304+
client := &fakeGitHubPRClient{
305+
prs: tc.prs,
306+
ClosePRErr: tc.closeErr,
307+
DeleteRefErr: tc.deleteRefErr,
308+
}
309+
var buf bytes.Buffer
310+
closeGitHubPR(client, "org/repo", "myfork", "forkdb", "wl/rig/w-123", &buf)
311+
output := buf.String()
312+
for _, want := range tc.wantContains {
313+
if !strings.Contains(output, want) {
314+
t.Errorf("output %q should contain %q", output, want)
315+
}
316+
}
317+
for _, notWant := range tc.wantNotContains {
318+
if strings.Contains(output, notWant) {
319+
t.Errorf("output %q should not contain %q", output, notWant)
320+
}
321+
}
322+
if len(client.ClosePRCalls) != tc.wantCloseCalls {
323+
t.Errorf("ClosePR calls = %d, want %d", len(client.ClosePRCalls), tc.wantCloseCalls)
324+
}
325+
if len(client.AddCommentCalls) != tc.wantCommentCalls {
326+
t.Errorf("AddComment calls = %d, want %d", len(client.AddCommentCalls), tc.wantCommentCalls)
327+
}
328+
if len(client.DeleteRefCalls) != tc.wantDeleteCalls {
329+
t.Errorf("DeleteRef calls = %d, want %d", len(client.DeleteRefCalls), tc.wantDeleteCalls)
330+
}
331+
})
332+
}
333+
}
334+
145335
func TestExtractWantedID(t *testing.T) {
146336
tests := []struct {
147337
branch, want string

0 commit comments

Comments
 (0)