change error and log handling and add tests

py/selenium/webdriver/common/driver_finder.py

@@ -50,17 +50,17 @@ def get_path(service: Service, options: BaseOptions) -> str:
     @staticmethod
     def get_results(service: Service, options: BaseOptions) -> dict:
         path = service.path
-
-        if path is None:
-            try:
-                return SeleniumManager().results(DriverFinder._to_args(options))
-            except Exception as err:
-                msg = f"Unable to obtain driver for {options.capabilities['browserName']} using Selenium Manager."
-                raise NoSuchDriverException(msg) from err
-        elif not Path(path).is_file():
-            raise NoSuchDriverException(f"Provided path for {options.capabilities['browserName']} is invalid")
-        else:
-            return {"driver_path": path}
+        try:
+            if path:
+                logger.debug("Skipping Selenium Manager and using provided driver path: %s", path)
+                results = {"driver_path": service.path}
+            else:
+                results = SeleniumManager().results(DriverFinder._to_args(options))
+            DriverFinder._validate_results(results)
+            return results
+        except Exception as err:
+            msg = f"Unable to obtain driver for {options.capabilities['browserName']}."
+            raise NoSuchDriverException(msg) from err
 
     @staticmethod
     def _to_args(options: BaseOptions) -> list:
@@ -84,3 +84,10 @@ def _to_args(options: BaseOptions) -> list:
             args.append(value)
 
         return args
+
+    @staticmethod
+    def _validate_results(results):
+        for key, file_path in results.items():
+            if not Path(file_path).is_file():
+                raise ValueError(f"The path for '{key}' is not a valid file: {file_path}")
+        return True

py/selenium/webdriver/common/selenium_manager.py

