Repository: googleapis/api-linter
Pull request #189 (2019-09-24 00:57:43)

[feat] Problem testing redux
This is my (hopefully last ;-)) attempt to make problem testing reasonable. It adds a `Problems` type which is just `[]Problem`, and gives it an `Equal` method so that it can be passed to `cmp.Diff`.

There is also a `SetDescriptor` helper used for tests.

As before, I have migrated only a couple of tests to get feedback before migrating the rest.

And hopefully this is the last major interface change. :-)

lukesneeringer: Question to @mingzhi and @alin04: Can/should I leave the rule interface as `[]lint.Problem`?
lukesneeringer: > Question to @mingzhi and @alin04: Can/should I leave the rule interface as `[]lint.Problem`? I would assume and hope that a Problems object also satisfies `[]lint.Problem`.

After playing around with this, it _can_ be done but I expect it is not a good idea. It basically means I have to typecast in every test, which is annoying and not worth the value of being more generic in the interface.
mingzhi: Hi Luke, I am Okay with a `Problems`, but (strongly) against defining a `Equal` function in the main library. If it is for testing and to satisfy `cmp.Diff`, I suggest you to put them to a test util package.

For any public function, we should assume:
1. people will use it;
2. they will not read the comments;
3. they will not read the implementation.

So the method name should be accurate and informative -- `Equal` means "Equal", nothing else. 
mingzhi: I actually like Java [Truth](https://truth.dev/api/latest/com/google/common/truth/Truth.html) style, e.g.,:
```
assertThat(problems).hasOne().containsMessage("hello, world").containsDescriptor(desc);
```
It is very self-explanatory.
lukesneeringer: Our Golang docs say that assert libraries are bad, so that is out.
I can bring back `ProblemStubs` to accomplish this, but it requires having `ProblemStubs`.
lukesneeringer: I redid this, and I agree with a lot of the feedback:

  * I moved `Problems` into a testutils package.
  * I stopped calling `cmp.Diff` directly (essentially because it becomes a royal pain when using different types and no common interface) and just added a `Problems.Diff` that returns empty string for equivalence and calls out to `cmp.Diff` otherwise. This basically mimics the `cmp.Diff` interface without a lot of developer pain.
  * I am pretending with all of my might that `problems.Diff` is not an assert library even though it totally is.
  * All use of `Problems` in non-test code is reverted to `[]lint.Problem`
lukesneeringer: @robcapo / @mingzhi Can you review this? :)

