From 2214fa0eb61d4e74fb3545474a1aa5d308cfd4ac Mon Sep 17 00:00:00 2001
From: edithatogo <15080672+edithatogo@users.noreply.github.com>
Date: Fri, 31 Jul 2026 04:02:24 +0000
Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=AA=20Add=20Error=20Test=20for=20Dash?=
 =?UTF-8?q?=20Browser=20Smoke=20Check=20Timeout?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

🎯 **What:**
The `check_dash_browser_smoke.py` script waits for a spawned dash server with a timeout, and handles `subprocess.TimeoutExpired` by killing the process.
The missing testing gap for this `TimeoutExpired` exception block has been addressed. The actual bug where we forgot to call `process.wait()` after `process.kill()` inside `scripts/check_dash_browser_smoke.py` was also fixed to properly avoid zombie processes.

📊 **Coverage:**
- Covered the `subprocess.TimeoutExpired` catch block during the initial timeout for closing the spawned dash server process.
- Ensured `kill()` is correctly called.
- Ensured `wait()` is correctly called right after `kill()`.

✨ **Result:**
The improvement in test coverage prevents future regressions and potential un-waited zombie processes when timeouts trigger.
---
 scripts/check_dash_browser_smoke.py           | 11 +++++-
 .../scripts/test_check_dash_browser_smoke.py  | 39 +++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100644 tests/scripts/test_check_dash_browser_smoke.py

diff --git a/scripts/check_dash_browser_smoke.py b/scripts/check_dash_browser_smoke.py
index 97cdef5..4e17594 100644
--- a/scripts/check_dash_browser_smoke.py
+++ b/scripts/check_dash_browser_smoke.py
@@ -179,7 +179,11 @@ def _check_route(
     interaction_failures = (download_ok is False) or (provenance_ok is False)
     graph_failure = graph_count < ROUTE_MIN_GRAPHS.get(_route_key(route), 0)
     return BrowserSmokeResult(
-        ok=not missing_text and not serious_console and not failed_requests and not interaction_failures and not graph_failure,
+        ok=not missing_text
+        and not serious_console
+        and not failed_requests
+        and not interaction_failures
+        and not graph_failure,
         route=route,
         viewport=viewport_name,
         missing_text=missing_text,
@@ -215,7 +219,9 @@ def run_browser_smoke(
                     try:
                         print(f"checking {viewport_name} {route}", flush=True)
                         results.append(
-                            _check_route(page, base_url, route, viewport_name, viewport, timeout_seconds, screenshot_dir)
+                            _check_route(
+                                page, base_url, route, viewport_name, viewport, timeout_seconds, screenshot_dir
+                            )
                         )
                     finally:
                         page.close()
@@ -262,6 +268,7 @@ def main(argv: list[str] | None = None) -> int:
                 process.wait(timeout=10)
             except subprocess.TimeoutExpired:
                 process.kill()
+                process.wait()
 
     ok = all(result.ok for result in results)
     for result in results:
diff --git a/tests/scripts/test_check_dash_browser_smoke.py b/tests/scripts/test_check_dash_browser_smoke.py
new file mode 100644
index 0000000..7692f33
--- /dev/null
+++ b/tests/scripts/test_check_dash_browser_smoke.py
@@ -0,0 +1,39 @@
+import os
+import subprocess
+import sys
+from unittest.mock import MagicMock, patch
+
+# Add the root directory to sys.path so we can import scripts
+sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "../..")))
+
+from scripts.check_dash_browser_smoke import main
+
+
+def test_dash_browser_smoke_timeout_error():
+    """
+    Tests that when the server process times out during the wait() call,
+    the TimeoutExpired exception is caught, process.kill() is called, and then process.wait() is called.
+    """
+    with (
+        patch("scripts.check_dash_browser_smoke._free_port", return_value=12345),
+        patch("scripts.check_dash_browser_smoke._start_server") as mock_start_server,
+        patch("scripts.check_dash_browser_smoke._wait_for_server"),
+        patch("scripts.check_dash_browser_smoke.run_browser_smoke", return_value=[]),
+    ):
+        # Create a mock process
+        mock_process = MagicMock(spec=subprocess.Popen)
+        mock_start_server.return_value = mock_process
+
+        # Make the first call to wait() raise a TimeoutExpired exception
+        mock_process.wait.side_effect = [subprocess.TimeoutExpired(cmd=["fake_cmd"], timeout=5), None]
+
+        main([])
+
+        # The mock process should be terminated first
+        mock_process.terminate.assert_called_once()
+
+        # After terminate, it should have called kill() due to TimeoutExpired
+        mock_process.kill.assert_called_once()
+
+        # We also check that wait() was called again after kill()
+        assert mock_process.wait.call_count == 2