@@ -46,29 +46,30 @@ def get_binary() -> Path:
         """
 
         if (path := os.getenv("SE_MANAGER_PATH")) is not None:
-            return Path(path)
+            logger.debug("Selenium Manager set by env SE_MANAGER_PATH to: %s", path)
+            path = Path(path)
+        else:
+            dirs = {
+                ("darwin", "any"): "macos",
+                ("win32", "any"): "windows",
+                ("cygwin", "any"): "windows",
+                ("linux", "x86_64"): "linux",
+                ("freebsd", "x86_64"): "linux",
+                ("openbsd", "x86_64"): "linux",
+            }
 
-        dirs = {
-            ("darwin", "any"): "macos",
-            ("win32", "any"): "windows",
-            ("cygwin", "any"): "windows",
-            ("linux", "x86_64"): "linux",
-            ("freebsd", "x86_64"): "linux",
-            ("openbsd", "x86_64"): "linux",
-        }
+            arch = platform.machine() if sys.platform in ("linux", "freebsd", "openbsd") else "any"
 
-        arch = platform.machine() if sys.platform in ("linux", "freebsd", "openbsd") else "any"
+            directory = dirs.get((sys.platform, arch))
+            if directory is None:
+                raise WebDriverException(f"Unsupported platform/architecture combination: {sys.platform}/{arch}")
 
-        directory = dirs.get((sys.platform, arch))
-        if directory is None:
-            raise WebDriverException(f"Unsupported platform/architecture combination: {sys.platform}/{arch}")
+            if sys.platform in ["freebsd", "openbsd"]:
+                logger.warning("Selenium Manager binary may not be compatible with %s; verify settings", sys.platform)
 
-        if sys.platform in ["freebsd", "openbsd"]:
-            logger.warning("Selenium Manager binary may not be compatible with %s; verify settings", sys.platform)
+            file = "selenium-manager.exe" if directory == "windows" else "selenium-manager"
 
-        file = "selenium-manager.exe" if directory == "windows" else "selenium-manager"
-
-        path = Path(__file__).parent.joinpath(directory, file)
+            path = Path(__file__).parent.joinpath(directory, file)
 
         if not path.is_file():
             raise WebDriverException(f"Unable to obtain working Selenium Manager binary; {path}")
@@ -93,15 +94,7 @@ def results(self, args: List) -> dict:
         args.append("--output")
         args.append("json")
 
-        output = self.run(args)
-
-        driver_path = output["driver_path"]
-        if driver_path is None:
-            raise ValueError("No driver path was returned.")
-        elif not Path(driver_path).is_file():
-            raise FileNotFoundError(f"Driver path returned but is invalid: {driver_path}")
-
-        return output
+        return self.run(args)
 
     @deprecated(reason="Use results() function with argument list instead.")
     def driver_location(self, options: BaseOptions) -> str:
@@ -165,12 +158,15 @@ def run(args: List[str]) -> dict:
         except Exception as err:
             raise WebDriverException(f"Unsuccessful command executed: {command}") from err
 
-        for item in output["logs"]:
-            if item["level"] == "WARN":
-                logger.warning(item["message"])
-            if item["level"] == "DEBUG" or item["level"] == "INFO":
-                logger.debug(item["message"])
-
+        SeleniumManager.process_logs(output["logs"])
         if completed_proc.returncode:
             raise WebDriverException(f"Unsuccessful command executed: {command}.\n{result}{stderr}")
         return result
+
+    @staticmethod
+    def process_logs(log_items: List[dict]):
+        for item in log_items:
+            if item["level"] == "WARN":
+                logger.warning(item["message"])
+            elif item["level"] in ["DEBUG", "INFO"]:
+                logger.debug(item["message"])

py/test/selenium/webdriver/common/driver_finder_tests.py

@@ -0,0 +1,65 @@
+# 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.
+from pathlib import Path
+from unittest import mock
+
+import pytest
+
+from selenium import webdriver
+from selenium.common.exceptions import NoSuchDriverException
+from selenium.webdriver.common.driver_finder import DriverFinder
+
+
+def test_get_results_with_valid_path():
+    options = webdriver.ChromeOptions()
+    service = webdriver.ChromeService(executable_path="/valid/path/to/driver")
+
+    with mock.patch.object(Path, 'is_file', return_value=True):
+        result = DriverFinder.get_results(service, options)
+    assert result == {"driver_path": "/valid/path/to/driver"}
+
+
+def test_errors_with_invalid_path():
+    options = webdriver.ChromeOptions()
+    service = webdriver.ChromeService(executable_path="/invalid/path/to/driver")
+
+    with mock.patch.object(Path, 'is_file', return_value=False):
+        with pytest.raises(NoSuchDriverException) as excinfo:
+            DriverFinder.get_results(service, options)
+        assert "Provided path for chrome is invalid; For documentation on this error" in str(excinfo.value)
+
+
+def test_wraps_error_from_se_manager():
+    options = webdriver.ChromeOptions()
+    service = webdriver.ChromeService(executable_path="/valid/path/to/driver")
+
+    lib_path = "selenium.webdriver.common.selenium_manager.SeleniumManager"
+    with mock.patch(lib_path + '.results', side_effect=Exception("Error")):
+        with pytest.raises(NoSuchDriverException):
+            DriverFinder.get_results(service, options)
+
+
+def test_get_results_from_se_manager(monkeypatch):
+    options = webdriver.ChromeOptions()
+    service = webdriver.ChromeService(executable_path="/invalid/path/to/driver")
+    expected_output = {"driver_path": "/invalid/path/to/driver"}
+    monkeypatch.setattr(Path, "is_file", lambda _: True)
+
+    lib_path = "selenium.webdriver.common.selenium_manager.SeleniumManager"
+    with mock.patch(lib_path + '.results', return_value=expected_output):
+        result = DriverFinder.get_results(service, options)
+    assert result == expected_output

py/test/selenium/webdriver/common/selenium_manager_tests.py

@@ -14,88 +14,118 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
-
-from unittest.mock import Mock
+import json
+import sys
+from pathlib import Path
+from unittest import mock
 
 import pytest
 
+import selenium
 from selenium.common.exceptions import WebDriverException
-from selenium.webdriver.chrome.options import Options
-from selenium.webdriver.common.proxy import Proxy
 from selenium.webdriver.common.selenium_manager import SeleniumManager
 
 
-def test_browser_version_is_used_for_sm(mocker):
-    import subprocess
-
-    mock_run = mocker.patch("subprocess.run")
-    mocked_result = Mock()
-    mocked_result.configure_mock(
-        **{
-            "stdout.decode.return_value": '{"result": {"driver_path": "driver", "browser_path": "browser"}, "logs": []}',
-            "returncode": 0,
-        }
-    )
-    mock_run.return_value = mocked_result
-    options = Options()
-    options.capabilities["browserName"] = "chrome"
-    options.browser_version = "110"
-
-    _ = SeleniumManager().driver_location(options)
-    args, kwargs = subprocess.run.call_args
-    assert "--browser-version" in args[0]
-    assert "110" in args[0]
-
-
-def test_browser_path_is_used_for_sm(mocker):
-    import subprocess
-
-    mock_run = mocker.patch("subprocess.run")
-    mocked_result = Mock()
-    mocked_result.configure_mock(
-        **{
-            "stdout.decode.return_value": '{"result": {"driver_path": "driver", "browser_path": "browser"}, "logs": []}',
-            "returncode": 0,
-        }
-    )
-    mock_run.return_value = mocked_result
-    options = Options()
-    options.capabilities["browserName"] = "chrome"
-    options.binary_location = "/opt/bin/browser-bin"
-
-    _ = SeleniumManager().driver_location(options)
-    args, kwargs = subprocess.run.call_args
-    assert "--browser-path" in args[0]
-    assert "/opt/bin/browser-bin" in args[0]
-
-
-def test_proxy_is_used_for_sm(mocker):
-    import subprocess
-
-    mock_run = mocker.patch("subprocess.run")
-    mocked_result = Mock()
-    mocked_result.configure_mock(
-        **{
-            "stdout.decode.return_value": '{"result": {"driver_path": "driver", "browser_path": "browser"}, "logs": []}',
-            "returncode": 0,
-        }
-    )
-    mock_run.return_value = mocked_result
-    options = Options()
-    options.capabilities["browserName"] = "chrome"
-    proxy = Proxy()
-    proxy.http_proxy = "http-proxy"
-    options.proxy = proxy
-
-    _ = SeleniumManager().driver_location(options)
-    args, kwargs = subprocess.run.call_args
-    assert "--proxy" in args[0]
-    assert "http-proxy" in args[0]
-
-
-def test_stderr_is_propagated_to_exception_messages():
-    msg = r"Unsuccessful command executed:.*\n.* 'Invalid browser name: foo'.*"
-    with pytest.raises(WebDriverException, match=msg):
-        manager = SeleniumManager()
-        binary = manager.get_binary()
-        _ = manager.run([str(binary), "--browser", "foo"])
+def test_gets_results(monkeypatch):
+    expected_output = {"driver_path": "/path/to/driver"}
+    lib_path = "selenium.webdriver.common.selenium_manager.SeleniumManager"
+
+    with mock.patch(lib_path + ".get_binary", return_value="/path/to/sm") as mock_get_binary, \
+         mock.patch(lib_path + ".run", return_value=expected_output) as mock_run:
+        SeleniumManager().results([])
+
+        mock_get_binary.assert_called_once()
+        expected_run_args = ["/path/to/sm", "--language-binding", "python", "--output", "json"]
+        mock_run.assert_called_once_with(expected_run_args)
+
+
+def test_uses_environment_variable(monkeypatch):
+    monkeypatch.setenv("SE_MANAGER_PATH", "/path/to/manager")
+    monkeypatch.setattr(Path, "is_file", lambda _: True)
+
+    binary = SeleniumManager().get_binary()
+
+    assert str(binary) == "/path/to/manager"
+
+
+def test_uses_windows(monkeypatch):
+    monkeypatch.setattr(sys, "platform", "win32")
+    binary = SeleniumManager().get_binary()
+
+    project_root = Path(selenium.__file__).parent.parent
+    assert binary == project_root.joinpath("selenium/webdriver/common/windows/selenium-manager.exe")
+
+
+def test_uses_linux(monkeypatch):
+    monkeypatch.setattr(sys, "platform", "linux")
+    binary = SeleniumManager().get_binary()
+
+    project_root = Path(selenium.__file__).parent.parent
+    assert binary == project_root.joinpath("selenium/webdriver/common/linux/selenium-manager")
+
+
+def test_uses_mac(monkeypatch):
+    monkeypatch.setattr(sys, "platform", "darwin")
+    binary = SeleniumManager().get_binary()
+
+    project_root = Path(selenium.__file__).parent.parent
+    assert binary == project_root.joinpath("selenium/webdriver/common/macos/selenium-manager")
+
+
+def test_errors_if_not_file(monkeypatch):
+    monkeypatch.setattr(Path, "is_file", lambda _: False)
+
+    with pytest.raises(WebDriverException) as excinfo:
+        SeleniumManager().get_binary()
+    assert "Unable to obtain working Selenium Manager binary" in str(excinfo.value)
+
+
+def test_errors_if_invalid_os(monkeypatch):
+    monkeypatch.setattr(sys, "platform", "linux")
+    monkeypatch.setattr("platform.machine", lambda: "invalid")
+
+    with pytest.raises(WebDriverException) as excinfo:
+        SeleniumManager().get_binary()
+    assert "Unsupported platform/architecture combination" in str(excinfo.value)
+
+
+def test_error_if_invalid_env_path(monkeypatch):
+    monkeypatch.setenv("SE_MANAGER_PATH", "/path/to/manager")
+
+    with pytest.raises(WebDriverException) as excinfo:
+        SeleniumManager().get_binary()
+    assert "Unable to obtain working Selenium Manager binary; /path/to/manager" in str(excinfo.value)
+
+
+def test_warns_if_unix(monkeypatch, capsys):
+    monkeypatch.setattr(sys, "platform", "freebsd")
+
+    SeleniumManager().get_binary()
+
+    assert "Selenium Manager binary may not be compatible with freebsd" in capsys.readouterr().err
+
+
+def test_run_successful():
+    expected_result = {"driver_path": "/path/to/driver", "browser_path": "/path/to/browser"}
+    run_output = {"result": expected_result, "logs": []}
+    with mock.patch("subprocess.run") as mock_run, \
+         mock.patch("json.loads", return_value=run_output):
+        mock_run.return_value = mock.Mock(stdout=json.dumps(run_output).encode('utf-8'), stderr=b"", returncode=0)
+        result = SeleniumManager.run(["arg1", "arg2"])
+        assert result == expected_result
+
+
+def test_run_exception():
+    with mock.patch("subprocess.run", side_effect=Exception("Test Error")):
+        with pytest.raises(WebDriverException) as excinfo:
+            SeleniumManager.run(["/path/to/sm", "arg1", "arg2"])
+    assert "Unsuccessful command executed: /path/to/sm arg1 arg2" in str(excinfo.value)
+
+
+def test_run_non_zero_exit_code():
+    with mock.patch("subprocess.run") as mock_run, \
+         mock.patch("json.loads", return_value={"result": "", "logs": []}):
+        mock_run.return_value = mock.Mock(stdout=b'{}', stderr=b'Error Message', returncode=1)
+        with pytest.raises(WebDriverException) as excinfo:
+            SeleniumManager.run(["/path/to/sm", "arg1"])
+    assert "Message: Unsuccessful command executed: /path/to/sm arg1.\nError Message\n" in str(excinfo.value)