Formalize extra CI tests: new tests/extra/ pytest suite + URL-backend regression - #285
Open
yarikoptic wants to merge 9 commits into
Open
Formalize extra CI tests: new tests/extra/ pytest suite + URL-backend regression#285yarikoptic wants to merge 9 commits into
yarikoptic wants to merge 9 commits into
Conversation
yarikoptic
added
github_actions
Formalise the "extra" tests that CI runs on top of `git annex test` and DataLad's own battery, so new real-world-regression checks can be added by dropping in a file rather than editing a workflow step. Two parallel prototype implementations are shipped side-by-side (pytest and bats) to compare styles before picking one. New under tests/extra/: - README.md documents the layout, per-test skip semantics, and local invocation for both frameworks. - pytest/ (test_dynlibs.py, test_url_backend.py) and bats/ (helpers.bash, setup_suite.bash, dynlibs.bats, url_backend.bats). - Shared pytest helpers + version-reporting hook live in tests/conftest.py (rather than tests/extra/pytest/conftest.py) so `pytest_report_header` fires regardless of the caller's argument path — pytest loads ancestor conftests eagerly but descendants lazily during collection, too late for the header. Tests: - dynlibs: strace-based guard against libpcre ENOENT-lookup regressions during `git-annex version` / `git-annex init`. Linux-only (auto- skipped where strace is absent). Replaces the inline "Seek of dynlibs" step of the previous test-annex-more job, verbatim, in both frameworks. - url_backend: clones the real repronim/ReproTube DataLad dataset with --no-single-branch (need the git-annex branch for URL-key metadata), asserts `git annex whereis` decodes the URL-encoded key back to `youtube.com/watch?v=`, and `git annex get` retrieves the file (~18 MB, served over HTTPS by the origin remote, so no yt-dlp required). The `get` variant is xfailed (strict=False) below the fix version so old builds do not red the run but a fixed-build regression fails loudly. Bats has no xfail primitive; it uses `skip` as the nearest equivalent (documented in-file). - Fix version 10.20260420 identified from upstream commit 8fd9b67 "factor out extendUrlWithPath and use for git http remote key urls" (Joey Hess, 2026-02-16), first shipped in that release. The bug originally motivating this test is a URL-encoded key path (from `URL--yt&chttps&c%%…` produced by keyFile) yielding an invalid URI when concatenated with a git http remote base — see also the con/git-annex-side patch 20260212-43a3f3aaf2 that shipped the fix ahead of upstream (removed once absorbed upstream). Version-reporting hook (mirrors the dandi-schema pytest_plugin pattern): - pytest prints an "extra-tests tools" line with `git`, `bats`, `yt-dlp`, `youtube-dl`, `strace` first-lines (or `(missing)`); a `platform:` line; the critical fields from `git annex version` (git-annex has no `--json` for version, so the human output is parsed); and a "git-annex releases in this repo newer than installed (VER): N" line derived from `git tag --list "10.*"`. - bats prints the same header via setup_suite.bash -> print_versions in helpers.bash, using bats fd 3 so it's visible without opting into --show-output-of-passing-tests. Workflow template: - `test-annex-more` (Ubuntu/macOS-only, single inline "Seek of dynlibs" step) is replaced by `test-extra`, which runs on all four platforms (Ubuntu, macOS Intel, macOS ARM64, Windows). On each platform it installs the platform-appropriate bats (apt / brew / git-clone), sets up Python 3.12 for pytest, then runs both `python -m pytest -v tests/extra/pytest/` and `bats tests/extra/bats/`. Regenerated the four concrete build-*.yaml files via the existing mkworkflows.py. - REUSE.toml gets `tests/**` added to the MIT/DataLad-Team default block so `reuse lint` stays clean. The bats variant will be eliminated in a follow-up commit once the comparison has been reviewed; kept here so the review is against tree, not against a diff. Co-Authored-By: Claude Code 2.1.220 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five independent senior-engineer reviews of the prior
side-by-side bats + pytest prototype unanimously recommended pytest.
The short version of their reasoning, preserved in
tests/extra/README.md so this question doesn't get re-litigated:
- Cross-platform install cost: bats needs three distinct install
recipes (Ubuntu apt / macOS brew / Windows git-clone bootstrap)
and runs under Git Bash on Windows where `timeout`, `chmod -R u+w`,
and `sort -V` behave subtly differently. pytest is one `pip install`
on all four runners, and Python is already required by test-datalad.
- No real xfail primitive in bats: `skip` cannot distinguish
"known-broken on this version" from "unexpectedly passed" — a
materially weaker regression signal for the URL-backend `get` test.
- Fixtures and shared helpers: `conftest.py` gives cached
`git annex version` parsing, session-scoped module clones, and
parametrization out of the box; the bats port re-implemented each
by hand.
Removed:
- tests/extra/bats/{dynlibs.bats,url_backend.bats,helpers.bash,
setup_suite.bash}.
- The "Install bats" (apt / brew / git-clone) template branches and
the "Run bats suite" step in
.github/workflows/template/build-{{ostype}}.yaml.j2. The Ubuntu
step is now "Install strace" (previously bundled strace + bats).
- Regenerated the four concrete workflow YAMLs via
.github/workflows/template/mkworkflows.py; YAML-linted clean.
tests/extra/README.md rewritten to describe the pytest-only setup and
to explain why bats was evaluated and rejected. `reuse lint` still
clean (45/45). Local pytest run: 4 passed.
Concrete bugs the reviewers also flagged in the pytest side (strace
exit-code check, `from conftest import ...` fragility, xfail
evaluated-at-import without a git-annex-present guard, Windows
read-only rmtree cleanup, dead git_annex_repo fixture) are left for
a follow-up commit to keep the "drop bats" change reviewable in
isolation.
Co-Authored-By: Claude Code 2.1.220 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The fixture was seeded at the start of the extra-tests scaffolding work but no test ever consumed it — test_dynlibs and test_url_backend each build their own repo inline. Reviewer #1 flagged it as dead code; removing it (along with the now-unused Path import) keeps conftest.py to just the version-helper API and the report hook. No behaviour change; 4/4 pytest tests still pass. Co-Authored-By: Claude Code 2.1.220 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer #1/#2/#3/#5 flagged `from conftest import ...` in test_url_backend.py as fragile: it relies on pytest's default --import-mode=prepend adding tests/ to sys.path via the conftest at that level. Under --import-mode=importlib the import fails, and running the test file directly from an unusual cwd is at the mercy of pytest's rootdir heuristics. Fix: move URL_BACKEND_FIX_VERSION and the git_annex_version / git_annex_version_below / git_annex_releases_since helpers into a plain module tests/_helpers.py. conftest.py now only holds the `pytest_report_header` hook (plus its two report-specific helpers `_first_line` and `_git_annex_summary`), and imports the shared bits from `_helpers`. To make `from _helpers import ...` work under any --import-mode, setup.cfg gets a `[tool:pytest]` section with `pythonpath = tests`. This is the pytest-native way to guarantee a directory ends up on sys.path regardless of import-mode / rootdir resolution. Verified both import modes: $ pytest tests # prepend (default) 4 passed $ pytest --import-mode=importlib tests/extra/... # importlib 2 passed Both invocations still print the "extra-tests tools" / git-annex version / releases-newer-than-installed header. Co-Authored-By: Claude Code 2.1.220 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
All five reviewers flagged the same issue: `subprocess.run(["strace", ...])` had no `check=`, no timeout, and no sanity check on stderr. On a runner where strace can't attach (`kernel.yama.ptrace_scope=1` or `=2`, seccomp filters, some container profiles) stderr is empty, the ENOENT count is 0, and the assertion `0 < 7` / `0 < 260` passes vacuously — meaning a regression that reintroduces hundreds of lookups would be missed. Fix: on top of the existing invocation, `_count_enoent` now - passes `timeout=120` so a hung strace fails the test instead of the whole CI job, - raises `RuntimeError` on non-zero exit, echoing the last 2KB of stderr for diagnostics, - raises when stderr contains no syscall / exit lines at all (`+++ exited`, ` ENOENT `, ` = `), which is the observable symptom of a blocked/refused strace. Local run still: 2 passed. Co-Authored-By: Claude Code 2.1.220 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewers #3/#4 flagged: the module-scoped `cloned_repo` fixture used `tmp_path_factory.mktemp` and returned the repo, leaving cleanup to pytest's own later `shutil.rmtree`. On Windows that fails because git-annex sets object files (and their containing key directories) to read-only; the same trap bites POSIX teardown when the containing directory is 0500 — locally reproduced with a `PermissionError` on unlinking `.git/annex/objects/vJ/G8/URL--yt...`. Fix: - Convert the fixture to `yield` + explicit teardown. - Walk the tree bottom-up and chmod every dir and file `u+rwx` before rmtree (mirrors `chmod -R u+w` from the dropped bats teardown). - Keep an rmtree `onexc` / `onerror` handler as a belt-and-braces fallback if a new read-only entry appears between the walk and the unlink. Uses `onexc` on Python 3.12+ and `onerror` on older Pythons (workflow pins 3.12, but local dev on 3.11 stays green). Local run: 4 passed; the parent `/tmp/pytest-of-USER/pytest-N/` directory is now empty on exit. Co-Authored-By: Claude Code 2.1.220 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Moved from tests/_helpers.py to tests/extra/pytest/test_url_backend.py so the fix-version constant, the DEP-3-style comment explaining WHY that specific version, and the xfail marker that uses it all live in one place. _helpers.py drops back to purely generic version-parsing utilities that make no assumption about which bug is being probed. No behaviour change; 4/4 pytest tests still pass. Co-Authored-By: Claude Code 2.1.220 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
On CI we build a specific git-annex and run against it: every failure
is signal. The xfail(strict=False) marker on test_get_url_backend_key
was designed for the case "local dev on an older branch where the bug
is known" — silently accepting the failure keeps CI reviewers focused
on real regressions. That trade-off doesn't apply on CI itself:
there, an XFAIL is a hidden failure.
Fix: gate the xfail condition on `not os.environ.get("CI")`. In
concrete terms:
- Locally, xfail engages when installed git-annex < fix version, as
before. Silent XFAIL / XPASS on older versions; loud FAIL / PASS
on current versions.
- On CI (CI env var set — GitHub Actions, Travis, GitLab, and every
other standard runner set this), the xfail marker is inert. A
regression that happens to hit an old version on CI (e.g. a
workflow_dispatch of a pre-fix commitish) will red the run — which
is what we want, because CI's job is to say pass or fail on the
exact build under test.
Verified: `pytest tests` and `CI=1 pytest tests` both green locally
on 10.20260421 (post-fix, xfail inert either way).
Co-Authored-By: Claude Code 2.1.220 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
=== Do not change lines below ===
{
"chain": [
"024459120c9868becff366655e3e5bc1533dddae"
],
"cmd": "make -C .github/workflows/template",
"exit": 0,
"extra_inputs": [],
"inputs": [],
"outputs": [],
"pwd": "."
}
^^^ Do not change lines above ^^^
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.
Summary
Formalises the "extra tests" that CI runs on top of
git annex testand DataLad's own battery into a real pytest suite undertests/extra/, and adds a first new regression test for URL-backend-key retrieval (theURL--yt&chttps&c%%…bug fixed upstream in 8fd9b67 / release 10.20260420).New in this PR:
tests/extra/— a small, upstreamable pytest suite. Each test declares its own skip / xfail conditions; the default is "run on every platform, skip only where a dep is unavailable".dynlibstest — the previous inlineSeek of dynlibsstep oftest-annex-more, rewritten as pytest with proper timeout + strace exit-code check so a seccomp-restricted runner can't silently pass it.url_backendtest — clonesdatasets.datalad.org/repronim/ReproTube/DataLad/.git/with--no-single-branch(need the git-annex branch for URL-key metadata), assertsgit annex whereisdecodes the URL-encoded key back toyoutube.com/watch?v=, andgit annex getretrieves the ~18 MB video file (served over HTTPS by the origin remote, no yt-dlp needed).test-extraCI job — replacestest-annex-more(which was Ubuntu/macOS only, single inline step), runs on all four platforms (Ubuntu, macOS Intel, macOS ARM64, Windows), onepip install pyteston each.pytest_report_headerprints the versions ofgit,git-annex(all critical fields fromgit annex version),yt-dlp,youtube-dl,strace, plus a "git-annex releases in this repo newer than installed (VER): N" line derived fromgit tag --list "10.*".Design decisions made along the way
tests/extra/directory was prototyped in both frameworks side-by-side; five independent senior-engineer code reviews unanimously recommended pytest. Reasons preserved intests/extra/README.md: (a) cross-platform install cost — bats needs three distinct install recipes and its Windows story via Git Bash is fragile; (b) no realxfailprimitive in bats (onlyskip, which hides regressions); (c)conftest.py+ fixtures give session-cached version parsing and module-scoped clones out of the box. The bats prototype was dropped in0168c89909.xfailmarker.test_get_url_backend_keyisxfail(condition=git_annex_version < 10.20260420, strict=False)so local dev on an old branch doesn't red the run, but on CI ($CIset) the xfail is forcibly disabled — every failure on the specific build under test is loud.tests/_helpers.py+pythonpath = testsin setup.cfg. Sofrom _helpers import …works under any pytest--import-mode. Prior review flaggedfrom conftest import …as fragile._make_tree_writablewalk (mirroringchmod -R u+w) plus anonexcrmtreehandler, because git-annex marks both object files and their containing key directories as 0500.Test plan
pytest testspasses locally on git-annex10.20260421(4 passed).CI=1 pytest testsalso passes (xfail path forcibly disabled).pytest --import-mode=importlib tests/extra/pytest/test_url_backend.pypasses (import path is not conftest-magic-dependent).build-*.yaml.reuse lintstill reports full compliance.build-{ubuntu,macos,macos-arm64,windows}.yamlruns land green on the first cron / dispatch after merge.