tests: fix unit test failure when auto_populated_fields is set by parthea · Pull Request #16944 · googleapis/google-cloud-python

parthea · 2026-05-05T16:22:36Z

Fixes b/508597813

gemini-code-assist

Code Review

This pull request implements auto-population for the request_id field in CreateJob, CancelJob, and DeleteJob methods, as specified in the service configuration and verified in unit tests. Feedback indicates that the auto-population logic should be moved from the sample code into the generated RPC methods to fully comply with AIP-4235. Furthermore, the unit tests should be updated to use re.fullmatch for more robust UUID validation and to use empty strings instead of None when resetting proto3 string fields.

gemini-code-assist · 2026-05-05T16:26:06Z

+        if not request.request_id:
+            request.request_id = str(uuid.uuid4())


According to AIP-4235, auto-population of fields should be handled by the client library's RPC methods. If the generator is updated to support auto_populated_fields, this logic should ideally be placed within the generated RPC methods rather than the sample code. This keeps the samples clean and ensures the feature is available to all users of the library, not just those following the samples.

References

According to AIP-4235, auto-population of fields should be handled by the client library's RPC methods. ^(link)

gemini-code-assist · 2026-05-05T16:26:06Z

+        assert re.match(r"[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}", args[0].request_id)
+        # clear UUID field so that the check below succeeds
+        args[0].request_id = None


There are two improvements that can be made here for better robustness and adherence to proto3 standards:

Use re.fullmatch instead of re.match to ensure the entire request_id string conforms to the UUID v4 format, preventing false positives from longer strings.

Set the field to an empty string "" instead of None. In proto3, string fields default to "". While proto-plus might handle None by converting it to the default, it is not valid for standard google.protobuf messages and can lead to TypeError or unexpected behavior in equality checks.

Suggested change

assert re.match(r"[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}", args[0].request_id)

# clear UUID field so that the check below succeeds

args[0].request_id = None

assert re.fullmatch(r"[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}", args[0].request_id)

# clear UUID field so that the check below succeeds

args[0].request_id = ""

References

In proto3, string fields default to an empty string (""). Using None is not valid for standard google.protobuf messages. ^(link)

parthea · 2026-05-05T16:28:44Z

The error in https://github.com/googleapis/google-cloud-python/actions/runs/25388575659/job/74456652962?pr=16944 matches what we have in b/508597813

                actual_params = req.call_args.kwargs['params']
>               assert expected_params == actual_params
E               AssertionError: assert [('jobId', '')] == [('requestId'...('jobId', '')]
E                 
E                 At index 0 diff: ('jobId', '') != ('requestId', 'a309a08f-90b8-42ec-a5cd-1490831692bd')
E                 Right contains one more item: ('jobId', '')
E                 Use -v to get more diff

tests/integration/storagebatchoperations_py_gapic_srcjar.py/tests/unit/gapic/storagebatchoperations_v1/test_storage_batch_operations.py:4136: AssertionError

chalmerlowe · 2026-05-05T17:41:25Z

        call.assert_called()
        _, args, _ = call.mock_calls[0]
+        # Ensure that the uuid4 field is set according to AIP 4235
+        assert re.match(r"[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}", args[0].request_id)


NIT:

If this is generated by template, then all the match patterns should be identical. But it seems that the template could be better designed to have only one copy of the match pattern and have all match calls ref that pattern.

get_uuid4_re() is a macro in the template and is used in this PR. Can you share more details if this isn't what you meant?

google-cloud-python/packages/gapic-generator/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_macros.j2

Lines 2198 to 2200 in ce71af0

{% macro get_uuid4_re() -%}

[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}

{%- endmacro %}{# uuid_re #}

main...repro-508597813#diff-a241fd9719e65e65cf1c9221d8c4704c7e5d7487f86b68d341d5055fde12d2a2R1231

…populated

ohmayr · 2026-05-06T17:39:13Z

+        if not request.request_id:
+            request.request_id = str(uuid.uuid4())
+


nit: can this be introduced as a helper once in the client which can simply be called in each methods?

ohmayr · 2026-05-06T17:40:54Z

        call.assert_called()
        _, args, _ = call.mock_calls[0]
+        # Ensure that the uuid4 field is set according to AIP 4235
+        assert re.match(r"[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}", args[0].request_id)


nit: can the regex be defined once and re-used everywhere?

ohmayr · 2026-05-06T17:46:22Z

+    if isinstance(request, dict):
+        request['request_id'] = "explicit value for autopopulate-able field"
+    else:
+        request.request_id = "explicit value for autopopulate-able field"


nit: Can this be included as part of the parametrize above?

ohmayr · 2026-05-06T18:00:04Z

+            found_field = None
+            for i, (key, value) in enumerate(req.call_args.kwargs['params']):
+                if key == "requestId":
+                    assert re.match(r"[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}", value)
+                    found_field = i
+                    break
+            if found_field is not None:
+                del req.call_args.kwargs['params'][found_field]


I don't know how I feel about deleting a field in a mock test.

A couple of options:

We include request_id within expected_params and have the value set to mock.ANY since we're already asserting what the value should look like against the regex later on.

We can filter the params we want to compare against the expected_params.

gemini-code-assist Bot reviewed May 5, 2026

View reviewed changes

parthea marked this pull request as ready for review May 5, 2026 17:25

parthea requested a review from a team as a code owner May 5, 2026 17:25

Base automatically changed from add-storagebatchoperations-golden to main May 5, 2026 17:39

chalmerlowe reviewed May 5, 2026

View reviewed changes

parthea added 2 commits May 5, 2026 17:47

tests: fix unit test failure when auto_populated_fields is set

478ecd8

fix *rest_required_fields test actual vs expected when field is auto-…

69a9623

…populated

parthea force-pushed the repro-508597813 branch from f195518 to 69a9623 Compare May 5, 2026 17:49

parthea assigned chalmerlowe, daniel-sanche and ohmayr May 5, 2026

ohmayr reviewed May 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: fix unit test failure when auto_populated_fields is set#16944

tests: fix unit test failure when auto_populated_fields is set#16944
parthea wants to merge 2 commits intomainfrom
repro-508597813

parthea commented May 5, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot May 5, 2026

Uh oh!

gemini-code-assist Bot May 5, 2026

Uh oh!

parthea commented May 5, 2026

Uh oh!

chalmerlowe May 5, 2026

Uh oh!

parthea May 5, 2026

Uh oh!

ohmayr May 6, 2026

Uh oh!

ohmayr May 6, 2026

Uh oh!

ohmayr May 6, 2026

Uh oh!

ohmayr May 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		if not request.request_id:
		request.request_id = str(uuid.uuid4())

	{% macro get_uuid4_re() -%}
	[a-f0-9]{8}-?[a-f0-9]{4}-?4[a-f0-9]{3}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12}
	{%- endmacro %}{# uuid_re #}

Conversation

parthea commented May 5, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 5, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 5, 2026

Choose a reason for hiding this comment

Uh oh!

parthea commented May 5, 2026

Uh oh!

chalmerlowe May 5, 2026

Choose a reason for hiding this comment

NIT:

Uh oh!

parthea May 5, 2026

Choose a reason for hiding this comment

Uh oh!

ohmayr May 6, 2026

Choose a reason for hiding this comment

Uh oh!

ohmayr May 6, 2026

Choose a reason for hiding this comment

Uh oh!

ohmayr May 6, 2026

Choose a reason for hiding this comment

Uh oh!

ohmayr May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants