Skip to content
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

Merged
merged 6 commits into from Dec 2, 2021

Conversation

kbandes
Copy link
Contributor

@kbandes kbandes commented Dec 1, 2021

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:

  1. The version of api_core with the rest operations client should be released (added in feat: add operations rest client to support long-running operations. python-api-core#311, not yet released).
  2. The missing signature field fix in showcase should be released (fixed in fix!: add parent to method signature for Messaging.SearchBlurbs() gapic-showcase#930, not yet released).
  3. The extended operations module should be moved to api_core and released (no PR yet that I know of).

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.

@kbandes kbandes requested review from a team as code owners December 1, 2021 18:31
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 1, 2021
Copy link
Contributor

@vam-google vam-google left a 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed?

Copy link
Contributor Author

@kbandes kbandes Dec 1, 2021

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.

Copy link
Contributor Author

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):
Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 %}
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@kbandes kbandes requested a review from a team as a code owner December 1, 2021 21:40
@kbandes kbandes merged commit 0705d9c into googleapis:master Dec 2, 2021
gcf-merge-on-green bot pushed a commit that referenced this pull request Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants