test: Phase 0 verification harness for the 4.0.0 audit remediation - #113
Merged
Conversation
… fixtures Markers were declared in both conftest.py and pyproject.toml and could drift. The blanket DeprecationWarning/UserWarning ignores hid warnings that are part of Auto3D's public contract, so no test could assert them. Adds job_dir/isolated_input fixtures so pipeline tests stop sharing output directories -- the shared state behind the ordering flakiness that kept the end-to-end modules out of CI.
test_convergence_mask_excludes_oscillating and test_convergence_with_energy_stability re-implemented batchopt's flag derivation in the test body and asserted it against itself, so neither could ever fail. test_run_method_includes_gpu_cleanup asserted on the output of inspect.getsource -- a source-text grep, not behavior. Replaced with a test that runs the optimizer and asserts the invariant: a structure dropped for oscillation is never reported Converged=True.
Fix round 1 review finding: the new convergence-flag test asserted the Converged/Dropped_Oscillating invariant only inside a branch with no check that the branch was ever reached, so it could pass without exercising the oscillation path at all on some platform/torch version. Split into two assertions: the invariant now holds unconditionally over every output molecule (so it can never no-op), and a separate assertion confirms the oscillation path was actually taken, with a message that attributes failure to the fixture rather than the invariant.
The custom-NNP tests used E = sum(coords^2), where F = -2*coords is exactly checkable, but asserted only shape and finiteness -- so a sign flip in the adapter force path passed the fast gate. Now asserts values. Adds padding invariance per engine, which is the only test that would catch a change in torchani's -1 masked-atom convention that ANI2xt_no_rep.py documents as an unverified dependency.
Auto3D currently discards one geometric isomer of any achiral molecule with unspecified C=C stereo, because enantiomer([], []) returns True vacuously and FindMolChiralCenters never reports double-bond stereo. Verified: CC=CC enumerates both E and Z, and enantiomer_helper keeps only C/C=C/C. Fumaric and maleic acid collapse the same way despite differing by ~5 kcal/mol. Tautomer enumeration separately strips specified sp3 stereo: none of the five tautomers of C[C@H](C(=O)C)N retains the center. All marked xfail(strict=True) so CI stays green until Phase 2 fixes them, at which point the XPASS forces the marker's removal. Also marks test_enantiomer_helper_keeps_non_chiral, which asserted the buggy behavior was correct.
The M19 test previously drove bare RDKit ETKDG on a SMILES-built mol,
never touching Auto3D's RDKitSdfIsomer/RDKitSdfIsomerAdapter. That made
it permanently decoupled from a Phase 2 fix: it could never XPASS, so
it could never force the xfail marker's removal.
It now writes a genuine flat SDF into job_dir and drives it through the
real create_isomer_engine("rdkit_sdf", ...) -> RDKitSdfIsomerAdapter ->
RDKitSdfIsomer.run() path, then inspects Auto3D's own output SDF grouped
by species name. Both R and S still land as numbered conformers under
one species name, so the test keeps xfailing for the right reason.
Also drops the false C9 coverage claim from the module docstring (no
test here exercises it), and clarifies in the C2 test's docstring that
this stereocenter is genuinely epimerizable via its own enolization --
the bug is SetRemoveSp3Stereo(True) stripping it indiscriminately from
every tautomer, including ones enolized through the ketone's other,
non-stereogenic alpha carbon.
reorder_sdf's tmp+os.replace pattern and the del supp fix from 74474ed had no test at all -- grep for 'os.replace' across tests/ returned nothing, so the fix for the identified weak spot was the untested part. Now covered. opt_geometry does the opposite of reorder_sdf: it truncates the file it just read, so a failure mid-rewrite destroys a completed optimization. Marked xfail(strict=True) pending Phase 6.
The writer stand-ins in TestReorderSdfDurability fully replaced Chem.SDWriter and never touched disk, so both tests passed identically whether reorder_sdf used the tmp+os.replace pattern or a regression that wrote directly to the original path -- confirmed empirically that a direct-write regression produced the same "survived" result as the real implementation under the old stubs. Both stand-ins now wrap the real SDWriter (same _real_sdwriter technique already used by FlakyWriter in TestOptGeometryDurability) so the genuine truncate-on-open and tmp-file creation happen against whatever path reorder_sdf actually opens. Verified against two temporary, reverted regressions: writing directly to the original path now fails test_original_survives_a_writer_failure, and removing the tmp-file cleanup now fails test_no_temp_file_is_left_behind.
A cold model cache behind a firewall is the single most likely first-run failure, and it currently reports 'no 3D structure converged' with three candidate causes -- memory, invalid SMILES, patience -- none of which applies, because the model is constructed inside the spawned worker's blanket except. Also covers a typo'd registry name (resolvable offline via a bundled YAML, so validating it costs nothing) and a corrupted cache entry, which currently fails identically forever with no hint to delete the file.
The two model-failure tests only exercised the worker's blanket except and _finalize_output, never _validate_input. The natural fix for C8/M22 is a parent-side pre-flight before spawning workers (workers run in separate processes with no access to the parent's in-memory model/cache), so a fix landing there would never be observed and the tests would stay xfail forever. Each test now tries _validate_input() first and asserts on whatever it raises; only when that passes through harmlessly (today's behavior) does it fall through to the worker chain. A fix at either layer now makes the corresponding test XPASS.
… M28) CLIConfig constrains k, window, opt_steps, patience, threshold, batchsize_atoms, mpi_np, capacity and memory, and forbids extra keys. Auto3DOptions validates only k and window and only rejects strictly-negative -- so threshold=-1 is accepted end to end, which disables duplicate-conformer removal while the output is still presented as deduplicated. Also covers max_confs=0, k+window together, the charge guard missing from calc_spe, and smiles2mols silently ignoring three documented options while mutating the caller's config. Also adds an M17 test (duplicate InChIKey inputs silently merging in ranking's groupby, dropping one of two colliding SMILES) named in the brief's own Findings line but missing from its Step 1 code.
ANI2xt is constructed with periodic_table_index=False at every site and nothing passes True, so it expects 0-based indices H=0..Cl=6. Only padding.py converts; ASE/thermo.py and the CLI health check pass raw atomic numbers, sending H(Z=1) to the carbon network and C(Z=6) to the chlorine network, with N/O/F/S/Cl out of range entirely. ANI2x gets periodic_table_index=True at both its sites, which is why it is unaffected -- that asymmetry is the evidence this is a defect, not a convention.
A run can lose 9 of 10 chunks and exit 0, because _finalize_output raises only when zero outputs exist. One molecule with an element outside AIMNet2's set takes its entire chunk down via a bare except/continue. And find_smiles_not_in_sdf -- the reconciliation function that would catch all of this -- exists, is exported, is tested, and has zero production callers. Also adds the first end-to-end assertions on actual numbers. The existing end-to-end test's docstring is 'Check that the program runs' with no assertion on molecule count, conformer count, or energy.
test_every_input_is_present_or_reported previously ran 10 valid, mostly duplicate-structure molecules through the default pipeline and hoped at least one failed to converge -- a coin flip, not a defect demonstration, since 6 of those 10 rows share structures already proven to converge reliably elsewhere in the suite. Rewritten to combine the guaranteed-unconvertible sodium counterion with per-molecule job isolation (capacity=1, memory=1): the bad molecule's job now fails alone via optim_rank_wrapper's bare except/continue while the good molecules' jobs succeed independently, so the run produces real output (no OptimizationError) but the failing ID silently vanishes with nothing to reconcile it -- deterministic, not luck-dependent. Also drops the unused _count_records helper.
The Hessian is taken at the pre-optimization geometry whenever BFGS runs, because mol's conformer is synced only at the end of do_mol_thermo while the energy and moments of inertia come from the relaxed atoms. Nothing signals the mismatch, since the written coordinates are the relaxed ones. Nothing checks opt.run()'s return, so G is reported for non-minima, and the documented opt_tol is used only in a fallback branch. One malformed SDF record aborts a whole batch because the accessors run before the try -- SPE.py filters these for exactly this reason and thermo.py does not.
vib_hessian has exactly one production caller, do_mol_thermo, and the fix for the stale-Hessian bug most plausibly lands as a reordered sync inside that function rather than inside vib_hessian itself. Calling vib_hessian directly would never observe a fix made at that layer, so the test now drives do_mol_thermo and records what its real vib_hessian call returns via a pass-through spy -- catching a fix at either layer while keeping the same non-probabilistic geometry-identity assertion.
The slow job ran only three files; the end-to-end modules (test_auto3D, test_SPE, test_thermo, test_isomer_engine, test_tauto) ran in no CI job at all, so 55 of 744 test functions never executed -- including the only eV-vs-Hartree guard and the only padded-slot force guard. Task 1's isolation fixtures remove the shared output directories behind the ordering flakiness that motivated the exclusion. Adds a model-cache warm step that fails the job explicitly when the registry is unreachable, so an unavailable gate can never be mistaken for a passing one. Also lints tests/ and verifies clean collection separately, since --continue-on-collection-errors hides import failures.
… rules test_isomer_engine.py minted job directories from second-resolution time.strftime with no disambiguator in two tests, while three later tests in the same file already append a unique suffix for exactly this reason -- append a uuid4 fragment at the two unpatched sites so a same-second collision or a leftover directory from a prior failed run can no longer kill an unrelated test. Also drops F811 and SIM222 from the tests/ ruff narrowing added for CI wiring: both were firing on real bugs, not style debt -- a duplicate TestCombineSmi class silently shadowed one class's only test, and a CLI console test asserted "... or True", which can never fail. Renamed the shadowed class and fixed the assertion instead of silencing the rules that caught them.
28 xfail tests (27 strict=True, one strict=False by design), each failing on a426cf4 and mapped to the phase that owns its fix per the audit remediation design doc. When a phase lands, its strict tests XPASS and strict=True forces the marker's removal in the same PR -- so the gate is a tripwire in the code rather than a note in a PR description. Also records the passing coverage Phase 0 adds for gaps that had no test at all, why five findings are deliberately absent from the red list, per-phase handoff notes surfaced during the task loop (M19's fix site, M8's property names, C8/M22's XPASS conditions, M17's two independent sites), and watch items for the first CI slow run.
- Rebind the C2 tautomer-stereo tripwire to drive Auto3D's real rdkit tautomer engine (create_tautomer_engine -> TautomerEngine.rd_taut) instead of a bare RDKit TautomerEnumerator, so the xfail reproduces an Auto3D defect rather than upstream RDKit behavior. - Tighten the C1 enantiomer-helper marker in test_utils_stereochemistry.py from strict=False to strict=True; its body already asserts correct behavior, so the non-strict escape hatch was no longer justified. - Correct the slow-tier CI comment to describe what actually protects the five newly un-skipped modules (job naming, serial execution, GPU teardown) instead of a job_dir/isolated_input fixture none of them use, and drop the no-op -p no:randomly flag. - Add a non-vacuity guard to the M8 stationary-point-gating test so it cannot pass vacuously when a record never reports G_hartree, and fix an unrelated molecule-name/SMILES mismatch in the same fixture. - Expand the phase0 red list with handoff notes for Phases 1, 3, and 5 uncovered by this review (pad_from_mols 4-tuple fallout, Auto3DError vs bare ValueError, Thermo_failed naming precedent), two new slow-tier watch items, the deleted GPU-cleanup test, and a note on untracked working docs. No production code changes; git diff a426cf4 HEAD -- src/ remains empty.
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.
Builds the verification harness that the six audit-remediation phases will be measured against. No production code changes —
git diffagainst the base is empty undersrc/.The mechanism
Each of the 28 new tests asserts the behavior the code should have, which today's code violates, and carries
@pytest.mark.xfail(strict=True, reason="<finding-id>: ..."). The test fails now, so CI stays green. When the owning phase fixes the defect the test XPASSes, andstrict=Trueturns that into a hard failure — forcing the marker's removal in the same PR. The markers are tripwires in the code rather than a checklist in a document.docs/superpowers/plans/2026-07-30-phase0-red-list.mdmaps every marker to its owning phase, with handoff notes for the traps a fix could otherwise walk into.Coverage
28 tripwires — 19 fast-tier, 6 slow-tier, 3 requiring
torchani. They cover the stereochemistry identity defects (E/Z isomers discarded as enantiomers; tautomer enumeration erasing specified centers), the ANI2xt atomic-number/species-index mismatch, the thermochemistry Hessian-geometry mismatch, silent molecule loss with a zero exit code, model-load failures misreported as convergence failures, and the configuration validation gaps.Also fixed
Six tests that could never fail: three that re-implemented production logic in the test body or grepped
inspect.getsource, a duplicatedTestCombineSmiclass that shadowed an earlier one so its test never ran, and anassert ... or True. Theruffconfig now keeps F811 and SIM222 enabled fortests/so these cannot recur silently.New passing coverage where there was none: analytic force values on the toy NNP (a sign flip previously passed the gate), per-engine padding invariance, and the atomic-rewrite failure path in
reorder_sdf.CI
The slow job now runs the full
-m slowtier instead of three files — five end-to-end modules previously ran in no job at all. A model-cache warm step hard-fails when the registry is unreachable, so an unavailable gate can never read as a passing one.tests/is now linted.Verification
680 passed, 9 skipped, 19 xfailed, 0 xpassed, 0 failed.
ruff check src/Auto3D/ tests/clean.Note: 9 of the 28 tripwires have not been executed — they need a real model or
torchani, unavailable on the development box — and are asserted red by source-level tracing.strict=Truefails safe there: an unexpectedly-passing test goes red rather than dark. The red list names four specific items to watch on the first CI slow run.