chore: refactor rest transport bad request test macro #2141
Conversation
ohmayr
force-pushed
the
refactor-rest-bad-request-test
branch
from
4c2462f to
3eebe32
Compare
parthea
left a comment
parthea
left a comment
There was a problem hiding this comment.
Added non-blocking comment. Feel free to follow up in a later PR
| {# rest_method_bad_request_test generates tests for rest methods | ||
| # which raise a google.api.core.exceptions.BadRequest error. | ||
| #} | ||
| {% macro rest_method_bad_request_test(service, method, method_name, transport_name, async_prefix, await_prefix, async_decorator, is_async) %} |
There was a problem hiding this comment.
Should we add a TODO bug to follow up on making this test more generic to cover gRPC as well? For example, we could simulate a 400 response using either gRPC or REST
ohmayr
force-pushed
the
implement-async-rest-call-method-class
branch
2 times, most recently
from
c562932 to
8fd829c
Compare
ohmayr
force-pushed
the
refactor-rest-bad-request-test
branch
from
3eebe32 to
b466c69
Compare
| # which raise a google.api.core.exceptions.BadRequest error. | ||
| # TODO(https://github.com/googleapis/gapic-generator-python/issues/2157): Refactor this macro to include `gRPC` coverage. | ||
| #} | ||
| {% macro rest_method_bad_request_test(service, method, transport, is_async) %} |
There was a problem hiding this comment.
Since you're passing the transport in, and since you'll want to have gRPC in here:
| {% macro rest_method_bad_request_test(service, method, transport, is_async) %} | |
| {% macro bad_request_test(service, method, transport, is_async) %} |
| # which raise a google.api.core.exceptions.BadRequest error. | ||
| # TODO(https://github.com/googleapis/gapic-generator-python/issues/2157): Refactor this macro to include `gRPC` coverage. | ||
| #} | ||
| {% macro rest_method_bad_request_test(service, method, transport, is_async) %} |
There was a problem hiding this comment.
non-blocking, but desirable: Same comment as in the other PR: I'd have a macro-level fail-safe check that "rest" in transport that will cause a generation-time exception if we call the macro for non-REST. We can adapt/remove that as we factor gRPC functionality in here.
| assert transport.kind == "rest" | ||
|
|
||
|
|
||
| def test_list_instances_rest_bad_request(request_type=cloud_redis.ListInstancesRequest): |
There was a problem hiding this comment.
Do any of the goldens test the await_prefix, async_prefix, etc?
There was a problem hiding this comment.
Not here but in one of the follow up PRs where we turn on this test for async.
| in str(not_implemented_error.value) | ||
| ) | ||
|
|
||
| {% endif %}{# if 'rest' in transport #} |
There was a problem hiding this comment.
Have an else to raise a generation time error.
| req.return_value = response_value | ||
| {{ await_prefix }}client.{{ method_name }}(request) | ||
|
|
||
| {% endif %}{# if 'rest' in transport #} |
There was a problem hiding this comment.
Have an else to raise a generation time error. It's even more important here because we don't have rest in the name (as we shouldn't, since we pass transport).
Otherwise, if somebody else comes along and see the call to this macro, unless they look inside they could assume it will work for gRPC, and delete the existing gRPC-testing functionality where it is now in favor of calling this macro....and things will "work" because all that would do is remove the test code, not move it around.
Having the failsafe is a second line of defense. I strongly encourage it. I know I harp on this a lot, but I've found it useful to assume that future developers, including myself, will overlook something that is obvious now.
There was a problem hiding this comment.
As discussed offline, we'll be raising a runtime error within the macro just to safeguard if it's called incorrectly without updating it.
This will be addressed in a separate PR.
3 participants
This PR refactors the macro for testing a rest method call which raises a
google.api_core.exceptions.BadRequesterror.For now, the macro is guarded and only generates tests for sync REST methods. Once we implement the relevant logic in async rest, the guard can be removed. (This will be addressed in a follow up PR.)
This PR should be reviewed and merged after: #2140.