Add validation for invalid ChromeDriver executable path

[itzanway]

User description

Reference: #15629
This PR adds a check before invoking subprocess.Popen in the Service._start_process() method to ensure the executable path is valid.

Issue
Previously, if the path was None, subprocess.Popen() would throw a TypeError or FileNotFoundError, making it hard to trace the actual cause.

Added a ValueError raise if the executable path is missing:
if not path:
raise ValueError("Executable path must not be None or empty.")

---

PR Type

Bug fix, Tests

---

Description

• Add validation for ChromeDriver executable path existence and permissions

• Raise WebDriverException for invalid or non-executable paths

• Add test to verify exception on invalid executable path

• Minor test improvements and docstrings for clarity

---

Changes walkthrough 📝

	Relevant files
Bug fix	service.py Add executable path validation and improve error handling py/selenium/webdriver/common/service.py Add pre-checks for executable path existence and permissions in _start_process Raise WebDriverException for missing or non-executable driver paths Remove redundant or outdated comments and docstrings for clarity Refactor error handling for process startup +8/-43
Tests	chrome_service_tests.py Add and improve tests for invalid executable path handling py/test/selenium/webdriver/chrome/chrome_service_tests.py Add test to verify exception on invalid executable path Improve test docstrings for clarity Minor patch to environment variable test for path update +14/-2

---

Need help?

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

Check out the documentation for more information.

[CLAassistant]

All committers have signed the CLA.

[qodo-merge-pro]

PR Reviewer Guide 🔍

(Review updated until commit 4e99947)

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 Test Implementation The test_invalid_executable_raises_exception is expecting a ValueError but the implementation in service.py raises a WebDriverException for invalid paths. The test will fail since it's expecting the wrong exception type. bad_path = "/invalid/path/to/chromedriver" with pytest.raises(ValueError): Service(executable_path=bad_path) Path Validation The PR description mentions raising ValueError for None/empty paths, but the implementation raises WebDriverException. The validation should check for empty strings as mentioned in the PR description. if not os.path.isfile(path): raise WebDriverException(f"The executable at path '{path}' does not exist.") if not os.access(path, os.X_OK): raise WebDriverException(f"The executable at path '{path}' is not executable.")

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to 4e99947

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix incorrect exception type The test expects a ValueError but the implementation in service.py raises WebDriverException for invalid paths. Update the test to expect the correct exception type to match the implementation. py/test/selenium/webdriver/chrome/chrome_service_tests.py [123-129] def test_invalid_executable_raises_exception(): """ - Test that Service raises ValueError when given a non-existent or non-executable path. + Test that Service raises WebDriverException when given a non-existent or non-executable path. """ bad_path = "/invalid/path/to/chromedriver" - with pytest.raises(ValueError): + with pytest.raises(WebDriverException): Service(executable_path=bad_path) Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a mismatch between the test and the implementation: the code raises WebDriverException, not ValueError. Updating the test to expect the right exception ensures the test is valid and will not produce false negatives, which is important for test reliability.	Medium
More

---

Previous suggestions

Suggestions up to commit c2f4983

Category	Suggestion	Impact
Possible issue	Check if file already closed The stop() method attempts to close self.log_output without checking if it's already closed, which could lead to errors. Add a check to verify the file is not already closed before attempting to close it. py/selenium/webdriver/common/service.py [114-119] def stop(self) -> None: if self.log_output not in {PIPE, subprocess.DEVNULL}: - if isinstance(self.log_output, IOBase): + if isinstance(self.log_output, IOBase) and not self.log_output.closed: self.log_output.close() elif isinstance(self.log_output, int): - os.close(self.log_output) + try: + os.close(self.log_output) + except OSError: + pass Suggestion importance[1-10]: 7 __ Why: This suggestion improves robustness by preventing an attempt to close an already-closed file, which could raise an exception. It is a minor but useful enhancement to error handling and resource management.	Medium

Suggestions up to commit c2f4983

Category	Suggestion	Impact
Possible issue	Check if file already closed Add a check to verify if the file is already closed before attempting to close it. Trying to close an already closed file will raise a ValueError, which could disrupt the service shutdown process. py/selenium/webdriver/common/service.py [114-119] def stop(self) -> None: if self.log_output not in {PIPE, subprocess.DEVNULL}: - if isinstance(self.log_output, IOBase): + if isinstance(self.log_output, IOBase) and not self.log_output.closed: self.log_output.close() elif isinstance(self.log_output, int): os.close(self.log_output) Suggestion importance[1-10]: 8 __ Why: This suggestion prevents a potential ValueError by ensuring a file is not closed twice, improving the robustness of the shutdown process. It directly addresses a subtle but important error handling issue.	Medium
	Add missing pytest import The test is missing the pytest import which is required for the pytest.raises() context manager. Without this import, the test will fail with a NameError when trying to access the pytest module. py/test/selenium/webdriver/common/test_service.py [1-9] def test_reusing_closed_stdout_fails(): import sys + import pytest from selenium.webdriver.chrome.service import Service from selenium.common.exceptions import WebDriverException service = Service(log_output=sys.stdout) service._log_output.close() with pytest.raises(ValueError): Service(log_output=sys.stdout) Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that pytest is used but not imported, which would cause the test to fail with a NameError. Adding the import is necessary for the test to run, but this is a straightforward fix.	Medium

[itzanway]

@cgoldberg you can now review my pr

[cgoldberg]

Added a ValueError raise if the executable path is missing:
if not path:
? raise ValueError("Executable path must not be None or empty.")

That code doesn't exist in your changes. You check if it is a valid file and is executable then raise WebDriverException.

• The test belongs in /selenium/webdriver/chrome/chrome_service_tests.py since it is testing selenium.webdriver.chrome.service. Please move it there and remove the test file you added.

• Your test imports WebDriverException but never uses it. Where is ValueError error coming from?

• Please re-add the docstrings and comments. Why did you remove them?

Checking for the valid/executable file seems reasonable, but I am very confused about how this change relates to your test, the linked issue, and the title of your PR.

[itzanway]

@cgoldberg can you review the changes