Skip to content

fix(tests): skip network-bound version checks in the pre-push gate#623

Merged
joshjhall merged 1 commit into
mainfrom
feature/issue-615
Jun 28, 2026
Merged

fix(tests): skip network-bound version checks in the pre-push gate#623
joshjhall merged 1 commit into
mainfrom
feature/issue-615

Conversation

@joshjhall

Copy link
Copy Markdown
Owner

Summary

Fixes the multi-minute git push stalls golems hit under concurrent load
(#615). The lefthook pre-push hook runs tests/run_changed_tests.sh; when a
foundational file (tests/framework.sh, tests/framework/*, Dockerfile)
changes, that runner maps to ALL and execs the whole unit suite — which
includes tests/unit/bin/check-versions.sh, a test that invokes the real
bin/check-versions.sh and curls api.github.com once per tracked tool. With
several golems pushing at once those calls serialize and a push wedges for
minutes (observed: golem-599 ~20 min).

Approach (proposed solution #1)

Introduce one opt-in env flag, SKIP_NETWORK_TESTS, consulted through a
shared network_tests_disabled() helper in tests/framework.sh:

  • tests/run_changed_tests.sh (the pre-push runner) exports
    SKIP_NETWORK_TESTS=1 (overridable via ${VAR:-1}), so the push gate stays
    offline-safe on both the ALL path and per-file invocations.
  • CI is unaffected — it invokes run_unit_tests.sh directly and leaves the
    flag unset, so the full network matrix still runs there.
  • The 3 live-script tests in check-versions.sh and the existing
    check_network() guard in version-resolution.sh honor the flag.
  • A map_to_test() arm maps the runner to its new regression suite so future
    edits to it are exercised at push time.

No cross-worktree cache, no push lock, no weakening of foundational-change
coverage — the lowest-risk of the four options in the issue.

Test plan

  • SKIP_NETWORK_TESTS=1 ./tests/unit/bin/check-versions.sh → 3 live tests skip,
    25 mock tests pass, zero api.github.com calls.
  • SKIP_NETWORK_TESTS=1 ./tests/unit/base/version-resolution.sh → 16 network
    tests skip in ~0.6s.
  • Flag off (CI behavior): both suites run the live tests unchanged.
  • New tests/unit/run-changed-tests.sh (9 assertions) locks the wiring in,
    including the unset/CI path.
  • This PR's own push ran the full ALL-mapped suite (3128 tests) offline in
    57s — the exact scenario that previously stalled.

Review findings

Adversarial pre-PR review ran (5 dimensions + judge + gatekeeper). All findings
fixed on this PR, none deferred:

  • Fixed (blocking): bash -c BASH_SOURCE fragility in the behavioral test
    (rewritten to toggle the flag in-process); missing map_to_test() arm for the
    runner.
  • Fixed (deferrable, folded in): split multi-assertion test into one
    assertion per run_test; added the unset/CI-path behavioral case; bare curl
    command curl (full-path policy); guard check now asserts each live test by
    name instead of a >=3 count.

Closes #615

The lefthook pre-push hook runs tests/run_changed_tests.sh. When a
foundational file (tests/framework.sh, tests/framework/*, Dockerfile)
changes, that runner maps to ALL and execs the whole unit suite, which
includes tests/unit/bin/check-versions.sh — a test that invokes the real
bin/check-versions.sh and curls api.github.com once per tracked tool.
Under concurrent golems those calls serialize and a git push stalls for
minutes (observed: golem-599 wedged ~20 min).

Add a SKIP_NETWORK_TESTS flag (via a shared network_tests_disabled helper
in tests/framework.sh). The pre-push runner exports it so the push gate
stays offline-safe; CI invokes run_unit_tests.sh directly and leaves it
unset, so the full network matrix still runs there. Guard the 3
live-script tests in check-versions.sh and short-circuit the existing
check_network() helper in version-resolution.sh. Add a map_to_test() arm
so editing the runner triggers its own regression suite, and add
tests/unit/run-changed-tests.sh to lock the wiring in.

Closes #615

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pre-push: network-bound check-versions test in the full unit suite stalls golem pushes under concurrent load

2 participants