[py] improve driver logging (#12103)

[py] implement log_output() for flexibility and consistency of driver logging

Co-authored-by: Diego Molina <diemol@users.noreply.github.com>

py/selenium/types.py

@@ -23,4 +23,4 @@
 WaitExcTypes = typing.Iterable[typing.Type[Exception]]
 
 # Service Types
-SubprocessStdAlias = typing.Union[int, typing.IO[typing.Any]]
+SubprocessStdAlias = typing.Union[int, str, typing.IO[typing.Any]]

py/selenium/webdriver/chrome/service.py

@@ -16,6 +16,7 @@
 # under the License.
 import typing
 
+from selenium.types import SubprocessStdAlias
 from selenium.webdriver.chromium import service
 
 DEFAULT_EXECUTABLE_PATH = "chromedriver"
@@ -38,6 +39,7 @@ def __init__(
         port: int = 0,
         service_args: typing.Optional[typing.List[str]] = None,
         log_path: typing.Optional[str] = None,
+        log_output: SubprocessStdAlias = None,
         env: typing.Optional[typing.Mapping[str, str]] = None,
         **kwargs,
     ) -> None:
@@ -46,6 +48,7 @@ def __init__(
             port=port,
             service_args=service_args,
             log_path=log_path,
+            log_output=log_output,
             env=env,
             start_error_message="Please see https://chromedriver.chromium.org/home",
             **kwargs,

py/selenium/webdriver/chromium/service.py

@@ -15,7 +15,10 @@
 # specific language governing permissions and limitations
 # under the License.
 import typing
+import warnings
 
+from selenium.common import InvalidArgumentException
+from selenium.types import SubprocessStdAlias
 from selenium.webdriver.common import service
 
 
@@ -38,18 +41,30 @@ def __init__(
         port: int = 0,
         service_args: typing.Optional[typing.List[str]] = None,
         log_path: typing.Optional[str] = None,
+        log_output: SubprocessStdAlias = None,
         env: typing.Optional[typing.Mapping[str, str]] = None,
         start_error_message: typing.Optional[str] = None,
         **kwargs,
     ) -> None:
         self.service_args = service_args or []
-        if log_path:
-            self.service_args.append(f"--log-path={log_path}")
+        self.log_output = log_output
+        if log_path is not None:
+            warnings.warn("log_path has been deprecated, please use log_output", DeprecationWarning, stacklevel=2)
+            self.log_output = log_path
+
+        if "--append-log" in self.service_args or "--readable-timestamp" in self.service_args:
+            if isinstance(self.log_output, str):
+                self.service_args.append(f"--log-path={self.log_output}")
+                self.log_output = None
+            else:
+                msg = "Appending logs and readable timestamps require log output to be a string representing file path"
+                raise InvalidArgumentException(msg)
 
         super().__init__(
             executable=executable_path,
             port=port,
             env=env,
+            log_output=self.log_output,
             start_error_message=start_error_message,
             **kwargs,
         )

py/selenium/webdriver/common/service.py

@@ -19,6 +19,7 @@
 import os
 import subprocess
 import typing
+import warnings
 from abc import ABC
 from abc import abstractmethod
 from platform import system
@@ -51,14 +52,27 @@ def __init__(
         self,
         executable: str,
         port: int = 0,
-        log_file: SubprocessStdAlias = DEVNULL,
+        log_file: SubprocessStdAlias = None,
+        log_output: SubprocessStdAlias = None,
         env: typing.Optional[typing.Mapping[typing.Any, typing.Any]] = None,
         start_error_message: typing.Optional[str] = None,
         **kwargs,
     ) -> None:
+        if isinstance(log_output, str):
+            self.log_output = open(log_output, "a+", encoding="utf-8")
+        elif log_output is subprocess.STDOUT:
+            self.log_output = None
+        elif log_output is None or log_output is subprocess.DEVNULL:
+            self.log_output = open(os.devnull, "wb")
+        else:
+            self.log_output = log_output
+
+        if log_file is not None:
+            warnings.warn("log_file has been deprecated, please use log_output", DeprecationWarning, stacklevel=2)
+            self.log_output = open(log_file, "a+", encoding="utf-8")
+
         self._path = executable
         self.port = port or utils.free_port()
-        self.log_file = open(os.devnull, "wb") if not log_file == DEVNULL else log_file
         self.start_error_message = start_error_message or ""
         # Default value for every python subprocess: subprocess.Popen(..., creationflags=0)
         self.popen_kw = kwargs.pop("popen_kw", {})
@@ -129,10 +143,10 @@ def send_remote_shutdown_command(self) -> None:
 
     def stop(self) -> None:
         """Stops the service."""
-        if self.log_file != PIPE and not (self.log_file == DEVNULL):
+        if self.log_output != PIPE and not (self.log_output == DEVNULL):
             try:
                 # Todo: Be explicit in what we are catching here.
-                if hasattr(self.log_file, "close"):
+                if hasattr(self.log_output, "close"):
                     self.log_file.close()  # type: ignore
             except Exception:
                 pass
@@ -195,8 +209,8 @@ def _start_process(self, path: str) -> None:
                 cmd,
                 env=self.env,
                 close_fds=close_file_descriptors,
-                stdout=self.log_file,
-                stderr=self.log_file,
+                stdout=self.log_output,
+                stderr=self.log_output,
                 stdin=PIPE,
                 creationflags=self.creation_flags,
                 **self.popen_kw,

py/selenium/webdriver/edge/service.py

@@ -17,6 +17,7 @@
 import typing
 import warnings
 
+from selenium.types import SubprocessStdAlias
 from selenium.webdriver.chromium import service
 
 DEFAULT_EXECUTABLE_PATH = "msedgedriver"
@@ -41,6 +42,7 @@ def __init__(
         port: int = 0,
         verbose: bool = False,
         log_path: typing.Optional[str] = None,
+        log_output: SubprocessStdAlias = None,
         service_args: typing.Optional[typing.List[str]] = None,
         env: typing.Optional[typing.Mapping[str, str]] = None,
         **kwargs,
@@ -60,6 +62,7 @@ def __init__(
             port=port,
             service_args=service_args,
             log_path=log_path,
+            log_output=log_output,
             env=env,
             start_error_message="Please download from https://developer.microsoft.com/en-us/microsoft-edge/tools/webdriver/",
             **kwargs,

py/selenium/webdriver/firefox/service.py

@@ -15,8 +15,10 @@
 # specific language governing permissions and limitations
 # under the License.
 import typing
+import warnings
 from typing import List
 
+from selenium.types import SubprocessStdAlias
 from selenium.webdriver.common import service
 from selenium.webdriver.common import utils
 
@@ -41,17 +43,27 @@ def __init__(
         port: int = 0,
         service_args: typing.Optional[typing.List[str]] = None,
         log_path: typing.Optional[str] = None,
+        log_output: SubprocessStdAlias = None,
         env: typing.Optional[typing.Mapping[str, str]] = None,
         **kwargs,
     ) -> None:
-        # Todo: This is vastly inconsistent, requires a follow up to standardise.
-        file = log_path or "geckodriver.log"
-        log_file = open(file, "a+", encoding="utf-8")
         self.service_args = service_args or []
+        if log_path is not None:
+            warnings.warn("log_path has been deprecated, please use log_output", DeprecationWarning, stacklevel=2)
+            log_output = open(log_path, "a+", encoding="utf-8")
+
+        if log_path is None and log_output is None:
+            warnings.warn(
+                "Firefox will soon stop logging to geckodriver.log by default; Specify desired logs with log_output",
+                DeprecationWarning,
+                stacklevel=2,
+            )
+            log_output = open("geckodriver.log", "a+", encoding="utf-8")
+
         super().__init__(
             executable=executable_path,
             port=port,
-            log_file=log_file,
+            log_output=log_output,
             env=env,
             **kwargs,
         )

py/selenium/webdriver/ie/service.py

@@ -15,8 +15,10 @@
 # specific language governing permissions and limitations
 # under the License.
 import typing
+import warnings
 from typing import List
 
+from selenium.types import SubprocessStdAlias
 from selenium.webdriver.common import service
 
 DEFAULT_EXECUTABLE_PATH = "IEDriverServer.exe"
@@ -31,6 +33,7 @@ def __init__(
         port: int = 0,
         host: typing.Optional[str] = None,
         log_level: typing.Optional[str] = None,
+        log_output: SubprocessStdAlias = None,
         log_file: typing.Optional[str] = None,
         **kwargs,
     ) -> None:
@@ -51,11 +54,13 @@ def __init__(
         if log_level:
             self.service_args.append(f"--log-level={log_level}")
         if log_file:
+            warnings.warn("log_file has been deprecated, please use log_output", DeprecationWarning, stacklevel=2)
             self.service_args.append(f"--log-file={log_file}")
 
         super().__init__(
             executable_path,
             port=port,
+            log_output=log_output,
             start_error_message="Please download from https://www.selenium.dev/downloads/ and read up at https://github.com/SeleniumHQ/selenium/wiki/InternetExplorerDriver",
             **kwargs,
         )

py/selenium/webdriver/safari/service.py

@@ -16,8 +16,8 @@
 # under the License.
 
 import os
-import subprocess
 import typing
+import warnings
 
 from selenium.webdriver.common import service
 
@@ -30,7 +30,7 @@ class Service(service.Service):
 
     :param executable_path: install path of the safaridriver executable, defaults to `/usr/bin/safaridriver`.
     :param port: Port for the service to run on, defaults to 0 where the operating system will decide.
-    :param quiet: Suppress driver stdout & stderr, redirects to os.devnull if enabled.
+    :param quiet: (Deprecated) Suppress driver stdout & stderr, redirects to os.devnull if enabled.
     :param service_args: (Optional) List of args to be passed to the subprocess when launching the executable.
     :param env: (Optional) Mapping of environment variables for the new process, defaults to `os.environ`.
     """
@@ -39,21 +39,21 @@ def __init__(
         self,
         executable_path: str = DEFAULT_EXECUTABLE_PATH,
         port: int = 0,
-        quiet: bool = False,
+        quiet: bool = None,
         service_args: typing.Optional[typing.List[str]] = None,
         env: typing.Optional[typing.Mapping[str, str]] = None,
         reuse_service=False,
         **kwargs,
     ) -> None:
         self._check_executable(executable_path)
         self.service_args = service_args or []
-        self.quiet = quiet
+        if quiet is not None:
+            warnings.warn("quiet is no longer needed to supress output", DeprecationWarning, stacklevel=2)
+
         self._reuse_service = reuse_service
-        log_file = subprocess.PIPE if not self.quiet else open(os.devnull, "w", encoding="utf-8")
         super().__init__(
             executable=executable_path,
             port=port,
-            log_file=log_file,  # type: ignore
             env=env,
             **kwargs,
         )

py/test/selenium/webdriver/chrome/chrome_service_tests.py

@@ -0,0 +1,94 @@
+# Licensed to the Software Freedom Conservancy (SFC) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The SFC licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import os
+import subprocess
+import time
+
+import pytest
+
+from selenium.webdriver import Chrome
+from selenium.webdriver.chrome.service import Service
+
+
+def test_log_path_deprecated() -> None:
+    log_path = "chromedriver.log"
+    msg = "log_path has been deprecated, please use log_output"
+
+    with pytest.warns(match=msg, expected_warning=DeprecationWarning):
+        Service(log_path=log_path)
+
+
+def test_uses_chromedriver_logging() -> None:
+    log_file = "chromedriver.log"
+    service_args = ["--append-log"]
+
+    service = Service(log_output=log_file, service_args=service_args)
+    try:
+        driver1 = Chrome(service=service)
+        with open(log_file, "r") as fp:
+            lines = len(fp.readlines())
+        driver2 = Chrome(service=service)
+        with open(log_file, "r") as fp:
+            assert len(fp.readlines()) >= 2 * lines
+    finally:
+        driver1.quit()
+        driver2.quit()
+        os.remove(log_file)
+
+
+def test_log_output_as_filename() -> None:
+    log_file = "chromedriver.log"
+    service = Service(log_output=log_file)
+    try:
+        driver = Chrome(service=service)
+        with open(log_file, "r") as fp:
+            assert "Starting ChromeDriver" in fp.readline()
+    finally:
+        driver.quit()
+        os.remove(log_file)
+
+
+def test_log_output_as_file() -> None:
+    log_name = "chromedriver.log"
+    log_file = open(log_name, "w", encoding="utf-8")
+    service = Service(log_output=log_file)
+    try:
+        driver = Chrome(service=service)
+        time.sleep(1)
+        with open(log_name, "r") as fp:
+            assert "Starting ChromeDriver" in fp.readline()
+    finally:
+        driver.quit()
+        log_file.close()
+        os.remove(log_name)
+
+
+def test_log_output_as_stdout(capfd) -> None:
+    service = Service(log_output=subprocess.STDOUT)
+    driver = Chrome(service=service)
+
+    out, err = capfd.readouterr()
+    assert "Starting ChromeDriver" in out
+    driver.quit()
+
+
+def test_log_output_null_default(capfd) -> None:
+    driver = Chrome()
+
+    out, err = capfd.readouterr()
+    assert "Starting ChromeDriver" not in out
+    driver.quit()