Address PR feedback

Use table driven tests and `CombinedOutput`.
bazel-contrib · Jun 5, 2023 · a66f2f3 · a66f2f3
1 parent 8dfd83e
commit a66f2f3
Show file tree

Hide file tree

Showing 2 changed files with 107 additions and 50 deletions.
diff --git a/go/tools/builders/nolint_test.go b/go/tools/builders/nolint_test.go
@@ -20,28 +20,60 @@ import (
 )
 
 func TestParseNolint(t *testing.T) {
-	assert := func(text string, linters map[string]bool, valid bool) {
-		result, ok := parseNolint(text)
-		if valid != ok {
-			t.Fatalf("parseNolint expect %t got %t", valid, ok)
-		}
-		if !reflect.DeepEqual(result, linters) {
-			t.Fatalf("parseNolint expect %v got %v", linters, result)
-		}
+	tests := []struct {
+		Name    string
+		Comment string
+		Valid   bool
+		Linters []string
+	}{
+		{
+			Name:    "Invalid",
+			Comment: "not a comment",
+		},
+		{
+			Name:    "No match",
+			Comment: "// comment",
+		},
+		{
+			Name:    "All linters",
+			Comment: "//nolint",
+			Valid:   true,
+		},
+		{
+			Name:    "All linters (explicit)",
+			Comment: "//nolint:all",
+			Valid:   true,
+		},
+		{
+			Name:    "Single linter",
+			Comment: "// nolint:foo",
+			Valid:   true,
+			Linters: []string{"foo"},
+		},
+		{
+			Name:    "Multiple linters",
+			Comment: "// nolint:a,b,c",
+			Valid:   true,
+			Linters: []string{"a", "b", "c"},
+		},
 	}
 
-	assert("not a comment", nil, false)
-	assert("// comment", nil, false)
-	assert("//nolint", nil, true)
-	assert("//nolint:all", nil, true)
-	assert("// nolint:foo", map[string]bool{"foo": true}, true)
-	assert(
-		"// nolint:foo,bar,baz",
-		map[string]bool{
-			"foo": true,
-			"bar": true,
-			"baz": true,
-		},
-		true,
-	)
+	for _, tc := range tests {
+		t.Run(tc.Name, func(t *testing.T) {
+			result, ok := parseNolint(tc.Comment)
+			if tc.Valid != ok {
+				t.Fatalf("parseNolint expect %t got %t", tc.Valid, ok)
+			}
+			var linters map[string]bool
+			if len(tc.Linters) != 0 {
+				linters = make(map[string]bool)
+				for _, l := range tc.Linters {
+					linters[l] = true
+				}
+			}
+			if !reflect.DeepEqual(result, linters) {
+				t.Fatalf("parseNolint expect %v got %v", linters, result)
+			}
+		})
+	}
 }
diff --git a/tests/core/nogo/nolint/nolint_test.go b/tests/core/nogo/nolint/nolint_test.go
@@ -155,41 +155,66 @@ var V = struct {
 	})
 }
 
-func runTest(t *testing.T, target string, expected string) {
-	t.Helper()
-
-	cmd := bazel_testing.BazelCmd("build", target)
-	stderr := &bytes.Buffer{}
-	cmd.Stderr = stderr
-	err := cmd.Run()
-	output := stderr.String()
-	if expected != "" && err == nil {
-		t.Fatal("unexpected success", output)
-	}
-	if expected == "" && err != nil {
-		t.Fatal("unexpected failure", output)
-	}
-	if !strings.Contains(output, expected) {
-		t.Errorf("output did not contain expected: %s\n%s", expected, output)
-	}
-}
-
 func Test(t *testing.T) {
 	customRegister := `go_register_toolchains(nogo = "@//:nogo")`
 	if err := replaceInFile("WORKSPACE", "go_register_toolchains()", customRegister); err != nil {
 		t.Fatal(err)
 	}
 
-	// Execute tests that should filter out errors
-	runTest(t, "//:inline", "")
-	runTest(t, "//:inline_filter", "")
-	runTest(t, "//:block", "")
-	runTest(t, "//:block_multiline", "")
+	tests := []struct {
+		Name     string
+		Target   string
+		Expected string
+	}{
+		{
+			Name:   "Inline comment",
+			Target: "//:inline",
+		},
+		{
+			Name:   "Inline with lint filter",
+			Target: "//:inline_filter",
+		},
+		{
+			Name:   "Block comment",
+			Target: "//:block",
+		},
+		{
+			Name:   "Multiline block comment",
+			Target: "//:block_multiline",
+		},
+		{
+			Name:     "Inline with errors",
+			Target:   "//:inline_errors",
+			Expected: "inline_errors.go:9:15: nil dereference in load (nilness)",
+		},
+		{
+			Name:     "Inline comment on same column does not apply",
+			Target:   "//:inline_column",
+			Expected: "inline_column.go:8:27: redundant or: true || true (bools)",
+		},
+		{
+			Name:     "Inline comment does not apply to larger block",
+			Target:   "//:large_block",
+			Expected: "large_block.go:10:5: redundant or: true || true (bools)",
+		},
+	}
 
-	// Execute tests that should return unfiltered errors
-	runTest(t, "//:inline_errors", "inline_errors.go:9:15: nil dereference in load (nilness)")
-	runTest(t, "//:inline_column", "inline_column.go:8:27: redundant or: true || true (bools)")
-	runTest(t, "//:large_block", "large_block.go:10:5: redundant or: true || true (bools)")
+	for _, tc := range tests {
+		t.Run(tc.Name, func(t *testing.T) {
+			cmd := bazel_testing.BazelCmd("build", tc.Target)
+			b, err := cmd.CombinedOutput()
+			output := string(b)
+			if tc.Expected != "" && err == nil {
+				t.Fatal("unexpected success", output)
+			}
+			if tc.Expected == "" && err != nil {
+				t.Fatal("unexpected failure", output)
+			}
+			if !strings.Contains(output, tc.Expected) {
+				t.Errorf("output did not contain expected: %s\n%s", tc.Expected, output)
+			}
+		})
+	}
 }
 
 func replaceInFile(path, old, new string) error {