test(qchem): don't require an unconverged structure to leave no geometry #651

2026-10-22T19:04:07Z

rvnminers-A-and-N commented

2026-10-22 19:04:07 +00:00

@excho0 found this in #637's coverage runs (see #629): TestARealConformerSetIsOptimised fails at qchem_test.go:592 when xtb, given two atoms in one place, gives up after writing a last geometry. The structure comes back converged:false with an error, as expected, but with a geometry CID that the test said must be empty.

QchemResult.Geometry is documented as the optimised geometry, or the last one reached where the optimisation did not converge, so the result is right and the assertion was too strict. Whether xtb leaves a geometry depends on where it stops, which is why the test passes on some runs and not others.

The test now checks only what the contract promises for that structure: it did not converge, and it says why. The other assertions in the test, the three converged structures matching between SDF and XYZ and the count of converged structures, are unchanged.

A separate PR rather than part of the qchem Compare follow-up, so that #637 isn't held up behind it.

@excho0 found this in #637's coverage runs (see #629): `TestARealConformerSetIsOptimised` fails at `qchem_test.go:592` when xtb, given two atoms in one place, gives up after writing a last geometry. The structure comes back `converged:false` with an error, as expected, but with a `geometry` CID that the test said must be empty. `QchemResult.Geometry` is documented as the optimised geometry, *or the last one reached where the optimisation did not converge*, so the result is right and the assertion was too strict. Whether xtb leaves a geometry depends on where it stops, which is why the test passes on some runs and not others. The test now checks only what the contract promises for that structure: it did not converge, and it says why. The other assertions in the test, the three converged structures matching between SDF and XYZ and the count of converged structures, are unchanged. A separate PR rather than part of the qchem `Compare` follow-up, so that #637 isn't held up behind it.

excho0 approved these changes

2026-10-22 19:05:50 +00:00

excho0 left a comment

Reviewed the focused test change. QchemResult.Geometry explicitly permits the last geometry reached on non-convergence, so dropping only the Geometry == "" assertion makes this test match the result contract while retaining the non-convergence, error, structure-count and converged-count checks. Approval is subject to the required CI checks passing.

/cc @rvnminers-A-and-N

Reviewed the focused test change. `QchemResult.Geometry` explicitly permits the last geometry reached on non-convergence, so dropping only the `Geometry == ""` assertion makes this test match the result contract while retaining the non-convergence, error, structure-count and converged-count checks. Approval is subject to the required CI checks passing. /cc @rvnminers-A-and-N

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Sisyphus/Sisyphus#651