tests: Cover --hook-config=--tool-version resolution with pytest - #1004
Draft
MaxymVlasov wants to merge 12 commits into
Draft
Conversation
Adds tests/pytest/tool_version_test.py: black-box subprocess tests against hooks/*.sh asserting on exit code, cache filesystem state, and stdout/stderr - not on bash internals - so they need minimal changes if hooks are ever rewritten in another language. Covers: cache-hit reuse, --tool-version-mode=strict/prefer-local, --tf-path=terraform/opentofu/tofu selector (+ invalid value), the checkov no-op, the actionable not-found error, and one real cache-miss download. Opened against downloadable_and_version_controlled_hooks (not master) specifically to prove these tests actually run green in this repo's GH Actions matrix, not just locally. Adds a .flake8 per-file-ignores entry for the new test file (WPS202/WPS226), following the same precedent already set for tests/pytest/_cli_test.py. See openspec/changes/test-tool-version-resolution/ for the full proposal/design/spec/tasks. Assisted-by: Sisyphus:claude-sonnet-5 claude
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The 'tests' CI job runs pytest against a pre-built sdist tarball (via re-actors/checkout-python-sdist), not a git checkout. hatch.toml whitelisted only src/, tests/, and a few config files for the sdist, so hooks/*.sh - which the new tool_version_test.py invokes as subprocesses - never existed in that job's working directory. Add hooks/ to [build.targets.sdist].include. Also harden tool_version_test.py against this exact failure mode recurring silently: - _run_hook() now fails fast with a clear message if the resolved hook script doesn't exist, instead of letting a generic bash 'No such file or directory' surface only as an unrelated, confusing assertion mismatch several lines down. - Strengthened the two tests whose original assertions only checked for absence of specific substrings (never for a successful exit), so they could vacuously pass even when the hook script failed to execute at all. This is exactly how the CI failure above stayed undetected in 8 of the other tests failed loudly while these 2 passed for the wrong reason. Restructure the test file into two pytest classes (TestCacheAndModeResolution, TestTfPathSelectorAndErrorHandling) instead of adding a WPS202 per-file-ignore to .flake8: grouping methods this way keeps module-level members at 7 (the wemake- python-styleguide default limit) without suppressing the check or reducing test granularity. Only WPS226 (string-literal overuse, same precedent as tests/pytest/_cli_test.py) remains ignored for this file. Assisted-by: Sisyphus:claude-sonnet-5 claude
hooks/_common.sh also sources '$script_dir/../lib_getopt' and, for
--tool-version resolution specifically, reads installer scripts from
'$script_dir/../tools/install/${tool_name}.sh' - both live at the repo
root, sibling to hooks/, so adding hooks/ alone (previous commit)
wasn't enough; CI still failed with 'lib_getopt: No such file or
directory' from inside terraform_tflint.sh's sourced _common.sh.
Verified this time by actually building the sdist locally
(python -m build --sdist), extracting it to a clean directory, and
running the full pytest suite against only that extracted tree
(nothing from the live repo) - the exact scenario CI runs. All 21
tests pass against the real sdist contents, not just against my
working tree.
Assisted-by: Sisyphus:claude-sonnet-5 claude
hooks/_common.sh and tools/install/_common.sh both used
"${var^^}" for uppercasing (e.g. tflint -> TFLINT_VERSION), which
is bash 4+ only. macOS ships bash 3.2.57 by default (Apple's last
GPLv2 release) and this repo explicitly documents 'BASH 3.2.57 or
newer' as a supported baseline (README.md) - CI's macOS runner hit
exactly this with 'bad substitution' once the sdist-packaging fix
let it reach this code for the first time.
Replaced both with a portable 'tr' pipeline. Also fix
tools/install/opentofu.sh's GH_RELEASE_REGEX_SPECIFIC_VERSION: it
required a literal 'v' immediately before the version in the asset
*filename* (only the release *tag* has that 'v', e.g.
tofu_1.12.5_linux_amd64.tar.gz has no 'v'), so --tool-version pinning
for opentofu specifically always fell through to a blank curl URL.
(Same fix as already proposed standalone in PR #1001 against master;
duplicated here since this branch predates that PR and my test
suite's real-download coverage was tflint-only, so it never
exercised this path.)
Verified all three fixes against a real bash 3.2.57 (docker run
bash:3.2, matching macOS's exact version) end-to-end: real tflint
and real opentofu downloads both succeed and produce a working,
correctly-versioned cached binary.
Assisted-by: Sisyphus:claude-sonnet-5 claude
test_real_download_on_cache_miss makes a real, unauthenticated api.github.com call to resolve tflint's latest-releases listing. GITHUB_TOKEN was never actually forwarded into the pytest job's environment - tox.ini's 'pass_env = GITHUB_*' can only pick up a variable that's already there, and reusable-tox.yml never set it. With a 5-python x 4-OS matrix (20 jobs) all potentially sharing GitHub-hosted runners' pooled egress IPs, unauthenticated requests (60/hr per IP, shared with unrelated concurrent traffic) are a well-known source of exactly this kind of intermittent, job-specific failure - matches what was observed: a fast (~0.3s) failure on one matrix cell (pytest-3.12) while the rest of the matrix passed. This wires in the same GITHUB_TOKEN auto-authentication the project already documents as supported for version resolution (README.md, 'Most hooks: Pin a specific tool version') and already uses for Docker image builds - it was simply never connected for this job. Assisted-by: Sisyphus:claude-sonnet-5 claude
covdefaults enforces fail_under=100; my new test file was at 98.32% on two genuinely different gaps: - _fake_path_dir's 'hide' parameter and its exclusion check were dead code by construction: the only caller passes hide='tflint', which was never a member of the coreutils tuple to begin with, so the exclusion branch could never fire. Removed the parameter entirely rather than paper over it - none of these coreutil names will ever collide with a wrapped CLI tool name. - The 'found is not None' guard around each coreutil's shutil.which() result was also dead in every realistic CI target (ubuntu-24.04, macos-15/-intel all ship every one of these 9 tools; Windows is already skipped module-wide). Replaced the silent skip with an explicit assert, which also satisfies mypy's arg-type check on Path.symlink_to (str | None isn't assignable to str) and fails loudly instead of quietly building a broken PATH if a future CI image is ever missing one of them. _run_hook's own 'hook script not found' fail-fast guard is different: it's a genuine, load-bearing diagnostic for a failure mode that has already recurred multiple times in this exact CI (sdist packaging). By definition it can never fire in a passing run, and simulating a broken environment just to exercise it would test the test harness itself rather than the tool-version feature. Marked '# pragma: no cover' with an explanation, rather than writing a test whose only purpose is satisfying the coverage tool. Verified 100% via a bypassed-config coverage run locally (this sandbox's coverage/covdefaults versions can't parse .coveragerc's newer exclude_also option, so ran with --rcfile=/dev/null against just tests/pytest/ instead). Assisted-by: Sisyphus:claude-sonnet-5 claude
On win32, the whole module's tests are skipped (pytestmark), so none
of the file's own helpers/fixtures/test bodies ever execute - that's
by design (this repo doesn't guarantee Windows hook execution), but
it meant total coverage on the windows-2025 CI job cratered to
69.79%, well under covdefaults' fail_under=100.
covdefaults already ships exactly the mechanism for this: platform
pragmas built from {os.name, sys.platform, sys.implementation.name}.
'# pragma: win32 no cover' excludes a def/class (and everything
nested in it) from the coverage requirement only when actually
running on win32 - on every other platform it's inert and the code
is still required to be covered normally.
Applied it to every def/class in this file that's unreachable on
Windows: the two helpers, both fixtures, _run_hook, and both test
classes (methods don't need their own pragma - excluding a class
line cascades to its methods, verified empirically below).
Verified properly this time by actually installing a modern coverage
(>=7.2.2, understands .coveragerc's exclude_also) and covdefaults
locally, rather than reasoning from stale/bypassed local tooling:
100% coverage, 0 missed statements/branches, using the project's
real .coveragerc as-is. Also read covdefaults' own source
(_plat_impl_pragmas) to confirm the activation logic rather than
trusting docs alone: on Linux, 'win32' isn't in the running
platform's tag set, so 'pragma: win32 no cover' lines stay normal,
required, covered code here - matching what was observed (still
100% with zero regressions).
Assisted-by: Sisyphus:claude-sonnet-5 claude
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## downloadable_and_version_controlled_hooks #1004 +/- ##
=============================================================================
+ Coverage 96.53% 98.02% +1.48%
=============================================================================
Files 10 11 +1
Lines 260 455 +195
Branches 7 10 +3
=============================================================================
+ Hits 251 446 +195
Misses 9 9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
MaxymVlasov
changed the title
--hook-config=--tool-version resolution with pytest
MaxymVlasov
force-pushed
the
downloadable_and_version_controlled_hooks
branch
from
July 24, 2026 15:24
b55dcfc to
e24e21b
Compare
…dable_and_version_controlled_hooksatmos_append_2-tests
…dable_and_version_controlled_hooksatmos_append_2-tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Put an
xinto the box if that apply:Description of your changes
Why
Important
Somehow related/conflicts with #990. So far - I can't say what will be chosen, waiting for python-guru's review
Main idea - make some black-box testing for bash hooks on pytest, which then can be applied on hooks reimplementation on Python, when/if it time comes
Or if that's a shity approach - then just make some black-box testing, not in pytest
What
This section was generated by AI.
tests/pytest/tool_version_test.py: black-box pytest suite for the--hook-config=--tool-version=/--tool-version-mode=/--tf-pathselector resolution added ondownloadable_and_version_controlled_hooks. Tests invoke realhooks/*.shscripts as subprocesses and assert on exit code / cache filesystem state / stdout+stderr only - never on bash-internal function names - so they need minimal changes if a hook is ever rewritten in another language.--tool-version-mode=strictvsprefer-local,--tf-path=terraform|opentofu|tofuselector plus an invalid-value error case, the documentedcheckovno-op, the actionable "tool not found" error, and one real end-to-end download on a cache miss..flake8per-file-ignoresentry for the new file (WPS202,WPS226), following the existing precedent already set fortests/pytest/_cli_test.py.openspec/changes/test-tool-version-resolution/.How can we test changes
This section was generated by AI.
pytest tests/pytest/tool_version_test.py -vlocally (10/10 pass) andpytest tests/pytest/ -vfor the full suite (21/21 pass, no regressions in the two pre-existing test modules).downloadable_and_version_controlled_hooks(notmaster) specifically so this repo's GH Actions matrix runs the suite for real - including the one network-dependent download test - rather than only locally.ruff check,ruff format,wemake-python-styleguide,mypy(py3.10/3.12/3.14).Assisted-by
Specific models used per commit are specified in the commit messages.