Repository: reviewdog/reviewdog
Pull request #533 (2020-05-03 15:59:11)

Exit 1 if conclusion is failure and -fail-on-error for github-check reporter
For #446 and #403


Patch:
diff --git a/.github/workflows/reviewdog.yml b/.github/workflows/reviewdog.yml
index 4f2e62e8..e99bba62 100644
--- a/.github/workflows/reviewdog.yml
+++ b/.github/workflows/reviewdog.yml
@@ -84,7 +84,7 @@ jobs:
         env:
           REVIEWDOG_GITHUB_API_TOKEN: ${{ secrets.GITHUB_TOKEN }}
         run: |
-          reviewdog -reporter=github-pr-check -runners=golint,govet
+          reviewdog -reporter=github-pr-check -runners=golint,govet -fail-on-error
 
       - name: Run reviewdog (github-pr-review with tee)
         env:
@@ -106,8 +106,16 @@ jobs:
         run: |
           echo 'var LocalTest = 14' >> ./_testdata/golint.go
           cd ./_testdata/ && golint ./... | reviewdog -f=golint -diff="git diff" -fail-on-error || EXIT_CODE=$?
+          git reset --hard @
           test ${EXIT_CODE} = 1
 
+      - name: Run reviewdog (github-pr-check with fail-on-error)
+        continue-on-error: true
+        env:
+          REVIEWDOG_GITHUB_API_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        run: |
+          cd ./_testdata/ && golint ./... | reviewdog -f=golint -name=golint-pr-check-fail-on-error -reporter=github-pr-check -fail-on-error
+
   golangci-lint:
     if: github.event_name == 'pull_request'
     name: runner / golangci-lint
