[py] Do not close externally provided log_output streams

[jit3pam]

🔗 Related Issues

Fixes #15629

💥 What does this PR do?

This change fixes an issue where the Service.stop() method closes the stream provided via the log_output parameter even when the stream was not created by Selenium.
I could replicate the errors mentioned in #15629.

When users pass streams such as sys.stdout, the current implementation closes the global stream when the service stops. It causes error in new WebDriver instances that try to use the same stream.

The fix introduces tracking of the ownership of the log_output stream.
With this fix Selenium now closes the stream only if it was opened internally (for example when a file path is provided). Streams provided by the user, such as sys.stdout or other file-like objects, will not be closed by the stop method defined in Service class.

I have also added a test to verify that multiple WebDriver instances can be created sequentially using Service(log_output=sys.stdout) without failure.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

• Bug fix (backwards compatible)
• New feature (non-breaking change which adds functionality and tests!)

[qodo-code-review]

Review Summary by Qodo

Prevent closing externally provided log_output streams

🐞 Bug fix

Walkthroughs

Description

• Track ownership of log_output streams to prevent closing externally provided streams
• Only close streams opened internally by Selenium (e.g., file paths)
• Preserve user-provided streams like sys.stdout for reuse across multiple WebDriver instances
• Add test verifying multiple WebDriver instances can use sys.stdout sequentially

File Changes

1. py/selenium/webdriver/common/service.py 🐞 Bug fix +3/-1

Track and respect log_output stream ownership

• Add _owns_log_output flag to track stream ownership in __init__
• Set flag to True only when opening file paths internally
• Modify stop() method to only close streams when _owns_log_output is True
• Preserve user-provided streams (sys.stdout, sys.stderr, etc.) from being closed

py/selenium/webdriver/common/service.py

---

2. py/test/selenium/webdriver/common/test_service_logging.py 🧪 Tests +26/-0

Add test for reusable stdout logging across instances

• Create new test file for service logging functionality
• Test sequential creation of multiple WebDriver instances using sys.stdout
• Verify that sys.stdout can be reused across different Service instances
• Ensure session IDs are valid and cleanup is performed properly

py/test/selenium/webdriver/common/test_service_logging.py

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2) 📎 Requirement gaps (0)

1. Missing blank line before test 📘 Rule violation ✓ Correctness

Description

The new test function is not separated from imports by the expected two blank lines, which can
trigger formatting/lint failures. This risks CI failures under repository lint rules.

Code

py/test/selenium/webdriver/common/test_service_logging.py[R1-8]

+import sys
+import pytest
+
+from selenium import webdriver
+from selenium.webdriver.chrome.service import Service
+
+@pytest.mark.chrome
+def test_service_allows_reusing_stdout_for_logging():

Evidence

The formatting/lint compliance requires changed code to be format/lint clean. The added test starts
immediately after a single blank line following imports, which commonly violates lint rules
expecting two blank lines before top-level definitions.

AGENTS.md
py/test/selenium/webdriver/common/test_service_logging.py[1-8]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test is not separated from the import block by the typical two blank lines required by Python linters/formatters, which can cause CI lint failures.
## Issue Context
Repository compliance requires changed code to be format/lint clean.
## Fix Focus Areas
- py/test/selenium/webdriver/common/test_service_logging.py[1-8]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Test hard-codes Chrome ☑ 🐞 Bug ⛯ Reliability

Description

The new common-suite test always instantiates webdriver.Chrome() directly, so it can run even when
the suite is configured for a different --driver and fail due to missing Chrome/chromedriver. It
also bypasses the suite’s driver_executable and clean_driver fixtures, ignoring the configured
--driver-binary path.

Code

py/test/selenium/webdriver/common/test_service_logging.py[R7-20]

+@pytest.mark.chrome
+def test_service_allows_reusing_stdout_for_logging():
+    browser1 = None
+    browser2 = None
+    try:
+        service1 = Service(log_output=sys.stdout)
+        browser1 = webdriver.Chrome(service=service1) # lazy_import
+        assert browser1.session_id is not None
+        browser1.quit()
+        browser1 = None
+
+        service2 = Service(log_output=sys.stdout)
+        browser2 = webdriver.Chrome(service=service2) # lazy_import
+        assert browser2.session_id is not None

Evidence

Tests under py/test/selenium/webdriver/common are not filtered out by driver selection, and the
suite expects common tests to rely on fixtures (driver, clean_driver, driver_executable) to
respect --driver and executable configuration. The new test bypasses that infrastructure and
hard-codes Chrome.

