Biojava genome migration to junit5 - #1126
Conversation
|
In principle this looks good, thank you. However we currently have an issue with tests (due to a failing external resource used during testing) that we'd like to fix before going with this junit5 refactoring. |
josemduarte
left a comment
There was a problem hiding this comment.
Thank you, see some comments below.
There was a problem hiding this comment.
Unused now, should be removed
| @@ -21,25 +21,26 @@ | |||
| package org.biojava.nbio.genome.io.fastq; | |||
|
|
|||
| import org.junit.Assert; | |||
There was a problem hiding this comment.
Import from junit4. Could you move the import and corresponding usages below. AI tells me to use Assertions.assertThrows(X.class, () -> ...)
There was a problem hiding this comment.
Could you remove this import and then replace the usages below. AI recommends:
FileAssert.assertEquals() -> assertIterableEquals(Files.readAllLines(), Files.readAllLines(), "the text")
After that you can get rid of the old dependency in pom.xml
There was a problem hiding this comment.
You can remove this dependency once the comment above is solved (see my other comment). For instance, this is important because this dep brings in the old junit4 transitively
|
Also, please could you merge master into your branch. Then tests should be able to run cleanly. |
Replace FileAssert and leftover JUnit 4 APIs so genome tests compile without junit-addons.
Replace leftover JUnit 4 APIs so genome tests compile without junit-addons.
|
@josemduarte Thank you for reviewing my MR. I've removed junit-addons from dependencies and merged master branch changes but there are still some issues with biojava-protein-disorder tests https://github.com/biojava/biojava/actions/runs/33166795706/job/98834028902?pr=1126 |
josemduarte
left a comment
There was a problem hiding this comment.
Thank you for the changes, one more comment below.
Note that the failures were due to a UniProt server being down.
| new File("src/test/resources/volvox_length_reference.gff3")); | ||
| Assertions.assertEquals( | ||
| Files.readString(new File("src/test/resources/volvox_length_reference.gff3").toPath()), | ||
| Files.readString(gffFile.toPath()), |
There was a problem hiding this comment.
AI tells me that these 2 should be Files.readAllLines instead. Or otherwise tests will fail in windows (due to the hard-coded \n in
There was a problem hiding this comment.
Ok never mind: AI was wrong. It worked in CI. Though we are still seeing flakiness in UniProt servers
All tests in the biojava-genome module were migrated to junit5