Relax expectation for unspecified errors, fix expectation for cardinality violations #833
Conversation
…ified; fix expectation for cardinality violations to match spec; add cardinality violation tests for client-stream
| 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) { | ||
| expectedCode := fmt.Sprintf("%d (%s)", expected.Code, connect.Code(expected.Code).String()) |
There was a problem hiding this comment.
Could avoid initializing expectedCode for the case where len(otherCodes) > 0
There was a problem hiding this comment.
But we actually use this initialized value in that case, at line 478 below. So it seemed simplest to me to construct it once instead of having to do it 2x from both the if and else cases.
There was a problem hiding this comment.
Thanks, didn't catch that usage. I have a feeling this could be done a bit more elegantly (maybe looping over append(expected, otherCodes...) and avoiding the need to use or in the phrasing. But it's fine as-is.
There was a problem hiding this comment.
I reformulated, and also added a test to make sure it comes up with the expected string to present to users. Better?
| @@ -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 { | |||
There was a problem hiding this comment.
Would it make sense to use a map[string]struct{} or other Set type here to make searching easier?
There was a problem hiding this comment.
The list of codes is always small, so linear scan seemed fine (likely faster than having to create a map). As far as just simplifying the code (since perf is not a big concern here), I think constructing the map from the repeated proto field would be the same amount of code as the linear search through the slice, so didn't seem like a clear win. Also, the linear search is part of the standard library in Go 1.21, which introduces a new slices package, so we could upgrade in the near future and then remove some of this code.
I can add a comment to the inSlice function about removing it once we update to Go 1.21.
| @@ -141,4 +141,6 @@ message TestCase { | |||
| // Specifying an expected response explicitly in test definitions will override | |||
| // the auto-generation of the test runner. | |||
| ClientResponseResult expected_response = 3; | |||
|
|
|||
| repeated Code other_allowed_error_codes = 4; | |||
There was a problem hiding this comment.
Yeah, very good point. And I also need to update the docs on authoring test cases to refer to this value and provide context on when to use it.
| return false | ||
| } | ||
|
|
||
| func expectedCodeString(expectedCode conformancev1.Code, otherAllowedCodes []conformancev1.Code) string { |
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.
Instead of just allowing any error for these unspecified cases, I made a judgement call as to what the other acceptable codes are. While the exact error is not specified, it is pretty clear (at least IMO) that most of the codes do not apply at all and should not be used. We can always relax this further by adding more allowed codes. Though we don't want to accept any code because the docs on gRPC codes does explicitly prohibit a framework from using certain codes -- codes that are reserved for use by actual application logic (see the table at the very bottom of the page linked above).
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 (follow that link and search for "cardinality violation" in that page).
Sadly, neither connect-go nor grpc-go correctly handle the cardinality violations per the table in the above doc. So correcting these cases (and adding more) means we now have numerous more test cases configured as "known failing" for all implementations. We will address all of these issues in connect-go in the next release, so when we get that done and pulled into this repo, we can remove the known failing cases for the reference implementations.