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

…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?

This file does not touch this template at all. IOW, this is an existing issue at main. I'll open a separate PR to address it.

This will be addressed in a separate PR #17004

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?

This is already defined as a macro. You only need to change it in one place

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

Lines 2198 to 2200 in f5b392e

{% 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 #}

This will be addressed in #17006

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?

This PR doesn't touch this code. This is an existing issue at main. I'll open a separate PR to address it.

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

Comment thread ...ebatchoperations/tests/unit/gapic/storagebatchoperations_v1/test_storage_batch_operations.py

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

parthea assigned parthea and unassigned daniel-sanche, chalmerlowe and ohmayr May 7, 2026

This was referenced May 8, 2026

chore: refactor setting of auto-populated field in request #17004

Draft

chore: refactor templates to reduce duplication #17006

Open

		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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants