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 <alex@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
diff --git a/cmd/gopherbot/gopherbot.go b/cmd/gopherbot/gopherbot.go
index 9d4c5c1..2126877 100644
--- a/cmd/gopherbot/gopherbot.go
+++ b/cmd/gopherbot/gopherbot.go
@@ -208,6 +208,7 @@
ghc: ghc,
gerrit: gerrit,
mc: mc,
+ is: ghc.Issues,
deletedChanges: map[gerritChange]bool{
{"crypto", 35958}: true,
{"scratch", 71730}: true,
@@ -265,6 +266,7 @@
mc apipb.MaintnerServiceClient
corpus *maintner.Corpus
gorepo *maintner.GitHubRepo
+ is issuesService
knownContributors map[string]bool
@@ -279,6 +281,7 @@
}
}
+// tasks are executed in order every corpus update.
var tasks = []struct {
name string
fn func(*gopherbot, context.Context) error
@@ -340,6 +343,13 @@
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 @@
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 @@
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 @@
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 @@
}
// 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 @@
// 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
diff --git a/cmd/gopherbot/gopherbot_test.go b/cmd/gopherbot/gopherbot_test.go
index c274edd..91194be 100644
--- a/cmd/gopherbot/gopherbot_test.go
+++ b/cmd/gopherbot/gopherbot_test.go
@@ -13,6 +13,7 @@
"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 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 @@
},
}
- 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 @@
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)
}
}
}
