Fix tests that silently pass despite paired fastq misclassified as SINGLE - #163
Open
corneliusroemer wants to merge 2 commits into
Open
Fix tests that silently pass despite paired fastq misclassified as SINGLE#163corneliusroemer wants to merge 2 commits into
corneliusroemer wants to merge 2 commits into
Conversation
…NGLE This PR makes the tests fail the way they should. Since readtools version 2.3.0 all reads submissions are classified as LIBRARY_LAYOUT=SINGLE even if they are paired (paired fastq, bam, cram) The test that should have caught this lacked an assertion hence silently passed. For tests to pass, the fix at enasequence/readtools#25 or similar has to be merged, released and webin-cli has to point at the new readtools release. Wrongly classified experiments could potentially be fixed. The bug is live since webin-cli version 7.1.0 (2024-03-14) Related to enasequence/readtools#26 Related to enasequence/readtools#27
There was a problem hiding this comment.
Pull request overview
This PR strengthens the reads test suite so that paired FASTQ submissions are correctly asserted as LIBRARY_LAYOUT=PAIRED (and not silently accepted as SINGLE), aligning test behavior with the intended reads classification logic.
Changes:
- Adds an experiment XML regression test ensuring two FASTQ files yield
<PAIRED ...>and not<SINGLE ...>. - Fixes previously ineffective assertions in reads validation tests by explicitly asserting
isPaired()true/false. - Documents/clarifies that a single (interleaved) FASTQ file is not treated as paired by the validator.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/test/java/uk/ac/ebi/ena/webin/cli/context/reads/ReadsXmlTest.java | Adds a regression test asserting paired FASTQ submissions produce paired library layout in generated experiment XML. |
| src/test/java/uk/ac/ebi/ena/webin/cli/context/reads/ReadsValidationTest.java | Makes paired/unpaired expectations explicit (fixing previously no-op assertions) and clarifies behavior for single-file FASTQ submissions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+159
to
+161
| // Pairing is deliberately not derived from a single (interleaved) fastq file: | ||
| // readtools 7517a26 gated paired.set(true) on a two-file submission. | ||
| assertThat(executor.getValidationResponse().isPaired()).isFalse(); |
Comment on lines
+98
to
+100
| assertTrue(experimentXml.contains("<PAIRED")); | ||
| assertTrue(experimentXml.contains("NOMINAL_LENGTH=\"350\"")); | ||
| assertFalse(experimentXml.contains("<SINGLE")); |
Both paired-fastq assertions currently fail: ReadsValidator drops ValidatorWrapper.isPaired(), so LIBRARY_LAYOUT is always SINGLE. The single-file case fails for a second, independent reason - pairing is not derived from an interleaved fastq at all. Use JUnit's Assume rather than @ignore so the rest of each test keeps running: the assertion is placed last, leaving the manifest, validation and generated-file checks to fail hard on regression. Convert back to assertThat(...).isTrue() once readtools is fixed. Also assert the interleaved single-fastq case as paired, per the test name, instead of pinning today's behaviour. enasequence/readtools#26 enasequence/readtools#27 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Related to enasequence/readtools#26
This PR makes the tests fail the way they should.
Since readtools version 2.3.0 all reads submissions are classified as
LIBRARY_LAYOUT=SINGLE even if they are paired (paired fastq, bam, cram)
The test that should have caught this lacked an assertion hence silently
passed.
For tests to pass, the fix at enasequence/readtools#25
or similar has to be merged, released and webin-cli has to point at the
new readtools release.
Wrongly classified experiments could potentially be fixed.
The bug is live since webin-cli version 7.1.0 (2024-03-14)
The 1-file FASTQ test is expected to end up SINGLE even with the bugfix in readtools. Not sure if this is on purpose or not.
Also, all BAM/CRAM are classified as SINGLE even if actually paired, see enasequence/readtools#27