Repository: googleapis/api-linter
Pull request #536 (2020-05-01 07:23:24)

fix detection of lint problems for exit status
This PR fixes a false positive reading of the lint results by checking `result[*].Problems` for lint results per file

see https://github.com/googleapis/api-linter/pull/534#discussion_r416489984

lukesneeringer: Thank you @RoryQ!

Patch:
diff --git a/cmd/api-linter/cli.go b/cmd/api-linter/cli.go
index c602269..ff2b2e1 100644
--- a/cmd/api-linter/cli.go
+++ b/cmd/api-linter/cli.go
@@ -190,13 +190,22 @@ func (c *cli) lint(rules lint.RuleRegistry, configs lint.Configs) error {
 
 	// Return error on lint failure which subsequently
 	// exits with a non-zero status code
-	if c.ExitStatusOnLintFailure && len(results) > 0 {
+	if c.ExitStatusOnLintFailure && anyProblems(results) {
 		return ExitForLintFailure
 	}
 
 	return nil
 }
 
+func anyProblems(results []lint.Response) bool {
+	for i := range results {
+		if len(results[i].Problems) > 0 {
+			return true
+		}
+	}
+	return false
+}
+
 func loadFileDescriptors(filePaths ...string) (map[string]*desc.FileDescriptor, error) {
 	fds := []*dpb.FileDescriptorProto{}
 	for _, filePath := range filePaths {


Test patch:
diff --git a/cmd/api-linter/integration_test.go b/cmd/api-linter/integration_test.go
index 785b67a..734b720 100644
--- a/cmd/api-linter/integration_test.go
+++ b/cmd/api-linter/integration_test.go
@@ -144,28 +144,41 @@ internal/testdata/build_errors.proto:13:1: syntax error: unexpected '}', expecti
 }
 
 func TestExitStatusForLintFailure(t *testing.T) {
-	for _, test := range testCases {
-		// checks lint failure = true when lint problems found
-		t.Run(test.testName+"ReturnsFailure", func(t *testing.T) {
-			proto := test.proto
-			lintFailureStatus, result := runLinterWithFailureStatus(t, proto, "", []string{"--set-exit-status"})
-			expected := result != ""
-			if lintFailureStatus != expected {
-				t.Fatalf("Expected: %v Actual: %v", expected, lintFailureStatus)
-			}
-		})
+	type testCase struct{ testName, rule, proto string }
+	failCase := testCase{
+		testName: "GetRequestMessage",
+		rule:     "core::0131::request-message-name",
+		proto: `
+		syntax = "proto3";
 
-		// checks lint failure = false when no problems found
-		t.Run(test.testName+"ReturnsNoFailure", func(t *testing.T) {
-			disableInline := fmt.Sprintf("(-- api-linter: %s=disabled --)", test.rule)
-			proto := strings.Replace(test.proto, "disable-me-here", disableInline, -1)
-			lintFailureStatus, result := runLinterWithFailureStatus(t, proto, "", []string{"--set-exit-status"})
-			expected := result != ""
-			if lintFailureStatus != expected {
-				t.Fatalf("Expected: %v Actual: %v", expected, lintFailureStatus)
-			}
-		})
+		service Library {
+			// disable-me-here
+			rpc GetBook(Book) returns (Book);
+		}
+
+		message Book {}
+		`,
 	}
+	// checks lint failure = true when lint problems found
+	t.Run(failCase.testName+"ReturnsFailure", func(t *testing.T) {
+		lintFailureStatus, result := runLinterWithFailureStatus(t, failCase.proto, "", []string{"--set-exit-status"})
+		if lintFailureStatus == false {
+			t.Log(result)
+			t.Fatalf("Expected: %v Actual: %v", true, lintFailureStatus)
+		}
+	})
+
+	// checks lint failure = false when no problems found
+	t.Run(failCase.testName+"ReturnsNoFailure", func(t *testing.T) {
+		disableAll := `[ { "disabled_rules": [ "core" ] } ]`
+		lintFailureStatus, result := runLinterWithFailureStatus(t, failCase.proto, disableAll, []string{"--set-exit-status"})
+		if lintFailureStatus {
+			t.Log(result)
+		}
+		if lintFailureStatus == true {
+			t.Fatalf("Expected: %v Actual: %v", false, lintFailureStatus)
+		}
+	})
 
 	// checks lint failure = false when lint problems found but --set-exit-status not set
 	for _, test := range testCases {
@@ -177,7 +190,6 @@ func TestExitStatusForLintFailure(t *testing.T) {
 				t.Fatalf("Expected: %v Actual: %v", expected, lintFailureStatus)
 			}
 		})
-
 	}
 }