Skip to content

fix(tests): require pytest when probing for a virtualenv - #72447

Closed
BKStock wants to merge 1 commit into
NousResearch:mainfrom
BKStock:fix/run-tests-venv-probe
Closed

fix(tests): require pytest when probing for a virtualenv#72447
BKStock wants to merge 1 commit into
NousResearch:mainfrom
BKStock:fix/run-tests-venv-probe

Conversation

@BKStock

@BKStock BKStock commented Jul 27, 2026

Copy link
Copy Markdown

Problem

scripts/run_tests.sh picks a virtualenv on the existence of bin/activate alone:

for candidate in "$REPO_ROOT/.venv" "$REPO_ROOT/venv" "$HOME/.hermes/hermes-agent/venv"; do
  if [ -f "$candidate/bin/activate" ]; then

A checkout carrying a .venv that was created for something else — or that never had the dev extras installed — wins the probe and the run dies with a bare No module named pytest, even when a fully populated venv/ sits right next to it. The probe order reads like a preference list but behaves as a hard first-match.

Observed on a checkout with both directories present:

$ scripts/run_tests.sh tests/agent/test_subdirectory_hints.py
Discovered 1 test files (0 tests) ...
  ║ /Users/…/.venv/bin/python: No module named pytest
=== Summary: 1 files, 0 tests passed, 0 failed (0% complete) ===

Note the summary reports 0 failed — the run looks green at a glance. The practical workaround is to call pytest directly, which is exactly what this wrapper exists to prevent.

Change

A candidate now has to prove import pytest succeeds before it is accepted, so the order expresses preference rather than first-match. Two diagnostics come with it:

  • venvs that exist but lack pytest are named on stderr along with the one actually chosen, so a surprising interpreter selection is visible rather than silent
  • when nothing qualifies, the error lists every path tried and the pip install -e '.[dev]' command to fix it

Verification

Same checkout, after the change:

note: skipped venv without pytest: /Users/…/.venv
      using /Users/…/venv
=== Summary: 1 files, 32 tests passed, 0 failed (100% complete) ===

Both failure paths exercised against scratch directories:

  • no venv at all → error names all three probed paths
  • venv present without pytest → error names it and prints the pip command

bash -n clean.

run_tests.sh selected a venv on the existence of bin/activate alone. A repo
checkout carrying a .venv that was created for something else — or never had
the dev extras installed — won the probe and the run died with a bare
'No module named pytest', even when a fully populated venv/ sat right next to
it. The canonical runner was unusable and the workaround was to invoke pytest
directly, which is exactly what the wrapper exists to prevent.

Probe now requires 'import pytest' to succeed before a candidate is accepted,
so the order (.venv, venv, ~/.hermes/hermes-agent/venv) expresses preference
rather than a hard first-match. Candidates that exist but lack pytest are
reported on stderr along with the venv actually chosen, and when nothing
qualifies the error names every path tried plus the pip command to fix it.

Verified against a checkout whose .venv has no pytest: previously 0 tests
collected, now 32 passed via the wrapper.
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the clear reproduction and targeted test-runner fix. This is an automated hermes-sweeper review; current main already provides the requested behavior.

  • scripts/run_tests.sh:54-62 only selects a POSIX venv after bin/python -c 'import pytest' succeeds, otherwise it continues probing.
  • scripts/run_tests.sh:77-80 reports skipped venvs, and :93-97 reports the missing-pytest condition when no candidate qualifies.
  • Commit 35b1e578621af70c5dbffd2a6fd6c534a0a1a4b7 (fix(tests): a run that collects nothing can no longer look green) added this exact guard and diagnostics; current main has since extended the same path with native Windows venv support in 3e7a11ca2e5fb9a1faa4866dad493b20fc30b0a4.

Closing as implemented on main.

@teknium1 teknium1 closed this Jul 30, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 30, 2026
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:implemented-on-main Sweeper: behavior already present on current main type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants