From df412eeff730d203bee1023a2d1e65ed175c2758 Mon Sep 17 00:00:00 2001 From: sudolulo Date: Tue, 14 Jul 2026 01:41:25 +0000 Subject: [PATCH] test: make three vacuous tests actually test something; drop one duplicate The test suite is not bloated -- 3,701 lines of test code against 3,760 lines of product code, one duplicate pair in 356 tests, 8% single-assertion tests. The waste was not volume, it was four tests that looked like coverage and provided none: - test_parents_are_mounted_before_children: the fixture was already in depth order, so the sort it exists for was never exercised. Deleting `mounts.sort(key=_depth)` passed the whole suite. It now uses datasets whose NAME order differs from their MOUNTPOINT depth, which is the only case the sort is for. - test_it_NEVER_touches_the_current_run: the "current" snapshot was a minute old, so the age floor excluded it regardless and the same-snapname guard never ran. That guard only matters for a run that OUTLIVES the floor -- which a first full upload easily does, and where collecting it would yank the snapshot out from under a backup that is still reading from it. Now tested with a 12-hour-old current run. - the fingerprint/unknown test put the unreadable file in `providers`, which is not broken -- so fingerprint() skipped the whole module via is_broken() and the filter under test never executed. The blip has to land in the module that IS broken. - test_the_real_truenas_versions was a byte-identical copy of test_async_middleware_is_detected under a name promising more. Replaced with the fact actually worth pinning: the flavour probe must read STOCK source, because apply.sh re-runs on an already-patched overlay and our own SNAPSHOT_SYNC block is a plain `def create_snapshot` -- reading it would report a 25.10 box as synchronous and inject the wrong wrapper. All four now fail when the code they name is broken. --- tests/test_compat.py | 27 +++++++++++++++++---- tests/test_truecloud_nested.py | 44 ++++++++++++++++++++++++++++++---- 2 files changed, 63 insertions(+), 8 deletions(-) diff --git a/tests/test_compat.py b/tests/test_compat.py index 32b8bc4..5bad42c 100644 --- a/tests/test_compat.py +++ b/tests/test_compat.py @@ -304,9 +304,23 @@ class TestAsyncFlavour: broken["plugins/cloud_backup/sync.py"] = Unreadable("HTTP 429") assert compat.async_flavour(loader(broken)) is None - def test_the_real_truenas_versions(self): - # Pinning the actual fact this whole port exists for. - assert compat.async_flavour(loader(GOOD)) is True + def test_it_reads_STOCK_source_not_our_own_injected_block(self): + # This was a byte-identical copy of test_async_middleware_is_detected under a + # name that promised more. The fact worth pinning: apply.sh re-runs on an + # ALREADY-PATCHED overlay, so the probe must cut our block off first -- our own + # SNAPSHOT_SYNC wrapper is a plain `def create_snapshot`, and reading it would + # report a 25.10 box as synchronous and inject the wrong flavour. + patched = dict(GOOD) + patched["plugins/cloud/snapshot.py"] = ( + GOOD["plugins/cloud/snapshot.py"] + + "\n# TRUECLOUD_PATCH\n" + + 'def create_snapshot(middleware, path, name="x"):\n return "s", "p"\n' + ) + assert compat.async_flavour(loader(patched)) is True, ( + "the flavour probe read our own injected block and concluded the box is " + "synchronous -- it would then inject a sync wrapper into an async " + "middleware, and every nested backup would break" + ) class TestMiddlewareMethodsWeCall: @@ -530,6 +544,11 @@ class TestATransientNetworkBlipDoesNotWakeAnybody: return [{"ref": "master", "modules": check_files(files)}] def test_an_unreadable_file_does_not_change_the_fingerprint_of_a_broken_ref(self): + # The blip must land in the SAME module that is broken. Put it in `providers` + # (which is healthy) and `fingerprint()` skips the whole module via + # `is_broken(m)` -- so the `state` filter under test never runs and the test + # passes no matter what the code does. `nested` is the broken one here, so the + # unreadable file goes in `nested` too. broken = with_(**{ "plugins/cloud/snapshot.py": "async def create_snapshot(name, path, middleware):\n return 1, 2\n", @@ -537,7 +556,7 @@ class TestATransientNetworkBlipDoesNotWakeAnybody: clean = compat.fingerprint(self._rows(broken)) blipped = dict(broken) - blipped["rclone/remote/b2.py"] = Unreadable("HTTP 429") + blipped["plugins/cloud_backup/sync.py"] = Unreadable("HTTP 429") # nested assert compat.fingerprint(self._rows(blipped)) == clean, ( "a rate-limited fetch changed the fingerprint, so the bot rewrites the " "issue body and then rewrites it back tomorrow" diff --git a/tests/test_truecloud_nested.py b/tests/test_truecloud_nested.py index 69c86cd..455bb1f 100644 --- a/tests/test_truecloud_nested.py +++ b/tests/test_truecloud_nested.py @@ -143,13 +143,28 @@ class TestPlanStaging: ) def test_parents_are_mounted_before_children(self): - # A child's mountpoint dir only exists inside its parent's snapshot, so - # mounting a child first would fail. - mounts, _ = plan() + # A child's mountpoint dir only exists inside its PARENT's snapshot, so + # mounting a child first fails. + # + # The default fixture is already in depth order, so it never exercised the + # sort at all -- deleting `mounts.sort(key=_depth)` passed the whole suite. + # These datasets are deliberately in the WRONG order by name: `Tap/aaa` is + # three levels deep and `Tap/zzz` is one, so anything that preserves input + # order (or sorts by name) mounts the child first and would fail for real. + awkward = [ + ds("Tap", "/mnt/Tap"), + ds("Tap/aaa", "/mnt/Tap/zzz/deep/deeper"), + ds("Tap/zzz", "/mnt/Tap/zzz"), + ds("Tap/mmm", "/mnt/Tap/zzz/deep"), + ] + mounts, _ = plan(datasets=awkward) + seen = set() for _src, target in mounts: if target != ROOT: - assert os.path.dirname(target) in seen + assert os.path.dirname(target) in seen, ( + f"{target} is mounted before its parent exists" + ) seen.add(target) def test_backup_path_below_dataset_root(self): @@ -1088,6 +1103,27 @@ class TestGarbageCollectorSelection: names = [self.CURRENT, "Tap/apps/x@cloud_backup-5-20260714115900"] assert self.collect(names) == [] + def test_it_NEVER_touches_the_current_run_EVEN_WHEN_IT_IS_OLD(self): + # The one that matters, and the one that was not tested: a backup running + # longer than the age floor. The old test's `current` was a minute old, so the + # floor excluded it anyway and the same-snapname guard never ran -- deleting + # that guard passed the whole suite. + # + # A first full upload of a 100 GB pool takes hours. If the GC collected the + # snapshot of the run that is CURRENTLY READING FROM IT, restic would be + # yanked out from under itself mid-backup. + import datetime as dt + + old_current = "Tap@cloud_backup-5-20260714000000" # 12 hours old + names = [old_current, "Tap/apps/x@cloud_backup-5-20260714000000"] + assert tn.stale_snapshot_names( + "cloud_backup-5", old_current, names, + dt.datetime(2026, 7, 14, 12, 0, 0, tzinfo=dt.UTC), + ) == [], ( + "the GC collected the snapshot of the run that is using it. A long first " + "upload would be destroyed mid-flight." + ) + def test_it_NEVER_touches_a_periodic_snapshot(self): assert self.collect(["Tap/apps/x@auto-2026-07-13_03-00"]) == []