chore: refactor empty_call_test #2126
Conversation
gcf-owl-bot
bot
removed
the
owlbot:run
|
|
||
| {% macro empty_call_test(method, method_name, service, api, uuid4_re, is_async=False) %} | ||
| {% macro method_call_test(test_name, method, service, api, request, is_async=False) %} | ||
| {% with method_name = method.name|snake_case + "_unary" if method.operation_service else method.name|snake_case %} |
There was a problem hiding this comment.
Consider this for clarity:
| {% with method_name = method.name|snake_case + "_unary" if method.operation_service else method.name|snake_case %} | |
| {% with method_name = (method.name + ("_unary" if method.operation_service else "")) | snake_case %} |
| assert transport.kind == "{{ transport_name }}" | ||
|
|
||
| {% endmacro %} | ||
| {% endmacro %}{# transport_kind_test #} |
There was a problem hiding this comment.
nit: for consistency, consider removing the empty line just above this one, though that may involve tweaking the spacing elsewhere
| {% for method in service.methods.values() %}{# method #} | ||
| {% if not method.client_streaming %} | ||
| # This test is a coverage failsafe to make sure that totally empty calls, | ||
| # i.e. request == None and no flattened fields passed, work. |
There was a problem hiding this comment.
But we're applying this test even to methods where the API requires a non-empty request, right? It would be great if we type-hinted methods sufficiently so that for methods requiring a request, the type is specified as the proper request object, and for methods with optional requests, the type is specified as a nullable request of the appropriate type. Then this macro would be applied to just the latter set of methods (and we could also test that sending request=None to the former set would raise warnings or errors).
I realize you are just refactoring what was here before, so this is no worse, but it might be worth tracking this is an improvement for the future. WDYT? (Feel free to clarify anything I am misunderstanding.)
There was a problem hiding this comment.
|
|
||
|
|
||
| {% macro empty_call_test(method, method_name, service, api, uuid4_re, is_async=False) %} | ||
| {% macro method_call_test(test_name, method, service, api, request, is_async=False) %} |
There was a problem hiding this comment.
The comments within this macro suggest it's gRPC-only, so we should either clarify that in the name or add a TODO to say our intent is to expand this to REST as well.
|
|
||
|
|
||
| {% macro empty_call_test(method, method_name, service, api, uuid4_re, is_async=False) %} | ||
| {% macro method_call_test(test_name, method, service, api, request, is_async=False) %} |
There was a problem hiding this comment.
We should also make the name more specific, because most of our tests involve calling a method, but I take it this method will not be used in all our test cases?
There was a problem hiding this comment.
Tests in this code base have a lot of duplication. Ideally method_call_test will be used in the majority test cases, and where it can't be used, the name of other macros could be more descriptive. method_call_test_<with_customization>.
I've updated method_call_test to method_call_test_generic to reflect that tests that can't use this could be named method_call_test_<with_customization>.
2 participants
Towards #2091
empty_call_testwas refactored in #2110. This PR refactors the test further to use a shared marcomethod_call_testto reduce code duplication in similar tests.The following test functions have similar code:
gapic-generator-python/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2
Line 1811 in 2809753
gapic-generator-python/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2
Line 398 in 2809753
In PR #2133, I've updated the function
def test_{{ method.name|snake_case }}_routing_parameters():to use the shared marcomethod_call_testadded in this PR