feat: better grpc handling - #1104
Draft
michaelboulton wants to merge 5 commits into
Draft
michaelboulton wants to merge 5 commits into
michaelboulton wants to merge 5 commits into
Conversation
GRPCResponse._handle_grpc_response returned early for any non-OK response, which made both of the checks below it unreachable: expecting a 'body' alongside a non-OK status silently passed. Move the 'body specified but expected status is not OK' check above the early return, and fetch the response message inside a try/except so a failed call reports that the expected body could not be checked instead of being skipped. The grpc unit tests now assert the exception each spec is expected to raise and the status the server actually returned, rather than calling pytest.xfail() before doing any work.
The 'details' key of a grpc response was unusable: the code compared it against the details string of the response, but the schema said it was an object, so the only value it could ever have matched was rejected before the test ran. Split the two things which can be called the 'details' of a response: - 'error_message' checks the string the server passed to set_details or abort. It goes through the normal matching, so type sentinels like !re_search can be used on it. - 'details' checks the error details attached to the response - the messages packed into the google.rpc.Status in the trailing metadata, each converted to a mapping including its '@type'. As documented in https://grpc.io/docs/guides/error/ this is a Google convention for using grpc with protobuf rather than part of grpc itself, but as protobuf is what Tavern officially supports that is how it is checked. Fields which the server did not set are left out of the details, so only the fields actually returned need to be specified even with strict key checking on. The grpc example server now aborts with a google.rpc.Status when given an empty name so there is something real to check against, and the grpc plugin is registered in the unit test conftest so grpc test files can be schema checked there.
The single parametrized test covered three different things at once, with the spec carrying fields which only applied to some of them. Split it into one test per kind of spec: - test_grpc_bad_request: requests which don't match the service definition and never reach the server - test_grpc: requests which run and return the status, error message and details the test expects - test_grpc_bad_response: requests which run, but where the expected response doesn't match what the server returned The failure specs all assert on a substring of the error as well as its type, so they have to fail for the reason they were supposed to. Also renamed that field from 'expected_error' so it isn't confused with the 'error_message' of the response block, and removed 'actual_code' - the status is checked against 'code' in the tests which are expected to pass, and by the response verification itself in the ones which aren't.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
change grpc to use 'error_message' for the actual server error message, and 'details' for the rpc details returned in the trailer.
Technically a breaking change(?)