diff --git a/README.md b/README.md
index cef83901..e26b94ce 100644
--- a/README.md
+++ b/README.md
@@ -661,11 +661,13 @@ $ REVIEWDOG_GITHUB_API_TOKEN="<token>" reviewdog -reporter=github-pr-review
 ```
 
 ## Exit codes
-By default Reviewdog will return `0` as exit code once all actions done (linter report analyzed, comments posted and so on).
-
-Once `fail-on-error` flag passed - Reviewdog will return `1` as exit code if at least one violation was found/reported.
+By default reviewdog will return `0` as exit code even if it finds errors.
+If `-fail-on-error` flag is passed, reviewdog exits with `1` when at least one error was found/reported.
 This can be helpful when you are using it as a step in your CI pipeline and want to mark the step failed if any error found by linter.
 
+See also `-level` flag for [github-pr-check/github-check](#reporter-github-checks--reportergithub-pr-check) reporters.
+reviewdog will exit with `1` if reported check status is `failure` as well if `-fail-on-error=true`.
+
 ## Articles
 - [reviewdog — A code review dog who keeps your codebase healthy ](https://medium.com/@haya14busa/reviewdog-a-code-review-dog-who-keeps-your-codebase-healthy-d957c471938b)
 - [reviewdog ♡ GitHub Check — improved automated review experience](https://medium.com/@haya14busa/reviewdog-github-check-improved-automated-review-experience-58f89e0c95f3)
diff --git a/cmd/reviewdog/doghouse.go b/cmd/reviewdog/doghouse.go
index d034fa32..95a88633 100644
--- a/cmd/reviewdog/doghouse.go
+++ b/cmd/reviewdog/doghouse.go
@@ -17,7 +17,6 @@ import (
 
 	"github.com/reviewdog/reviewdog"
 	"github.com/reviewdog/reviewdog/cienv"
-	"github.com/reviewdog/reviewdog/difffilter"
 	"github.com/reviewdog/reviewdog/doghouse"
 	"github.com/reviewdog/reviewdog/doghouse/client"
 	"github.com/reviewdog/reviewdog/project"
@@ -42,7 +41,7 @@ func runDoghouse(ctx context.Context, r io.Reader, w io.Writer, opt *option, isP
 	if err != nil {
 		return err
 	}
-	filteredResultSet, err := postResultSet(ctx, resultSet, ghInfo, cli, forPr, opt.filterMode)
+	filteredResultSet, err := postResultSet(ctx, resultSet, ghInfo, cli, forPr, opt)
 	if err != nil {
 		return err
 	}
@@ -113,7 +112,7 @@ func checkResultSet(ctx context.Context, r io.Reader, opt *option, isProject boo
 }
 
 func postResultSet(ctx context.Context, resultSet *reviewdog.ResultMap,
-	ghInfo *cienv.BuildInfo, cli client.DogHouseClientInterface, forPr bool, filterMode difffilter.Mode) (*reviewdog.FilteredResultMap, error) {
+	ghInfo *cienv.BuildInfo, cli client.DogHouseClientInterface, forPr bool, opt *option) (*reviewdog.FilteredResultMap, error) {
 	var g errgroup.Group
 	wd, _ := os.Getwd()
 	gitRelWd, err := serviceutil.GitRelWorkdir()
@@ -138,7 +137,7 @@ func postResultSet(ctx context.Context, resultSet *reviewdog.ResultMap,
 			Level:       result.Level,
 			// If it's only for PR, do not report results outside diff.
 			OutsideDiff: !forPr,
-			FilterMode:  filterMode,
+			FilterMode:  opt.filterMode,
 		}
 		g.Go(func() error {
 			res, err := cli.Check(ctx, req)
@@ -146,7 +145,11 @@ func postResultSet(ctx context.Context, resultSet *reviewdog.ResultMap,
 				return fmt.Errorf("post failed for %s: %v", name, err)
 			}
 			if res.ReportURL != "" {
-				log.Printf("[%s] reported: %s", name, res.ReportURL)
+				conclusion := ""
+				if res.Conclusion != "" {
+					conclusion = fmt.Sprintf(" (conclusion=%s)", res.Conclusion)
+				}
+				log.Printf("[%s] reported: %s%s", name, res.ReportURL, conclusion)
 			} else if res.CheckedResults != nil {
 				// Fill results only when report URL is missing, which probably means
 				// it failed to report results with Check API.
@@ -156,7 +159,17 @@ func postResultSet(ctx context.Context, resultSet *reviewdog.ResultMap,
 				})
 			}
 			if res.ReportURL == "" && res.CheckedResults == nil {
-				return fmt.Errorf("no result found for %q", name)
+				return fmt.Errorf("[%s] no result found", name)
+			}
+			// If failOnError is on, return error when at least one report
+			// returns failure conclusion (status). Users can check this
+			// reviewdoc run status (#446) to merge PRs for example.
+			//
+			// Also, the individual report conclusions are associated to random check
+			// suite due to the GitHub bug (#403), so actually users cannot depends
+			// on each report as of writing.
+			if opt.failOnError && (res.Conclusion == "failure") {
+				return fmt.Errorf("[%s] Check conclusion is %q", name, res.Conclusion)
 			}
 			return nil
 		})
diff --git a/difffilter/filter.go b/difffilter/filter.go
index 304dfca1..a7251a86 100644
--- a/difffilter/filter.go
+++ b/difffilter/filter.go
@@ -51,7 +51,7 @@ type DiffFilter struct {
 
 	// Relative path to the project root (e.g. git) directory from current workdir.
 	// It can be empty if it doesn't find any project root directory.
-	relPathToProjectRoot string
+	projectRelPath string
 
 	strip int
 	mode  Mode
@@ -70,7 +70,10 @@ func New(diff []*diff.FileDiff, strip int, cwd string, mode Mode) *DiffFilter {
 		mode:      mode,
 		difflines: make(difflines),
 	}
-	df.relPathToProjectRoot, _ = serviceutil.GitRelWorkdir()
+	// If cwd is empty, projectRelPath should not have any meaningful data too.
+	if cwd != "" {
+		df.projectRelPath, _ = serviceutil.GitRelWorkdir()
+	}
 	df.addDiff(diff)
 	return df
 }
@@ -133,8 +136,8 @@ func (df *DiffFilter) normalizePath(path string) normalizedPath {
 			path = relPath
 		}
 	}
-	if !filepath.IsAbs(path) && df.relPathToProjectRoot != "" {
-		path = filepath.Join(df.relPathToProjectRoot, path)
+	if !filepath.IsAbs(path) && df.projectRelPath != "" {
+		path = filepath.Join(df.projectRelPath, path)
 	}
 	return normalizedPath{p: filepath.ToSlash(path)}
 }
diff --git a/doghouse/server/doghouse.go b/doghouse/server/doghouse.go
index 33a77924..486602ca 100644
--- a/doghouse/server/doghouse.go
+++ b/doghouse/server/doghouse.go
@@ -64,17 +64,18 @@ func (ch *Checker) Check(ctx context.Context) (*doghouse.CheckResponse, error) {
 		return nil, fmt.Errorf("failed to create check: %v", err)
 	}
 
-	checkRun, err := ch.postCheck(ctx, check.GetID(), filtered)
+	checkRun, conclusion, err := ch.postCheck(ctx, check.GetID(), filtered)
 	if err != nil {
 		return nil, fmt.Errorf("failed to post result: %v", err)
 	}
 	res := &doghouse.CheckResponse{
-		ReportURL: checkRun.GetHTMLURL(),
+		ReportURL:  checkRun.GetHTMLURL(),
+		Conclusion: conclusion,
 	}
 	return res, nil
 }
 
-func (ch *Checker) postCheck(ctx context.Context, checkID int64, checks []*reviewdog.FilteredCheck) (*github.CheckRun, error) {
+func (ch *Checker) postCheck(ctx context.Context, checkID int64, checks []*reviewdog.FilteredCheck) (*github.CheckRun, string, error) {
 	filterByDiff := ch.req.PullRequest != 0 && !ch.req.OutsideDiff
 	var annotations []*github.CheckRunAnnotation
 	for _, c := range checks {
@@ -85,7 +86,7 @@ func (ch *Checker) postCheck(ctx context.Context, checkID int64, checks []*revie
 	}
 	if len(annotations) > 0 {
 		if err := ch.postAnnotations(ctx, checkID, annotations); err != nil {
-			return nil, fmt.Errorf("failed to post annotations: %v", err)
+			return nil, "", fmt.Errorf("failed to post annotations: %v", err)
 		}
 	}
 
@@ -103,7 +104,11 @@ func (ch *Checker) postCheck(ctx context.Context, checkID int64, checks []*revie
 			Summary: github.String(ch.summary(checks)),
 		},
 	}
-	return ch.gh.UpdateCheckRun(ctx, ch.req.Owner, ch.req.Repo, checkID, opt)
+	checkRun, err := ch.gh.UpdateCheckRun(ctx, ch.req.Owner, ch.req.Repo, checkID, opt)
+	if err != nil {
+		return nil, "", err
+	}
+	return checkRun, conclusion, nil
 }
 
 func (ch *Checker) createCheck(ctx context.Context) (*github.CheckRun, error) {
diff --git a/doghouse/service.go b/doghouse/service.go
index bcb0890b..825b878e 100644
--- a/doghouse/service.go
+++ b/doghouse/service.go
@@ -63,7 +63,11 @@ type CheckResponse struct {
 	// TODO(haya14busa): Consider to move this type to this package to avoid
 	// (cyclic) import.
 	// Optional.
-	CheckedResults []*reviewdog.FilteredCheck
+	CheckedResults []*reviewdog.FilteredCheck `json:"checked_results"`
+
+	// Conclusion of check result, which is same as GitHub's conclusion of Check
+	// API. https://developer.github.com/v3/checks/runs/#parameters-1
+	Conclusion string `json:"conclusion,omitempty"`
 }
 
 // Annotation represents an annotation to file or specific line.


Test patch:
diff --git a/cmd/reviewdog/doghouse_test.go b/cmd/reviewdog/doghouse_test.go
index 82e2c7b2..b4d29d2c 100644
--- a/cmd/reviewdog/doghouse_test.go
+++ b/cmd/reviewdog/doghouse_test.go
@@ -3,6 +3,7 @@ package main
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -276,7 +277,8 @@ func TestPostResultSet_withReportURL(t *testing.T) {
 		SHA:         sha,
 	}
 
-	if _, err := postResultSet(context.Background(), &resultSet, ghInfo, fakeCli, true, difffilter.ModeAdded); err != nil {
+	opt := &option{filterMode: difffilter.ModeAdded}
+	if _, err := postResultSet(context.Background(), &resultSet, ghInfo, fakeCli, true, opt); err != nil {
 		t.Fatal(err)
 	}
 }
@@ -300,7 +302,8 @@ func TestPostResultSet_withoutReportURL(t *testing.T) {
 
 	ghInfo := &cienv.BuildInfo{Owner: owner, Repo: repo, PullRequest: prNum, SHA: sha}
 
-	resp, err := postResultSet(context.Background(), &resultSet, ghInfo, fakeCli, true, difffilter.ModeAdded)
+	opt := &option{filterMode: difffilter.ModeAdded}
+	resp, err := postResultSet(context.Background(), &resultSet, ghInfo, fakeCli, true, opt)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -316,6 +319,47 @@ func TestPostResultSet_withoutReportURL(t *testing.T) {
 	}
 }
 
+func TestPostResultSet_conclusion(t *testing.T) {
+	const (
+		owner = "haya14busa"
+		repo  = "reviewdog"
+		prNum = 14
+		sha   = "1414"
+	)
+
+	fakeCli := &fakeDoghouseServerCli{}
+	var resultSet reviewdog.ResultMap
+	resultSet.Store("name1", &reviewdog.Result{CheckResults: []*reviewdog.CheckResult{}})
+	ghInfo := &cienv.BuildInfo{Owner: owner, Repo: repo, PullRequest: prNum, SHA: sha}
+
+	tests := []struct {
+		conclusion  string
+		failOnError bool
+		wantErr     bool
+	}{
+		{conclusion: "failure", failOnError: true, wantErr: true},
+		{conclusion: "neutral", failOnError: true, wantErr: false},
+		{conclusion: "success", failOnError: true, wantErr: false},
+		{conclusion: "", failOnError: true, wantErr: false},
+		{conclusion: "failure", failOnError: false, wantErr: false},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		fakeCli.FakeCheck = func(ctx context.Context, req *doghouse.CheckRequest) (*doghouse.CheckResponse, error) {
+			return &doghouse.CheckResponse{ReportURL: "xxx", Conclusion: tt.conclusion}, nil
+		}
+		opt := &option{filterMode: difffilter.ModeAdded, failOnError: tt.failOnError}
+		id := fmt.Sprintf("[conclusion=%s, failOnError=%v]", tt.conclusion, tt.failOnError)
+		_, err := postResultSet(context.Background(), &resultSet, ghInfo, fakeCli, true, opt)
+		if tt.wantErr && err == nil {
+			t.Errorf("[%s] want err, but got nil.", id)
+		} else if !tt.wantErr && err != nil {
+			t.Errorf("[%s] got unexpected error: %v", id, err)
+		}
+	}
+}
+
 func TestPostResultSet_withEmptyResponse(t *testing.T) {
 	const (
 		owner = "haya14busa"
@@ -334,7 +378,8 @@ func TestPostResultSet_withEmptyResponse(t *testing.T) {
 
 	ghInfo := &cienv.BuildInfo{Owner: owner, Repo: repo, PullRequest: prNum, SHA: sha}
 
-	if _, err := postResultSet(context.Background(), &resultSet, ghInfo, fakeCli, true, difffilter.ModeAdded); err == nil {
+	opt := &option{filterMode: difffilter.ModeAdded}
+	if _, err := postResultSet(context.Background(), &resultSet, ghInfo, fakeCli, true, opt); err == nil {
 		t.Error("got no error but want report missing error")
 	}
 }
diff --git a/doghouse/server/doghouse_test.go b/doghouse/server/doghouse_test.go
index e4603010..32968dd5 100644
--- a/doghouse/server/doghouse_test.go
+++ b/doghouse/server/doghouse_test.go
@@ -31,16 +31,16 @@ func (f *fakeCheckerGitHubCli) UpdateCheckRun(ctx context.Context, owner, repo s
 	return f.FakeUpdateCheckRun(ctx, owner, repo, checkID, opt)
 }
 
-const sampleDiff = `--- a/doghouse/server/sample.old.txt	2016-10-13 05:09:35.820791185 +0900
-+++ b/doghouse/server/sample.new.txt	2016-10-13 05:15:26.839245048 +0900
+const sampleDiff = `--- a/sample.old.txt	2016-10-13 05:09:35.820791185 +0900
++++ b/sample.new.txt	2016-10-13 05:15:26.839245048 +0900
 @@ -1,3 +1,4 @@
  unchanged, contextual line
 -deleted line
 +added line
 +added line
  unchanged, contextual line
---- a/doghouse/server/nonewline.old.txt	2016-10-13 15:34:14.931778318 +0900
-+++ b/doghouse/server/nonewline.new.txt	2016-10-13 15:34:14.868444672 +0900
+--- a/nonewline.old.txt	2016-10-13 15:34:14.931778318 +0900
++++ b/nonewline.new.txt	2016-10-13 15:34:14.868444672 +0900
 @@ -1,4 +1,4 @@
  " vim: nofixeol noendofline
  No newline at end of both the old and new file