Conversation
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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)
Sorry, something went wrong.
| 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)
Sorry, something went wrong.
This PR fixes unit test failures in tests/test_discovery.py . See the failure in #2791 as an example
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ tests/test_discovery.py:976: in test_mtls_with_provided_client_cert_unset_environment_variable self.assertEqual(plus._baseUrl, base_url) E AssertionError: 'https://www.googleapis.com/plus/v1/' != 'https://www.mtls.googleapis.com/plus/v1/' E - https://www.googleapis.com/plus/v1/ E + https://www.mtls.googleapis.com/plus/v1/ E ?