-
Notifications
You must be signed in to change notification settings - Fork 69
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.
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
fix: ensure rest unit tests have complete coverage #1098
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with one question.
Also, can you please provide the links to the corresponding PRs that are expected to be released to unblock 100% coverage (i.e. for the 3 items in the PR description)
@@ -1770,7 +1837,7 @@ def test_{{ service.name|snake_case }}_rest_lro_client(): | |||
# Ensure that we have a api-core operations client. | |||
assert isinstance( | |||
transport.operations_client, | |||
operations_v1.OperationsClient, | |||
operations_v1.AbstractOperationsClient, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was at the request of software-dov. The change was made to api-core in a prior PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding related PRs: I'm not sure. I believe other people are making these changes and releasing them, I'm not sure of the timeline or PR numbers. The change to noxfile.py is small, but I won't know exactly how to do it until I know what the actual release numbers for showcase and api-core are.
|
||
{% if method.input.required_fields %} | ||
@staticmethod | ||
def _{{ method.name | snake_case }}_populate_required_fields(message_dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer it if we moved mutation to the caller, something like
__{{ method.name | snake_case }}_required_fields_default_values = {
{% for req_field in method.input.required_fields if req_field.is_primitive %}
"{{ req_field.name | camel_case }}" : {% if req_field.field_pb.default_value is string %}"{{req_field.field_pb.default_value }}"{% else %}{{ req_field.field_pb.default_value }}{% endif %}{# default is str #}
{% endfor %}
}
def _{{ method.name | snake_case }}_get_unset_required_fields(message_dict):
return {k, v for k, v in __{{ method.name | snake_case }}_required_fields_default_values.items() if k not in message_dict}
....
query_params.update(self._{{ method.name | snake_case }}_get_unset_required_fields(query_params)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -1210,6 +1212,61 @@ def test_{{ method_name }}_rest(transport: str = 'rest', request_type={{ method. | |||
{% endif %} | |||
|
|||
|
|||
{% if method.input.required_fields %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be reasonable to have a test or two just for the hidden required fields update method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean. Which method is hidden?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, once we refactor as you describe above.
This is quite tricky to test, which is a lot of motivation for these changes. The problem is that typically, though not always, a required field is going to have an expected template in the http rule, so the default value will cause the transcoding to fail. This can be worked around by mocking the transcoding function, but it gets convoluted and ugly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what to do here. No code is un-covered. I would argue that testing this logic in the context of the api method itself doesn't necessarily add anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've added logic to test the actual api method with default-valued required fields.
🤖 I have created a release \*beep\* \*boop\* --- ## [0.58.0](https://www.github.com/googleapis/gapic-generator-python/compare/v0.57.0...v0.58.0) (2021-12-07) ### Features * add support for long-running operations with rest transport. ([#1094](https://www.github.com/googleapis/gapic-generator-python/issues/1094)) ([e89fd23](https://www.github.com/googleapis/gapic-generator-python/commit/e89fd23609625c5aa49acd6c6ee67f87fce324fd)) ### Bug Fixes * ensure rest unit tests have complete coverage ([#1098](https://www.github.com/googleapis/gapic-generator-python/issues/1098)) ([0705d9c](https://www.github.com/googleapis/gapic-generator-python/commit/0705d9c5dbbea793867551e64991be37d8339c6b)) * fix resource path args for paths with =** ([#1089](https://www.github.com/googleapis/gapic-generator-python/issues/1089)) ([309cc66](https://www.github.com/googleapis/gapic-generator-python/commit/309cc66e880e07940866864b03c744310ef56762)) * **snippetgen:** don't create duplicate requests for required oneofs ([#1088](https://www.github.com/googleapis/gapic-generator-python/issues/1088)) ([5531795](https://www.github.com/googleapis/gapic-generator-python/commit/55317956397370a91b1a06ecd476e55f58789807)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This PR fixes gaps in the coverage of the generated unit tests, specifically when the rest transport is included.
This required refactoring the logic for required field handling into a separate static method corresponding to each API method with required fields in its request message. This allows that method to tested thoroughly without elaborate and repetitive mocking.
The intention for these changes is that the nox
showcase_unit
session should include both grpc and rest transports. However, there are several requirements before the noxfile should be changed:parent
to method signature forMessaging.SearchBlurbs()
gapic-showcase#930, not yet released).Once these changes are release, the noxfile for the present repo should be updated to reference those releases. At that point, a full unit test of showcase with
transport=grpc+rest
should succeed and provide 100% coverage.