fix(tests): a run that collects nothing can no longer look green
Three foot-guns in the canonical test runner, each of which cost real debugging time by making an unverified run look verified. 1. Zero collection across the whole run reported success-shaped output. Per-file rc=5 is rewritten to rc=0 so a platform-gated file (every test skipped on this OS) doesn't fail the suite — correct, but it also meant a run where NOTHING was collected anywhere printed "0 tests passed, 0 failed (100% complete)" and, with no failures recorded, could exit 0. Now the run-level guard counts every collected outcome (passed/failed/skipped/errors/xfailed/xpassed): an all-skipped file still passes, but zero-collected-anywhere prints an explicit "✗ NO TESTS RAN — this is NOT a pass" block naming the likely causes and returns 1. 2. A venv without pytest was selected merely for existing. The probe accepted any directory with bin/activate, so in a checkout/worktree without a local .venv it picked the RELEASE venv (~/.hermes/hermes-agent/venv, no pytest). Every file then died with "No module named pytest" and the run reported 0 tests. Candidates are now import-checked for pytest — the same guard the HERMES_PYTHON fallback already applied — and a skipped candidate is named on stderr. 3. Pytest node ids were silently discarded. This runner is file-granular, so `tests/foo.py::TestBar::test_baz` isn't an existing path: discovery dropped it and the run ended "No test files to run" while the selector looked accepted. Node ids are now translated to the FILE plus an inferred `-k` on the leaf name (parametrized ids reduced to the function name), with a note explaining the translation. An explicit caller `-k` wins over the inferred one. Tests: 4 behavior contracts in tests/test_run_tests_parallel.py. Verified by sabotage — reverting the runner fails 3 of the 4 (the fourth pins the pre-existing all-skipped tolerance so fix 1 can't regress it).
This commit is contained in:
teknium1
Teknium
parent
689b51bef6
commit
35b1e57862
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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 <file>`` 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
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue