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 trailers and wire validation #777

Closed
wants to merge 2 commits into from

Conversation

smaye81
Copy link
Member

@smaye81 smaye81 commented Feb 8, 2024

This PR seeks to add additional wire validation tests for gRPC, gRPC-web, and Connect. Essentially, it adds tests for all three protocols to verify how they handle trailers on the wire. Connect and gRPC-web should not send trailers and gRPC should.

To accomplish this, two new test yaml files were added - connect.yaml and grpc.yaml which house all tests specific to those protocols. The tests for verifying the wire details were added to this files.

In addition, any other files that were previously Connect-only have been deleted and merged into the connect.yaml file.

@smaye81 smaye81 requested a review from jhump February 8, 2024 02:06
Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

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

Sorry that I wasn't ver clear in the ticket for what to do with these. I had intended these checks to work like they did in the old cross tests (#538).

So I don't think we need new test cases for these.

I think instead we want the test runner to have new assertions applied to every test case. It should require that the actual HTTP trailers are always empty for gRPC-Web and Connect protocols. For gRPC, require that the grpc-status and grpc-message keys are present and that the actual trailers are a super-set of the normal response trailers reported.

@jhump
Copy link
Member

jhump commented Feb 8, 2024

For gRPC, require that ...

Actually, it is okay if gRPC sends back no trailers. So only require this when the trailers are present and non-empty. If empty, then require that the reported response trailers are also empty and that there were no response messages (to double-check that this was a "trailers-only" response, where all data is actually in the HTTP headers).

@jhump
Copy link
Member

jhump commented Feb 8, 2024

I just realized you can't easily only check those things based no protocol because you don't know the protocol at that point.

I guess I can cherry-pick this little change related to that from a work-in-progress branch: https://github.com/connectrpc/conformance/compare/jh/tighten-up#diff-37ae526ec60373f25e21af5d36aebcdbd965e22fbcd5293bcf5bc4e55f47b537R159

That way, you have the entire test case available when making assertions.

@jhump
Copy link
Member

jhump commented Feb 8, 2024

I guess I can cherry-pick this little change related to that from a work-in-progress branch

I've opened a PR for this: #778

@smaye81 smaye81 closed this Feb 8, 2024
@smaye81 smaye81 deleted the sayers/wire_validation branch February 16, 2024 20:57
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.

2 participants