Repository: srvaroa/labeler
Pull request #25 (2021-03-23 21:09:22)

chore: fix tests by mocking out diff request

srvaroa: This commit is contained in the other PR, right? I can merge both there if so
garethjevans: It may be work merging them separately, I'll rebase the other one after this one is merged as they are dependant on each other.
srvaroa: Sounds good, i think this branch got the same first 2 commits after pushing the fmt fix. 

![image](https://user-images.githubusercontent.com/346110/112291243-38bcab00-8c90-11eb-92e4-8e4b0fbdf370.png)

e392918 is meant to be only in the other PR right? 
garethjevans: @srvaroa, sorry about the delay.  I've fixed this one and I think its good to merge.  I'll take a look at rebasing the schedule PR next.

Patch:
diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
new file mode 100644
index 0000000..cf0287e
--- /dev/null
+++ b/.github/workflows/go.yml
@@ -0,0 +1,23 @@
+---
+name: Build and test Go
+on:
+  pull_request:
+
+jobs:
+  build:
+    name: Build
+    runs-on: ubuntu-latest
+    steps:
+      - name: Set up Go 1.16
+        uses: actions/setup-go@v2
+        with:
+          go-version: 1.16
+
+      - name: Check out source code
+        uses: actions/checkout@v2
+
+      - name: Build
+        run: make build
+
+      - name: Test
+        run: make test
diff --git a/.gitignore b/.gitignore
index 1377554..a19b0ab 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,1 +1,4 @@
 *.swp
+.idea
+coverage.out
+action
diff --git a/Makefile b/Makefile
new file mode 100644
index 0000000..2e6e388
--- /dev/null
+++ b/Makefile
@@ -0,0 +1,25 @@
+SHELL := /bin/bash
+GO := GO111MODULE=on GO15VENDOREXPERIMENT=1 go
+GO_NOMOD := GO111MODULE=off go
+
+# set dev version unless VERSION is explicitly set via environment
+VERSION ?= $(shell echo "$$(git describe --abbrev=0 --tags 2>/dev/null)-dev+$(REV)" | sed 's/^v//')
+
+GO_VERSION       := $(shell $(GO) version | sed -e 's/^[^0-9.]*\([0-9.]*\).*/\1/')
+PACKAGE_DIRS     := $(shell $(GO) list ./... | grep -v /vendor/ | grep -v e2e)
+PEGOMOCK_PACKAGE := github.com/petergtz/pegomock
+GO_DEPENDENCIES  := $(shell find . -type f -name '*.go')
+
+BUILDFLAGS := -trimpath
+CGO_ENABLED = 0
+BUILDTAGS :=
+
+GOPATH1=$(firstword $(subst :, ,$(GOPATH)))
+
+export PATH := $(PATH):$(GOPATH1)/bin
+
+build: $(GO_DEPENDENCIES)
+	CGO_ENABLED=$(CGO_ENABLED) $(GO) build $(BUILDTAGS) $(BUILDFLAGS) -o action cmd/action.go
+
+test:
+	DISABLE_SSO=true CGO_ENABLED=$(CGO_ENABLED) $(GO) test -coverprofile=coverage.out $(PACKAGE_DIRS)
diff --git a/cmd/action.go b/cmd/action.go
index 6ab7639..13b1a5a 100644
--- a/cmd/action.go
+++ b/cmd/action.go
@@ -168,6 +168,8 @@ func newLabeler(gh *github.Client, config *labeler.LabelerConfigV1) *labeler.Lab
 			}
 			return labels, err
 		},
+
+		Client: labeler.NewDefaultHttpClient(),
 	}
 	return &l
 }
diff --git a/pkg/http.go b/pkg/http.go
new file mode 100644
index 0000000..ac6a752
--- /dev/null
+++ b/pkg/http.go
@@ -0,0 +1,19 @@
+package labeler
+
+import "net/http"
+
+type HttpClient interface {
+	Do(req *http.Request) (*http.Response, error)
+}
+
+type DefaultHttpClient struct {
+	client *http.Client
+}
+
+func NewDefaultHttpClient() HttpClient {
+	return &DefaultHttpClient{client: &http.Client{}}
+}
+
+func (d *DefaultHttpClient) Do(req *http.Request) (*http.Response, error) {
+	return d.client.Do(req)
+}
diff --git a/pkg/labeler.go b/pkg/labeler.go
index e0aab87..96482cc 100644
--- a/pkg/labeler.go
+++ b/pkg/labeler.go
@@ -42,6 +42,7 @@ type Labeler struct {
 	FetchRepoConfig    func(owner string, repoName string) (*LabelerConfigV1, error)
 	ReplaceLabelsForPr func(owner string, repoName string, prNumber int, labels []string) error
 	GetCurrentLabels   func(owner string, repoName string, prNumber int) ([]string, error)
+	Client             HttpClient
 }
 
 type Condition struct {
@@ -99,7 +100,7 @@ func NewBaseBranchCondition() Condition {
 	}
 }
 
