[py] Use mock.patch for environment variables in tests

[cgoldberg]

User description

💥 What does this PR do?

This PR adds better use of unittest.mock.patch for mocking environment variables in some of the internal Python tests.

Several tests were directly modifying environment variables (via os.environ) in tests, and often not resetting them at the end of the test. It is generally bad practice to modify environment variables since they can stay set to the wrong value if the test fails. Also, not resetting them at the end of the test can leak state into another test. These changes make sure all environment variables are changed within a mocked/patched copy of os.environ.

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix, Tests

---

Description

• Replaced direct os.environ modifications with unittest.mock.patch.

• Improved test isolation by mocking environment variables.

• Updated tests across multiple browser service test files.

• Ensured environment variable changes do not leak between tests.

---

Changes walkthrough 📝

Relevant files

Tests

chrome_service_tests.py Refactored Chrome service tests to use `patch.dict`            py/test/selenium/webdriver/chrome/chrome_service_tests.py Replaced direct os.environ modification with patch.dict. Improved test isolation for test_updates_path_after_setting_env_variable. +3/-4      proxy_tests.py Refactored Chrome proxy tests to use `patch.dict`                py/test/selenium/webdriver/chrome/proxy_tests.py Used patch.dict to mock proxy-related environment variables. Removed manual cleanup of os.environ in tests. +5/-10    edge_service_tests.py Refactored Edge service tests to use `patch.dict`                py/test/selenium/webdriver/edge/edge_service_tests.py Replaced os.environ modification with patch.dict. Enhanced test isolation for Edge service tests. +3/-4      firefox_service_tests.py Refactored Firefox service tests to use `patch.dict`          py/test/selenium/webdriver/firefox/firefox_service_tests.py Replaced os.environ modification with patch.dict. Improved test isolation for Firefox service tests. +3/-4      safari_service_tests.py Refactored Safari service tests to use `patch.dict`            py/test/selenium/webdriver/safari/safari_service_tests.py Used patch.dict to mock environment variables. Enhanced test isolation for Safari service tests. +3/-4      remote_connection_tests.py Refactored remote connection tests to use `patch.dict`      py/test/unit/selenium/webdriver/remote/remote_connection_tests.py Replaced direct os.environ manipulation with patch.dict. Improved test isolation for proxy-related tests. +11/-13

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Missing Driver Initialization The test_bad_proxy_doesnt_interfere function has assertions about driver properties, but the driver variable is only initialized inside the patch.dict context manager. The assertions outside this context may be accessing an undefined variable. with patch.dict("os.environ", {"http_proxy": "bad", "https_proxy": "bad"}): driver = clean_driver(**chrome_kwargs) assert hasattr(driver, "command_executor")

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix incorrect assertion The test is asserting that "chromedriver" is in the executable_path, but it's not actually checking if the environment variable update affects the path. The test should verify that the path is updated based on the mocked environment variable. py/test/selenium/webdriver/chrome/chrome_service_tests.py [128-132] def test_updates_path_after_setting_env_variable(self, service): service.executable_path = self.service_path # Simulating the update with patch.dict("os.environ", {"SE_CHROMEDRIVER": "/foo/bar"}): - assert "chromedriver" in service.executable_path + service.update_binary_location() + assert "/foo/bar" in service.executable_path [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical issue in the test logic. The current test doesn't actually verify that the environment variable affects the path - it only checks if "chromedriver" is in the path, which would be true regardless of the environment variable. Adding the call to update_binary_location() and checking for "/foo/bar" properly tests the intended functionality.	High
Update

[shbenzer]

LGTM

[cgoldberg]

CI failures are caused by #15608 ... all other tests pass.