test(storage): isolate environment in client tests#16664
test(storage): isolate environment in client tests#16664chalmerlowe wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates several unit tests in test_client.py to isolate the environment by patching os.environ during client initialization. The review feedback suggests using mock.patch.dict with clear=True instead of mock.patch to ensure more robust environment isolation and to avoid potential issues if os.environ was imported directly in the source code.
| with mock.patch("os.environ", {}): | ||
| client = self._make_one(credentials=credentials) |
There was a problem hiding this comment.
Using mock.patch("os.environ", {}) replaces the os.environ object entirely with a standard dictionary. This can be brittle because it won't affect code that has already imported environ using from os import environ. Additionally, it loses the special behaviors of the os._Environ object (such as calling putenv). It is recommended to use mock.patch.dict with clear=True instead, as it modifies the existing object in place and is more robust for environment isolation.
| with mock.patch("os.environ", {}): | |
| client = self._make_one(credentials=credentials) | |
| with mock.patch.dict("os.environ", {}, clear=True): | |
| client = self._make_one(credentials=credentials) |
| with mock.patch("os.environ", {}): | ||
| client = self._make_one( | ||
| client_options={"api_endpoint": custom_endpoint}, | ||
| use_auth_w_custom_endpoint=False, | ||
| ) |
There was a problem hiding this comment.
Using mock.patch("os.environ", {}) replaces the os.environ object entirely. It is safer and more idiomatic to use mock.patch.dict("os.environ", {}, clear=True) to ensure that the environment is isolated regardless of how os.environ was imported in the source code.
| with mock.patch("os.environ", {}): | |
| client = self._make_one( | |
| client_options={"api_endpoint": custom_endpoint}, | |
| use_auth_w_custom_endpoint=False, | |
| ) | |
| with mock.patch.dict("os.environ", {}, clear=True): | |
| client = self._make_one( | |
| client_options={"api_endpoint": custom_endpoint}, | |
| use_auth_w_custom_endpoint=False, | |
| ) |
| with mock.patch("os.environ", {}): | ||
| client = self._make_one( | ||
| credentials=credentials, client_options={"api_endpoint": custom_endpoint} | ||
| ) |
There was a problem hiding this comment.
Using mock.patch("os.environ", {}) replaces the os.environ object entirely. It is safer and more idiomatic to use mock.patch.dict("os.environ", {}, clear=True) to ensure that the environment is isolated regardless of how os.environ was imported in the source code.
| with mock.patch("os.environ", {}): | |
| client = self._make_one( | |
| credentials=credentials, client_options={"api_endpoint": custom_endpoint} | |
| ) | |
| with mock.patch.dict("os.environ", {}, clear=True): | |
| client = self._make_one( | |
| credentials=credentials, client_options={"api_endpoint": custom_endpoint} | |
| ) |
This PR resolves a unit test failure where the project ID leaked from the local environment into the client constructor tests, causing assertions to fail.
Problem: Tests like
test_ctor_wo_project,test_ctor_w_custom_endpoint_bypass_auth, andtest_ctor_w_custom_endpoint_w_credentialswere asserting thatclient.projectwasNoneor a specific project ID (like"PROJECT"). However, they were receiving a project ID set in the user's local environment (e.g.,"precise-truck-742"), causing an AssertionError.Solution: Wrapped the client creation in these specific tests with a
code block to ensure the environment is clean and isolated during test execution.