Patch:
diff --git a/go.mod b/go.mod
index acc0c2d..fc8bb8d 100644
--- a/go.mod
+++ b/go.mod
@@ -5,6 +5,7 @@ go 1.12
 require (
 	github.com/bmatcuk/doublestar v1.1.5
 	github.com/golang/protobuf v1.3.2
+	github.com/google/go-cmp v0.3.1
 	github.com/jhump/protoreflect v1.5.0
 	github.com/stoewer/go-strcase v1.0.2
 	github.com/urfave/cli v1.22.1
diff --git a/go.sum b/go.sum
index b315993..3b24757 100644
--- a/go.sum
+++ b/go.sum
@@ -15,6 +15,8 @@ github.com/golang/protobuf v1.3.1 h1:YF8+flBXS5eO826T4nzqPrxfhQThhXl0YzfuUPu4SBg
 github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
 github.com/golang/protobuf v1.3.2 h1:6nsPYzhq5kReh6QImI3k5qWzO4PEbvbIW2cwSfR/6xs=
 github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
+github.com/google/go-cmp v0.3.1 h1:Xye71clBPdm5HgqGwUkwhbynsUJZhDbS20FvLhQ2izg=
+github.com/google/go-cmp v0.3.1/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU=
 github.com/jhump/protoreflect v1.5.0 h1:NgpVT+dX71c8hZnxHof2M7QDK7QtohIJ7DYycjnkyfc=
 github.com/jhump/protoreflect v1.5.0/go.mod h1:eaTn3RZAmMBcV0fifFvlm6VHNz3wSkYyXYWUh7ymB74=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
@@ -51,8 +53,6 @@ google.golang.org/appengine v1.4.0/go.mod h1:xpcJRLb0r/rnEns0DIKYYv+WjYCduHsrkT7
 google.golang.org/genproto v0.0.0-20170818010345-ee236bd376b0 h1:ZvI3lsq5AIkr7axxmT3tfwFlJVRFLqe6Fp0W03+MJ38=
 google.golang.org/genproto v0.0.0-20170818010345-ee236bd376b0/go.mod h1:JiN7NxoALGmiZfu7CAH4rXhgtRTLTxftemlI0sWmxmc=
 google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8/go.mod h1:JiN7NxoALGmiZfu7CAH4rXhgtRTLTxftemlI0sWmxmc=
-google.golang.org/genproto v0.0.0-20190916214212-f660b8655731 h1:Phvl0+G5t5k/EUFUi0wPdUUeTL2HydMQUXHnunWgSb0=
-google.golang.org/genproto v0.0.0-20190916214212-f660b8655731/go.mod h1:IbNlFCBrqXvoKpeg0TB2l7cyZUmoaFKYIwrEpbDKLA8=
 google.golang.org/genproto v0.0.0-20190926190326-7ee9db18f195 h1:dWzgMaXfaHsnkRKZ1l3iJLDmTEB40JMl/dqRbJX4D/o=
 google.golang.org/genproto v0.0.0-20190926190326-7ee9db18f195/go.mod h1:IbNlFCBrqXvoKpeg0TB2l7cyZUmoaFKYIwrEpbDKLA8=
 google.golang.org/genproto v0.0.0-20190927181202-20e1ac93f88c h1:hrpEMCZ2O7DR5gC1n2AJGVhrwiEjOi35+jxtIuZpTMo=
diff --git a/rules/internal/utils/http.go b/internal/utils/http.go
similarity index 100%
rename from rules/internal/utils/http.go
rename to internal/utils/http.go
diff --git a/rules/internal/utils/http_test.go b/internal/utils/http_test.go
similarity index 100%
rename from rules/internal/utils/http_test.go
rename to internal/utils/http_test.go
diff --git a/rules/aip0131/message_rules.go b/rules/aip0131/message_rules.go
index 678e242..935bee9 100644
--- a/rules/aip0131/message_rules.go
+++ b/rules/aip0131/message_rules.go
@@ -65,7 +65,7 @@ var unknownFields = &lint.MessageRule{
 			if _, ok := allowedFields[string(field.GetName())]; !ok {
 				problems = append(problems, lint.Problem{
 					Message: fmt.Sprintf(
-						"Get RPCs must only contain fields explicitly described in AIPs, not %q.",
+						"Unexpected field: Get RPCs must only contain fields explicitly described in AIPs, not %q.",
 						string(field.GetName()),
 					),
 					Descriptor: field,
diff --git a/rules/aip0131/method_rules.go b/rules/aip0131/method_rules.go
index 53b2c30..d1780f2 100644
--- a/rules/aip0131/method_rules.go
+++ b/rules/aip0131/method_rules.go
@@ -17,8 +17,8 @@ package aip0131
 import (
 	"fmt"
 
+	"github.com/googleapis/api-linter/internal/utils"
 	"github.com/googleapis/api-linter/lint"
-	"github.com/googleapis/api-linter/rules/internal/utils"
 	"github.com/jhump/protoreflect/desc"
 )
 
diff --git a/rules/internal/utils/doc.go b/rules/internal/utils/doc.go
deleted file mode 100644
index 4922d85..0000000
--- a/rules/internal/utils/doc.go
+++ /dev/null
@@ -1,2 +0,0 @@
-// Package utils contains helper functions for writing rules.
-package utils
diff --git a/rules/rules.go b/rules/rules.go
index 10c1f9a..1f3a8eb 100644
--- a/rules/rules.go
+++ b/rules/rules.go
@@ -20,6 +20,7 @@ import (
 	"github.com/googleapis/api-linter/rules/aip0131"
 	"github.com/googleapis/api-linter/rules/aip0132"
 	"github.com/googleapis/api-linter/rules/aip0140"
+	"github.com/googleapis/api-linter/rules/aip0158"
 	"github.com/googleapis/api-linter/rules/aip0191"
 )
 
@@ -27,6 +28,7 @@ func init() {
 	aip0131.AddRules(coreRules)
 	aip0132.AddRules(coreRules)
 	aip0140.AddRules(coreRules)
+	aip0158.AddRules(coreRules)
 	aip0191.AddRules(coreRules)
 }
 


Test patch:
diff --git a/internal/testutils/problems.go b/internal/testutils/problems.go
new file mode 100644
index 0000000..f285185
--- /dev/null
+++ b/internal/testutils/problems.go
@@ -0,0 +1,74 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 		https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package testutils
+
+import (
+	"strings"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/googleapis/api-linter/lint"
+	"github.com/jhump/protoreflect/desc"
+)
+
+// Problems is a slice of individual Problem objects.
+type Problems []lint.Problem
+
+// Diff determines whether a Problem is sufficiently similar to another
+// to be considered equivalent, and returns a diff otherwise.
+//
+// This is intended for unit tests and is intentially generous on what
+// constitutes equality.
+func (problems Problems) Diff(other []lint.Problem) string {
+	// If the problems differ in length, they are by definition unequal.
+	if len(problems) != len(other) {
+		return cmp.Diff(problems, other)
+	}
+
+	// Iterate over the individual problems and determine whether they are
+	// sufficiently equivalent.
+	for i := range problems {
+		x, y := problems[i], other[i]
+
+		// The descriptors must exactly match, otherwise the problems are unequal.
+		if x.Descriptor != y.Descriptor {
+			return cmp.Diff(problems, other)
+		}
+
+		// The suggestions, if present, must exactly match.
+		if x.Suggestion != y.Suggestion {
+			return cmp.Diff(problems, other)
+		}
+
+		// When comparing messages, we want to know if the test string is a
+		// substring of the actual one.
+		if !strings.Contains(y.Message, x.Message) {
+			return cmp.Diff(problems, other)
+		}
+	}
+
+	// These sets of problems are sufficiently equal.
+	return ""
+}
+
+// SetDescriptor sets the given descriptor to every Problem in the slice and
+// returns the slice back.
+//
+// This is intended primarily for use in unit tests.
+func (problems Problems) SetDescriptor(d desc.Descriptor) Problems {
+	for i := range problems {
+		problems[i].Descriptor = d
+	}
+	return problems
+}
diff --git a/internal/testutils/problems_test.go b/internal/testutils/problems_test.go
new file mode 100644
index 0000000..5fd51b0
--- /dev/null
+++ b/internal/testutils/problems_test.go
@@ -0,0 +1,97 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 		https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package testutils
+
+import (
+	"testing"
+
+	"github.com/googleapis/api-linter/lint"
+	"github.com/jhump/protoreflect/desc/builder"
+)
+
+func TestDiffEquivalent(t *testing.T) {
+	// Build a message for the descriptor test.
+	m, err := builder.NewMessage("Foo").Build()
+	if err != nil {
+		t.Fatalf("Could not build descriptor.")
+	}
+
+	// Declare a series of tests that should all be equal.
+	tests := []struct {
+		name string
+		x    Problems
+		y    []lint.Problem
+	}{
+		{"NilNil", nil, nil},
+		{"ProblemNil", Problems{}, nil},
+		{"Descriptor", Problems{{Descriptor: m}}, []lint.Problem{{Descriptor: m}}},
+		{"Suggestion", Problems{{Suggestion: "foo"}}, []lint.Problem{{Suggestion: "foo"}}},
+		{"MessageExact", Problems{{Message: "foo"}}, []lint.Problem{{Message: "foo"}}},
+		{"MessageSubstr", Problems{{Message: "foo"}}, []lint.Problem{{Message: "foo bar"}}},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if diff := test.x.Diff(test.y); diff != "" {
+				t.Errorf("Problems were unequal (x, y):\n%v", diff)
+			}
+		})
+	}
+}
+
+func TestDiffNotEquivalent(t *testing.T) {
+	// Build a message for the descriptor test.
+	m1, err1 := builder.NewMessage("Foo").Build()
+	m2, err2 := builder.NewMessage("Bar").Build()
+	if err1 != nil || err2 != nil {
+		t.Fatalf("Could not build descriptor.")
+	}
+
+	// Declare a series of tests that should all be equal.
+	tests := []struct {
+		name string
+		x    Problems
+		y    []lint.Problem
+	}{
+		{"ProblemNil", Problems{{Descriptor: m1}}, nil},
+		{"EmptyProblemNil", Problems{{}}, nil},
+		{"LengthMismatch", Problems{{}}, []lint.Problem{{}, {}}},
+		{"Descriptor", Problems{{Descriptor: m1}}, []lint.Problem{{Descriptor: m2}}},
+		{"Suggestion", Problems{{Suggestion: "foo"}}, []lint.Problem{{Suggestion: "bar"}}},
+		{"Message", Problems{{Message: "foo"}}, []lint.Problem{{Message: "bar"}}},
+		{"MessageSuperstr", Problems{{Message: "foo bar"}}, []lint.Problem{{Message: "foo"}}},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if diff := test.x.Diff(test.y); diff == "" {
+				t.Errorf("Got no diff (x, y); expected one.")
+			}
+		})
+	}
+}
+
+func TestSetDescriptor(t *testing.T) {
+	m, err := builder.NewMessage("Foo").Build()
+	if err != nil {
+		t.Fatalf("Could not build descriptor.")
+	}
+	problems := Problems{{}, {}, {}}.SetDescriptor(m)
+	for _, p := range problems {
+		if p.Descriptor != m {
+			t.Errorf("Got %v, expected %v", p.Descriptor, m)
+		}
+	}
+}
diff --git a/rules/aip0131/message_rules_test.go b/rules/aip0131/message_rules_test.go
index e178c01..a49ef6e 100644
--- a/rules/aip0131/message_rules_test.go
+++ b/rules/aip0131/message_rules_test.go
@@ -17,6 +17,7 @@ package aip0131
 import (
 	"testing"
 
+	"github.com/googleapis/api-linter/internal/testutils"
 	"github.com/jhump/protoreflect/desc"
 	"github.com/jhump/protoreflect/desc/builder"
 	fpb "google.golang.org/genproto/protobuf/field_mask"
@@ -28,14 +29,11 @@ func TestStandardFields(t *testing.T) {
 		testName      string
 		messageName   string
 		nameFieldName string
-		nameFieldType *builder.FieldType
-		problemCount  int
-		errPrefix     string
+		problems      testutils.Problems
 	}{
-		{"Valid", "GetBookRequest", "name", builder.FieldTypeString(), 0, "False positive"},
-		{"InvalidName", "GetBookRequest", "id", builder.FieldTypeString(), 1, "False negative"},
-		{"InvalidType", "GetBookRequest", "name", builder.FieldTypeBytes(), 1, "False negative"},
-		{"Irrelevant", "AcquireBookRequest", "id", builder.FieldTypeString(), 0, "False positive"},
+		{"Valid", "GetBookRequest", "name", testutils.Problems{}},
+		{"InvalidName", "GetBookRequest", "id", testutils.Problems{{Message: "name"}}},
+		{"Irrelevant", "AcquireBookRequest", "id", testutils.Problems{}},
 	}
 
 	// Run each test individually.
@@ -43,21 +41,42 @@ func TestStandardFields(t *testing.T) {
 		t.Run(test.testName, func(t *testing.T) {
 			// Create an appropriate message descriptor.
 			message, err := builder.NewMessage(test.messageName).AddField(
-				builder.NewField(test.nameFieldName, test.nameFieldType),
+				builder.NewField(test.nameFieldName, builder.FieldTypeString()),
 			).Build()
 			if err != nil {
 				t.Fatalf("Could not build %s message.", test.messageName)
 			}
 
-			// Run the lint rule, and establish that it returns the correct
-			// number of problems.
-			if problems := standardFields.Lint(message.GetFile()); len(problems) != test.problemCount {
-				t.Errorf("%s on rule %s: %#v", test.errPrefix, standardFields.Name, problems)
+			// Run the lint rule, and establish that it returns the correct problems.
+			problems := standardFields.Lint(message.GetFile())
+			if diff := test.problems.SetDescriptor(message).Diff(problems); diff != "" {
+				t.Errorf("Problems did not match: %v", diff)
 			}
 		})
 	}
 }
 
+func TestStandardFieldsInvalidType(t *testing.T) {
+	// Create an appropriate message descriptor.
+	message, err := builder.NewMessage("GetBookRequest").AddField(
+		builder.NewField("name", builder.FieldTypeBytes()),
+	).Build()
+	if err != nil {
+		t.Fatalf("Could not build descriptor.")
+	}
+
+	// Run the lint rule, and establish that it returns the correct
+	// number of problems.
+	wantProblems := testutils.Problems{{
+		Descriptor: message.GetFields()[0],
+		Message:    "string",
+	}}
+	gotProblems := standardFields.Lint(message.GetFile())
+	if diff := wantProblems.Diff(gotProblems); diff != "" {
+		t.Errorf(diff)
+	}
+}
+
 func TestUnknownFields(t *testing.T) {
 	// Get the correct message type for google.protobuf.FieldMask.
 	fieldMask, err := desc.LoadMessageDescriptorForMessage(&fpb.FieldMask{})
@@ -67,17 +86,18 @@ func TestUnknownFields(t *testing.T) {
 
 	// Set up the testing permutations.
 	tests := []struct {
-		testName     string
-		messageName  string
-		fieldName    string
-		fieldType    *builder.FieldType
-		problemCount int
-		errPrefix    string
+		testName    string
+		messageName string
+		fieldName   string
+		fieldType   *builder.FieldType
+		problems    testutils.Problems
 	}{
-		{"ReadMask", "GetBookRequest", "read_mask", builder.FieldTypeImportedMessage(fieldMask), 0, "False positive"},
-		{"View", "GetBookRequest", "view", builder.FieldTypeEnum(builder.NewEnum("View")), 0, "False positive"},
-		{"Invalid", "GetBookRequest", "application_id", builder.FieldTypeString(), 1, "False negative"},
-		{"Irrelevant", "AcquireBookRequest", "application_id", builder.FieldTypeString(), 0, "False positive"},
+		{"ReadMask", "GetBookRequest", "read_mask", builder.FieldTypeImportedMessage(fieldMask), testutils.Problems{}},
+		{"View", "GetBookRequest", "view", builder.FieldTypeEnum(builder.NewEnum("View")), testutils.Problems{}},
+		{"Invalid", "GetBookRequest", "application_id", builder.FieldTypeString(), testutils.Problems{{
+			Message: "Unexpected field",
+		}}},
+		{"Irrelevant", "AcquireBookRequest", "application_id", builder.FieldTypeString(), testutils.Problems{}},
 	}
 
 	// Run each test individually.
@@ -93,10 +113,11 @@ func TestUnknownFields(t *testing.T) {
 				t.Fatalf("Could not build GetBookRequest message.")
 			}
 
-			// Run the lint rule, and establish that it returns the correct
-			// number of problems.
-			if problems := unknownFields.Lint(message.GetFile()); len(problems) != test.problemCount {
-				t.Errorf("%s on rule %s: %#v", test.errPrefix, unknownFields.Name, problems)
+			// Run the lint rule, and establish that it returns the correct problems.
+			wantProblems := test.problems.SetDescriptor(message.FindFieldByName(test.fieldName))
+			gotProblems := unknownFields.Lint(message.GetFile())
+			if diff := wantProblems.Diff(gotProblems); diff != "" {
+				t.Errorf(diff)
 			}
 		})
 	}