Relax expectation for unspecified errors, fix expectation for cardina…

…lity violations (#833)

There are numerous error conditions in gRPC where the actual behavior of
a client or server in the face of such an error is unspecified. In
particular, there is no specification for how a client or server should
react to an invalid/unexpected HTTP method or content-type. So this adds
a mechanism to supply additional acceptable error codes.

One that thing that **is** specified, but was codified with the wrong
expectation in these tests, is related to cardinality violations. A
cardinality violation is when exactly one request or response is
expected (for a unary RPC or the non-streaming half of a non-bidi
streaming RPC), but there is actually zero or more than one. This is
specified in a table that accompanies the docs for gRPC
codes. Search for "cardinality violation" in this page:
   https://grpc.github.io/grpc/core/md_doc_statuscodes.html

Makefile

@@ -96,8 +96,10 @@ runconformance: runservertests runclienttests
 .PHONY: runservertests
 runservertests: $(BIN)/connectconformance $(BIN)/referenceserver $(BIN)/grpcserver
 	$(BIN)/connectconformance -v --conf ./testing/reference-impls-config.yaml --mode server --trace \
+		--known-failing @./testing/referenceserver-known-failing.txt \
 		-- $(BIN)/referenceserver
 	$(BIN)/connectconformance -v --conf ./testing/grpc-impls-config.yaml --mode server --trace \
+		--known-failing @./testing/grpcserver-known-failing.txt \
 		-- $(BIN)/grpcserver
 	$(BIN)/connectconformance -v --conf ./testing/grpc-web-server-impl-config.yaml --mode server --trace \
 		--known-failing @./testing/grpcserver-web-known-failing.txt \
@@ -106,6 +108,7 @@ runservertests: $(BIN)/connectconformance $(BIN)/referenceserver $(BIN)/grpcserv
 .PHONY: runclienttests
 runclienttests: $(BIN)/connectconformance $(BIN)/referenceclient $(BIN)/grpcclient buildgrpcweb
 	$(BIN)/connectconformance -v --conf ./testing/reference-impls-config.yaml --mode client --trace \
+		--known-failing @./testing/referenceclient-known-failing.txt \
 		-- $(BIN)/referenceclient
 	$(BIN)/connectconformance -v --conf ./testing/grpc-impls-config.yaml --mode client --trace \
 		--known-failing @./testing/grpcclient-known-failing.txt \

docs/authoring_test_cases.md

@@ -101,6 +101,40 @@ or must be specified together. If they are omitted, the runner will auto-populat
  >
  > If a test is specific to one of the first four fields, it should instead be indicated in the directives for the test suite itself.
 
+### Expected responses
+
+The expected response for a test, in the `expectedResponse` field, can be auto-generated based on the request details.
+The conformance runner will determine what the response should be according to the values specified in the individual
+test case requests.
+
+You also have the ability to explicitly specify your own expected response directly in the test definition. However,
+this is typically only needed for exception test cases. If the expected response is mostly re-stating the response
+definition that appears in the requests, you should rely on the auto-generation if possible. Otherwise, specifying
+an expected response can make the test YAML overly verbose and harder to read, write, and maintain.
+
+If the test induces behavior that prevents the server from sending or client from receiving the full response
+definition, it will be necessary to define the expected response explicitly. Timeouts, cancellations, and exceeding
+message size limits are good examples of this.
+
+If you do need to specify an explicit response, simply define an `expectedResponse` block for your test case and
+this will override the auto-generated expected response in the test runner.
+
+To see tests denoting an explicit response, search the [test suites][test-suite-dir] directory for the word `expectedResponse`.
+
+#### Lenience in Expected Error Codes
+
+There are some cases where a condition is obviously an error, based on the protocol specification, but that
+specification does not actually indicate how a client or server should behave in the face of that error. One
+example is if a gRPC server receives an HTTP/2 request that has an unexpected content-type or an HTTP method
+other than POST.
+
+For these cases, the test author should put the most sensible error code as the expected error code in the
+`expectedResponse` field of the test case, but then can also add other acceptable error codes in the
+`otherAllowedErrorCodes` field. Even when the error code is not specified, it is not good practice to allow
+_any_ error code since many error codes will obviously not apply, and some error codes should never be used
+by a client or server implementation library but are reserved for use by actual application logic (see the
+table at the bottom of the [gRPC status codes documentation](https://grpc.github.io/grpc/core/md_doc_statuscodes.html)).
+
 ### Raw Payloads
 
 There are two message types in the test case schema worth noting here - [`RawHTTPRequest`][raw-http-request] and 
@@ -169,25 +203,6 @@ for a specific protocol or only running the unary tests within a suite.
 
 Aside from the `/` for separating elements on the name, test case names should use `kebab-case` convention.
 
-## Expected responses
-
-The expected response for a test is auto-generated based on the request details. The conformance runner will determine 
-what the response should be according to the values specified in the test suite and individual test cases. 
-
-You also have the ability to explicitly specify your own expected response directly in the test definition itself. However, 
-this is typically only needed for exception test cases. If the expected response is mostly re-stating the response definition
-that appears in the requests, you should rely on the auto-generation if possible. Otherwise, specifying an expected response 
-can make the test YAML overly verbose and harder to read, write, and maintain. 
-
-If the test induces behavior that prevents the server from sending or client from receiving the full response definition, it 
-will be necessary to define the expected response explicitly. Timeouts, cancellations, and exceeding message size limits are 
-good examples of this.
-
-If you do need to specify an explicit response, simply define an `expectedResponse` block for your test case and this will
-override the auto-generated expected response in the test runner. 
-
-To see tests denoting an explicit response, search the [test suites][test-suite-dir] directory for the word `expectedResponse`.
-
 ## Running and Debugging New Tests
 
 To test new test cases, you can use `make runconformance`, to run the reference implementations against the new test

internal/app/connectconformance/results.go

@@ -158,7 +158,7 @@ func (r *testResults) assert(
 	expected := definition.ExpectedResponse
 	var errs multiErrors
 
-	errs = append(errs, checkError(expected.Error, actual.Error)...)
+	errs = append(errs, checkError(expected.Error, actual.Error, definition.OtherAllowedErrorCodes)...)
 	errs = append(errs, checkPayloads(expected.Payloads, actual.Payloads)...)
 
 	if len(expected.Payloads) == 0 &&
@@ -459,7 +459,7 @@ func checkPayloads(expected, actual []*conformancev1.ConformancePayload) multiEr
 	return errs
 }
 
-func checkError(expected, actual *conformancev1.Error) multiErrors {
+func checkError(expected, actual *conformancev1.Error, otherCodes []conformancev1.Code) multiErrors {
 	switch {
 	case expected == nil && actual == nil:
 		// nothing to do
@@ -471,13 +471,14 @@ func checkError(expected, actual *conformancev1.Error) multiErrors {
 	}
 
 	var errs multiErrors
-	if expected.Code != actual.Code {
-		errs = append(errs, fmt.Errorf("actual error code %d (%s) does not match expected code %d (%s)",
-			actual.Code, connect.Code(actual.Code).String(), expected.Code, connect.Code(expected.Code).String()))
+	if expected.Code != actual.Code && !inSlice(actual.Code, otherCodes) {
+		expectedCodes := expectedCodeString(expected.Code, otherCodes)
+		errs = append(errs, fmt.Errorf("actual error {code: %d (%s), message: %q} does not match expected code %s",
+			actual.Code, connect.Code(actual.Code).String(), actual.GetMessage(), expectedCodes))
 	}
 	if expected.Message != nil && expected.GetMessage() != actual.GetMessage() {
-		errs = append(errs, fmt.Errorf("actual error message %q does not match expected message %q",
-			actual.GetMessage(), expected.GetMessage()))
+		errs = append(errs, fmt.Errorf("actual error {code: %d (%s), message: %q} does not match expected message %q",
+			actual.Code, connect.Code(actual.Code).String(), actual.GetMessage(), expected.GetMessage()))
 	}
 	if len(expected.Details) != len(actual.Details) {
 		// TODO: Should this be more lenient? Are we okay with a Connect implementation adding extra
@@ -530,3 +531,28 @@ func indent(s string) string {
 	}
 	return strings.Join(lines, "\n")
 }
+
+func inSlice[T comparable](elem T, slice []T) bool {
+	// TODO: delete this function when this repo is using Go 1.21
+	//	     and update call sites to instead use slices.Contains
+	for _, item := range slice {
+		if item == elem {
+			return true
+		}
+	}
+	return false
+}
+
+func expectedCodeString(expectedCode conformancev1.Code, otherAllowedCodes []conformancev1.Code) string {
+	allowedCodes := make([]string, len(otherAllowedCodes)+1)
+	for i, code := range append([]conformancev1.Code{expectedCode}, otherAllowedCodes...) {
+		allowedCodes[i] = fmt.Sprintf("%d (%s)", code, connect.Code(code).String())
+		if i == len(allowedCodes)-1 && i != 0 {
+			allowedCodes[i] = "or " + allowedCodes[i]
+		}
+	}
+	if len(allowedCodes) < 3 {
+		return strings.Join(allowedCodes, " ")
+	}
+	return strings.Join(allowedCodes, ", ")
+}

internal/app/connectconformance/results_test.go

@@ -16,6 +16,7 @@ package connectconformance
 
 import (
 	"errors"
+	"fmt"
 	"strings"
 	"testing"
 
@@ -329,7 +330,7 @@ func TestResults_Assert_ReportsAllErrors(t *testing.T) {
 				}
 			}`,
 			expectedErrors: []string{
-				"actual error code 11 (out_of_range) does not match expected code 5 (not_found)",
+				`actual error {code: 11 (out_of_range), message: "foobar"} does not match expected code 5 (not_found)`,
 			},
 		},
 		{
@@ -347,7 +348,7 @@ func TestResults_Assert_ReportsAllErrors(t *testing.T) {
 				}
 			}`,
 			expectedErrors: []string{
-				`actual error message "oof!" does not match expected message "foobar"`,
+				`actual error {code: 5 (not_found), message: "oof!"} does not match expected message "foobar"`,
 			},
 		},
 		{
@@ -842,6 +843,44 @@ func TestCanonicalizeHeaderVals(t *testing.T) {
 	}
 }
 
+func TestExpectedCodeString(t *testing.T) {
+	t.Parallel()
+	testCases := []struct {
+		expectedCode   conformancev1.Code
+		otherCodes     []conformancev1.Code
+		expectedString string
+	}{
+		{
+			expectedCode:   conformancev1.Code_CODE_ABORTED,
+			expectedString: "10 (aborted)",
+		},
+		{
+			expectedCode:   conformancev1.Code_CODE_ABORTED,
+			otherCodes:     []conformancev1.Code{conformancev1.Code_CODE_INTERNAL},
+			expectedString: "10 (aborted) or 13 (internal)",
+		},
+		{
+			expectedCode:   conformancev1.Code_CODE_ABORTED,
+			otherCodes:     []conformancev1.Code{conformancev1.Code_CODE_INTERNAL, conformancev1.Code_CODE_CANCELED},
+			expectedString: "10 (aborted), 13 (internal), or 1 (canceled)",
+		},
+		{
+			expectedCode: conformancev1.Code_CODE_ABORTED,
+			otherCodes: []conformancev1.Code{
+				conformancev1.Code_CODE_INTERNAL, conformancev1.Code_CODE_CANCELED, conformancev1.Code_CODE_ALREADY_EXISTS,
+			},
+			expectedString: "10 (aborted), 13 (internal), 1 (canceled), or 6 (already_exists)",
+		},
+	}
+	for _, testCase := range testCases {
+		testCase := testCase
+		t.Run(fmt.Sprintf("%d_other_codes", len(testCase.otherCodes)), func(t *testing.T) {
+			t.Parallel()
+			require.Equal(t, testCase.expectedString, expectedCodeString(testCase.expectedCode, testCase.otherCodes))
+		})
+	}
+}
+
 func makeKnownFailing() *testTrie {
 	return parsePatterns([]string{"known-to-fail/**"})
 }

internal/app/connectconformance/testsuites/data/client_unexpected.yaml

@@ -27,18 +27,3 @@ testCases:
   expectedResponse:
     error:
       code: CODE_UNKNOWN
-- request:
-     testName: unexpected-content-type
-     service: connectrpc.conformance.v1.ConformanceService
-     method: Unary
-     streamType: STREAM_TYPE_UNARY
-     requestMessages:
-     - "@type": type.googleapis.com/connectrpc.conformance.v1.UnaryRequest
-       responseDefinition:
-         rawResponse:
-           headers:
-             - name: Content-Type
-               value: ["image/jpeg"]
-  expectedResponse:
-     error:
-       code: CODE_UNKNOWN

internal/app/connectconformance/testsuites/data/connect_client_unexpected.yaml

@@ -24,6 +24,77 @@ testCases:
       error:
         # mapped from 412 status code (invalid body ignored)
         code: CODE_FAILED_PRECONDITION
+
+  - request:
+      testName: client-stream/ok-but-no-response
+      streamType: STREAM_TYPE_CLIENT_STREAM
+      requestMessages:
+        - "@type": type.googleapis.com/connectrpc.conformance.v1.ClientStreamRequest
+          responseDefinition:
+            rawResponse:
+              status_code: 200
+              headers:
+                - name: content-type
+                  value: [ "application/connect+proto" ]
+              stream:
+                items:
+                  - flags: 2
+                    payload:
+                      text: |
+                        {
+                          "code": "out_of_range",
+                          "message": "oops",
+                        }
+    expectedResponse:
+      error:
+        code: CODE_UNIMPLEMENTED
+  - request:
+      testName: client-stream/multiple-responses
+      streamType: STREAM_TYPE_CLIENT_STREAM
+      requestMessages:
+        - "@type": type.googleapis.com/connectrpc.conformance.v1.ClientStreamRequest
+          responseDefinition:
+            rawResponse:
+              status_code: 200
+              headers:
+                - name: content-type
+                  value: [ "application/connect+proto" ]
+              stream:
+                items:
+                  - flags: 0
+                    payload:
+                      binary_message:
+                        "@type": "type.googleapis.com/connectrpc.conformance.v1.ClientStreamResponse"
+                  - flags: 0
+                    payload:
+                      binary_message:
+                        "@type": "type.googleapis.com/connectrpc.conformance.v1.ClientStreamResponse"
+                  - flags: 2
+                    payload:
+                      text: |
+                        {
+                          "code": "out_of_range",
+                          "message": "oops",
+                        }
+    expectedResponse:
+      error:
+        code: CODE_UNIMPLEMENTED
+
+  - request:
+      testName: unexpected-content-type
+      service: connectrpc.conformance.v1.ConformanceService
+      method: Unary
+      streamType: STREAM_TYPE_UNARY
+      requestMessages:
+        - "@type": type.googleapis.com/connectrpc.conformance.v1.UnaryRequest
+          responseDefinition:
+            rawResponse:
+              headers:
+                - name: Content-Type
+                  value: ["image/jpeg"]
+    expectedResponse:
+      error:
+        code: CODE_UNKNOWN
   - request:
       testName: unexpected-codec
       streamType: STREAM_TYPE_UNARY

internal/app/connectconformance/testsuites/data/connect_server_unexpected.yaml

@@ -7,6 +7,45 @@ relevantCompressions:
 relevantCodecs:
   - CODEC_PROTO
 testCases:
+  - request:
+      testName: server-stream/no-request
+      streamType: STREAM_TYPE_SERVER_STREAM
+      requestMessages:
+        - "@type": type.googleapis.com/connectrpc.conformance.v1.ServerStreamRequest
+      rawRequest:
+        verb: POST
+        uri: /connectrpc.conformance.v1.ConformanceService/ServerStream
+        headers:
+          - name: content-type
+            value: [ "application/connect+proto" ]
+    expectedResponse:
+      error:
+        code: CODE_UNIMPLEMENTED
+  - request:
+      testName: server-stream/multiple-requests
+      streamType: STREAM_TYPE_SERVER_STREAM
+      requestMessages:
+        - "@type": type.googleapis.com/connectrpc.conformance.v1.ServerStreamRequest
+      rawRequest:
+        verb: POST
+        uri: /connectrpc.conformance.v1.ConformanceService/ServerStream
+        headers:
+          - name: content-type
+            value: [ "application/connect+proto" ]
+        stream:
+          items:
+            - flags: 0
+              payload:
+                binary_message:
+                  "@type": "type.googleapis.com/connectrpc.conformance.v1.ServerStreamRequest"
+            - flags: 0
+              payload:
+                binary_message:
+                  "@type": "type.googleapis.com/connectrpc.conformance.v1.ServerStreamRequest"
+    expectedResponse:
+      error:
+        code: CODE_UNIMPLEMENTED
+
   - request:
       testName: unexpected-verb
       streamType: STREAM_TYPE_UNARY