py/test/selenium/webdriver/common/test_service_logging.py[7-20]
py/conftest.py[165-172]
py/conftest.py[403-420]
py/conftest.py[571-584]
py/test/selenium/webdriver/chrome/chrome_service_tests.py[62-69]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A new test under `py/test/selenium/webdriver/common/` hard-codes `webdriver.Chrome()` and does not use the shared fixtures that honor `--driver` and `--driver-binary`. This can fail when running the suite for non-Chrome drivers and can ignore configured chromedriver paths.
## Issue Context
The suite filters tests by directory name via `pytest_ignore_collect`, and common tests should be driver-agnostic (use the `driver`/`clean_driver` fixtures).
## Fix Focus Areas
- py/test/selenium/webdriver/common/test_service_logging.py[1-26]
### Suggested approach
- Prefer moving this test into the Chrome-specific directory (so it’s only collected when running Chrome):
- Move to `py/test/selenium/webdriver/chrome/chrome_service_tests.py` and implement using `clean_driver`, `clean_options`, and `driver_executable`.
- Or, if keeping it in `common`, skip unless the selected driver is Chrome and still use `driver_executable` for the service executable.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. # lazy_import comment unclear ☑ 📘 Rule violation ✓ Correctness

Description

The added inline # lazy_import comment states what is happening but does not explain why it is
needed. This reduces maintainability and violates the comment guidance.

Code

py/test/selenium/webdriver/common/test_service_logging.py[13]

+        browser1 = webdriver.Chrome(service=service1) # lazy_import

Evidence

The compliance checklist requires comments to explain rationale rather than restating behavior. The
added # lazy_import comment does not provide rationale or constraints for future readers.

AGENTS.md
py/test/selenium/webdriver/common/test_service_logging.py[13-13]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The inline `# lazy_import` comment does not explain rationale; it reads like a restatement and may confuse maintainers.
## Issue Context
Comments should capture intent/constraints (the "why"), not just describe what the code is doing.
## Fix Focus Areas
- py/test/selenium/webdriver/common/test_service_logging.py[13-13]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Unknown pytest marker ☑ 🐞 Bug ⛯ Reliability

Description

The new test uses @pytest.mark.chrome, but this marker is not registered in the repo’s pytest
configuration, so it will produce an unknown-mark warning (and could fail if strict marker
enforcement is enabled).

Code

py/test/selenium/webdriver/common/test_service_logging.py[R1-8]

+import sys
+import pytest
+
+from selenium import webdriver
+from selenium.webdriver.chrome.service import Service
+
+@pytest.mark.chrome
+def test_service_allows_reusing_stdout_for_logging():

Evidence

Repo pytest markers are explicitly listed in pyproject.toml and do not include chrome, so this
mark is not a recognized marker in this suite.

py/test/selenium/webdriver/common/test_service_logging.py[1-8]
py/pyproject.toml[85-105]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`@pytest.mark.chrome` is used but not registered in `[tool.pytest].markers`, leading to unknown-marker warnings.
## Issue Context
This suite relies primarily on directory-based collection filtering (`pytest_ignore_collect`) and the registered markers listed in `py/pyproject.toml`.
## Fix Focus Areas
- py/test/selenium/webdriver/common/test_service_logging.py[1-8]
### Suggested approach
- Remove `@pytest.mark.chrome` and instead scope by moving the test under `py/test/selenium/webdriver/chrome/`.
- If a marker is desired, add an appropriate registered marker (or register the new marker in `py/pyproject.toml`), but directory scoping is the existing pattern.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[CLAassistant]

All committers have signed the CLA.

[cgoldberg]

Thanks... looks good... just a few minor things:

1. Can you remove the blank lines in your test, so it looks like other tests in that file?
2. The CI Lint job is going to fail on this due to some minor formatting issues. You can fix them by running ./scripts/format.sh (if you have bazel setup)... or tox -c ./py/tox.ini -e linting (pip install tox if you need it)
3. Since this affects the base Service class, we should probably test it on other browsers. Can you add a similar test to:
   • ./py/selenium/webdriver/edge/edge_service_tests.py
   • ./py/selenium/webdriver/firefox/firefox_service_tests.py
   • ./py/selenium/webdriver/safari/safari_service_tests.py

[jit3pam]

@cgoldberg I have added more tests and also followed linting suggestions using tox.
Please review and let me know if more changes are required.
Thank you in advance.

[cgoldberg]

few more tiny changes...

Also, I think it was better the way you were quitting the browser in a finally block in your previous version... Can you re-add that?

[jit3pam]

few more tiny changes...

Also, I think it was better the way you were quitting the browser in a finally block in your previous version... Can you re-add that?

I have updated the test methods with finally block. Infact I have restored the previous version.
I have also added the same test method for firefox, safari, and edge.

[cgoldberg]

@jit3pam LGTM 👍

Thanks for being responsive with your updates. I'll merge this once CI runs.

[cgoldberg]

The failure in CI is unrelated.. all the Python tests ran fine. Merging now.