cmd/gopherbot: improve test fake for GitHub issues

This change adds an interface and a fake implementation of a GitHub
service for labeling GitHub issues. This allows us to test more
thoroughly, as well as avoid swapping globally scoped function vars out
in tests.

There are still more places to improve testing of GitHub API calls, but
this is a start.

Change-Id: I76d007163d65e513d74c98d0211cbb92296bfa0c
Reviewed-on: https://go-review.googlesource.com/c/build/+/209717
Run-TryBot: Alexander Rakoczy <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Carlos Amedee <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>

Author: toothrot

cmd/gopherbot/gopherbot.go

@@ -208,6 +208,7 @@ func main() {
 		ghc:    ghc,
 		gerrit: gerrit,
 		mc:     mc,
+		is:     ghc.Issues,
 		deletedChanges: map[gerritChange]bool{
 			{"crypto", 35958}:  true,
 			{"scratch", 71730}: true,
@@ -265,6 +266,7 @@ type gopherbot struct {
 	mc     apipb.MaintnerServiceClient
 	corpus *maintner.Corpus
 	gorepo *maintner.GitHubRepo
+	is     issuesService
 
 	knownContributors map[string]bool
 
@@ -279,6 +281,7 @@ type gopherbot struct {
 	}
 }
 
+// tasks are executed in order every corpus update.
 var tasks = []struct {
 	name string
 	fn   func(*gopherbot, context.Context) error
@@ -340,6 +343,13 @@ func (b *gopherbot) doTasks(ctx context.Context) []error {
 	return errs
 }
 
+// issuesService represents portions of github.IssuesService that we want to override in tests.
+type issuesService interface {
+	ListLabelsByIssue(ctx context.Context, owner string, repo string, number int, opt *github.ListOptions) ([]*github.Label, *github.Response, error)
+	AddLabelsToIssue(ctx context.Context, owner string, repo string, number int, labels []string) ([]*github.Label, *github.Response, error)
+	RemoveLabelForIssue(ctx context.Context, owner string, repo string, number int, label string) (*github.Response, error)
+}
+
 func (b *gopherbot) addLabel(ctx context.Context, gi *maintner.GitHubIssue, label string) error {
 	return b.addLabels(ctx, gi, []string{label})
 }
@@ -359,13 +369,7 @@ func (b *gopherbot) addLabels(ctx context.Context, gi *maintner.GitHubIssue, lab
 		return nil
 	}
 
-	return addLabelsToIssue(ctx, b.ghc.Issues, int(gi.Number), toAdd)
-}
-
-// addLabelsToIssue adds labels to the issue in golang/go with the given issueNum.
-// TODO: Proper stubs via interfaces.
-var addLabelsToIssue = func(ctx context.Context, issues *github.IssuesService, issueNum int, labels []string) error {
-	_, _, err := issues.AddLabelsToIssue(ctx, "golang", "go", issueNum, labels)
+	_, _, err := b.is.AddLabelsToIssue(ctx, "golang", "go", int(gi.Number), toAdd)
 	return err
 }
 
@@ -389,7 +393,7 @@ func (b *gopherbot) removeLabels(ctx context.Context, gi *maintner.GitHubIssue,
 		return nil
 	}
 
-	ghLabels, err := labelsForIssue(ctx, b.ghc.Issues, int(gi.Number))
+	ghLabels, err := labelsForIssue(ctx, b.is, int(gi.Number))
 	if err != nil {
 		return err
 	}
@@ -400,7 +404,7 @@ func (b *gopherbot) removeLabels(ctx context.Context, gi *maintner.GitHubIssue,
 
 	for _, l := range ghLabels {
 		if toRemove[l] {
-			if err := removeLabelFromIssue(ctx, b.ghc.Issues, int(gi.Number), l); err != nil {
+			if err := removeLabelFromIssue(ctx, b.is, int(gi.Number), l); err != nil {
 				log.Printf("Could not remove label %q from issue %d: %v", l, gi.Number, err)
 				continue
 			}
@@ -410,8 +414,7 @@ func (b *gopherbot) removeLabels(ctx context.Context, gi *maintner.GitHubIssue,
 }
 
 // labelsForIssue returns all labels for the given issue in the golang/go repo.
-// TODO: Proper stubs via interfaces.
-var labelsForIssue = func(ctx context.Context, issues *github.IssuesService, issueNum int) ([]string, error) {
+func labelsForIssue(ctx context.Context, issues issuesService, issueNum int) ([]string, error) {
 	ghLabels, _, err := issues.ListLabelsByIssue(ctx, "golang", "go", issueNum, &github.ListOptions{PerPage: 100})
 	if err != nil {
 		return nil, fmt.Errorf("could not list labels for golang/go#%d: %v", issueNum, err)
@@ -425,8 +428,7 @@ var labelsForIssue = func(ctx context.Context, issues *github.IssuesService, iss
 
 // removeLabelForIssue removes the given label from golang/go with the given issueNum.
 // If the issue did not have the label already (or the label didn't exist), return nil.
-// TODO: Proper stubs via interfaces.
-var removeLabelFromIssue = func(ctx context.Context, issues *github.IssuesService, issueNum int, label string) error {
+func removeLabelFromIssue(ctx context.Context, issues issuesService, issueNum int, label string) error {
 	_, err := issues.RemoveLabelForIssue(ctx, "golang", "go", issueNum, label)
 	if ge, ok := err.(*github.ErrorResponse); ok && ge.Response != nil && ge.Response.StatusCode == http.StatusNotFound {
 		return nil

cmd/gopherbot/gopherbot_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/google/go-github/github"
+
 	"golang.org/x/build/devapp/owners"
 	"golang.org/x/build/maintner"
 )
@@ -178,16 +179,65 @@ func TestLabelMutations(t *testing.T) {
 	}
 }
 
-func TestAddLabels(t *testing.T) {
-	oldFunc := addLabelsToIssue
-	defer func() { addLabelsToIssue = oldFunc }()
+type fakeIssuesService struct {
+	labels map[int][]string
+}
 
-	var added []string
-	addLabelsToIssue = func(_ context.Context, _ *github.IssuesService, _ int, labels []string) error {
-		added = labels
-		return nil
+func (f *fakeIssuesService) ListLabelsByIssue(ctx context.Context, owner string, repo string, number int, opt *github.ListOptions) ([]*github.Label, *github.Response, error) {
+	var labels []*github.Label
+	if ls, ok := f.labels[number]; ok {
+		for _, l := range ls {
+			name := l
+			labels = append(labels, &github.Label{Name: &name})
+		}
 	}
+	return labels, nil, nil
+}
 
+func (f *fakeIssuesService) AddLabelsToIssue(ctx context.Context, owner string, repo string, number int, labels []string) ([]*github.Label, *github.Response, error) {
+	if f.labels == nil {
+		f.labels = map[int][]string{number: labels}
+		return nil, nil, nil
+	}
+	ls, ok := f.labels[number]
+	if !ok {
+		f.labels[number] = labels
+		return nil, nil, nil
+	}
+	for _, label := range labels {
+		var found bool
+		for _, l := range ls {
+			if l == label {
+				found = true
+			}
+		}
+		if found {
+			continue
+		}
+		f.labels[number] = append(f.labels[number], label)
+	}
+	return nil, nil, nil
+}
+
+func (f *fakeIssuesService) RemoveLabelForIssue(ctx context.Context, owner string, repo string, number int, label string) (*github.Response, error) {
+	if ls, ok := f.labels[number]; ok {
+		for i, l := range ls {
+			if l == label {
+				f.labels[number] = append(f.labels[number][:i], f.labels[number][i+1:]...)
+				return nil, nil
+			}
+		}
+	}
+	// The GitHub API returns a NotFound error if the label did not exist.
+	return nil, &github.ErrorResponse{
+		Response: &http.Response{
+			Status:     http.StatusText(http.StatusNotFound),
+			StatusCode: http.StatusNotFound,
+		},
+	}
+}
+
+func TestAddLabels(t *testing.T) {
 	testCases := []struct {
 		desc   string
 		gi     *maintner.GitHubIssue
@@ -222,60 +272,49 @@ func TestAddLabels(t *testing.T) {
 		},
 	}
 
-	b := &gopherbot{ghc: github.NewClient(http.DefaultClient)}
+	b := &gopherbot{}
 	for _, tc := range testCases {
-		// Clear any previous state from stubbed addLabelsToIssue function above
-		// since some test cases may skip calls to it.
-		added = nil
+		// Clear any previous state from fake addLabelsToIssue since some test cases may skip calls to it.
+		fis := &fakeIssuesService{}
+		b.is = fis
 
 		if err := b.addLabels(context.Background(), tc.gi, tc.labels); err != nil {
 			t.Errorf("%s: b.addLabels got unexpected error: %v", tc.desc, err)
 			continue
 		}
-		if diff := cmp.Diff(added, tc.added); diff != "" {
+		if diff := cmp.Diff(fis.labels[int(tc.gi.ID)], tc.added); diff != "" {
 			t.Errorf("%s: labels added differ: (-got, +want)\n%s", tc.desc, diff)
 		}
 	}
 }
 
 func TestRemoveLabels(t *testing.T) {
-	oldLabelsForIssue := labelsForIssue
-	defer func() { labelsForIssue = oldLabelsForIssue }()
-
-	labelsForIssue = func(_ context.Context, _ *github.IssuesService, _ int) ([]string, error) {
-		return []string{"help wanted", "NeedsFix"}, nil
-	}
-
-	oldRemoveLabelFromIssue := removeLabelFromIssue
-	defer func() { removeLabelFromIssue = oldRemoveLabelFromIssue }()
-
-	var removed []string
-	removeLabelFromIssue = func(_ context.Context, _ *github.IssuesService, _ int, label string) error {
-		removed = append(removed, label)
-		return nil
-	}
-
 	testCases := []struct {
 		desc     string
 		gi       *maintner.GitHubIssue
+		ghLabels []string
 		toRemove []string
-		removed  []string
+		want     []string
 	}{
 		{
 			"basic remove",
 			&maintner.GitHubIssue{
+				Number: 123,
 				Labels: map[int64]*maintner.GitHubLabel{
 					0: {Name: "NeedsFix"},
+					1: {Name: "help wanted"},
 				},
 			},
+			[]string{"NeedsFix", "help wanted"},
 			[]string{"NeedsFix"},
-			[]string{"NeedsFix"},
+			[]string{"help wanted"},
 		},
 		{
 			"label not present in maintner",
 			&maintner.GitHubIssue{},
 			[]string{"NeedsFix"},
-			nil,
+			[]string{"NeedsFix"},
+			[]string{"NeedsFix"},
 		},
 		{
 			"label not present in GitHub",
@@ -284,23 +323,26 @@ func TestRemoveLabels(t *testing.T) {
 					0: {Name: "foo"},
 				},
 			},
+			[]string{"NeedsFix"},
 			[]string{"foo"},
-			nil,
+			[]string{"NeedsFix"},
 		},
 	}
 
-	b := &gopherbot{ghc: github.NewClient(http.DefaultClient)}
+	b := &gopherbot{}
 	for _, tc := range testCases {
-		// Clear any previous state from stubbed removeLabelFromIssue function above
-		// since some test cases may skip calls to it.
-		removed = nil
+		// Clear any previous state from fakeIssuesService since some test cases may skip calls to it.
+		fis := &fakeIssuesService{map[int][]string{
+			int(tc.gi.Number): tc.ghLabels,
+		}}
+		b.is = fis
 
 		if err := b.removeLabels(context.Background(), tc.gi, tc.toRemove); err != nil {
 			t.Errorf("%s: b.addLabels got unexpected error: %v", tc.desc, err)
 			continue
 		}
-		if diff := cmp.Diff(removed, tc.removed); diff != "" {
-			t.Errorf("%s: labels removed differ: (-got, +want)\n%s", tc.desc, diff)
+		if diff := cmp.Diff(fis.labels[int(tc.gi.Number)], tc.want); diff != "" {
+			t.Errorf("%s: labels differ: (-got, +want)\n%s", tc.desc, diff)
 		}
 	}
 }