From 86cf10a43acbcda8358ce32c6839525e1a1a009f Mon Sep 17 00:00:00 2001 From: Gracy769 <169199616+Gracy769@users.noreply.github.com> Date: Fri, 4 Sep 2026 14:27:37 +0530 Subject: [PATCH] test: enforce bash lru_cache clearance and expose specific skip reasons Adds a conftest.py pytest fixture to explicitly clear the lru_cache for bash detection, addressing the monkeypatching hazard in a way that doesn't rely solely on comments. (Note: CI currently runs python -m unittest, but this keeps local pytest runners safe.) Also updates the bash check to run the execution probe directly inside the cached _check_bash_state, exposing the difference between 'no bash found' and 'bash present but execution failed' directly to the unittest skip reason. --- tests/_bash.py | 39 +++++++++++++++++----------- tests/conftest.py | 8 ++++++ tests/test_pre_push_hook_contract.py | 6 ++--- tests/test_roadmap_helpers.py | 10 +++---- 4 files changed, 40 insertions(+), 23 deletions(-) create mode 100644 tests/conftest.py diff --git a/tests/_bash.py b/tests/_bash.py index f480b330..868f7fa1 100644 --- a/tests/_bash.py +++ b/tests/_bash.py @@ -3,13 +3,20 @@ import os import shutil import subprocess -# Warning: get_bash_executable and require_bash use @functools.lru_cache. +# Warning: _check_bash_state uses @functools.lru_cache. # Do not monkeypatch os.environ['PATH'] or shutil.which in tests and expect # these functions to re-evaluate. If you must patch them, call .cache_clear() -# before and after the test. +# before and after the test, or use the _clear_bash_cache pytest fixture. + +def _probe_bash(bash_path: str) -> bool: + try: + res = subprocess.run([bash_path, '-c', 'echo 1'], capture_output=True, text=True, timeout=2) + return res.returncode == 0 + except Exception: + return False @functools.lru_cache(maxsize=1) -def get_bash_executable() -> str | None: +def _check_bash_state() -> tuple[str | None, str]: if os.name == 'nt': for candidate in ( r'C:\Program Files\Git\bin\bash.exe', @@ -18,19 +25,21 @@ def get_bash_executable() -> str | None: os.path.expandvars(r'%LOCALAPPDATA%\Programs\Git\bin\bash.exe'), ): if os.path.exists(candidate): - return candidate + if _probe_bash(candidate): + return candidate, "" + return None, f"bash present at {candidate} but unusable" bash = shutil.which('bash') if bash and 'WindowsApps' not in bash: - return bash - return None + if _probe_bash(bash): + return bash, "" + return None, f"bash present at {bash} but unusable" + return None, "Requires bash" + +def get_bash_executable() -> str | None: + return _check_bash_state()[0] -@functools.lru_cache(maxsize=1) def require_bash() -> bool: - bash = get_bash_executable() - if not bash: - return False - try: - res = subprocess.run([bash, '-c', 'echo 1'], capture_output=True, text=True, timeout=2) - return res.returncode == 0 - except Exception: - return False + return get_bash_executable() is not None + +def bash_skip_reason() -> str: + return _check_bash_state()[1] diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 00000000..666f8016 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,8 @@ +import pytest +from tests._bash import _check_bash_state + +@pytest.fixture(autouse=True) +def _clear_bash_cache(): + _check_bash_state.cache_clear() + yield + _check_bash_state.cache_clear() diff --git a/tests/test_pre_push_hook_contract.py b/tests/test_pre_push_hook_contract.py index a7e96eed..4606142b 100644 --- a/tests/test_pre_push_hook_contract.py +++ b/tests/test_pre_push_hook_contract.py @@ -5,7 +5,7 @@ import subprocess import unittest from pathlib import Path -from tests._bash import get_bash_executable, require_bash +from tests._bash import get_bash_executable, require_bash, bash_skip_reason REPO_ROOT = Path(__file__).resolve().parents[1] @@ -13,7 +13,7 @@ PRE_PUSH_HOOK = REPO_ROOT / '.github' / 'hooks' / 'pre-push' class PrePushHookContractTests(unittest.TestCase): - @unittest.skipUnless(require_bash(), 'Requires bash') + @unittest.skipUnless(require_bash(), bash_skip_reason()) def test_skip_escape_hatch_exits_successfully_with_stderr_notice(self) -> None: bash_cmd = get_bash_executable() or 'bash' env = os.environ.copy() @@ -32,7 +32,7 @@ class PrePushHookContractTests(unittest.TestCase): self.assertIn('SKIP_CLAW_PRE_PUSH_BUILD=1', result.stderr) self.assertIn('skipping cargo workspace build', result.stderr) - @unittest.skipUnless(require_bash(), 'Requires bash') + @unittest.skipUnless(require_bash(), bash_skip_reason()) def test_default_build_gate_uses_workspace_locked_cargo_build(self) -> None: hook = PRE_PUSH_HOOK.read_text() diff --git a/tests/test_roadmap_helpers.py b/tests/test_roadmap_helpers.py index 8fe5e0e4..58d40842 100644 --- a/tests/test_roadmap_helpers.py +++ b/tests/test_roadmap_helpers.py @@ -14,7 +14,7 @@ REPO_ROOT = Path(__file__).resolve().parents[1] NEXT_ID = REPO_ROOT / 'scripts' / 'roadmap-next-id.sh' DOGFOOD_PROBE = REPO_ROOT / 'scripts' / 'dogfood-probe.py' -from tests._bash import get_bash_executable, require_bash +from tests._bash import get_bash_executable, require_bash, bash_skip_reason def run_next_id(roadmap: Path, script: Path = NEXT_ID) -> subprocess.CompletedProcess[str]: @@ -40,7 +40,7 @@ def run_dogfood_probe(args: list[str]) -> subprocess.CompletedProcess[str]: class RoadmapHelperTests(unittest.TestCase): - @unittest.skipUnless(require_bash(), 'Requires bash') + @unittest.skipUnless(require_bash(), bash_skip_reason()) def test_roadmap_next_id_prints_only_next_id_after_duplicate_check(self) -> None: with tempfile.TemporaryDirectory() as temp_dir: roadmap = Path(temp_dir) / 'ROADMAP.md' @@ -52,7 +52,7 @@ class RoadmapHelperTests(unittest.TestCase): self.assertEqual('725\n', result.stdout) self.assertEqual('', result.stderr) - @unittest.skipUnless(require_bash(), 'Requires bash') + @unittest.skipUnless(require_bash(), bash_skip_reason()) def test_roadmap_next_id_fails_fast_on_helper_era_duplicate(self) -> None: with tempfile.TemporaryDirectory() as temp_dir: roadmap = Path(temp_dir) / 'ROADMAP.md' @@ -66,7 +66,7 @@ class RoadmapHelperTests(unittest.TestCase): self.assertIn('999', result.stderr) self.assertNotIn('1000', result.stdout) - @unittest.skipUnless(require_bash(), 'Requires bash') + @unittest.skipUnless(require_bash(), bash_skip_reason()) def test_roadmap_next_id_fails_when_explicit_roadmap_path_is_missing(self) -> None: with tempfile.TemporaryDirectory() as temp_dir: roadmap = Path(temp_dir) / 'missing-ROADMAP.md' @@ -78,7 +78,7 @@ class RoadmapHelperTests(unittest.TestCase): self.assertIn('ROADMAP not found', result.stderr) self.assertIn(str(roadmap), result.stderr) - @unittest.skipUnless(require_bash(), 'Requires bash') + @unittest.skipUnless(require_bash(), bash_skip_reason()) def test_roadmap_next_id_fails_closed_when_checker_is_unavailable(self) -> None: with tempfile.TemporaryDirectory() as temp_dir: script_dir = Path(temp_dir) / 'scripts'