diff --git a/.claude/skills/bioengine-maintainer/SKILL.md b/.claude/skills/bioengine-maintainer/SKILL.md index 8c4a36a5..2b7b10da 100644 --- a/.claude/skills/bioengine-maintainer/SKILL.md +++ b/.claude/skills/bioengine-maintainer/SKILL.md @@ -118,9 +118,16 @@ Local artifact development: `export BIOENGINE_LOCAL_ARTIFACT_PATH=/path/to/bioen ### Run tests ```bash -pytest tests/end_to_end/ -v +pytest tests/ # offline tests only +pytest tests/end_to_end/ -v --live # deploys to and calls a real cluster ``` +Tests that act on a live cluster are deselected without `--live`, so the +end-to-end command above exits 5 ("no tests collected") if you omit it. +Whenever a credential resolves, the run prints the server, the workspace each +token is scoped to, and how many live-reaching tests are enabled — with or +without `--live`, so a finished run's log says what it could have touched. + Test organisation: - `tests/end_to_end/` — integration tests for the core worker - `tests/apps/` — per-app tests, one subdirectory per app (`tests/apps/cellpose/`, …) diff --git a/pytest.ini b/pytest.ini index 66ed1a36..18aaa402 100644 --- a/pytest.ini +++ b/pytest.ini @@ -4,6 +4,7 @@ markers = integration: marks tests that use real Ray or other subsystems unit: marks tests that use mocked external dependencies requires_gpu: marks tests that require a GPU runtime deployment to be running + live: marks tests that act on a real Hypha cluster; deselected unless --live is passed testpaths = tests python_files = test_*.py python_classes = Test* diff --git a/tests/README.md b/tests/README.md index 90f8bdc1..f5fbc246 100644 --- a/tests/README.md +++ b/tests/README.md @@ -19,6 +19,27 @@ pip install -r requirements-test.txt ### 3. Environment Configuration The `.env` file in the project root contains required environment variables including `HYPHA_TOKEN`. This is automatically loaded by the test configuration. +### 4. Tests that act on a live cluster + +Some tests deploy applications to, and call services on, the production workspaces at `https://hypha.aicell.io`. They are deselected unless you pass `--live`: + +```bash +pytest tests/ # offline tests only +pytest tests/ --live # also runs tests that act on the live cluster +``` + +A test counts as live if it requests `hypha_client`, `hypha_token` or `model_runner`, or is marked `@pytest.mark.live`. Anything that reaches a cluster by some other route — a browser driven at the deployed app, a client built inline — has to carry the marker. Liveness has nothing to do with the `--ignore` lists in circulation: `tests/apps/model-runner/` and `tests/test_artifact_version.py` are live and neither lives under `tests/end_to_end/`. + +**Do not rely on your working directory to keep a credential away from the suite.** There are at least three ways one arrives, and only the last is under your control: + +- `load_dotenv()` above searches **upward** from `tests/`, not just the directory you started in. A worktree under `/.claude/worktrees/` has no `.env` of its own but still reaches the repo root's, so it is *not* protected. A worktree outside the repo tree (say under `/tmp`) does stop the walk. +- `tests/test_artifact_version.py` loads the repo-root `.env` by explicit path, wherever you run it from. +- `source .env`, `export HYPHA_TOKEN=…`, or a token already in your shell defeats all of the above regardless of location. + +Running inside the worker image with only the repo bind-mounted also stops the upward walk, because the search cannot escape the mount. + +Because none of that is reliable, the gate does not depend on it. Whenever a credential resolves, the run prints — before the first test, and again next to the wall clock at the end — which server it is, which workspace each token is scoped to, and how many live-reaching tests are enabled. That last number is printed **with or without `--live`**: the flag only prevents the accidental case, so once someone has opted in deliberately the count in the log is what makes an after-the-fact audit possible. Runtime is the corroborating signal; the same scope takes roughly 30s offline and several minutes against a cluster. + ## Running Tests ### All Tests diff --git a/tests/apps/cellpose/test_ui_e2e.py b/tests/apps/cellpose/test_ui_e2e.py index ef1a576a..a37d0b48 100644 --- a/tests/apps/cellpose/test_ui_e2e.py +++ b/tests/apps/cellpose/test_ui_e2e.py @@ -6,6 +6,11 @@ import pytest from playwright.sync_api import Page, expect +# Every test here drives a browser at the deployed app and injects HYPHA_TOKEN +# into its localStorage. The `page` fixture is not in LIVE_FIXTURES, so the +# marker is what gates them. +pytestmark = pytest.mark.live + SERVER_URL = "https://hypha.aicell.io" APP_WORKSPACE = os.environ.get("HYPHA_TEST_WORKSPACE", "ri-scale") APP_URL = f"{SERVER_URL}/{APP_WORKSPACE}/view/cellpose-finetuning" diff --git a/tests/apps/model-runner/conftest.py b/tests/apps/model-runner/conftest.py index 10acde07..b2b39d25 100644 --- a/tests/apps/model-runner/conftest.py +++ b/tests/apps/model-runner/conftest.py @@ -3,7 +3,10 @@ Requires the model-runner app to be deployed to bioimage-io/bioengine-worker. Set BIOIMAGE_IO_TOKEN (or HYPHA_TOKEN) in the environment before running. - pytest tests/apps/model-runner/ -v -o "addopts=" +These call the live service, so they need --live; without it they are +deselected and the command exits 5 with no tests collected. + + pytest tests/apps/model-runner/ -v -o "addopts=" --live """ import io diff --git a/tests/conftest.py b/tests/conftest.py index db3fe064..f8761206 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -8,14 +8,17 @@ """ import asyncio +import base64 +import json import os import re import subprocess import sys import tempfile +import time from datetime import datetime from pathlib import Path -from typing import AsyncGenerator, Generator +from typing import AsyncGenerator, Dict, Generator, List, Optional, Tuple import pytest import pytest_asyncio @@ -28,6 +31,172 @@ # Load environment variables from .env file load_dotenv() +# Every Hypha connection in the suite is hard-coded to this deployment; no +# environment variable redirects it. +HYPHA_SERVER_URL = "https://hypha.aicell.io" + +# Requesting one of these fixtures means the test authenticates against the +# real deployment above. Anything else that reaches a cluster has to say so +# with @pytest.mark.live. +LIVE_FIXTURES = frozenset({"hypha_client", "hypha_token", "model_runner"}) + +# Credentials the suite picks up on its own, in the order a reader should +# worry about them. `load_dotenv()` above supplies these from a repo-root +# .env, so whether they resolve depends on which checkout pytest was run from. +LIVE_TOKEN_VARS = ("HYPHA_TOKEN", "BIOIMAGE_IO_TOKEN") + +_START_TIME = pytest.StashKey[float]() + + +def pytest_addoption(parser: pytest.Parser) -> None: + parser.addoption( + "--live", + action="store_true", + default=False, + help=( + "Run tests that deploy to and call a real Hypha cluster. Without " + "this flag they are deselected even when a token is available." + ), + ) + + +def _token_workspace(token: str) -> str: + """Workspace a Hypha JWT is scoped to, or '' if it is not a readable JWT.""" + try: + segment = token.split(".")[1] + claims = json.loads( + base64.urlsafe_b64decode(segment + "=" * (-len(segment) % 4)) + ) + scope = str(claims.get("scope", "")) + except Exception: + # A token we cannot parse must not abort the session; the caller + # reports the credential as present with an unknown workspace. + return "" + for part in scope.split(): + if part.startswith("wid:"): + return part[len("wid:") :] + return "" + + +def resolved_live_targets( + environ: Optional[Dict[str, str]] = None, +) -> List[Tuple[str, str]]: + """(variable, workspace) for every cluster credential visible to this run.""" + environ = os.environ if environ is None else environ + return [ + (var, _token_workspace(environ[var]) or "") + for var in LIVE_TOKEN_VARS + if environ.get(var) + ] + + +def live_exposure_banner( + targets: List[Tuple[str, str]], live_count: int, enabled: bool +) -> str: + """The text printed before the first test when a cluster credential resolves. + + The count is stated whether or not --live was given: the flag only stops + the accidental case, and once someone has opted in deliberately the count + in the log is the only thing that makes an after-the-fact audit possible. + """ + lines = [ + "=" * 22 + " live cluster credentials resolved " + "=" * 22, + f"{live_count} collected test(s) can reach the live cluster at " + f"{HYPHA_SERVER_URL}, and a credential for it resolved:", + ] + lines += [f" {var} -> workspace {workspace}" for var, workspace in targets] + lines.append( + f"THEY WILL RUN: --live was given, so {live_count} test(s) will act on " + "that workspace." + if enabled + else f"Deselected {live_count} test(s); pass --live to run them." + ) + lines.append("=" * 78) + return "\n".join(lines) + + +def _emit(config: pytest.Config, text: str) -> None: + """Write where the controller will actually see it. + + Under pytest-xdist -- which `addopts` turns on with `--numprocesses=1` -- + collection runs inside a worker whose terminalreporter output is dropped + and whose deselection stats never reach the summary. The worker's stderr + is inherited by the controller, so that is the channel that survives. + """ + worker = getattr(config, "workerinput", None) + if worker is not None: + if worker.get("workerid", "gw0") == "gw0": + sys.stderr.write(text + "\n") + sys.stderr.flush() + return + + reporter = config.pluginmanager.get_plugin("terminalreporter") + if reporter is None: + print(text) + else: + reporter.write_line(text) + + +def _is_live(item: pytest.Item) -> bool: + if any(item.iter_markers("live")): + return True + return bool(LIVE_FIXTURES.intersection(getattr(item, "fixturenames", ()))) + + +@pytest.hookimpl(tryfirst=True) +def pytest_collection_modifyitems( + config: pytest.Config, items: List[pytest.Item] +) -> None: + # Attaching the marker is what makes `-m live` agree with `--live`: most + # live tests are detected from their fixture closure, so without this they + # are gated but unnamed and `-m live` returns a wrong subset. It has to + # happen before pytest's own mark filtering -- which a conftest hookimpl + # already does under pluggy's reverse-registration order, so tryfirst is a + # guarantee against a plugin registering an earlier modifyitems, not a + # load-bearing fix. Switching it to trylast does break the invariant. + live, offline = [], [] + for item in items: + if _is_live(item): + item.add_marker(pytest.mark.live) + live.append(item) + else: + offline.append(item) + + enabled = config.getoption("--live") + if live and not enabled: + items[:] = offline + config.hook.pytest_deselected(items=live) + + targets = resolved_live_targets() + if targets: + _emit(config, live_exposure_banner(targets, len(live), enabled)) + + +def pytest_configure(config: pytest.Config) -> None: + config.stash[_START_TIME] = time.time() + + +def pytest_terminal_summary( + terminalreporter, exitstatus: int, config: pytest.Config +) -> None: + """Restate the exposure next to the wall clock. + + Runtime is the cheapest signal that live tests ran -- the same command + takes ~30s offline and several minutes against a cluster -- so an audit + needs the credential and the duration in one place. + """ + targets = resolved_live_targets() + if not targets: + return + elapsed = time.time() - config.stash.get(_START_TIME, time.time()) + workspaces = ", ".join(workspace for _, workspace in targets) + terminalreporter.write_line( + f"live cluster exposure: credentials for {workspaces} on " + f"{HYPHA_SERVER_URL} were in scope for this {elapsed:.1f}s run " + f"({'--live GIVEN' if config.getoption('--live') else '--live not given'}).", + red=True, + ) + @pytest.fixture( scope="session", @@ -192,7 +361,7 @@ def head_node_port(ray_address: str) -> int: @pytest.fixture(scope="session") def server_url() -> str: """Return Hypha server URL for test connections.""" - return "https://hypha.aicell.io" + return HYPHA_SERVER_URL @pytest.fixture(scope="session") diff --git a/tests/test_artifact_version.py b/tests/test_artifact_version.py index 8cc8cc63..b68975e4 100644 --- a/tests/test_artifact_version.py +++ b/tests/test_artifact_version.py @@ -10,10 +10,14 @@ 3. Re-saving the same version → rejected (would overwrite a published release). 4. Saving an older version → rejected. +These act on ``bioimage-io/bioengine-worker`` in production: they call +``upload_app`` and delete the artifacts again. They are the most destructive +tests in the suite, hence the module-wide ``live`` marker. + Run with: conda activate bioengine source .env - pytest tests/test_artifact_version.py -v + pytest tests/test_artifact_version.py -v --live """ import os @@ -26,6 +30,8 @@ load_dotenv(Path(__file__).parent.parent / ".env") +pytestmark = pytest.mark.live + # ── helpers ──────────────────────────────────────────────────────────────────── diff --git a/tests/test_live_gate.py b/tests/test_live_gate.py new file mode 100644 index 00000000..dd48da73 --- /dev/null +++ b/tests/test_live_gate.py @@ -0,0 +1,322 @@ +"""The gate that keeps a stray .env from pointing the suite at production. + +`tests/conftest.py` calls `load_dotenv()` unconditionally and `find_dotenv()` +walks *up*, so a credential can arrive without anyone asking for it. These +tests pin the three consequences: live tests are deselected unless ``--live`` +is passed, every resolved credential is announced before the first test with +the count it enables, and that announcement survives the xdist worker that +``pytest.ini``'s ``addopts`` puts collection inside. + +Sub-runs either deselect everything or use ``--collect-only``, and the tokens +are synthetic, so nothing here contacts a cluster. +""" + +import ast +import base64 +import functools +import importlib.util +import json +import os +import re +import subprocess +import sys +from pathlib import Path +from typing import Dict, List, Tuple + +import pytest + +from tests.conftest import ( + HYPHA_SERVER_URL, + _token_workspace, + live_exposure_banner, + resolved_live_targets, +) + +REPO_ROOT = Path(__file__).resolve().parent.parent + +MODEL_RUNNER = "tests/apps/model-runner" + +# One live test in tests/apps/model-runner/, by node name. +A_LIVE_TEST = "test_search_models_returns_list" + +# A scope holding all three kinds at once: live by fixture closure, live by +# module marker, and offline. The marker test below needs the mix, because +# the failure it guards is a marker set that covers only the second kind. +MIXED_SCOPE = (MODEL_RUNNER, "tests/test_artifact_version.py", "tests/_app") + +# pytest.ini's addopts carries --numprocesses=1, so a sub-run that honours it +# needs xdist. It is in requirements-test.txt; ad-hoc runners sometimes +# install a narrower set. +needs_shipped_addopts = pytest.mark.skipif( + importlib.util.find_spec("xdist") is None, + reason="pytest-xdist absent, so pytest.ini's addopts cannot be honoured", +) + + +def _fake_jwt(workspace: str) -> str: + claims = {"scope": f"ws:{workspace}#a wid:{workspace}", "sub": "auth0|test"} + payload = base64.urlsafe_b64encode(json.dumps(claims).encode()).decode() + return f"header.{payload.rstrip('=')}.signature" + + +def _collect( + extra_args: List[str], + env_overrides: Dict[str, str], + scope: Tuple[str, ...] = (MODEL_RUNNER,), +) -> str: + """A sub-run with addopts neutralised -- the fast path, for selection logic.""" + return _run_cached( + ("-o", "addopts=", "--collect-only", "-q", *extra_args), + tuple(sorted(env_overrides.items())), + scope, + ) + + +def _run_with_default_addopts( + extra_args: List[str], env_overrides: Dict[str, str] +) -> str: + """A sub-run under pytest.ini as shipped. + + `addopts` carries `--numprocesses=1`, so collection happens inside an + xdist worker whose terminalreporter output is discarded. Passing + `-o addopts=` -- as every other sub-run here does -- hides that entirely, + which is why this variant exists. + """ + return _run_cached( + tuple(extra_args), tuple(sorted(env_overrides.items())), (MODEL_RUNNER,) + ) + + +@functools.lru_cache(maxsize=None) +def _run_cached( + extra_args: Tuple[str, ...], + overrides: Tuple[Tuple[str, str], ...], + scope: Tuple[str, ...], +) -> str: + """Run pytest over `scope` and return its combined output. + + Both token variables are always set explicitly: python-dotenv does not + override a variable that is already in the environment, so passing an + empty string is what actually neutralises the repo-root .env -- which + `load_dotenv()` finds by walking up, even from a worktree. + """ + env = { + **os.environ, + "HYPHA_TOKEN": "", + "BIOIMAGE_IO_TOKEN": "", + **dict(overrides), + } + result = subprocess.run( + [ + sys.executable, + "-m", + "pytest", + "-p", + "no:cacheprovider", + *scope, + *extra_args, + ], + cwd=REPO_ROOT, + env=env, + capture_output=True, + text=True, + ) + # 5 is EXIT_NOTESTSCOLLECTED, which is exactly what a fully deselected + # scope produces. + assert result.returncode in (0, 5), result.stdout + result.stderr + return result.stdout + result.stderr + + +def test_token_workspace_reads_the_wid_scope() -> None: + assert _token_workspace(_fake_jwt("bioimage-io")) == "bioimage-io" + + +def test_token_workspace_tolerates_a_non_jwt() -> None: + assert _token_workspace("not-a-jwt") == "" + + +def test_resolved_live_targets_is_empty_without_a_credential() -> None: + assert resolved_live_targets({"HYPHA_TOKEN": "", "BIOIMAGE_IO_TOKEN": ""}) == [] + + +def test_resolved_live_targets_names_every_credential_in_scope() -> None: + assert resolved_live_targets( + { + "HYPHA_TOKEN": _fake_jwt("ws-user-github|1"), + "BIOIMAGE_IO_TOKEN": _fake_jwt("bioimage-io"), + } + ) == [("HYPHA_TOKEN", "ws-user-github|1"), ("BIOIMAGE_IO_TOKEN", "bioimage-io")] + + +def test_banner_names_the_server_the_workspace_and_the_count() -> None: + banner = live_exposure_banner([("BIOIMAGE_IO_TOKEN", "bioimage-io")], 25, False) + assert HYPHA_SERVER_URL in banner + assert "bioimage-io" in banner + assert "25" in banner + + +def test_banner_distinguishes_deselected_from_about_to_run() -> None: + targets = [("BIOIMAGE_IO_TOKEN", "bioimage-io")] + assert "WILL RUN" in live_exposure_banner(targets, 25, True) + assert "WILL RUN" not in live_exposure_banner(targets, 25, False) + + +def test_every_resolved_credential_is_announced_before_the_first_test() -> None: + stdout = _collect( + [], + { + "HYPHA_TOKEN": _fake_jwt("ws-user-github|1"), + # The model-runner gate prefers this one, so a run can reach + # production even with HYPHA_TOKEN scoped somewhere harmless. + "BIOIMAGE_IO_TOKEN": _fake_jwt("bioimage-io"), + }, + ) + assert "live cluster credentials resolved" in stdout + assert "HYPHA_TOKEN -> workspace ws-user-github|1" in stdout + assert "BIOIMAGE_IO_TOKEN -> workspace bioimage-io" in stdout + assert HYPHA_SERVER_URL in stdout + assert "25 collected test(s) can reach" in stdout + + +def test_nothing_is_announced_when_no_credential_resolves() -> None: + stdout = _collect([], {}) + assert "live cluster credentials resolved" not in stdout + + +def test_live_tests_are_deselected_by_default() -> None: + stdout = _collect([], {}) + assert A_LIVE_TEST not in stdout + assert "25 deselected" in stdout + + +def test_live_tests_are_selected_with_the_flag() -> None: + stdout = _collect(["--live"], {}) + assert A_LIVE_TEST in stdout + assert "deselected" not in stdout + + +@needs_shipped_addopts +def test_the_banner_survives_the_default_addopts() -> None: + """pytest.ini runs collection inside an xdist worker; the banner must + still reach the controller, count and all.""" + output = _run_with_default_addopts([], {"HYPHA_TOKEN": _fake_jwt("bioimage-io")}) + assert "live cluster credentials resolved" in output + assert "HYPHA_TOKEN -> workspace bioimage-io" in output + assert "25 collected test(s) can reach" in output + + +@needs_shipped_addopts +def test_the_wall_clock_summary_states_the_exposure_even_with_the_flag() -> None: + """--live stops nothing being reported: the count and the runtime are the + only audit trail left once someone has opted in on purpose.""" + output = _run_with_default_addopts( + ["--live", "--collect-only"], {"HYPHA_TOKEN": _fake_jwt("bioimage-io")} + ) + assert "25 collected test(s) can reach" in output + assert "live cluster exposure:" in output + assert "--live GIVEN" in output + + +def _selected(output: str) -> int: + """Tests pytest reports as collected, from a `-q --collect-only` run.""" + if re.search(r"^no tests collected", output, re.M): + return 0 + match = re.search(r"^(\d+)(?:/\d+)? tests? collected", output, re.M) + assert match, output + return int(match.group(1)) + + +def _deselected(output: str) -> int: + match = re.search(r"\((\d+) deselected\)", output) + return int(match.group(1)) if match else 0 + + +def test_m_live_selects_exactly_what_the_gate_deselects() -> None: + """The `live` marker has to describe every test the gate acts on. + + Most are detected from their fixture closure rather than from a marker + anyone wrote, so `-m live` only agrees with the gate if the hook attaches + the marker to them. Without that attachment `-m live --live` silently + returns just the modules that declare the marker themselves -- a wrong + subset that looks like a complete answer, which is worse than the option + not working at all. + + Comparing the two counts rather than asserting a literal keeps this test + correct as live tests are added, and it is exactly the invariant that + breaks: registering a `live` marker in pytest.ini while `-m live` resolves + a different set than `--live` gates is the trap. + """ + gated = _deselected(_collect([], {}, MIXED_SCOPE)) + assert gated > 0, "scope must contain live tests for this to mean anything" + assert _selected(_collect(["-m", "live", "--live"], {}, MIXED_SCOPE)) == gated + + +def test_m_live_without_the_flag_selects_nothing() -> None: + """Deselection wins over selection: naming the marker must not be a way + in.""" + assert _selected(_collect(["-m", "live"], {}, MIXED_SCOPE)) == 0 + + +def test_the_artifact_version_tests_are_gated() -> None: + """Four tests that call upload_app and delete artifacts on + bioimage-io/bioengine-worker. Their fixture asserts on the token rather + than skipping, so without the gate they error rather than skip.""" + result = subprocess.run( + [ + sys.executable, + "-m", + "pytest", + "-o", + "addopts=", + "-p", + "no:cacheprovider", + "--collect-only", + "-q", + "tests/test_artifact_version.py", + ], + cwd=REPO_ROOT, + env={**os.environ, "HYPHA_TOKEN": "", "BIOIMAGE_IO_TOKEN": ""}, + capture_output=True, + text=True, + ) + assert "4 deselected" in result.stdout, result.stdout + + +def _module_pytestmarks(relpath: str) -> List[str]: + tree = ast.parse((REPO_ROOT / relpath).read_text()) + return [ + ast.unparse(node.value) + for node in tree.body + if isinstance(node, ast.Assign) + and any(isinstance(t, ast.Name) and t.id == "pytestmark" for t in node.targets) + ] + + +@pytest.mark.parametrize( + "relpath", + [ + # Drives a browser at the deployed app and injects HYPHA_TOKEN into + # its localStorage. Uses only the `page` fixture, so the marker is + # the only thing that gates it. Checked on the source rather than by + # collection because playwright is not installed here -- which is + # also why it collects as an import error, and why nobody noticed. + "tests/apps/cellpose/test_ui_e2e.py", + # Calls upload_app and deletes artifacts on bioimage-io/bioengine-worker. + # The fixture-name rule catches these too, but only because the file + # happens to call its own fixture `hypha_client`; rename it and the + # gate silently stops applying. The marker is what makes it deliberate, + # so assert on the marker and not just on the deselection below. + "tests/test_artifact_version.py", + ], +) +def test_the_modules_a_marker_alone_can_gate_carry_one(relpath: str) -> None: + assert _module_pytestmarks(relpath) == ["pytest.mark.live"] + + +def test_a_malformed_token_does_not_abort_the_session() -> None: + """A payload decoding to a JSON scalar used to raise AttributeError out of + a collection hook, which pytest turns into INTERNALERROR.""" + scalar_payload = base64.urlsafe_b64encode(b"123").decode().rstrip("=") + output = _collect([], {"HYPHA_TOKEN": f"header.{scalar_payload}.sig"}) + assert "INTERNALERROR" not in output + assert "HYPHA_TOKEN -> workspace " in output