andrew/improve system diagnostics

.pre-commit-config.yaml

@@ -18,12 +18,12 @@ repos:
       - id: check-toml
       - id: debug-statements
   - repo: https://github.com/asottile/add-trailing-comma
-    rev: v3.2.0
+    rev: v4.0.0
     hooks:
       - id: add-trailing-comma
   - repo: https://github.com/astral-sh/ruff-pre-commit
     #  Ruff version.
-    rev: v0.13.3
+    rev: v0.14.4
     hooks:
       #  Run the linter.
       - id: ruff
@@ -48,7 +48,7 @@ repos:
           }\
         }"]
   - repo: https://github.com/rhysd/actionlint
-    rev: v1.7.7
+    rev: v1.7.8
     hooks:
       - id: actionlint
         name: Lint GitHub Actions workflow files
@@ -81,14 +81,14 @@ repos:
         entry: detect-secrets-hook
         args: ['']
   - repo: https://github.com/rvben/rumdl-pre-commit
-    rev: v0.0.153  # Use the latest release tag
+    rev: v0.0.173  # Use the latest release tag
     hooks:
       - id: rumdl
         # To only check (default):
         # args: []
         # To automatically fix issues:
         # args: [--fix]
   - repo: https://github.com/RobertCraigie/pyright-python
-    rev: v1.1.406   # pin a tag; latest as of 2025-10-01
+    rev: v1.1.407   # pin a tag; latest as of 2025-10-01
     hooks:
       - id: pyright

CHANGELOG.txt

@@ -1,3 +1,8 @@
+- Improve error messaging
+- Organize functions internally
+- Improve choreo_diagnose
+- Add Roadmap
+- Bump logistro dependency
 v1.2.0
 - Delete zipfile after downloading
 - Upgrade logistro to reduce sideeffects

ROADMAP.md

@@ -0,0 +1,17 @@
+# Roadmap
+
+- [ ] Working on better diagnostic information
+  - [ ] Explain to user when their system has security restrictions
+- [ ] Eliminate synchronous API: it's unused, hard to maintain, and nearly
+worthless
+- [ ] Diagnose function should collect a JSON and then print that
+- [ ] Allow user to build and send their own JSONS
+  - [ ] Get serialization out of the lock
+- [ ]
+- [ ] Support Firefox
+- [ ] Support LadyBird (!)
+- [ ] Test against multiple docker containers
+- [ ] Do browser-rolling tests
+- [ ] Do documentation
+- [ ] Browser Open/Close Status/PipeCheck status should happen at broker level
+  - [ ] Broker should probably be opening browser and running watchdog...

choreographer/browser_async.py

@@ -169,7 +169,11 @@ def run() -> subprocess.Popen[bytes] | subprocess.Popen[str]:  # depends on args
                 "The browser seemed to close immediately after starting.",
                 "You can set the `logging.Logger` level lower to see more output.",
                 "You may try installing a known working copy of Chrome by running ",
-                "`$ choreo_get_chrome`. It may be your copy auto-updated.",
+                "`$ choreo_get_chrome`."
+                ""
+                "It may be your browser auto-updated and will now work upon "
+                "restart. The browser we tried to start is located at "
+                f"{self._browser_impl.path}.",
             ) from e
 
     async def __aenter__(self) -> Self:

choreographer/browsers/_interface_type.py

@@ -15,6 +15,20 @@
 class BrowserImplInterface(Protocol):
     """Defines the basic interface of a channel."""
 
+    path: str | Path | None
+    """The OS path to the operating system."""
+
+    @classmethod
+    def find_browser(
+        cls,
+        *,
+        skip_local: bool,
+        skip_typical: bool = False,
+    ) -> str | None: ...
+
+    ### Tries to find a working copy of itself, using our OS methods
+    ### as well as its own methods. See the Chromium implementation.
+
     @classmethod
     def logger_parser(
         cls,

choreographer/browsers/chromium.py

@@ -42,28 +42,6 @@ def _is_exe(path: str | Path) -> bool:
         return False
 
 
-def _find_a_chromium_based_browser(
-    *,
-    skip_local: bool,
-    skip_typical: bool = False,
-) -> str | None:
-    for name, browser_data in chromium_based_browsers.items():
-        _logger.debug(f"Looking for a {name} browser.")
-        path = get_browser_path(
-            executable_names=browser_data.exe_names,
-            skip_local=skip_local,
-            ms_prog_id=browser_data.ms_prog_id,
-        )
-        if not path and not skip_typical:
-            for candidate in browser_data.typical_paths:
-                if _is_exe(candidate):
-                    path = candidate
-                    break
-        if path:
-            return path
-    return None
-
-
 _logs_parser_regex = re.compile(r"\d*:\d*:\d*\/\d*\.\d*:")
 
 
@@ -91,6 +69,30 @@ class Chromium:
     tmp_dir: TmpDirectory
     """A reference to a temporary directory object the chromium needs to store data."""
 
+    @classmethod
+    def find_browser(
+        cls,
+        *,
+        skip_local: bool,
+        skip_typical: bool = False,
+    ) -> str | None:
+        """Find a chromium based browser."""
+        for name, browser_data in chromium_based_browsers.items():
+            _logger.debug(f"Looking for a {name} browser.")
+            path = get_browser_path(
+                executable_names=browser_data.exe_names,
+                skip_local=skip_local,
+                ms_prog_id=browser_data.ms_prog_id,
+            )
+            if not path and not skip_typical:
+                for candidate in browser_data.typical_paths:
+                    if _is_exe(candidate):
+                        path = candidate
+                        break
+            if path:
+                return path
+        return None
+
     @classmethod
     def logger_parser(
         cls,
@@ -197,11 +199,12 @@ def __init__(
             )
 
         if not self.path:
-            self.path = _find_a_chromium_based_browser(skip_local=self.skip_local)
+            self.path = Chromium.find_browser(skip_local=self.skip_local)
         if not self.path:
             raise ChromeNotFoundError(
-                "Browser not found. You can use get_chrome(), "
-                "please see documentation.",
+                "Browser not found. You can use get_chrome() or "
+                "choreo_get_chrome from bash. please see documentation. "
+                f"Local copy ignored: {self.skip_local}.",
             )
         _logger.info(f"Found chromium path: {self.path}")
 

choreographer/cli/_cli_utils_no_qa.py

@@ -28,10 +28,9 @@
 
 
 def diagnose() -> None:
-    logistro.getLogger().setLevel(6)
-    logistro.getLogger().error("Test")
+    logistro.betterConfig(level=1)
     from choreographer import Browser, BrowserSync
-    from choreographer.browsers.chromium import _find_a_chromium_based_browser
+    from choreographer.browsers.chromium import Chromium
     from choreographer.utils._which import browser_which
 
     parser = argparse.ArgumentParser(
@@ -52,42 +51,52 @@ def diagnose() -> None:
     print(platform.release())
     print(platform.version())
     print(platform.uname())
+    print("*".center(50, "*"))
     print("BROWSER:".center(50, "*"))
     try:
-        print("Found local: {browser_which(verify_local=True)}")
+        print(f"Found local: {browser_which([], verify_local=True)}")
     except RuntimeError:
         print("Didn't find local.")
-    browser_path = _find_a_chromium_based_browser(skip_local=True)
+    browser_path = Chromium.find_browser(skip_local=True)
     print(browser_path)
+    print("*".center(50, "*"))
     print("BROWSER_INIT_CHECK (DEPS)".center(50, "*"))
     if not browser_path:
         print("No browser, found can't check for deps.")
     else:
         b = Browser()
         b._browser_impl.pre_open()
         cli = b._browser_impl.get_cli()
-        env = b._browser_impl.get_env()
+        env = b._browser_impl.get_env()  # noqa: F841
+        args = b._browser_impl.get_popen_args()
         b._browser_impl.clean()
         del b
-        print("cli:")
+        print("*** cli:")
         for arg in cli:
-            print(arg)
-        print("env:")
-        for k, v in env.items():
-            print(f"{k}:{v}")
+            print(" " * 8 + str(arg))
+
+        # potential security issue
+        # print("*** env:")
+        # for k, v in env.items():
+        #     print(" " * 8 + f"{k}:{v}")
+
+        print("*** Popen args:")
+        for k, v in args.items():
+            print(" " * 8 + f"{k}:{v}")
+    print("*".center(50, "*"))
     print("VERSION INFO:".center(50, "*"))
     try:
-        print("PIP:".center(25, "*"))
+        print("pip:".center(25, "*"))
         print(subprocess.check_output([sys.executable, "-m", "pip", "freeze"]).decode())
     except Exception as e:
         print(f"Error w/ pip: {e}")
     try:
-        print("UV:".center(25, "*"))
+        print("uv:".center(25, "*"))
         print(subprocess.check_output(["uv", "pip", "freeze"]).decode())
     except Exception as e:
         print(f"Error w/ uv: {e}")
     try:
-        print("GIT:".center(25, "*"))
+        print("git:".center(25, "*"))
         print(
             subprocess.check_output(
                 ["git", "describe", "--tags", "--long", "--always"],
@@ -99,18 +108,10 @@ def diagnose() -> None:
         print(sys.version)
         print(sys.version_info)
         print("Done with version info.".center(50, "*"))
+
     if run:
-        try:
-            print("Skipping sync test...")
-            # print("Sync Test Headless".center(50, "*"))
-            # browser = BrowserSync(headless=headless)
-            # browser.open()
-            # time.sleep(3)
-            # browser.close()
-        except Exception as e:
-            fail.append(("Sync test headless", e))
-        finally:
-            print("Done with sync test headless".center(50, "*"))
+        print("*".center(50, "*"))
+        print("Actual Run Tests".center(50, "*"))
 
         async def test_headless() -> None:
             browser = await Browser(headless=headless)
@@ -141,5 +142,7 @@ async def test_headless() -> None:
             except Exception:
                 print("Couldn't print traceback for:")
                 print(str(exception))
-        raise Exception("There was an exception, see above.")
+        raise Exception(
+            "There was an exception during full async run, see above.",
+        )
     print("Thank you! Please share these results with us!")

pyproject.toml

@@ -26,7 +26,7 @@ maintainers = [
   {name = "Andrew Pikul", email = "ajpikul@gmail.com"},
 ]
 dependencies = [
-  "logistro>=2.0.0",
+  "logistro>=2.0.1",
   "simplejson>=3.19.3",
 ]
 

tests/test_browser_search.py

@@ -4,7 +4,7 @@
 from pathlib import Path
 
 import pytest
-from choreographer.browsers.chromium import _find_a_chromium_based_browser
+from choreographer.browsers import chromium
 
 
 def test_internal(tmp_path):
@@ -23,7 +23,7 @@ def test_internal(tmp_path):
         paths.append(p)
 
     for _p, _n in zip(paths, names):
-        _r = _find_a_chromium_based_browser(skip_local=True, skip_typical=True)
+        _r = chromium.Chromium.find_browser(skip_local=True, skip_typical=True)
         assert _r
         assert Path(_r).stem == _n
         _p.unlink()