From 146ed0cf5300a8fdb225bd1f9ec758bb5351918e Mon Sep 17 00:00:00 2001 From: Blaise deB Frederick Date: Tue, 29 Sep 2026 22:18:13 -0400 Subject: [PATCH] Split the two self-drive sweeps into one case per corpus pair xdist distributes whole tests, so a test that loops the corpus pins the whole corpus to one worker. The two self-drive sweeps were the suite's slowest tests for that reason -- ~400 s and ~200 s on a single core while the others idled -- and no `-n` could reach inside them. They are now one case per archive/PDF pair, 21 and 6. The pair runs in 133 s against ~600 s, at 583% CPU, and the suite's slowest test is no longer either of them: the floor drops from ~400 s to ~97 s, which is an unrelated conversion sweep, with the worst self-drive case at 82 s. Full suite green at 3943 passed, 26 skipped. The cost is real and each sweep keeps a companion test for it. A loop can count what it drove and assert a floor -- `checked >= 10`, `available >= 4` -- which is what stopped these passing on a corpus that had quietly stopped being discoverable. One case per pair cannot: an empty parameter set is reported as `1 skipped, got empty parameter set`, and zero cases passing reads exactly like every case passing. Verified, and both companions falsified by truncating their lists: `only 2 self-drivable pairs discovered; assert 2 >= 10` and `only 1 of the named exceptions are present; assert 1 >= 4`. Splitting also made the sweep stricter, by forcing a decision the loop could dodge. The loop moved on silently when a printout stopped pairing with its archive (`if not report.matched: continue`), so a pair could leave the sweep without saying so. A case has to decide, so it asserts the pairing -- and all 21 pass, meaning that branch had never once been taken. Note the two guards now overlap: an emptied parameter set is a *skip* in tests.test_exar_patch, so the CI report check added for the duplicate-run fix reports it as well. A guard phrased as coverage rather than outcome catches failure modes invented after it was written. CLAUDE.md's "one test is the floor" paragraph predicted this fix and is rewritten with the measured numbers. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 36 ++++++-- tests/test_exar_patch.py | 182 +++++++++++++++++++++++++++++---------- 2 files changed, 166 insertions(+), 52 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index a16ea5f..9b9d180 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -111,16 +111,36 @@ See `Design.md` for the design and `README.md` for usage. thing before reporting done -- the corpus sweeps are where a change to one reader surfaces in another. - Two things still bound it. **Other sessions share this machine**: a second + One thing still bounds it. **Other sessions share this machine**: a second full run -- and a *serial* one from another session is the usual case -- pushed the load average past 50 and a parallel run to 13-14 minutes, so - check `ps` for a running `pytest` before starting another. And **one test - is the floor**: xdist distributes whole tests, and - `test_driving_every_console_archive_from_its_own_pdf_writes_nothing` takes - ~480 s on one worker, with its sibling `..._self_drive_exceptions_...` at - ~200 s. No `-n` gets the suite under that; splitting those sweeps into - parametrized cases is what would. `--durations=30` names the current - worst offenders. + check `ps` for a running `pytest` before starting another. + + **The single-test floor is gone, and removing it is worth reading as a + pattern.** xdist distributes whole tests, so the two self-drive sweeps -- + one loop each over the whole corpus -- held one worker for ~400 s and + ~200 s while the rest idled, and no `-n` could reach inside a test. They + are now one case per archive/PDF pair, 21 and 6 of them, and the two + together run in **133 s** against ~600 s. `--durations=30` names the + current worst offenders; when the slowest is a *loop*, the fix is + parametrizing it rather than a bigger `-n`. + + What that cost, and it is the part to copy: **a loop can assert things a + parametrized case cannot.** Both sweeps counted what they had driven and + asserted a floor -- `checked >= 10`, `available >= 4` -- which is what + stopped them passing on a corpus that had quietly stopped being + discoverable. One case per pair cannot say that: an empty parameter set is + reported as a *skip*, and zero cases passing reads exactly like every case + passing. So each sweep keeps a companion test asserting the length of the + list its cases are generated from, and splitting a sweep without one + reintroduces the skip-reads-like-a-pass failure the whole guard structure + exists to prevent. + + It also *gained* something, by accident of having to be explicit. The old + loop moved on silently when a printout stopped pairing with its archive + (`if not report.matched: continue`), so a pair could leave the sweep + without saying so. A case has to decide, so it asserts the pairing -- + and all 21 pass, which means that branch had never once been taken. - **Redirect a suite run to a file; never pipe it.** A shell pipeline reports the *last* command's status, so `pytest -n auto | tail -60` exits 0 on a diff --git a/tests/test_exar_patch.py b/tests/test_exar_patch.py index 9040ad8..65ba96b 100644 --- a/tests/test_exar_patch.py +++ b/tests/test_exar_patch.py @@ -2089,8 +2089,61 @@ def _is_voi_dimension(step: object, label: str) -> bool: return kernel in VOI_KERNELS +def _self_drive_pairs() -> list[tuple[str, str]]: + """Every archive/PDF pair the self-drive invariant applies to. + + A pair is in when the archive carries protocols, its printout sits beside + it, and it is not one of the wholly excluded names -- an empty entry in + ``SELF_DRIVE_EXCEPTIONS`` means the pair legitimately disagrees about + everything and nothing can be asserted of it. + + Computed at import so the sweep can be one test case per pair rather than + one test over every pair: xdist distributes whole tests, so a loop here + pins the whole corpus to a single worker. + + Returns + ------- + list of tuple of str + ``(archive path, printout path)``, in corpus order. + """ + pairs = [] + for path, _version in EXAR_PROTOCOL_FILES: + pdf = os.path.splitext(path)[0] + ".pdf" + allowed = SELF_DRIVE_EXCEPTIONS.get(os.path.basename(path)) + if os.path.exists(pdf) and not (allowed is not None and not allowed): + pairs.append((path, pdf)) + return pairs + + +SELF_DRIVE_PAIRS = _self_drive_pairs() +SELF_DRIVE_IDS = [os.path.basename(one) for one, _pdf in SELF_DRIVE_PAIRS] + + +@requires_exar +def test_the_self_drive_sweep_still_covers_the_corpus() -> None: + """The non-vacuity half of the sweep, which parametrizing moved out here. + + The sweep used to count the pairs it drove and assert it had driven at + least ten, which is what stopped it passing on a corpus that had quietly + stopped being discoverable. One test case per pair cannot make that claim + -- an empty parameter set is reported as a skip, and zero cases passing + reads exactly like every case passing -- so the count is asserted here + instead, against the same list the cases are generated from. + + Returns + ------- + None + """ + assert ( + len(SELF_DRIVE_PAIRS) >= 10 + ), f"only {len(SELF_DRIVE_PAIRS)} self-drivable pairs discovered; this proves little" + + @requires_exar -def test_driving_every_console_archive_from_its_own_pdf_writes_nothing() -> None: +@pytest.mark.parametrize(("archive_file", "pdf_file"), SELF_DRIVE_PAIRS, ids=SELF_DRIVE_IDS) +def test_driving_a_console_archive_from_its_own_pdf_writes_nothing( + archive_file: str, pdf_file: str +) -> None: """A self-drive must be a no-op on every console-authored pair. The single-archive version of this check has been here for a while and it @@ -2108,67 +2161,108 @@ def test_driving_every_console_archive_from_its_own_pdf_writes_nothing() -> None holds there too, and it is the one pair that exercises it on a protocol we wrote rather than one we only read. + One case per pair, because this is the slowest test in the suite and + xdist distributes whole tests: as one loop it held a single worker for + ~400 s while the others idled, and no ``-n`` could help. Pairing is + asserted rather than skipped past -- the loop this replaces moved on + silently when a printout stopped matching its archive, which is the one + way a pair could leave the sweep without saying so. + + Parameters + ---------- + archive_file : str + One archive holding protocols, with a printout beside it. + pdf_file : str + That archive's own printed export. + Returns ------- None """ - checked, offenders = 0, [] + name = os.path.basename(archive_file) + allowed = SELF_DRIVE_EXCEPTIONS.get(name) or set() + archive = read(archive_file) + report = build.apply_protocol(archive, parse_document(pdf_file).protocol.to_dict()) + assert report.matched, f"{name} no longer pairs with its own printout" + steps = {one.name: one for one in archive.steps} + written = [ + one + for one in report.applied + if build._moved(one) + and f"{one.step}: {one.label}" not in allowed + and not _is_voi_dimension(steps.get(one.step), one.label) + ] + assert not written, ( + f"driving {name} from its own PDF wrote values: " + f"{[f'{o.step}: {o.label}' for o in written[:3]]}" + ) + + +def _self_drive_exception_pairs() -> list[tuple[str, str]]: + """The named self-drive exceptions that are actually present. + + Returns + ------- + list of tuple of str + ``(archive path, printout path)`` for each exception with a printout. + """ + pairs = [] for path, _version in EXAR_PROTOCOL_FILES: pdf = os.path.splitext(path)[0] + ".pdf" - name = os.path.basename(path) - allowed = SELF_DRIVE_EXCEPTIONS.get(name) - if not os.path.exists(pdf) or (allowed is not None and not allowed): - continue - archive = read(path) - report = build.apply_protocol(archive, parse_document(pdf).protocol.to_dict()) - if not report.matched: - continue - checked += 1 - steps = {one.name: one for one in archive.steps} - written = [ - one - for one in report.applied - if build._moved(one) - and f"{one.step}: {one.label}" not in (allowed or set()) - and not _is_voi_dimension(steps.get(one.step), one.label) - ] - if written: - offenders.append((name, [f"{o.step}: {o.label}" for o in written[:3]])) - assert checked >= 10, f"only {checked} pairs self-driven; this proves little" - assert not offenders, f"driving an archive from its own PDF wrote values: {offenders}" + if os.path.basename(path) in SELF_DRIVE_EXCEPTIONS and os.path.exists(pdf): + pairs.append((path, pdf)) + return pairs + + +EXCEPTION_PAIRS = _self_drive_exception_pairs() +EXCEPTION_IDS = [os.path.basename(one) for one, _pdf in EXCEPTION_PAIRS] @requires_exar -def test_the_self_drive_exceptions_are_all_still_exceptions() -> None: +def test_the_named_exceptions_are_present_to_be_checked() -> None: + """The non-vacuity half of the check below, for the reason given there. + + Returns + ------- + None + """ + assert ( + len(EXCEPTION_PAIRS) >= 4 + ), f"only {len(EXCEPTION_PAIRS)} of the named exceptions are present" + + +@requires_exar +@pytest.mark.parametrize(("archive_file", "pdf_file"), EXCEPTION_PAIRS, ids=EXCEPTION_IDS) +def test_a_self_drive_exception_is_still_an_exception(archive_file: str, pdf_file: str) -> None: """Each named exception must still write something. An exception that has stopped being one is a mapping that improved, and leaving it listed would hide the next real offender behind it. + Parameters + ---------- + archive_file : str + One archive named in ``SELF_DRIVE_EXCEPTIONS``. + pdf_file : str + That archive's own printed export. + Returns ------- None """ - available, quiet = 0, [] - for path, _version in EXAR_PROTOCOL_FILES: - name = os.path.basename(path) - pdf = os.path.splitext(path)[0] + ".pdf" - if name not in SELF_DRIVE_EXCEPTIONS or not os.path.exists(pdf): - continue - available += 1 - parsed = parse_document(pdf).protocol.to_dict() - # Driven per program: a printout covers one protocol, and an archive - # holding the same one twice would otherwise double every scan name - # and be refused wholesale by the repeated-name guard. - moved = 0 - for index, _program in enumerate(read(path).programs): - archive = read(path) - report = build.apply_protocol(archive, parsed, archive.programs[index]) - moved += len([one for one in report.applied if build._moved(one)]) - if not moved: - quiet.append(name) - assert available >= 4, f"only {available} of the named exceptions are present" - assert not quiet, f"these no longer write anything; drop them from the list: {quiet}" + parsed = parse_document(pdf_file).protocol.to_dict() + # Driven per program: a printout covers one protocol, and an archive + # holding the same one twice would otherwise double every scan name + # and be refused wholesale by the repeated-name guard. + moved = 0 + for index, _program in enumerate(read(archive_file).programs): + archive = read(archive_file) + report = build.apply_protocol(archive, parsed, archive.programs[index]) + moved += len([one for one in report.applied if build._moved(one)]) + assert moved, ( + f"{os.path.basename(archive_file)} no longer writes anything; " + "drop it from SELF_DRIVE_EXCEPTIONS" + ) @requires_exar