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

Improve asserts when checking metadata in autogenerated unit tests #2024

Open
parthea opened this issue May 6, 2024 · 0 comments
Open

Improve asserts when checking metadata in autogenerated unit tests #2024

parthea opened this issue May 6, 2024 · 0 comments
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: cleanup An internal cleanup or hygiene concern.

Comments

@parthea
Copy link
Contributor

parthea commented May 6, 2024

See this comment and this comment from PR #1999 related to improving the asserts in unit tests when metadata is tested.

metadata = ()
{% if not method.explicit_routing and method.field_headers %}
metadata = tuple(metadata) + (
gapic_v1.routing_header.to_grpc_metadata((
{% for field_header in method.field_headers %}
{% if not method.client_streaming %}
('{{ field_header.raw }}', ''),
{% endif %}
{% endfor %}
)),
)
{% endif %}
pager = client.{{ method_name }}(request={})
assert pager._metadata == metadata

See feedback below:
1 - Headers may include more than 1 value
2 - The tests are brittle as tuples are equal only if they're equal element-wise; order matters.

IIUC, tuple + tuple == long-tuple. This seems brittle, since in general headers (maybe not the ones we're checking so far) can have more than one value. We should .. be a bit more structured in our tests here.

It seems this would be true by accident, in the following sense: Tuples are equal if they're equal element-wise; order matters. However, HTTP headers are not guaranteed to be in a particular order. So here the test would pass because the order in which you set the expectations happens happens to match the order in which the underlying library put the headers in the HTTP requests. If anything changes (the order in which the headers are set, or the order in which we add expectations), this would break. So we should be more robust about this as well.

@parthea parthea added the type: cleanup An internal cleanup or hygiene concern. label May 6, 2024
@vchudnov-g vchudnov-g added the priority: p2 Moderately-important priority. Fix may not be included in next release. label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

No branches or pull requests

2 participants