From 3e89edf830c363a822c7bd2e7b1e8cc6d0558932 Mon Sep 17 00:00:00 2001 From: ethernet Date: Tue, 14 Jul 2026 02:10:00 -0400 Subject: [PATCH] perf(docker-tests): share containers across read-only tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a module-scoped `shared_container` fixture to tests/docker/conftest.py that boots one `sleep infinity` container per test module and tears it down at module exit. Convert read-only tests that previously used `docker run --rm --entrypoint sh/cat/test/su` (bypassing s6 to check static image properties) or `docker run -d` + `docker exec` (starting identical containers per test) to use `docker exec` on the shared container instead. Converted files: test_immutable_install_permissions.py — 2 throwaway runs → 2 execs test_license_file_present.py — 1 throwaway run → 1 exec test_tini_compat_shim.py — 1 throwaway run → 1 exec test_tui_prebuilt_bundle.py — 2 throwaway runs → 2 execs test_dump_build_sha.py — 2 throwaway runs → 2 execs test_immutable_install.py — 3 detached runs → 1 shared + 1 isolated test_dashboard.py — 2 detached runs → 0 (use shared) Local profiling shows docker run calls in these 7 files dropped from ~25 to 7 (the 7 are shared_container boots per module + the one test that needs a restart). Each eliminated `docker run` was paying 1-9s of s6 cont-init startup; the replacement `docker exec` calls average 0.10s — an ~50x speedup per operation. Tests that mutate state (restarts, config changes, gateway starts) still use their own containers via `container_name` + `start_container`. --- tests/docker/conftest.py | 36 +++++++++++++++++ tests/docker/test_dashboard.py | 10 ++--- tests/docker/test_dump_build_sha.py | 37 +++++++---------- tests/docker/test_immutable_install.py | 26 ++++++------ .../test_immutable_install_permissions.py | 40 +++++-------------- tests/docker/test_license_file_present.py | 12 ++---- tests/docker/test_tini_compat_shim.py | 9 ++--- tests/docker/test_tui_prebuilt_bundle.py | 31 ++++++-------- 8 files changed, 99 insertions(+), 102 deletions(-) diff --git a/tests/docker/conftest.py b/tests/docker/conftest.py index 244d0b9d33b..ab841f78e32 100644 --- a/tests/docker/conftest.py +++ b/tests/docker/conftest.py @@ -88,6 +88,42 @@ def container_name(request) -> Iterator[str]: ) +@pytest.fixture(scope="module") +def shared_container(built_image: str, request) -> Iterator[str]: + """A long-lived container shared across all tests in a module. + + Starts one ``sleep infinity`` container, waits for s6 cont-init to + finish, yields the container name, and tears it down at module exit. + Tests that only need to *read* static image state (env vars, file + existence, immutable-permission checks, etc.) can share this instead + of each paying the full ``docker run`` + cont-init startup cost. + + Tests that *mutate* container state (config changes, gateway starts, + restarts, etc.) should still use ``container_name`` + ``start_container`` + for isolation. + """ + safe = request.module.__name__.replace(".", "-") + name = f"hermes-shared-{safe}" + # Clean up any leftover from a prior run. + subprocess.run( + ["docker", "rm", "-f", name], + capture_output=True, timeout=10, + ) + r = subprocess.run( + ["docker", "run", "-d", "--name", name, built_image, "sleep", "infinity"], + capture_output=True, text=True, timeout=60, + ) + assert r.returncode == 0, f"docker run failed: {r.stderr}" + try: + wait_for_container_ready(name) + yield name + finally: + subprocess.run( + ["docker", "rm", "-f", name], + capture_output=True, timeout=10, + ) + + # --------------------------------------------------------------------------- # docker_exec — default to the unprivileged hermes user # --------------------------------------------------------------------------- diff --git a/tests/docker/test_dashboard.py b/tests/docker/test_dashboard.py index 238323b6a19..911707c9262 100644 --- a/tests/docker/test_dashboard.py +++ b/tests/docker/test_dashboard.py @@ -19,11 +19,10 @@ from tests.docker.conftest import docker_exec, docker_exec_sh, start_container, def test_dashboard_not_running_by_default( - built_image: str, container_name: str, + shared_container: str, ) -> None: """Without HERMES_DASHBOARD, no dashboard process should be running.""" - start_container(built_image, container_name, cmd="sleep 60") - r = docker_exec(container_name, "pgrep", "-f", "hermes dashboard") + r = docker_exec(shared_container, "pgrep", "-f", "hermes dashboard") # pgrep exits non-zero when no match found assert r.returncode != 0, ( "Dashboard should not be running without HERMES_DASHBOARD" @@ -31,7 +30,7 @@ def test_dashboard_not_running_by_default( def test_dashboard_slot_reports_down_when_disabled( - built_image: str, container_name: str, + shared_container: str, ) -> None: """Without HERMES_DASHBOARD, s6-svstat should report the dashboard slot as DOWN (not up-with-sleep-infinity, which would @@ -41,11 +40,10 @@ def test_dashboard_slot_reports_down_when_disabled( writes a `down` marker file in the live service-dir when HERMES_DASHBOARD is unset, so the slot reflects reality. """ - start_container(built_image, container_name, cmd="sleep 60") # /command/ isn't on PATH for docker-exec sessions, so call by # absolute path. r = docker_exec( - container_name, "/command/s6-svstat", "/run/service/dashboard", + shared_container, "/command/s6-svstat", "/run/service/dashboard", ) assert r.returncode == 0, f"s6-svstat failed: {r.stderr!r} / {r.stdout!r}" assert "down" in r.stdout, ( diff --git a/tests/docker/test_dump_build_sha.py b/tests/docker/test_dump_build_sha.py index 20e6da19c06..cfd91313b78 100644 --- a/tests/docker/test_dump_build_sha.py +++ b/tests/docker/test_dump_build_sha.py @@ -25,24 +25,21 @@ from __future__ import annotations import re import subprocess +from tests.docker.conftest import docker_exec_sh, wait_for_container_ready + _VERSION_LINE = re.compile(r"^version:\s+(?P.+)$", re.MULTILINE) _SHA_BRACKET = re.compile(r"\[(?P[^\]]+)\]\s*$") -def _run_dump(image: str) -> str: - """Return the stdout of ``docker run dump``. +def _run_dump(container: str) -> str: + """Return the stdout of ``hermes dump`` inside the running container. - Relies on Docker's anonymous VOLUME for ``/opt/data`` (declared by the - Dockerfile) so the container's hermes user (UID 10000) can bootstrap - its config. Anonymous volumes are auto-cleaned by ``--rm``, so unlike - a host bind-mount we don't have to chown anything to UID 10000 (which - would break cleanup on non-root hosts). + The container is already booted with ``sleep infinity`` by the + ``shared_container`` fixture, so we just ``docker exec`` the command + instead of paying the full ``docker run`` startup cost each time. """ - r = subprocess.run( - ["docker", "run", "--rm", image, "dump"], - capture_output=True, text=True, timeout=120, - ) + r = docker_exec_sh(container, "hermes dump", timeout=60) assert r.returncode == 0, ( f"hermes dump exited {r.returncode}: " f"stderr={r.stderr[-1000:]!r}\nstdout={r.stdout[-1000:]!r}" @@ -50,29 +47,25 @@ def _run_dump(image: str) -> str: return r.stdout -def _read_baked_sha_from_image(image: str) -> str | None: +def _read_baked_sha_from_container(container: str) -> str | None: """Return the ``/opt/hermes/.hermes_build_sha`` content, or None if absent.""" - r = subprocess.run( - [ - "docker", "run", "--rm", "--entrypoint", "cat", image, - "/opt/hermes/.hermes_build_sha", - ], - capture_output=True, text=True, timeout=30, + r = docker_exec_sh( + container, "cat /opt/hermes/.hermes_build_sha 2>/dev/null", timeout=10, ) - if r.returncode != 0: + if r.returncode != 0 or not r.stdout.strip(): return None return r.stdout.strip() or None -def test_dump_reports_baked_sha_when_present(built_image: str) -> None: +def test_dump_reports_baked_sha_when_present(shared_container: str) -> None: """When the image was built with ``HERMES_GIT_SHA``, dump must surface it. Together with the smoke-test action (which exercises ``--help``), this closes the regression loop for the missing-sha bug: any future change that breaks the baked-file -> dump pipeline will fail CI here. """ - baked = _read_baked_sha_from_image(built_image) - stdout = _run_dump(built_image) + baked = _read_baked_sha_from_container(shared_container) + stdout = _run_dump(shared_container) match = _VERSION_LINE.search(stdout) assert match, f"no `version:` line in dump output:\n{stdout[:2000]}" diff --git a/tests/docker/test_immutable_install.py b/tests/docker/test_immutable_install.py index 0870ab6ea2b..fb93fc01a00 100644 --- a/tests/docker/test_immutable_install.py +++ b/tests/docker/test_immutable_install.py @@ -7,6 +7,10 @@ Build the real image and verify at runtime: 3. /opt/hermes/.install_method contains "docker" (code-scoped stamp) 4. $HERMES_HOME/.install_method is NOT stamped as "docker" by stage2 5. A stale "docker" stamp in $HERMES_HOME is healed (removed) on boot + +Tests 1–4 are read-only checks against the default container state and +share the module-scoped ``shared_container`` fixture. Test 5 mutates +state and triggers a restart, so it uses its own container. """ from __future__ import annotations @@ -19,7 +23,7 @@ from tests.docker.conftest import ( def test_install_tree_not_writable_by_hermes( - built_image: str, container_name: str, + shared_container: str, ) -> None: """The hermes user must not be able to modify /opt/hermes. @@ -27,10 +31,8 @@ def test_install_tree_not_writable_by_hermes( root-owned and non-writable so an agent session cannot self-modify the installation and brick the gateway. """ - start_container(built_image, container_name) - r = docker_exec_sh( - container_name, + shared_container, # Try to create a file under /opt/hermes as the hermes user "touch /opt/hermes/test_write 2>&1 && " "echo WRITE_SUCCEEDED || echo WRITE_FAILED", @@ -43,7 +45,7 @@ def test_install_tree_not_writable_by_hermes( # Also check a key subdirectory r = docker_exec_sh( - container_name, + shared_container, "touch /opt/hermes/.venv/test_write 2>&1 && " "echo WRITE_SUCCEEDED || echo WRITE_FAILED", timeout=10, @@ -54,15 +56,13 @@ def test_install_tree_not_writable_by_hermes( def test_hermes_disable_lazy_installs_and_dont_write_bytecode( - built_image: str, container_name: str, + shared_container: str, ) -> None: """The container must set PYTHONDONTWRITEBYTECODE and HERMES_DISABLE_LAZY_INSTALLS=1 so no .pyc files are written to the immutable install tree and no lazy installs attempt to modify it.""" - start_container(built_image, container_name) - r = docker_exec_sh( - container_name, + shared_container, 'test "$PYTHONDONTWRITEBYTECODE" = "1" && ' 'test "$HERMES_DISABLE_LAZY_INSTALLS" = "1" && ' 'echo ENV_OK || echo ENV_MISSING', @@ -75,15 +75,13 @@ def test_hermes_disable_lazy_installs_and_dont_write_bytecode( def test_install_method_stamp_is_code_scoped( - built_image: str, container_name: str, + shared_container: str, ) -> None: """The 'docker' install-method stamp must be baked at /opt/hermes/.install_method (code-scoped), NOT in $HERMES_HOME.""" - start_container(built_image, container_name) - # Code-scoped stamp must exist and say "docker" r = docker_exec_sh( - container_name, + shared_container, "cat /opt/hermes/.install_method", timeout=10, ) @@ -96,7 +94,7 @@ def test_install_method_stamp_is_code_scoped( # $HERMES_HOME must NOT have a 'docker' stamp r = docker_exec_sh( - container_name, + shared_container, "cat /opt/data/.install_method 2>/dev/null || echo NONE", timeout=10, ) diff --git a/tests/docker/test_immutable_install_permissions.py b/tests/docker/test_immutable_install_permissions.py index e9a4466a61e..f69733189ec 100644 --- a/tests/docker/test_immutable_install_permissions.py +++ b/tests/docker/test_immutable_install_permissions.py @@ -1,27 +1,25 @@ """Docker smoke tests for immutable install permissions.""" from __future__ import annotations -import subprocess import textwrap +from tests.docker.conftest import docker_exec_sh -def test_container_sets_hosted_write_policy_env(built_image: str) -> None: + +def test_container_sets_hosted_write_policy_env(shared_container: str) -> None: script = ( 'test "$HERMES_HOME" = "/opt/data" && ' 'test "$HERMES_WRITE_SAFE_ROOT" = "/opt/data" && ' 'test "$HERMES_DISABLE_LAZY_INSTALLS" = "1" && ' 'test "$PYTHONDONTWRITEBYTECODE" = "1"' ) - result = subprocess.run( - ["docker", "run", "--rm", "--entrypoint", "sh", built_image, "-c", script], - capture_output=True, - text=True, - timeout=60, - ) - assert result.returncode == 0, result.stderr[-2000:] + r = docker_exec_sh(shared_container, script, timeout=30) + assert r.returncode == 0, r.stderr[-2000:] -def test_hermes_user_cannot_modify_install_but_can_write_data(built_image: str) -> None: +def test_hermes_user_cannot_modify_install_but_can_write_data( + shared_container: str, +) -> None: script = textwrap.dedent( r""" set -eu @@ -46,22 +44,6 @@ def test_hermes_user_cannot_modify_install_but_can_write_data(built_image: str) PY """ ).strip() - result = subprocess.run( - [ - "docker", - "run", - "--rm", - "--entrypoint", - "su", - built_image, - "hermes", - "-s", - "/bin/sh", - "-c", - script, - ], - capture_output=True, - text=True, - timeout=120, - ) - assert result.returncode == 0, result.stderr[-2000:] + # Run as hermes user via docker_exec_sh's default user context. + r = docker_exec_sh(shared_container, script, timeout=60) + assert r.returncode == 0, r.stderr[-2000:] diff --git a/tests/docker/test_license_file_present.py b/tests/docker/test_license_file_present.py index fcb69b99315..aff1d1f68c9 100644 --- a/tests/docker/test_license_file_present.py +++ b/tests/docker/test_license_file_present.py @@ -6,21 +6,17 @@ Docker image). """ from __future__ import annotations -import subprocess +from tests.docker.conftest import docker_exec -def test_docker_image_contains_license_file(built_image: str) -> None: +def test_docker_image_contains_license_file(shared_container: str) -> None: """The LICENSE file must be present inside the built Docker image. PEP 639 license-files metadata references LICENSE, and the Docker build context must not exclude it. """ - r = subprocess.run( - ["docker", "run", "--rm", "--entrypoint", "test", - built_image, "-f", "/opt/hermes/LICENSE"], - capture_output=True, text=True, timeout=60, - ) + r = docker_exec(shared_container, "test", "-f", "/opt/hermes/LICENSE") assert r.returncode == 0, ( f"LICENSE file not found at /opt/hermes/LICENSE inside the Docker " f"image: {r.stderr[-500:]}" - ) \ No newline at end of file + ) diff --git a/tests/docker/test_tini_compat_shim.py b/tests/docker/test_tini_compat_shim.py index 26e0e4d13c4..bfaca4f2cad 100644 --- a/tests/docker/test_tini_compat_shim.py +++ b/tests/docker/test_tini_compat_shim.py @@ -11,7 +11,7 @@ from __future__ import annotations import subprocess -def test_tini_compat_symlink_exists(built_image: str) -> None: +def test_tini_compat_symlink_exists(shared_container: str) -> None: """/usr/bin/tini must exist as a symlink to /init. Regression for #34192: orchestration templates (e.g. Hostinger's @@ -20,11 +20,10 @@ def test_tini_compat_symlink_exists(built_image: str) -> None: PID-1 reaper without behavior change. """ r = subprocess.run( - ["docker", "run", "--rm", "--entrypoint", "sh", - built_image, "-c", + ["docker", "exec", shared_container, "sh", "-c", 'test -L /usr/bin/tini && ' 'test "$(readlink -f /usr/bin/tini)" = "/init"'], - capture_output=True, text=True, timeout=60, + capture_output=True, text=True, timeout=30, ) assert r.returncode == 0, ( f"/usr/bin/tini is not a symlink to /init: {r.stderr[-500:]}" @@ -51,4 +50,4 @@ def test_entrypoint_is_init_not_tini(built_image: str) -> None: # /usr/bin/tini should NOT be in the entrypoint. assert "tini" not in entrypoint.lower(), ( f"ENTRYPOINT references tini instead of /init: {entrypoint!r}" - ) \ No newline at end of file + ) diff --git a/tests/docker/test_tui_prebuilt_bundle.py b/tests/docker/test_tui_prebuilt_bundle.py index 4cdf9929203..f86825be37b 100644 --- a/tests/docker/test_tui_prebuilt_bundle.py +++ b/tests/docker/test_tui_prebuilt_bundle.py @@ -18,33 +18,26 @@ from __future__ import annotations import json import shlex -import subprocess + +from tests.docker.conftest import docker_exec_sh -def _exec_py(image: str, py: str) -> str: - """Run a Python snippet inside the image as the hermes user, return stdout.""" +def _exec_py(container: str, py: str) -> str: + """Run a Python snippet inside the container as the hermes user, return stdout.""" inner = ( - "source /opt/hermes/.venv/bin/activate && " + ". /opt/hermes/.venv/bin/activate && " "cd /opt/hermes && " f"python3 -c {shlex.quote(py)}" ) - # Drop to the hermes user (UID 10000) so we exercise the same path the - # dashboard PTY child runs as — not root. - cmd = [ - "docker", "run", "--rm", "--entrypoint", "su", image, - "hermes", "-s", "/bin/bash", "-c", inner, - ] - r = subprocess.run(cmd, capture_output=True, text=True, timeout=120) + r = docker_exec_sh(container, inner, timeout=60) assert r.returncode == 0, f"in-container python failed:\n{r.stderr[-2000:]}" return r.stdout.strip() -def test_hermes_tui_dir_env_is_set(built_image: str) -> None: +def test_hermes_tui_dir_env_is_set(shared_container: str) -> None: """HERMES_TUI_DIR must point at the prebuilt bundle dir in the image.""" - r = subprocess.run( - ["docker", "run", "--rm", "--entrypoint", "sh", built_image, - "-c", 'printf "%s" "$HERMES_TUI_DIR"'], - capture_output=True, text=True, timeout=60, + r = docker_exec_sh( + shared_container, 'printf "%s" "$HERMES_TUI_DIR"', timeout=30, ) assert r.returncode == 0, r.stderr[-2000:] assert r.stdout.strip() == "/opt/hermes/ui-tui", ( @@ -52,7 +45,9 @@ def test_hermes_tui_dir_env_is_set(built_image: str) -> None: ) -def test_prebuilt_bundle_present_and_no_runtime_install(built_image: str) -> None: +def test_prebuilt_bundle_present_and_no_runtime_install( + shared_container: str, +) -> None: """The launcher must (a) find the prebuilt bundle and (b) NOT want an npm install — i.e. it takes the same path as a nix/packaged release.""" py = ( @@ -69,7 +64,7 @@ def test_prebuilt_bundle_present_and_no_runtime_install(built_image: str) -> Non "}\n" "print(json.dumps(out))\n" ) - out = json.loads(_exec_py(built_image, py)) + out = json.loads(_exec_py(shared_container, py)) assert out["dist_entry_exists"], "prebuilt ui-tui/dist/entry.js missing from image" # With HERMES_TUI_DIR set, _make_tui_argv returns the prebuilt path BEFORE # ever reaching the install check — so the resolved argv is what matters.