-
Notifications
You must be signed in to change notification settings - Fork 76
Description
See this comment and this comment from PR #1999 related to improving the asserts in unit tests when metadata is tested.
gapic-generator-python/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2
Lines 856 to 870 in 617222d
| 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.