Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ class Async{{service.name}}RestTransport(_Base{{ service.name }}RestTransport):

It sends JSON representations of protocol buffers over HTTP/1.1
"""
def __init__(self, *,
def __init__(self,
*,
host: str{% if service.host %} = '{{ service.host }}'{% endif %},
credentials: Optional[ga_credentials_async.Credentials] = None,
client_info: gapic_v1.client_info.ClientInfo = DEFAULT_CLIENT_INFO,
Expand Down
79 changes: 50 additions & 29 deletions gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2
Original file line number Diff line number Diff line change
Expand Up @@ -1522,33 +1522,6 @@ def test_{{ method_name }}_rest_interceptors(null_interceptor):
{% endif %}{# streaming #}


def test_{{ method_name }}_rest_bad_request(transport: str = 'rest', request_type={{ method.input.ident }}):
client = {{ service.client_name }}(
credentials=ga_credentials.AnonymousCredentials(),
transport=transport,
)

# send a request that will satisfy transcoding
request_init = {{ method.http_options[0].sample_request(method) }}
request = request_type(**request_init)
{% if method.client_streaming %}
requests = [request]
{% endif %}

# Mock the http request call within the method and fake a BadRequest error.
with mock.patch.object(Session, 'request') as req, pytest.raises(core_exceptions.BadRequest):
# Wrap the value into a proper Response obj
response_value = Response()
response_value.status_code = 400
response_value.request = Request()
req.return_value = response_value
{% if method.client_streaming %}
client.{{ method_name }}(iter(requests))
{% else %}
client.{{ method_name }}(request)
{% endif %}


{% if method.flattened_fields and not method.client_streaming %}
def test_{{ method_name }}_rest_flattened():
client = {{ service.client_name }}(
Expand Down Expand Up @@ -1975,20 +1948,21 @@ def test_unsupported_parameter_rest_asyncio():
# 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 %}
{% 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) }}
{% else %}
{{ bad_request_test(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 'rest' in transport %}
{% set await_prefix = get_await_prefix(is_async) %}
{% set async_prefix = get_async_prefix(is_async) %}
{% set async_decorator = get_async_decorator(is_async) %}
Expand All @@ -2014,6 +1988,7 @@ def test_unsupported_parameter_rest_asyncio():
in str(not_implemented_error.value)
)

{% endif %}{# if 'rest' in transport #}
Copy link
Contributor

Choose a reason for hiding this comment

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

Have an else to raise a generation time error.

{% endmacro %}

{# initialize_client_with_transport_test adds coverage for transport clients.
Expand All @@ -2035,3 +2010,49 @@ def test_initialize_client_w_{{transport_name}}():
assert client is not None

{% endmacro %}

{# bad_request_test generates tests for rest methods
# 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 bad_request_test(service, method, transport, is_async) %}
{% if 'rest' in transport %}
{% 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 %}
{% set mocked_session = "AsyncAuthorizedSession" if is_async else "Session" %}
{{ async_decorator }}
{{ async_prefix }}def test_{{ method_name }}_{{transport_name}}_bad_request(request_type={{ method.input.ident }}):
{% 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}}"
)
# send a request that will satisfy transcoding
request_init = {{ method.http_options[0].sample_request(method) }}
request = request_type(**request_init)

# Mock the http request call within the method and fake a BadRequest error.
with mock.patch.object({{mocked_session}}, 'request') as req, pytest.raises(core_exceptions.BadRequest):
# Wrap the value into a proper Response obj
response_value = mock.Mock()
{% if is_async %}
response_value.read = mock.AsyncMock(return_value=b'{}')
{% else %}
json_return_value = ''
response_value.json = mock.Mock(return_value={})
{% endif %}{# if is_async #}
response_value.status_code = 400
response_value.request = mock.Mock()
req.return_value = response_value
{{ await_prefix }}client.{{ method_name }}(request)

{% endif %}{# if 'rest' in transport #}
Copy link
Contributor

@vchudnov-g vchudnov-g Sep 13, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ohmayr ohmayr Sep 13, 2024

Choose a reason for hiding this comment

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

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.

{% endmacro %}
Loading