Conversation
There was a problem hiding this comment.
Code Review
This pull request updates mTLS tests in tests/test_discovery.py to clear the CLOUDSDK_CONTEXT_AWARE_USE_CLIENT_CERTIFICATE environment variable, preventing it from bypassing auto-discovery of the certificate configuration. It also mocks os.path.exists to return True. The feedback suggests refining the global mock of os.path.exists to only return True for the specific configuration file path, avoiding potential side effects on other file checks within the test suite.
| with mock.patch("os.path.exists", return_value=True): | ||
| with mock.patch.dict( | ||
| "os.environ", | ||
| {"GOOGLE_API_CERTIFICATE_CONFIG": config_filename}, | ||
| ): | ||
| plus = build_from_document( | ||
| discovery, | ||
| credentials=self.MOCK_CREDENTIALS, | ||
| client_options={ | ||
| "client_encrypted_cert_source": self.client_encrypted_cert_source | ||
| }, | ||
| ) | ||
| self.assertIsNotNone(plus) | ||
| self.assertEqual(plus._baseUrl, base_url) |
There was a problem hiding this comment.
Mocking os.path.exists globally to always return True can cause unexpected side effects if other parts of the test or standard library rely on it to verify the existence of real files. A safer and more robust approach is to use a side_effect that only returns True for the specific mock configuration file (config_filename) and delegates to the original os.path.exists for all other paths.
original_exists = os.path.exists
with mock.patch(
"os.path.exists",
side_effect=lambda p: True if p == config_filename else original_exists(p),
):
with mock.patch.dict(
"os.environ",
{"GOOGLE_API_CERTIFICATE_CONFIG": config_filename},
):
plus = build_from_document(
discovery,
credentials=self.MOCK_CREDENTIALS,
client_options={
"client_encrypted_cert_source": self.client_encrypted_cert_source
},
)
self.assertIsNotNone(plus)
self.assertEqual(plus._baseUrl, base_url)| with mock.patch("os.path.exists", return_value=True): | ||
| with mock.patch.dict( | ||
| "os.environ", | ||
| {"GOOGLE_API_CERTIFICATE_CONFIG": config_filename}, | ||
| ): | ||
| plus = build_from_document( | ||
| discovery, | ||
| credentials=self.MOCK_CREDENTIALS, | ||
| adc_cert_path=self.ADC_CERT_PATH, | ||
| adc_key_path=self.ADC_KEY_PATH, | ||
| ) | ||
| self.assertIsNotNone(plus) | ||
| self.assertEqual(plus._baseUrl, base_url) |
There was a problem hiding this comment.
Mocking os.path.exists globally to always return True can cause unexpected side effects if other parts of the test or standard library rely on it to verify the existence of real files. A safer and more robust approach is to use a side_effect that only returns True for the specific mock configuration file (config_filename) and delegates to the original os.path.exists for all other paths.
original_exists = os.path.exists
with mock.patch(
"os.path.exists",
side_effect=lambda p: True if p == config_filename else original_exists(p),
):
with mock.patch.dict(
"os.environ",
{"GOOGLE_API_CERTIFICATE_CONFIG": config_filename},
):
plus = build_from_document(
discovery,
credentials=self.MOCK_CREDENTIALS,
adc_cert_path=self.ADC_CERT_PATH,
adc_key_path=self.ADC_KEY_PATH,
)
self.assertIsNotNone(plus)
self.assertEqual(plus._baseUrl, base_url)
This PR fixes unit test failures in
tests/test_discovery.py. See the failure in #2791 as an exampleCLOUDSDK_CONTEXT_AWARE_USE_CLIENT_CERTIFICATE: WhenGOOGLE_API_USE_CLIENT_CERTIFICATEis unset,google-authfalls back toCLOUDSDK_CONTEXT_AWARE_USE_CLIENT_CERTIFICATE. Clearing it ensures pre-existing"true"values in Cloud SDK environments do not bypass the auto-discovery of the mock certificate.os.path.exists: Addmock.patch("os.path.exists", return_value=True)sogoogle-authdetects the mock certificate config file before opening it.