diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh index 24ad3e8409ec7..ff7e5cf470a95 100755 --- a/scripts/run_tests.sh +++ b/scripts/run_tests.sh @@ -41,14 +41,31 @@ REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" # Probe local venvs first; fall back to the Nix devShell's editable venv # (HERMES_PYTHON is exported by the devShell hook and ships [dev] extras: # pytest, pytest-asyncio, pytest-timeout, ruff, ty). +# +# A candidate must have pytest INSTALLED, not merely exist. The release venv +# at ~/.hermes/hermes-agent/venv has bin/activate but no pytest, so an +# existence-only probe selected it in checkouts/worktrees without a local +# .venv — every file then died with "No module named pytest" and the run +# reported "0 tests passed" (which reads green at a glance even though the +# exit code is 1). Skip such a venv and keep probing instead. VENV="" +SKIPPED_VENVS="" for candidate in "$REPO_ROOT/.venv" "$REPO_ROOT/venv" "$HOME/.hermes/hermes-agent/venv"; do if [ -f "$candidate/bin/activate" ]; then - VENV="$candidate" - break + if "$candidate/bin/python" -c 'import pytest' 2>/dev/null; then + VENV="$candidate" + break + fi + SKIPPED_VENVS="$SKIPPED_VENVS $candidate" fi done +if [ -n "$SKIPPED_VENVS" ]; then + for skipped in $SKIPPED_VENVS; do + echo "▶ skipping venv without pytest: $skipped" >&2 + done +fi + if [ -n "$VENV" ]; then PYTHON="$VENV/bin/python" elif [ -n "${HERMES_PYTHON:-}" ] && [ -x "$HERMES_PYTHON" ] \ @@ -59,8 +76,11 @@ elif [ -n "${HERMES_PYTHON:-}" ] && [ -x "$HERMES_PYTHON" ] \ PYTHON="$HERMES_PYTHON" echo "▶ no local venv — using Nix dev venv via HERMES_PYTHON: $PYTHON" else - echo "error: no virtualenv found in $REPO_ROOT/.venv or $REPO_ROOT/venv," >&2 + echo "error: no virtualenv with pytest found in $REPO_ROOT/.venv or $REPO_ROOT/venv," >&2 echo " and HERMES_PYTHON is not a python with pytest (enter the Nix devShell or create a venv)" >&2 + if [ -n "$SKIPPED_VENVS" ]; then + echo " (skipped for missing pytest:$SKIPPED_VENVS — install dev extras there, or create $REPO_ROOT/.venv)" >&2 + fi exit 1 fi diff --git a/scripts/run_tests_parallel.py b/scripts/run_tests_parallel.py index 797f78b2bda85..9deb46cae04c2 100755 --- a/scripts/run_tests_parallel.py +++ b/scripts/run_tests_parallel.py @@ -361,9 +361,12 @@ def _run_one_file_once( output += "\n" if rc == 5: - # No tests collected — every test in the file was filtered out. - # Treat as a pass; surface info in a slightly distinct status - # so the operator can spot it. + # No tests collected in THIS file — legitimate per-file: a + # platform-gated or fully-marker-filtered file (e.g. a win32-only + # suite on Linux) collects nothing and must not fail the suite. + # Tolerated here; the RUN-level guard in main() still fails when + # NOTHING was collected across every file, so a broken invocation + # (venv without pytest, -k that matches nothing) can't report green. rc = 0 summary = _parse_pytest_summary(output) subproc_wall = time.monotonic() - subproc_start @@ -794,6 +797,46 @@ def main() -> int: i += 1 args = parser.parse_args(our_args) + + # ── Node-id selectors → file + ``-k`` filter ──────────────────────────── + # This runner is FILE-granular: it spawns one ``pytest `` per test + # file. A pytest node id (``tests/foo.py::TestBar::test_baz``) is not an + # existing path, so discovery silently dropped it and the run exited with + # "No test files to run" — the selector looked accepted but nothing ran. + # Translate instead: run the FILE and narrow with ``-k`` on the last + # segment, which is what the caller meant. + node_id_selectors: List[Tuple[str, str]] = [] + if args.paths_positional: + translated: List[str] = [] + for raw in args.paths_positional: + if "::" not in raw: + translated.append(raw) + continue + file_part, _, selector = raw.partition("::") + leaf = selector.rsplit("::", 1)[-1] + # Strip a parametrized id (``test_x[case]``) down to the function + # name; ``-k`` matches substrings, and brackets are -k syntax. + leaf = leaf.split("[", 1)[0] + node_id_selectors.append((raw, leaf)) + translated.append(file_part) + if node_id_selectors: + args.paths_positional = translated + keys = [leaf for _, leaf in node_id_selectors] + expr = " or ".join(dict.fromkeys(keys)) + for raw, leaf in node_id_selectors: + print( + f"note: '{raw}' is a pytest node id; this runner is " + f"file-granular. Running the file with -k {leaf!r}.", + file=sys.stderr, + ) + # Only inject -k when the caller didn't pass one themselves; their + # explicit filter wins over our inferred one. + if not any( + t == "-k" or t.startswith("-k=") or (t.startswith("-k") and len(t) > 2) + for t in bare_passthrough + explicit_passthrough + ): + bare_passthrough = bare_passthrough + ["-k", expr] + # Bare flags run before any explicit ``--`` passthrough so ordering is # intuitive (``run_tests.sh tests/foo.py -q -- --tb=long`` → ``-q --tb=long``). pytest_passthrough = bare_passthrough + explicit_passthrough @@ -894,10 +937,16 @@ def main() -> int: fail_count = 0 tests_passed = 0 tests_failed = 0 + # Every collected outcome, not just pass/fail: a legitimately all-skipped + # (platform-gated) file reports "2 skipped" and must NOT trip the + # nothing-ran guard, whereas a file that died before collection reports + # nothing at all and must. + tests_collected = 0 lock = threading.Lock() - def _on_done(file: Path, started_at: float, fut: "Future[Tuple[Path, int, str, dict[str, int], float]]") -> None: + def _on_done(file: Path, started_at: float, fut: "Future[Tuple[Path, int, str, Dict[str, int], float]]") -> None: nonlocal files_done, tests_done, pass_count, fail_count, tests_passed, tests_failed + nonlocal tests_collected n_tests = test_counts.get(file, 0) try: fpath, rc, output, summary, subproc_wall = fut.result() @@ -921,6 +970,10 @@ def main() -> int: # Accumulate test-level counts from parsed summary. tests_passed += summary.get("passed", 0) tests_failed += summary.get("failed", 0) + tests_collected += sum( + summary.get(k, 0) + for k in ("passed", "failed", "skipped", "errors", "xfailed", "xpassed") + ) file_times.append((fpath, subproc_wall)) if rc == 0: pass_count += 1 @@ -959,6 +1012,27 @@ def main() -> int: pct = min(100, (tests_done / approx_total_tests * 100)) if approx_total_tests else 0 print(f"=== Summary: {len(files)} files, {tests_passed} tests passed, {tests_failed} failed ({pct:.0f}% complete) in {elapsed:.1f}s ({args.jobs} workers) ===") + # Zero tests collected across the WHOLE run is NOT a pass. Per-file rc=5 + # is deliberately tolerated above (platform-gated files), but if NOTHING + # ran anywhere the invocation itself was broken — a venv without pytest, a + # -k/-m filter that matched nothing, or collection erroring everywhere. + # The summary line above reads green at a glance ("0 failed ... 100% + # complete"), which has been misread as a successful verification, so say + # it plainly AND fail the exit code. + no_tests_ran_at_all = bool(files) and tests_collected == 0 + if no_tests_ran_at_all: + print() + print( + "=== ✗ NO TESTS RAN — 0 collected across " + f"{len(files)} file{'s' if len(files) != 1 else ''}. " + "This is NOT a pass. ===" + ) + print( + " Common causes: the selected venv has no pytest; a -k/-m filter " + "matched nothing; or collection errored in every file." + ) + print(" Check the per-file output above for the real error.") + # Flaky files: failed once, passed on the automatic retry. Green, but # loudly reported so they get fixed instead of silently re-flaking. if _FLAKY_RESULTS: @@ -1032,6 +1106,9 @@ def main() -> int: print(f" {_format_file(file, repo_root)}") return 1 + if no_tests_ran_at_all: + return 1 + return 0 diff --git a/tests/test_run_tests_parallel.py b/tests/test_run_tests_parallel.py index db5436f6f1ef8..846c6719ca485 100644 --- a/tests/test_run_tests_parallel.py +++ b/tests/test_run_tests_parallel.py @@ -359,3 +359,75 @@ def test_file_retry_does_not_launder_deterministic_failure(tmp_path: Path) -> No assert proc.returncode == 1, proc.stdout assert "deterministic regression" in proc.stdout assert "FLAKY file" not in proc.stdout + + +# --------------------------------------------------------------------------- +# Zero-collection is not a pass; node ids are translated, not dropped. +# +# Both behaviors were real foot-guns: a run where NOTHING was collected printed +# "0 tests passed, 0 failed (100% complete)" (reads green), and a pytest node id +# (`file.py::Class::test`) was silently discarded by path discovery so the run +# ended with "No test files to run" while looking like an accepted selector. + + +def test_zero_collected_across_run_fails_and_says_so(tmp_path: Path) -> None: + """A -k that matches nothing must FAIL, not report a green summary.""" + probe_dir = _make_probe_dir(tmp_path) + proc = _run_runner(probe_dir, "-k", "zzz_matches_nothing") + assert proc.returncode == 1, proc.stdout + assert "NO TESTS RAN" in proc.stdout + assert "NOT a pass" in proc.stdout + + +def test_all_skipped_file_is_still_a_pass(tmp_path: Path) -> None: + """Per-file zero-collection stays tolerated. + + A platform-gated file (every test skipped) reports "N skipped" — collected, + just not executed — and must NOT trip the nothing-ran guard. + """ + probe_dir = tmp_path / "skipprobe" + probe_dir.mkdir() + (probe_dir / "test_allskipped.py").write_text( + "import pytest\n\n" + "pytestmark = pytest.mark.skip(reason='platform-gated')\n\n" + "def test_one():\n assert True\n\n" + "def test_two():\n assert True\n" + ) + proc = _run_runner(probe_dir) + assert proc.returncode == 0, proc.stdout + assert "NO TESTS RAN" not in proc.stdout + + +def test_node_id_selector_runs_the_named_test(tmp_path: Path) -> None: + """``file.py::test_alpha`` runs that test instead of discovering nothing.""" + probe_dir = _make_probe_dir(tmp_path) + target = probe_dir / "test_flagprobe.py" + repo_root = Path(__file__).resolve().parent.parent + proc = subprocess.run( + [sys.executable, str(repo_root / "scripts" / "run_tests_parallel.py"), + f"{target}::test_alpha", "-j", "1", "--file-timeout", "30"], + cwd=repo_root, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + text=True, timeout=60, + ) + assert proc.returncode == 0, proc.stdout + assert "No test files to run" not in proc.stdout + assert "node id" in proc.stdout # explains the translation + # Ran exactly the one selected test, not both in the file. + assert "1 tests passed" in proc.stdout + + +def test_explicit_k_wins_over_node_id_inference(tmp_path: Path) -> None: + """A caller's own ``-k`` is not overridden by the node-id translation.""" + probe_dir = _make_probe_dir(tmp_path) + target = probe_dir / "test_flagprobe.py" + repo_root = Path(__file__).resolve().parent.parent + proc = subprocess.run( + [sys.executable, str(repo_root / "scripts" / "run_tests_parallel.py"), + f"{target}::test_alpha", "-k", "test_beta", + "-j", "1", "--file-timeout", "30"], + cwd=repo_root, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, + text=True, timeout=60, + ) + # -k test_beta wins: one test ran, and it wasn't filtered to nothing. + assert proc.returncode == 0, proc.stdout + assert "1 tests passed" in proc.stdout