Cover hydrate_for_rescore with a real test, replacing one that could not fail - #56
Merged
Conversation
…not fail The only test guarding the HPC per-file rescore worker was a placeholder: a #[test] holding a TODO and the comment "Intentionally no assertions yet". It could not fail. That is why a `format_version != 2` gate in hydrate_for_rescore -- which, once the reconciliation envelope went to v3, rejected every envelope the pipeline could write and left the worker dead on arrival -- sat in main for nine days with CI green. Replace it with a test that builds the real on-disk trio a worker is handed (scores parquet, first-pass FDR sidecar, current-format reconciliation.json), calls hydrate_for_rescore, and asserts the reconstructed state: stubs from the parquet, the sidecar overlay actually applied (score / q-value values that exist only in the sidecar), the planner action joined from entry_id onto the stub's vec index, and the join-wide stem list. It pins RECONCILIATION_FORMAT_VERSION rather than a literal, so a future format bump cannot silently re-introduce the same class of gate. Verified the test fails against the old gate (reintroduced it locally: "has format_version 3 (expected 2)") and passes without it, so it genuinely catches the regression rather than merely passing. The test lives in pipeline.rs because the parquet and sidecar writers are private to that module; rescore.rs keeps a pointer to it.
There was a problem hiding this comment.
Pull request overview
Improves confidence in the Stage 6/HPC rescore worker by replacing a non-asserting placeholder “hydration” test with a real on-disk round-trip regression test that fails when hydrate_for_rescore rejects the current reconciliation envelope format.
Changes:
- Removed the placeholder hydration test in
rescore.rsand replaced it with a pointer to the real coverage. - Added
hydrate_for_rescore_accepts_a_current_format_envelopeinpipeline.rs, constructing a realistic parquet + sidecar + reconciliation.json trio and asserting hydrated state.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/osprey/src/rescore.rs | Removes the placeholder test module and documents where hydration is now covered. |
| crates/osprey/src/pipeline.rs | Adds an end-to-end regression test for hydrate_for_rescore against the current reconciliation format version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The test's comment claimed the overlay carries score / q-values / PEP, but the assertions only checked score and run_precursor_qvalue on a single entry -- 2 of the 7 f64s the sidecar actually carries, on 1 of the 2 entries. A partial check misses drift in whatever it skips, which is exactly the failure mode this test exists to prevent. Now asserts all seven overlaid fields (score, both run q-values, both experiment q-values, PEP, run protein q-value) on BOTH entries. Also writes the sidecar records in REVERSE order relative to the parquet stubs. load_fdr_scores_sidecar matches by entry_id, not by position; with the reversal and per-entry distinct values, a regression to positional overlay makes entry 7 wear entry 9's numbers and the assertions catch it. That invariant is called out in the loader's own comments and previously had no test.
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.
Summary
The HPC per-file rescore worker had no real test coverage. The one test that named it,
hydrate_round_trips_a_synthetic_pair, was a placeholder: a#[test]containing a TODOand the comment "Intentionally no assertions yet". It could not fail.
That is not a hypothetical gap. It is exactly why the
format_version != 2gate inhydrate_for_rescore(fixed in #55) survived inmainfor nine days with CI green:once the reconciliation envelope went to v3, that gate rejected every envelope the
pipeline could write, and the worker was dead on arrival. A test that cannot fail is
worse than no test, because it reads as coverage.
hydrate_for_rescore_accepts_a_current_format_envelope,which builds the real on-disk trio a worker is handed --
.scores.parquet, thefirst-pass FDR sidecar, and a current-format
reconciliation.json-- then callshydrate_for_rescoreand asserts the reconstructed state: stubs from the parquet, thesidecar overlay actually landing (score and q-values that exist ONLY in the sidecar),
the planner action joined from
entry_idonto the stub's vec index, and the join-widestem list the worker hashes for the merge node.
RECONCILIATION_FORMAT_VERSIONrather than a literal, so a future format bumpcannot silently re-introduce the same class of gate.
pipeline.rsbecause the parquet and sidecar writers are private tothat module;
rescore.rskeeps a pointer to it.Test plan
cargo test --all-features- greencargo clippy --all-targets --all-features -- -D warnings- cleancargo fmt --check- clean!= 2gate locally and confirmed thenew test FAILS with "has format_version 3 (expected 2)", then passes with it
removed. The test catches the regression rather than merely passing alongside it.