[parthea]

This PR updates the showcase_unit nox session to include a REST only configuration.

Fixes #18969

This PR is needed to unblock #18955

[gemini-code-assist]

Code Review

This pull request updates the transport configuration for the Redis integration test from 'grpc+rest' to 'rest' only, resulting in the removal of gRPC-related code, async clients, async pagers, and async code samples from the generated golden files. Feedback on the changes highlights that although the async client is no longer exported, the 'async_client.py' file is still being generated as dead code; the generator should be updated to completely omit this file when the configured transport does not support or export an async client.

I am having trouble creating individual review comments. Click here to see my feedback.

packages/gapic-generator/tests/integration/goldens/redis/google/cloud/redis_v1/services/cloud_redis/async_client.py (306-332)

With transport = "rest", CloudRedisAsyncClient is no longer exported in __init__.py, and all async code samples have been removed. However, the async_client.py file itself is still being generated. This results in dead code in the generated library. The generator should be updated to completely omit generating async_client.py when the configured transport (like REST-only) does not support or export an async client.

References

1. Do not suggest manual code modifications, optimizations, or style fixes for generated files, as any changes should be implemented in the generator or templates to prevent them from being overwritten.

[parthea]

The failing presubmit GitHub Actions Scan will be fixed via #18962

[chalmerlowe]

QUESTION/NIT: Is there a motivating bug OR Issue that this fixes? I did not see one linked in the PR description.

[parthea]

QUESTION/NIT: Is there a motivating bug OR Issue that this fixes? I did not see one linked in the PR description.

Good point. I created #18969 and linked it in the PR description

[chalmerlowe]

BLOCKER

The mix of assert statements in test_{{ service.name|snake_case }}_base_transport_with_adc() mixes two opposing behaviors (_ignore_credentials=False vs. _ignore_credentials=True + host port normalization) in one function. I recommend we separate these into two separate tests.

The test as written does test host="squid.clam.whelk" (verifying :443 is appended when no port is present), but I think it omits the companion boundary where host already includes a port (e.g., host="squid.clam.whelk:8080"), which exercises the ":" not in host branch in base transport init.

Lastly, I believe the preferred paradigm is to use spec_set=True if we are using autospec=True

Suggested change

	# Test _ignore_credentials=True skips ADC and host without port defaults to :443.
	class IgnoreCredsTransport(transports.{{ service.name }}Transport):
	_ignore_credentials = True
	with mock.patch.object(google.auth, 'default', autospec=True) as adc:
	transport = IgnoreCredsTransport(host="squid.clam.whelk")
	adc.assert_not_called()
	assert transport._credentials is None
	assert transport.host == "squid.clam.whelk:443"
	# Test _ignore_credentials=True skips Application Default Credentials (ADC)
	# and a host without a port defaults to :443.
	@pytest.mark.parametrize(
	"host,expected_host",
	[
	pytest.param("squid.clam.whelk", "squid.clam.whelk:443", id="host_without_port_defaults_to_443"),
	pytest.param("squid.clam.whelk:8080", "squid.clam.whelk:8080", id="host_with_port_preserved"),
	],
	)
	def test_{{ service.name|snake_case }}_base_transport_ignore_credentials(host, expected_host):
	class IgnoreCredsTransport(transports.{{ service.name }}Transport):
	_ignore_credentials = True
	with mock.patch.object(
	google.auth, "default", autospec=True, spec_set=True
	) as adc:
	transport = IgnoreCredsTransport(host=host)
	adc.assert_not_called()
	assert transport._credentials is None
	assert transport.host == expected_host

[chalmerlowe]

QUESTION/SUGGESTION

I think a parametrization is likely a good candidate here. I think request can be either None OR an empty dict. If that is correct, we should exercise both, right?

Suggested change

	{{ async_decorator }}
	{{ async_prefix }}def test_{{ method_name }}_empty_call_{{ transport_name }}():
	{{ async_decorator }}
	@pytest.mark.parametrize(
	"request_val",
	[
	pytest.param(None, id="none_request"),
	pytest.param({}, id="empty_dict_request"),
	],
	)
	{{ async_prefix }}def test_{{ method_name }}_empty_call_{{ transport_name }}():

I also think we would benefit by adding to the mock.patch.object:

autospec=True,
spec_set=True,

Of course we would need to incorporate request_val into the function:

{{ await_prefix }}client.{{ method_name }}(request=request_val)

[chalmerlowe]

SUGGESTION

See note above.

Suggested change

	assert args[0].{{ method.flattened_field_to_key[field.name] }} == {{ field.mock_value }}
	assert args[1].{{ method.flattened_field_to_key[field.name] }} == {{ field.mock_value }}

[chalmerlowe]

SUGGESTION

Adding autospec=True, spec_set=True enforces signature verification on the transport callable (and I believe it also shifts the bound self argument to args[0], placing the request protobuf at args[1])

Suggested change

	) as call:
	"__call__",
	autospec=True,
	spec_set=True,
	) as call:

[chalmerlowe]

Approve pending resolution of the comments.
If you strongly disagree with any given comment, let me know and we can work something out.