Skip to content

chore: refactor unimplemented method test macro #2138

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.

Sign up for GitHub

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

Merged
ohmayr merged 3 commits into from
Sep 12, 2024
Merged

chore: refactor unimplemented method test macro #2138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d70ee30
chore: refactor not implemented method macro
ohmayr Sep 10, 2024
d57a630
address PR comments
ohmayr Sep 12, 2024
72f84ab
address more PR comments
ohmayr Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1057,6 +1057,7 @@ def test_transport_adc(transport_class):
{% endfor %}
{% for conf in configs %}
{{ test_macros.transport_kind_test(**conf) }}
{{ test_macros.run_transport_tests_for_config(**conf) }}
{% endfor %}
{# TODO(https://github.com/googleapis/gapic-generator-python/issues/2121): Remove the below macro when async rest is GA. #}
{% if rest_async_io_enabled %}
Expand Down
132 changes: 111 additions & 21 deletions gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1756,25 +1756,7 @@ def test_{{ method_name }}_rest_pager(transport: str = 'rest'):
assert page_.raw_page.next_page_token == token


{%- else %}{# paged_result_field #}

def test_{{ method_name }}_rest_error():
client = {{ service.client_name }}(
credentials=ga_credentials.AnonymousCredentials(),
transport='rest'
)
{%- if not method.http_options %}
# Since a `google.api.http` annotation is required for using a rest transport
# method, this should error.
with pytest.raises(NotImplementedError) as not_implemented_error:
client.{{ method_name }}({})
assert (
"Method {{ method.name }} is not available over REST transport"
in str(not_implemented_error.value)
)

{%- endif %}{# not method.http_options #}
{% endif %}{# flattened_fields #}
{%- endif %}{# paged_result_field #}

{% else %}{# this is an lro or streaming method #}
def test_{{ method_name }}_rest_unimplemented():
Expand Down Expand Up @@ -1906,8 +1888,8 @@ def test_transport_kind_{{ transport_name }}():


{% macro transport_close_test(service, transport, is_async) %}
{% set async_prefix = "async " if is_async else "" %}
{% set async_decorator = "@pytest.mark.asyncio " if is_async else "" %}
{% set async_prefix = get_async_prefix(is_async) %}
{% set async_decorator = get_async_decorator(is_async) %}
{% set transport_name = get_transport_name(transport, is_async) %}
{% set close_session = {
'rest': "_session",
Expand Down Expand Up @@ -1947,3 +1929,111 @@ def test_unsupported_parameter_rest_asyncio():
)

{% endmacro %}

{# get_await_prefix sets an "await" keyword
# to a method call if is_async=True.
#}
{% macro get_await_prefix(is_async) %}
{{- "await " if is_async else "" -}}
{% endmacro %}

{# get_async_prefix sets an "async" keyword
# to a method definition if is_async=True.
#}
{% macro get_async_prefix(is_async) %}
{{- "async " if is_async else "" -}}
{% endmacro %}

{# get_async_decorator sets a "@pytest.mark.asyncio" decorator
# to an async test method if is_async=True.
#}
{% macro get_async_decorator(is_async) %}
{{- "@pytest.mark.asyncio " if is_async else "" -}}
{% endmacro %}

{# is_rest_unsupported_method renders:
# 'True' if transport is async REST.
# 'True' if transport is sync REST and method is a client streaming method.
# 'False' otherwise.
#}
{# TODO(https://github.com/googleapis/gapic-generator-python/issues/2152): Update this method as we add support for methods in async REST.
# There are no plans to add support for client streaming.
#}
{% macro is_rest_unsupported_method(method, is_async) %}
{%- if method.client_streaming or is_async -%}
{{'True'}}
Copy link
Contributor

@vchudnov-g vchudnov-g Sep 11, 2024

Choose a reason for hiding this comment

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

Can you have the macro return True and False as booleans rather than strings?

Copy link
Contributor Author

@ohmayr ohmayr Sep 12, 2024

Choose a reason for hiding this comment

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

No. Macros can only return strings and this is the best thing I could come up with.

Copy link
Contributor

@vchudnov-g vchudnov-g Sep 12, 2024

Choose a reason for hiding this comment

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

Right, right.

I guess what I was thinking would be more clearly described in terms of defining the functionality of this macro in Python proper (eg def is_supported(method, is_async):...), so the invocations below would call something like {% if is_supported(method, is_async) %} ... {% endif %} without the string comparison, which grates a little. That would be more elegant. But if you expect this macro to be temporary anyhow, we can skip it. What you have is fine.

{%- else -%}
{{'False'}}
{%- endif -%}
{% endmacro %}

{# run_transport_tests_for_config generates all the rest specific tests for both
# sync and async transport.
# TODO(https://github.com/googleapis/gapic-generator-python/issues/2142): Continue migrating the test cases
# in macro::run_transport_tests_for_config into here, and then delete that macro in favor of this one.
# TODO(https://github.com/googleapis/gapic-generator-python/issues/2153): As a follow up, migrate gRPC test cases
# into `run_transport_tests_for_config` and make any of the rest specific specific macros which are called within more generic.
#}
{% macro run_transport_tests_for_config(service, transport, is_async) %}
{% if 'rest' in transport %}
Copy link
Contributor

@vchudnov-g vchudnov-g Sep 11, 2024

Choose a reason for hiding this comment

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

I support having this rest-checking failsafe here, it but it does seem somewhat redundant/uninformative. At the very least least you should error if transport is wrong:

{% if 'rest' not in transport %}
{{ raise GenerationError }}
{% else %}
...
{% endif %}

More generally, though, I would support changing the name of this macro so it can be used for both REST and gRPC. Obviously we would do the gRPC refactoring later, not in this PR. WDYT? If this sounds feasible, consider editing as below, and let's file an issue. This would help us ensure we're thorough in our tests and consistent for both transports.

{% macro run_tests_for_config(service, transport, is_async) %}
{% if 'grpc' in transport %}
{{ raise GenerationError("gRPC tests will be moved here") }}
{% else %} {# rest #}
...
{% endif %}

Copy link
Contributor Author

@ohmayr ohmayr Sep 12, 2024

Choose a reason for hiding this comment

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

  1. I've filed an issue and linked it to follow up with adding gRPC tests here as suggested.
  2. I've updated the macro name to be more generic.

I don't think we need to raise a generation error for gRPC tests since it would add unnecessary toil in the test suite (i.e. this suite shouldn't be run against gRPC transport which is why we're skipping for it).

I think It's fine to skip generation for it as long as we have a tracking bug to include gRPC tests.

Copy link
Contributor

@vchudnov-g vchudnov-g Sep 12, 2024

Choose a reason for hiding this comment

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

Thanks!

I don't think we need to raise a generation error for gRPC tests since it would add unnecessary toil in the test suite (i.e. this suite shouldn't be run against gRPC transport which is why we're skipping for it).

The idea is to just have a fail-safe to ensure we're indeed skipping it. It protects us from our own future programming errors. I've found this defensive practice valuable.

Not a blocker for this PR, but I still say it's a nice to have.

Copy link
Contributor Author

@ohmayr ohmayr Sep 12, 2024

Choose a reason for hiding this comment

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

I agree that it's nice to have a fail-safe. Let's sync on this offline and we can address this as a follow up if needed.

{% for method in service.methods.values() %}
{% if is_rest_unsupported_method(method, is_async) == 'True' or not method.http_options %}
{{ rest_method_not_implemented_error(service, method, transport, is_async) }}
{% endif %}{# is_rest_unsupported_method(method, is_async) == 'False' and method.http_options #}
{% endfor %}
{% endif %}{# if 'rest' in transport #}
{{ initialize_client_with_transport_test(service, transport, is_async) }}
{% endmacro %}

{# rest_method_not_implemented_error generates tests for methods
# which are not supported for rest transport.
#}
{% macro rest_method_not_implemented_error(service, method, transport, is_async) %}
{% if not is_async %}{# TODO(b/362949446): Remove this guard once a not implemented __call__ class method is added to async rest for every wrapper.Method. #}
{% set await_prefix = get_await_prefix(is_async) %}
{% set async_prefix = get_async_prefix(is_async) %}
{% set async_decorator = get_async_decorator(is_async) %}
{% set transport_name = get_transport_name(transport, is_async) %}
{% set method_name = method.name|snake_case %}
{{async_decorator}}
{{async_prefix}}def test_{{ method_name }}_{{transport_name}}_error():
{% if transport_name == 'rest_asyncio' %}
if not HAS_GOOGLE_AUTH_AIO:
{# TODO(https://github.com/googleapis/google-auth-library-python/pull/1577): Update the version of google-auth once the linked PR is merged. #}
pytest.skip("google-auth > 2.x.x is required for async rest transport.")
{% endif %}

client = {{ get_client(service, is_async) }}(
credentials={{get_credentials(is_async)}},
transport="{{transport_name}}"
)

with pytest.raises(NotImplementedError) as not_implemented_error:
{{await_prefix}}client.{{ method_name }}({})
assert (
"Method {{ method.name }} is not available over REST transport"
in str(not_implemented_error.value)
)

{% endif %}{# if is_async #}
{% endmacro %}

{# initialize_client_with_transport_test adds coverage for transport clients.
# Note: This test case is needed because we aren't unconditionally
# generating the not implemented coverage test for every client.
#}
{% macro initialize_client_with_transport_test(service, transport, is_async) %}
{% set transport_name = get_transport_name(transport, is_async) %}
def test_initialize_client_w_{{transport_name}}():
{% if transport_name == 'rest_asyncio' %}
if not HAS_GOOGLE_AUTH_AIO:
{# TODO(https://github.com/googleapis/google-auth-library-python/pull/1577): Update the version of google-auth once the linked PR is merged. #}
pytest.skip("google-auth > 2.x.x is required for async rest transport.")
{% endif %}
client = {{ get_client(service, is_async) }}(
credentials={{get_credentials(is_async)}},
transport="{{transport_name}}"
)
assert client is not None

{% endmacro %}
Loading