Skip to content

Commit 6667276

Browse files
authored
Allow server to cancel when deadline expires (#999)
This makes some assertions for timeout cases a little more lenient. The tests previously asserted that a server would return a "deadline exceeded" error when the client's deadline expires. However, not all gRPC servers behave this way. The gRPC Go reference implementation was recently updated to sometimes just cancel the stream instead of sending back an error response. This doesn't seem unreasonable, so instead of updating the "known failing" list of cases for the grpc-go server, this PR makes this behavior allowed (which means it will be allowed in any other implementation-under-test, too). The test case structure lets us define other acceptable error codes for test cases, however we don't actually want to add "canceled" as generally acceptable for these tests. It's really only acceptable for the server (clients should always report "deadline exceeded" for these cases). So we need to update the test assertions logic to get that level of nuance.
1 parent 3ea288e commit 6667276

File tree

4 files changed

+33
-19
lines changed

4 files changed

+33
-19
lines changed

internal/app/connectconformance/connectconformance.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,7 @@ func run( //nolint:gocyclo
312312
}
313313
}
314314

315-
results := newResults(filteredTestCount, knownFailing, knownFlaky, trace)
315+
results := newResults(mode, filteredTestCount, knownFailing, knownFlaky, trace)
316316

317317
for _, clientInfo := range clients {
318318
clientProcess, err := runClient(ctx, clientInfo.start)

internal/app/connectconformance/results.go

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ const timeoutCheckGracePeriodMillis = 500
5353
// log writer. It can also incorporate data provided out-of-band by a reference
5454
// server, when testing a client implementation.
5555
type testResults struct {
56+
mode conformancev1.TestSuite_TestMode
5657
totalTestCount int
5758
knownFailing *testTrie
5859
knownFlaky *testTrie
@@ -66,8 +67,9 @@ type testResults struct {
6667
serverSideband map[string]string
6768
}
6869

69-
func newResults(totalTestCount int, knownFailing, knownFlaky *testTrie, tracer *tracer.Tracer) *testResults {
70+
func newResults(mode conformancev1.TestSuite_TestMode, totalTestCount int, knownFailing, knownFlaky *testTrie, tracer *tracer.Tracer) *testResults {
7071
return &testResults{
72+
mode: mode,
7173
totalTestCount: totalTestCount,
7274
knownFailing: knownFailing,
7375
knownFlaky: knownFlaky,
@@ -168,7 +170,19 @@ func (r *testResults) assert(
168170
expected := definition.ExpectedResponse
169171
var errs multiErrors
170172

171-
errs = append(errs, checkError(expected.Error, actual.Error, definition.OtherAllowedErrorCodes)...)
173+
otherErrors := definition.OtherAllowedErrorCodes
174+
if definition.Request.TimeoutMs != nil && r.mode == conformancev1.TestSuite_TEST_MODE_SERVER &&
175+
expected.Error != nil && expected.Error.Code == conformancev1.Code_CODE_DEADLINE_EXCEEDED &&
176+
definition.Request.HttpVersion > conformancev1.HTTPVersion_HTTP_VERSION_1 && len(otherErrors) == 0 {
177+
// When testing a server, the reference client does not set an actual timeout, in order to
178+
// see if the server enforces the timeout. Typically, a server will return a "deadline exceeded"
179+
// error when it notices that time is up. But it is also possible that the server chooses to
180+
// simply cancel the stream (only possible with HTTP/2 or HTTP/3) -- why send back an error
181+
// response if the client has already "hung up"? So we allow for the reference client to observe
182+
// a CANCELED error in these cases, as an alternative to DEADLINE_EXCEEDED.
183+
otherErrors = []conformancev1.Code{conformancev1.Code_CODE_CANCELED}
184+
}
185+
errs = append(errs, checkError(expected.Error, actual.Error, otherErrors)...)
172186
errs = append(errs, checkPayloads(expected.Payloads, actual.Payloads)...)
173187

174188
if len(expected.Payloads) == 0 &&

internal/app/connectconformance/results_test.go

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import (
3030

3131
func TestResults_SetOutcome(t *testing.T) {
3232
t.Parallel()
33-
results := newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
33+
results := newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
3434
results.setOutcome("foo/bar/1", false, nil)
3535
results.setOutcome("foo/bar/2", true, errors.New("fail"))
3636
results.setOutcome("foo/bar/3", false, errors.New("fail"))
@@ -60,7 +60,7 @@ func TestResults_SetOutcome(t *testing.T) {
6060

6161
func TestResults_FailedToStart(t *testing.T) {
6262
t.Parallel()
63-
results := newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
63+
results := newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
6464
results.failedToStart([]*conformancev1.TestCase{
6565
{Request: &conformancev1.ClientCompatRequest{TestName: "foo/bar/1"}},
6666
{Request: &conformancev1.ClientCompatRequest{TestName: "known-to-fail/1"}},
@@ -80,7 +80,7 @@ func TestResults_FailedToStart(t *testing.T) {
8080

8181
func TestResults_FailRemaining(t *testing.T) {
8282
t.Parallel()
83-
results := newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
83+
results := newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
8484
results.setOutcome("foo/bar/1", false, nil)
8585
results.setOutcome("known-to-fail/1", false, errors.New("fail"))
8686
results.failRemaining([]*conformancev1.TestCase{
@@ -107,7 +107,7 @@ func TestResults_FailRemaining(t *testing.T) {
107107

108108
func TestResults_Failed(t *testing.T) {
109109
t.Parallel()
110-
results := newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
110+
results := newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
111111
results.failed("foo/bar/1", &conformancev1.ClientErrorResult{Message: "fail"})
112112
results.failed("known-to-fail/1", &conformancev1.ClientErrorResult{Message: "fail"})
113113

@@ -124,7 +124,7 @@ func TestResults_Failed(t *testing.T) {
124124

125125
func TestResults_Assert(t *testing.T) {
126126
t.Parallel()
127-
results := newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
127+
results := newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
128128
payload1 := &conformancev1.ClientResponseResult{
129129
Payloads: []*conformancev1.ConformancePayload{
130130
{Data: []byte{0, 1, 2, 3, 4}},
@@ -685,7 +685,7 @@ func TestResults_Assert_ReportsAllErrors(t *testing.T) {
685685
for _, testCase := range testCases {
686686
t.Run(testCase.name, func(t *testing.T) {
687687
t.Parallel()
688-
results := newResults(0, &testTrie{}, &testTrie{}, nil)
688+
results := newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, &testTrie{}, &testTrie{}, nil)
689689

690690
expected := &conformancev1.TestCase{
691691
Request: &conformancev1.ClientCompatRequest{StreamType: conformancev1.StreamType_STREAM_TYPE_UNARY},
@@ -722,7 +722,7 @@ func TestResults_Assert_ReportsAllErrors(t *testing.T) {
722722

723723
func TestResults_ServerSideband(t *testing.T) {
724724
t.Parallel()
725-
results := newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
725+
results := newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
726726
results.setOutcome("foo/bar/1", false, nil)
727727
results.setOutcome("foo/bar/2", false, errors.New("fail"))
728728
results.setOutcome("foo/bar/3", false, nil)
@@ -747,50 +747,50 @@ func TestResults_ServerSideband(t *testing.T) {
747747

748748
func TestResults_Report(t *testing.T) {
749749
t.Parallel()
750-
results := newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
750+
results := newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
751751
logger := &internal.SimplePrinter{}
752752

753753
// No test cases? Report success.
754754
success := results.report(logger)
755755
require.True(t, success)
756756

757757
// Only successful outcomes? Report success.
758-
results = newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
758+
results = newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
759759
results.setOutcome("foo/bar/1", false, nil)
760760
success = results.report(logger)
761761
require.True(t, success)
762762

763763
// Unexpected failure? Report failure.
764-
results = newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
764+
results = newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
765765
results.setOutcome("foo/bar/1", false, errors.New("ruh roh"))
766766
success = results.report(logger)
767767
require.False(t, success)
768768

769769
// Unexpected failure during setup? Report failure.
770-
results = newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
770+
results = newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
771771
results.setOutcome("foo/bar/1", true, errors.New("ruh roh"))
772772
success = results.report(logger)
773773
require.False(t, success)
774774

775775
// Expected failure? Report success.
776-
results = newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
776+
results = newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
777777
results.setOutcome("known-to-fail/1", false, errors.New("ruh roh"))
778778
success = results.report(logger)
779779
require.True(t, success)
780780

781781
// Setup error from expected failure? Report failure (setup errors never acceptable).
782-
results = newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
782+
results = newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
783783
results.setOutcome("known-to-fail/1", true, errors.New("ruh roh"))
784784
success = results.report(logger)
785785
require.False(t, success)
786786

787787
// Flaky? Report success whether it passes or fails
788-
results = newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
788+
results = newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
789789
results.setOutcome("known-to-flake/1", false, nil) // succeeds
790790
success = results.report(logger)
791791
require.True(t, success)
792792

793-
results = newResults(0, makeKnownFailing(), makeKnownFlaky(), nil)
793+
results = newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, 0, makeKnownFailing(), makeKnownFlaky(), nil)
794794
results.setOutcome("known-to-flake/1", false, errors.New("ruh roh"))
795795
success = results.report(logger)
796796
require.True(t, success)

internal/app/connectconformance/server_runner_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ func TestRunTestCasesForServer(t *testing.T) {
195195
for _, testCase := range testCases {
196196
t.Run(testCase.name, func(t *testing.T) {
197197
t.Parallel()
198-
results := newResults(len(requests), &testTrie{}, &testTrie{}, nil)
198+
results := newResults(conformancev1.TestSuite_TEST_MODE_UNSPECIFIED, len(requests), &testTrie{}, &testTrie{}, nil)
199199

200200
var procAddr atomic.Pointer[process] // populated when server process created
201201
var actualSvrRequest bytes.Buffer

0 commit comments

Comments
 (0)