You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The test creates a service, closes its log_output, but then creates a new service without any connection to the first one. This doesn't properly test reuse of a closed stdout.
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 undefined pytest variable.
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)
Apply / Chat
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that pytest is used but not imported, which would cause a NameError and prevent the test from running. Adding the import is necessary for the test's correctness, but it is a standard fix and not critical to application logic.
Medium
Use public attribute
The test accesses service._log_output which appears to be a private attribute (prefixed with underscore), but the Service class shown in the diff doesn't expose this attribute. The test should use the public service.log_output attribute instead.
service = Service(log_output=sys.stdout)
-service._log_output.close()+service.log_output.close()
Apply / Chat
Suggestion importance[1-10]: 6
__
Why: The suggestion improves code style and encapsulation by recommending the use of the public log_output attribute instead of the private _log_output. This enhances maintainability and aligns with best practices, but does not affect core functionality.
Low
Learned best practice
Add error handling for cleanup
Wrap the log_output closing operations in a try-finally block to ensure resources are properly cleaned up even if exceptions occur. This prevents resource leaks when closing operations fail.
def stop(self) -> None:
- if self.log_output not in {PIPE, subprocess.DEVNULL}:- if isinstance(self.log_output, IOBase):- self.log_output.close()- elif isinstance(self.log_output, int):- os.close(self.log_output)+ try:+ if self.log_output not in {PIPE, subprocess.DEVNULL}:+ if isinstance(self.log_output, IOBase):+ self.log_output.close()+ elif isinstance(self.log_output, int):+ os.close(self.log_output)+ except Exception:+ logger.error("Error closing log output.", exc_info=True)
if hasattr(self, "process"):
try:
self.send_remote_shutdown_command()
except TypeError:
pass
finally:
self._terminate_process()
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 6
__
Why:
Relevant best practice - Use try-finally blocks to ensure proper resource cleanup, especially for disposable resources, even when exceptions occur during execution.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Reference Fixes #15629
@cgoldberg
In the given pr it improves error handling and robustness in the Service class by adding a pre-check before starting the subprocess:
Checks if the executable_path exists and is executable using os.path.isfile() and os.access().
Raises a clear WebDriverException with helpful messages if the path is invalid or lacks execution permissions.
Prevents cryptic or low-level errors when the executable is missing or misconfigured.
Fixes an IndentationError in _start_process caused by missing indentation.
PR Type
Bug fix, Tests
Description
Prevents reuse of closed sys.stdout in Service log output
Adds pre-check for executable path existence and permissions
Improves error handling with clear WebDriverException messages
Adds regression test for closed stdout reuse scenario
Changes walkthrough 📝
service.py
Add executable path validation and error handling in Servicepy/selenium/webdriver/common/service.py
test_service.py
Add regression test for closed stdout reuse in Servicepy/test/selenium/webdriver/common/test_service.py