-func NewFilesCondition() Condition {
+func NewFilesCondition(l *Labeler) Condition {
 	prFiles := []string{}
 
 	return Condition{
@@ -113,7 +114,7 @@ func NewFilesCondition() Condition {
 
 			if len(prFiles) == 0 {
 				var err error
-				prFiles, err = getPrFileNames(pr)
+				prFiles, err = l.getPrFileNames(pr)
 				if err != nil {
 					return false, err
 				}
@@ -251,7 +252,7 @@ func (l *Labeler) findMatches(pr *gh.PullRequest, config *LabelerConfigV1) (Labe
 		NewBaseBranchCondition(),
 		NewIsMergeableCondition(),
 		NewSizeCondition(),
-		NewFilesCondition(),
+		NewFilesCondition(l),
 	}
 
 	for _, matcher := range config.Labels {
@@ -296,7 +297,8 @@ func (l *Labeler) findMatches(pr *gh.PullRequest, config *LabelerConfigV1) (Labe
 }
 
 // getPrFileNames returns all of the file names (old and new) of files changed in the given PR
-func getPrFileNames(pr *gh.PullRequest) ([]string, error) {
+func (l *Labeler) getPrFileNames(pr *gh.PullRequest) ([]string, error) {
+	log.Printf("getPrFileNames for pr - " + pr.GetURL())
 	ghToken := os.Getenv("GITHUB_TOKEN")
 	diffReq, err := http.NewRequest("GET", pr.GetURL(), nil)
 
@@ -304,9 +306,13 @@ func getPrFileNames(pr *gh.PullRequest) ([]string, error) {
 		return nil, err
 	}
 
-	diffReq.Header.Add("Authorization", "Bearer "+ghToken)
+	if ghToken != "" {
+		diffReq.Header.Add("Authorization", "Bearer "+ghToken)
+	} else {
+		log.Printf("Env var GITHUB_TOKEN is missing, using annonymous request")
+	}
 	diffReq.Header.Add("Accept", "application/vnd.github.v3.diff")
-	diffRes, err := http.DefaultClient.Do(diffReq)
+	diffRes, err := l.Client.Do(diffReq)
 
 	if err != nil {
 		return nil, err
@@ -318,12 +324,16 @@ func getPrFileNames(pr *gh.PullRequest) ([]string, error) {
 	prFiles := make([]string, 0)
 	if diffRes.StatusCode == http.StatusOK {
 		diffRaw, err = ioutil.ReadAll(diffRes.Body)
+		if err != nil {
+			return nil, err
+		}
 
+		diff, err := diffparser.Parse(string(diffRaw))
 		if err != nil {
 			return nil, err
 		}
 
-		diff, _ := diffparser.Parse(string(diffRaw))
+		log.Printf("got diff %s, parsed %+v", string(diffRaw), diff)
 		prFilesSet := map[string]struct{}{}
 		// Place in a set to remove duplicates
 		for _, file := range diff.Files {
@@ -334,6 +344,9 @@ func getPrFileNames(pr *gh.PullRequest) ([]string, error) {
 		for k := range prFilesSet {
 			prFiles = append(prFiles, k)
 		}
+		log.Printf("diff files %s", prFiles)
+	} else {
+		log.Printf("failed with status %s", diffRes.Status)
 	}
 
 	return prFiles, nil


Test patch:
diff --git a/cmd/action_test.go b/cmd/action_test.go
index f0cbb96..9727dfa 100644
--- a/cmd/action_test.go
+++ b/cmd/action_test.go
@@ -32,24 +32,24 @@ func TestGetLabelerConfigV0(t *testing.T) {
 	}
 
 	expectMatchers := map[string]l.LabelMatcher{
-		"WIP": l.LabelMatcher{
+		"WIP": {
 			Label: "WIP",
 			Title: "^WIP:.*",
 		},
-		"WOP": l.LabelMatcher{
+		"WOP": {
 			Label: "WOP",
 			Title: "^WOP:.*",
 		},
-		"S": l.LabelMatcher{
+		"S": {
 			Label:     "S",
 			SizeBelow: "10",
 		},
-		"M": l.LabelMatcher{
+		"M": {
 			Label:     "M",
 			SizeAbove: "9",
 			SizeBelow: "100",
 		},
-		"L": l.LabelMatcher{
+		"L": {
 			Label:     "L",
 			SizeAbove: "100",
 		},
@@ -93,32 +93,32 @@ func TestGetLabelerConfigV1(t *testing.T) {
 	expect := l.LabelerConfigV1{
 		Version: 1,
 		Labels: []l.LabelMatcher{
-			l.LabelMatcher{
+			{
 				Label:  "WIP",
 				Branch: "wip",
 			},
-			l.LabelMatcher{
+			{
 				Label: "WIP",
 				Title: "^WIP:.*",
 			},
-			l.LabelMatcher{
+			{
 				Label: "WOP",
 				Title: "^WOP:.*",
 			},
-			l.LabelMatcher{
+			{
 				Label:     "S",
 				SizeBelow: "10",
 			},
-			l.LabelMatcher{
+			{
 				Label:     "M",
 				SizeAbove: "9",
 				SizeBelow: "100",
 			},
-			l.LabelMatcher{
+			{
 				Label:     "L",
 				SizeAbove: "100",
 			},
-			l.LabelMatcher{
+			{
 				Label: "TestFileMatch",
 				Files: []string{
 					"cmd/.*.go",
@@ -156,11 +156,11 @@ func TestGetLabelerConfig2V1(t *testing.T) {
 	}
 
 	expectMatchers := map[string]l.LabelMatcher{
-		"TestLabel": l.LabelMatcher{
+		"TestLabel": {
 			Label: "TestLabel",
 			Title: ".*",
 		},
-		"TestFileMatch": l.LabelMatcher{
+		"TestFileMatch": {
 			Label: "TestFileMatch",
 			Files: []string{"cmd/.*.go", "pkg/.*.go"},
 		},
diff --git a/pkg/labeler_test.go b/pkg/labeler_test.go
index a15b087..261e9a1 100644
--- a/pkg/labeler_test.go
+++ b/pkg/labeler_test.go
@@ -2,10 +2,13 @@ package labeler
 
 import (
 	"fmt"
+	"io"
 	"io/ioutil"
+	"net/http"
 	"os"
 	"reflect"
 	"sort"
+	"strings"
 	"testing"
 )
 
@@ -29,20 +32,20 @@ func TestHandleEvent(t *testing.T) {
 
 	// These all use the payload in payload files
 	testCases := []TestCase{
-		TestCase{
+		{
 			payloads:       []string{"create_pr", "reopen_pr"},
 			name:           "Empty config",
 			config:         LabelerConfigV1{},
 			initialLabels:  []string{"Fix"},
 			expectedLabels: []string{"Fix"},
 		},
-		TestCase{
+		{
 			payloads: []string{"create_pr", "reopen_pr"},
 			name:     "Config with no rules",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label: "WIP",
 					},
 				},
@@ -50,13 +53,13 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{"Fix"},
 			expectedLabels: []string{"Fix"},
 		},
-		TestCase{
+		{
 			payloads: []string{"create_pr", "reopen_pr"},
 			name:     "Add a label when not set and config matches",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label: "WIP",
 						Title: "^WIP:.*",
 					},
@@ -65,13 +68,13 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{"WIP"},
 		},
-		TestCase{
+		{
 			payloads: []string{"create_pr", "reopen_pr"},
 			name:     "Remove a label when set and config does not match",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label: "Fix",
 						Title: "Fix: .*",
 					},
@@ -80,13 +83,13 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{"Fix"},
 			expectedLabels: []string{},
 		},
-		TestCase{
+		{
 			payloads: []string{"create_pr", "reopen_pr"},
 			name:     "Respect a label when set, and not present in config",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label: "Fix",
 						Title: "^Fix.*",
 					},
@@ -95,17 +98,17 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{"SomeLabel"},
 			expectedLabels: []string{"SomeLabel"},
 		},
-		TestCase{
+		{
 			payloads: []string{"create_pr", "reopen_pr"},
 			name:     "A combination of all cases",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label: "WIP",
 						Title: "^WIP:.*",
 					},
-					LabelMatcher{
+					{
 						Label: "ShouldRemove",
 						Title: "^MEH.*",
 					},
@@ -114,13 +117,13 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{"ShouldRemove", "ShouldRespect"},
 			expectedLabels: []string{"WIP", "ShouldRespect"},
 		},
-		TestCase{
+		{
 			payloads: []string{"create_pr", "reopen_pr"},
 			name:     "Add a label with two conditions, both matching",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label:     "WIP",
 						Title:     "^WIP:.*",
 						Mergeable: "False",
@@ -130,13 +133,13 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{"WIP"},
 		},
-		TestCase{
+		{
 			payloads: []string{"create_pr", "reopen_pr"},
 			name:     "Add a label with two conditions, one not matching (1)",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label:     "WIP",
 						Title:     "^WIP:.*",
 						Mergeable: "True",
@@ -146,7 +149,7 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{},
 		},
-		TestCase{
+		{
 			// covers evaluation order making a True in the last
 			// condition, while previous ones are false
 			payloads: []string{"create_pr", "reopen_pr"},
@@ -154,7 +157,7 @@ func TestHandleEvent(t *testing.T) {
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label:     "WIP",
 						Title:     "^DOES NOT MATCH:.*",
 						Mergeable: "False",
@@ -164,13 +167,13 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{},
 		},
-		TestCase{
+		{
 			payloads: []string{"small_pr"},
 			name:     "Test the size_below rule",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label:     "S",
 						SizeBelow: "10",
 					},
@@ -179,13 +182,13 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{"S"},
 		},
-		TestCase{
+		{
 			payloads: []string{"mid_pr"},
 			name:     "Test the size_below and size_above rules",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label:     "M",
 						SizeAbove: "9",
 						SizeBelow: "100",
@@ -195,13 +198,13 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{"M"},
 		},
-		TestCase{
+		{
 			payloads: []string{"big_pr"},
 			name:     "Test the size_above rule",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label:     "L",
 						SizeAbove: "100",
 					},
@@ -210,13 +213,13 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{"L"},
 		},
-		TestCase{
+		{
 			payloads: []string{"small_pr"},
 			name:     "Test the branch rule (matching)",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label:  "Branch",
 						Branch: "^srvaroa-patch.*",
 					},
@@ -225,13 +228,13 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{"Branch"},
 		},
-		TestCase{
+		{
 			payloads: []string{"small_pr"},
 			name:     "Test the branch rule (not matching)",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label:  "Branch",
 						Branch: "^does/not-match/*",
 					},
@@ -240,13 +243,13 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{},
 		},
-		TestCase{
+		{
 			payloads: []string{"create_pr"},
 			name:     "Test the base branch rule (matching)",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label:      "Branch",
 						BaseBranch: "^master",
 					},
@@ -255,13 +258,13 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{"Branch"},
 		},
-		TestCase{
+		{
 			payloads: []string{"create_pr"},
 			name:     "Test the base branch rule (not matching)",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label:      "Branch",
 						BaseBranch: "^does/not-match/*",
 					},
@@ -270,16 +273,16 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{},
 		},
-		TestCase{
+		{
 			payloads: []string{"diff_pr"},
 			name:     "Test the files rule",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label: "Files",
 						Files: []string{
-							"^pkg/.*_test.go",
+							"^.*.md",
 						},
 					},
 				},
@@ -287,17 +290,17 @@ func TestHandleEvent(t *testing.T) {
 			initialLabels:  []string{},
 			expectedLabels: []string{"Files"},
 		},
-		TestCase{
+		{
 			payloads: []string{"small_pr"},
 			name:     "Multiple conditions for the same tag function as OR",
 			config: LabelerConfigV1{
 				Version: 1,
 				Labels: []LabelMatcher{
-					LabelMatcher{
+					{
 						Label:  "Branch",
 						Branch: "^srvaroa-patch.*",
 					},
-					LabelMatcher{
+					{
 						Label:  "Branch",
 						Branch: "WONT MATCH",
 					},
@@ -309,19 +312,21 @@ func TestHandleEvent(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		for _, file := range tc.payloads {
-			payload, err := loadPayload(file)
-			if err != nil {
-				t.Fatal(err)
-			}
+		t.Run(tc.name, func(t *testing.T) {
+			for _, file := range tc.payloads {
+				payload, err := loadPayload(file)
+				if err != nil {
+					t.Fatal(err)
+				}
 
-			fmt.Println(tc.name)
-			l := NewTestLabeler(t, tc)
-			err = l.HandleEvent("pull_request", &payload)
-			if err != nil {
-				t.Fatal(err)
+				fmt.Println(tc.name)
+				l := NewTestLabeler(t, tc)
+				err = l.HandleEvent("pull_request", &payload)
+				if err != nil {
+					t.Fatal(err)
+				}
 			}
-		}
+		})
 	}
 }
 
@@ -342,5 +347,30 @@ func NewTestLabeler(t *testing.T, tc TestCase) Labeler {
 			return fmt.Errorf("%s: Expecting %+v, got %+v",
 				tc.name, tc.expectedLabels, labels)
 		},
+		Client: &FakeHttpClient{},
+	}
+}
+
+type FakeHttpClient struct {
+}
+
+func (f *FakeHttpClient) Do(req *http.Request) (*http.Response, error) {
+	file, err := os.Open("../test_data/diff_response")
+	if err != nil {
+		return nil, err
+	}
+
+	data, err := ioutil.ReadAll(file)
+	if err != nil {
+		return nil, err
+	}
+
+	diffReader := strings.NewReader(string(data))
+	diffReadCloser := io.NopCloser(diffReader)
+
+	response := http.Response{
+		StatusCode: http.StatusOK,
+		Body:       diffReadCloser,
 	}
+	return &response, nil
 }
diff --git a/test_data/diff_response b/test_data/diff_response
new file mode 100644
index 0000000..6873999
--- /dev/null
+++ b/test_data/diff_response
@@ -0,0 +1,49 @@
+diff --git a/README.md b/README.md
+index d8827a8..3ef9a3c 100644
+--- a/README.md
++++ b/README.md
+@@ -30,7 +30,7 @@ jobs:
+         GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
+ ```
+
+-Then add a new file `./github/labeler.yml` with the configuration as
++Then add a new file `.github/labeler.yml` with the configuration as
+ described below in the `Configuration` section.
+
+ This action will avoid failing in all cases, so if you're experiencing
+@@ -44,7 +44,7 @@ Typical errors are:
+
+ ## Configuration
+
+-Configuration can be stored at `./github/labeler.yml` as a plain list of
++Configuration can be stored at `.github/labeler.yml` as a plain list of
+ label matchers, which consist of a label and a set of conditions for
+ each.  When *all* conditions for a label match, then the Action will set
+ the given label.  When *any* condition for a label does not match, then
+@@ -58,7 +58,7 @@ Here is an example of a matcher for label "Example":
+ <condition_name>: <condition_parameters>
+ ```
+
+-For example, this `./github/labeler.yml` contains a single matcher with
++For example, this `.github/labeler.yml` contains a single matcher with
+ a single condition:
+
+ ```yaml
+@@ -76,7 +76,7 @@ Each label may combine multiple conditions.  The action combines all
+ conditions with an AND operation.  That is, the label will be applied if
+ *all* conditions are satisfied, removed otherwise.
+
+-For example, given this `./github/labeler.yml`:
++For example, given this `.github/labeler.yml`:
+
+ ```yaml
+ version: 1
+@@ -151,7 +151,7 @@ the PR is within given thresholds.
+  The number of changed lines is calculated as the sum of all `additions +
+  deletions` in the PR.
+
+ -For example, given this `./github/labeler.yml`:
+ +For example, given this `.github/labeler.yml`:
+
+  ```yaml
+  - label: "S"
\ No newline at end of file