Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for cancellation #726

Merged
merged 13 commits into from Dec 11, 2023
Merged

Add tests for cancellation #726

merged 13 commits into from Dec 11, 2023

Conversation

smaye81
Copy link
Member

@smaye81 smaye81 commented Dec 7, 2023

This adds tests for cancellation logic based on the cancel_timing oneof in the test request.

@smaye81 smaye81 requested a review from jhump December 7, 2023 16:22
# - "@type": type.googleapis.com/connectrpc.conformance.v1.BidiStreamRequest
# requestData: "dGVzdCByZXNwb25zZQ=="
# # Override
# expectedResponse:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to put these into the populateExpectedResponse methods, but want to refactor that to use JSON first so it doesn't cause the test to balloon even further and make the refactor even more difficult.

@@ -0,0 +1,100 @@
name: Cancellation
testCases:
# TODO - The server stream tests are failing with grpc due to issues with
Copy link
Member Author

@smaye81 smaye81 Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed offline, this is because of the stream.Header call in the gRPC client implementation. That call blocks until header metadata is sent, but the server stream endpoint sends the header information with the first response. So, the stream.Header call blocks until a response is sent and then the context is cancelled. So right now, the expectations for responses on server streams won't work.

This can be fixed with a change on the reference server and the gRPC server:

Reference Server

  • should call stream.Send(nil) after setting the headers.

gRPC Server

  • should call stream.SendHeader(headerMD) (instead of SetHeader)

However, doing the above causes a "protocol error: incomplete envelope: context deadline exceeded" error to be thrown by Connect-Go on the server stream timeout tests (presumably from here).

@smaye81 smaye81 marked this pull request as ready for review December 7, 2023 20:18
Comment on lines 2 to 4
relevantHttpVersions:
- HTTP_VERSION_2
- HTTP_VERSION_3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: FWIW, this shouldn't be necessary. The runner knows that full-duplex isn't allowed with HTTP 1.1 and won't bother applying test cases with full-duplex bidi stream type to that HTTP version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I combined bidi.yaml into basic.yaml then since we didn't need to specify HTTP versions.

internal/cancellation.go Outdated Show resolved Hide resolved
internal/app/grpcclient/impl.go Outdated Show resolved Hide resolved
internal/app/grpcclient/impl.go Outdated Show resolved Hide resolved
@smaye81 smaye81 requested a review from jhump December 8, 2023 02:58
@smaye81 smaye81 merged commit d3a355e into main Dec 11, 2023
4 checks passed
@smaye81 smaye81 deleted the sayers/cancellation branch December 11, 2023 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants