Skip to content
This repository was archived by the owner on Mar 26, 2026. It is now read-only.

test: improve routing parameter assert - #2131

Merged
parthea merged 10 commits into
mainfrom
fix-routing-parameter-assert
Sep 16, 2024
Merged

test: improve routing parameter assert#2131
parthea merged 10 commits into
mainfrom
fix-routing-parameter-assert

Conversation

@parthea

Copy link
Copy Markdown
Contributor

This PR fixes the assert in test_*_routing_parameters to verify the routing headers in kw['metadata'].

The resolve function in tests/unit/schema/wrappers/test_routing.py was moved to gapic/schema/wrappers.py so that it can be used in gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2

 # This test doesn't assert anything useful.
assert kw['metadata']

Towards #2091

@product-auto-labelproduct-auto-labelBot added the size: m Pull request size is medium. label Sep 9, 2024
@parthea
partheaforce-pushed the fix-routing-parameter-assert branch from a5f2c67 to 42f4a17CompareSeptember 9, 2024 00:32
@partheaparthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 9, 2024
@gcf-owl-botgcf-owl-botBot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 9, 2024
@parthea
partheaforce-pushed the fix-routing-parameter-assert branch from 42f4a17 to 0586c9aCompareSeptember 9, 2024 00:53
@parthea
partheaforce-pushed the fix-routing-parameter-assert branch from 0586c9a to d9664afCompareSeptember 9, 2024 12:39
@parthea
parthea marked this pull request as ready for review September 9, 2024 12:41
@parthea
parthea requested a review from a teamSeptember 9, 2024 12:41
@partheaparthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 9, 2024
@gcf-owl-botgcf-owl-botBot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 9, 2024

@vchudnov-gvchudnov-g left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, but I have some questions to make sure I understand the (pre-existing) structure of the code touched here.

Comment threadgapic/schema/wrappers.py Outdated
Comment threadgapic/schema/wrappers.py Outdated
Comment threadgapic/schema/wrappers.py Outdated
Comment threadgapic/schema/wrappers.py Outdated
def resolve(cls, routing_rule: routing_pb2.RoutingRule, request: Union[dict, str]) -> dict:
"""Resolves the routing header which should be sent along with the request.
This function performs dynamic header resolution, identical to what's in `_client_macros.j2`.
https://github.com/googleapis/gapic-generator-python/blob/4c5de8791795f8101f6ec66f80b8a8e5e9a21822/gapic/templates/%25namespace/%25name_%25version/%25sub/services/%25service/_client_macros.j2#L150-L164

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Consider using a TinyURL or similar, and enclosing the URL in <> so that it's rendered clickable by some tools.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Superseded by review comment #2131 (comment)

Comment threadgapic/schema/wrappers.py Outdated
@classmethod
def resolve(cls, routing_rule: routing_pb2.RoutingRule, request: Union[dict, str]) -> dict:
"""Resolves the routing header which should be sent along with the request.
This function performs dynamic header resolution, identical to what's in `_client_macros.j2`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I would add a TODO referencing #2160 about refactoring the common code and then removing this sentence.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed in 4bbfd9f

Comment threadgapic/schema/wrappers.py Outdated
@parthea
parthea enabled auto-merge (squash) September 16, 2024 21:09
@parthea
parthea merged commit 1a49b4b into mainSep 16, 2024
@parthea
parthea deleted the fix-routing-parameter-assert branch September 16, 2024 21:46
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

size: mPull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@parthea@vchudnov-g@ohmayr