[py] Fix broken test for chromedriver logging

[cgoldberg]

User description

💥 What does this PR do?

This is a fix to the internal Python test suite.

This PR fixes the test_uses_chromedriver_logging test in py/test/selenium/webdriver/chrome/chrome_service_tests.py.

Previously, this test was failing because it was attempting to launch 2 browsers using the same Service object. This is not possible and results in an error:

E       selenium.common.exceptions.SessionNotCreatedException: Message: session not created
E       from unknown error: cannot find Chrome binary

This PR creates 2 Service objects so each driver instance can use its own.

(This test was somehow passing in CI, but I have no idea why... it consistently fails for me locally until this fix is applied)

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix, Tests

---

Description

• Fixed a broken test for ChromeDriver logging in Python.

• Updated test_uses_chromedriver_logging to use separate Service objects.

• Resolved issue with launching multiple browsers using the same Service.

• Ensured test consistency across local and CI environments.

---

Changes walkthrough 📝

	Relevant files
Tests	chrome_service_tests.py Fix and improve ChromeDriver logging test                                py/test/selenium/webdriver/chrome/chrome_service_tests.py Updated test_uses_chromedriver_logging to use two Service objects. Fixed issue with reusing the same Service for multiple drivers. Improved test reliability and logging validation. +10/-3

---

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: 1 🔵⚪⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The test doesn't properly handle the case where driver1 might not be initialized. If driver1 creation fails, the finally block would throw an error trying to quit a non-existent driver. driver1.quit() if driver2:

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to c53d0cf

Category	Suggestion	Impact
General	Check file exists before removal Add a check to verify if the log file exists before attempting to remove it, as the file might not be created if the test fails early. py/test/selenium/webdriver/chrome/chrome_service_tests.py [59] -os.remove(log_file) +if os.path.exists(log_file): + os.remove(log_file) Apply this suggestion Suggestion importance[1-10]: 7 __ Why: Adding a check for file existence before removal is a good defensive programming practice that prevents potential FileNotFoundError exceptions if the test fails before the log file is created. This improves error handling and test robustness.	Medium
More

---

Previous suggestions

✅ Suggestions up to commit e6957ec

Category	Suggestion	Impact
Possible issue	✅ Missing driver cleanup Suggestion Impact: The commit implemented the suggested driver2 cleanup in the finally block, and also added a similar check for driver1. This addresses the resource leak issue identified in the suggestion. code diff: if driver2: driver2.quit() The test is missing cleanup for driver2. If driver2 is successfully created, it should be quit in the finally block to properly release resources. py/test/selenium/webdriver/chrome/chrome_service_tests.py [53-54] finally: driver1.quit() + if driver2: + driver2.quit() [Suggestion has been applied] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a resource leak where driver2 is not being properly cleaned up in the finally block. This is a significant issue that could lead to resource leaks and potentially affect subsequent tests.	High
Possible issue	✅ Handle undefined variable safely Suggestion Impact: The commit implemented the suggestion's intent by adding a check before quitting driver1. It used a slightly different approach by initializing driver1 to None at the beginning and then using a simpler 'if driver1' check instead of 'if driver1 in locals() and driver1'. code diff: + driver1 = None driver2 = None try: driver1 = clean_driver(service=service1) @@ -51,7 +52,8 @@ with open(log_file) as fp: assert len(fp.readlines()) >= 2 * lines finally: - driver1.quit() + if driver1: + driver1.quit() The test is missing a reference to driver1 before quitting it in the finally block. Since driver1 is defined inside the try block, it might not exist if an exception occurs before its creation, causing a NameError. py/test/selenium/webdriver/chrome/chrome_service_tests.py [54] -driver1.quit() +if 'driver1' in locals() and driver1: + driver1.quit() [Suggestion has been applied] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical issue where the test could fail with a NameError if an exception occurs before driver1 is created. The improved code safely checks if driver1 exists before attempting to quit it, preventing potential test failures.	High

[cgoldberg]

Fixed first PR review suggestion.

If driver1 creation fails, the finally block would throw an error trying to quit a non-existent driver

This suggestion by AI already exists... no idea what it's talking about.

[cgoldberg]

one of the test failures in CI was fixed in #15580 ... the other failure is unrelated to this PR.