testsuite: don't run transport-agnostic archiver tests over rest:// (#9324)#9837
Merged
ThomasWaldmann merged 3 commits intoJul 1, 2026
Conversation
…orgbackup#9324) The remote (rest://) archiver variant spawns a borgstore-server-rest subprocess and does synchronous HTTP-over-stdio round trips per object, so a remote test run costs several times its local twin (e.g. test_prune_repository_example: ~2.8s local vs ~12.2s remote). Profiling the suite showed every one of the 40 slowest tests was a remote_archiver variant. Most of those tests exercise pure command logic that operates on metadata or only formats output (prune scheduling, diff/list/info rendering, rename, delete/undelete, recreate, key handling, debug, return codes, fuse mount) - this behaves identically regardless of transport, and the data path over rest is already covered by the create/extract/check/compact/repo/lock/transfer tests. Switch those files to kinds="local,binary" and document the policy for picking kinds in generate_archiver_tests(). Effect (local, -n auto, no coverage): remote_archiver invocations 374 -> 178; full suite 242s -> 154s (-36%), no test failures. The win on the overloaded macOS CI runners (cold subprocess spawns + coverage) should be relatively larger. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9837 +/- ##
==========================================
+ Coverage 85.14% 85.17% +0.02%
==========================================
Files 93 93
Lines 15337 15346 +9
Branches 2306 2308 +2
==========================================
+ Hits 13059 13071 +12
+ Misses 1586 1581 -5
- Partials 692 694 +2 ☔ View full report in Codecov by Harness. |
check_cmd_test.py's remaining tests only exercise the generic store list/load/delete calls also covered by other remote-tested files (create, extract, ...), so drop remote from its kinds like borgbackup#9324 did elsewhere. Repository.check() does have one transport-specific path though: the REST backend computes pack hashes server-side (nothing downloaded), unlike other backends. No existing test actually exercised a plain --repository-only check catching a corrupted pack via that hash comparison (existing corruption tests all go through --verify-data, which decrypts client-side instead). Add that as its own local+remote test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… tests lock_cmds, repo-create, repo-delete, repo-space and tar all only exercise the generic store list/load/store/delete forwarding that every backend implements identically: - lock_cmds: Lock only does list/store/delete on locks/*, and its one host-specific check (process_alive across hosts) isn't exercised differently by this test infra regardless of "kind" anyway. - repo-create/repo-delete: store.create()/destroy() are trivial 1:1 RPC forwards in the REST backend; still exercised over the wire incidentally via every remaining remote test file's setup (repo-create) and via checks_test.py (repo-delete), which stays on remote. - repo-space: grepped borg's own code - it never calls store.quota() at all, only generic store_list/store/delete on config/space-reserve.*. - tar cmds: do_export_tar/do_import_tar route through the same Archive/Repository.get()/put() machinery as extract/create, so this is redundant with their remote coverage (same category as recreate). create, extract, check and transfer keep "remote" because they exercise backends that genuinely branch per transport (partial reads/content-hash upload headers, server-side hashing, two simultaneous store connections). Rewrote the kinds-picking policy comment in generate_archiver_tests() to state that rule directly instead of "moves real archive data". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
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.
What
Stop running the
remote(rest://) archiver variant for tests whose behavior does not depend on the repository transport.Why
Profiling the testsuite (
-n auto, no coverage) showed every one of the 40 slowest tests was aremote_archivervariant. The remote variant spawns aborgstore-server-restsubprocess and does synchronous HTTP-over-stdio round trips per object, so a remote run costs several times its local twin — e.g.test_prune_repository_example: ~2.8s local vs ~12.2s remote (4.4x).Most of those tests exercise pure command logic that operates on metadata or only formats output (prune scheduling, diff/list/info rendering, rename, delete/undelete, recreate, key handling, debug, return codes, fuse mount, lock_cmds, repo-create/repo-delete, repo-space, tar). That logic is identical regardless of transport — none of it calls a store method whose implementation actually differs per backend — and the wire-transport-specific behavior is already covered by the create/extract/check/compact/transfer tests.
What changed
kinds="local,remote,binary"(or"local,remote") tokinds="local,binary"(or"local").generate_archiver_tests()'s docstring now states the rule directly: only keepremotewhere a backend genuinely branches its implementation per transport (not just "moves real archive data"), and that branch isn't already exercised by another remote-tested file.check_cmd_remote_test.py:check_cmd_test.pyitself droppedremote(its tests only exercise generic store list/load/delete, same as prune/list/etc.), butRepository.check()'s repository-only mode relies onstore.hash(), which the REST backend computes server-side (nothing downloaded) instead of the default download-then-hash. No existing test actually exercised a plain--repository-onlycheck catching pack corruption via that hash comparison (the existing corruption tests all go through--verify-data, which decrypts client-side instead), so this closes a real coverage gap rather than just moving it.remoteis now kept only where the store I/O pattern itself genuinely differs by backend:Repository.get()'sstore.load(offset=, size=)(partial reads — HTTP Range requests for REST vs. seek/read elsewhere) andstore.store()'s content-hash upload header.check_cmd_remote_test.py):store.hash()computed server-side.local,remotechecks_test.py(shared setup/teardown for the check suite).Commands whose remote testing turned out to be redundant on closer look (all confirmed by reading the actual command implementation, not just assumption):
export-tar/import-tar): route through the exact sameArchive/Repository.get()/put()machinery as extract/create, so their own remote run is redundant — same category asrecreate.store.create()/store.destroy()are trivial 1:1 RPC forwards in the REST backend (no content-dependent divergence); still incidentally exercised over the wire by every remaining remote test file's ownrepo-createsetup step, and bychecks_test.py'srepo-delete..quota(— it's never called.repo_space_cmd.pyonly does genericstore_list/store_store/store_deleteonconfig/space-reserve.*, despitequota()being a genuinely backend-specific method in borgstore that borg simply doesn't use.Lockonly does genericlist/store/deleteonlocks/*; its one host-specific check (process_alive()across hosts) isn't exercised differently by this test infra regardless of "kind" anyway (client hostid is the same host either way).Effect
Two independent measurements, both
-n auto, no coverage,-k "not binary":The 374 → 178 → 128 invocation counts and the original 242.7s → 154.2s wall-clock come from the PR author's machine (see the numbers earlier in this description). On top of that, this update was verified with a same-machine, apples-to-apples before/after (12-core machine, checked out the pre-PR commit into the working tree, ran the full archiver suite, then restored the branch and re-ran):
-n auto)remote_archiverinvocations (archiver suite)The one failure in the "before" run (
test_fuse_allow_damaged_files, a FUSE-daemonization timeout) is an unrelated, known-flaky test (stale/slow FUSE mount teardown), not caused by this change — it did not reproduce in the "after" run. Test counts differ between the two runs because kind-pruning also removes the parametrized[remote_archiver]variants themselves (not just their execution time), so "after" collects fewer test IDs overall.Notes for review
The categorization is per-file (the existing granularity). Previously-open borderline calls, now resolved:
remote(covered by create+extract); tar is now dropped too, for the same reason (redundant with create+extract's coverage of the actual wire-specificRepository.get()/put()behavior).remote"since it is borg's integrity command." On closer inspection, only itsstore.hash()-based repository-only corruption check is actually backend-specific; the rest ofcheck_cmd_test.py's 17 tests are transport-agnostic (metadata presence/absence,--verify-datadecryption-based corruption which never touchesstore.hash()). Droppedremotefromcheck_cmd_test.pyand added one small dedicatedcheck_cmd_remote_test.pytest for the hash-comparison path instead — which also fixed a real gap, since no prior test exercised plain--repository-onlycorruption detection under any transport.get_kind() == "remote"branches in the touched files (test_repo_list_from_borg1,test_migrate_lock_alive) were alreadypytest.skip("only works locally"), so nothing was left dead.Refs #9324.
🤖 Generated with Claude Code