Removing scala test infrastructure #1640
Conversation
* We tried adding support for scala based tests but it never really caught on. It did complicate the testing infrastructure and reporting considerably and caused complication in the build. * Removing scala and scala test and the associated gradle plugins * Updating gradle 5.2.1 -> 7.5.1 * Converting from the java plugin to the java library plugin remove buggy map initialization and change verbosity remove extraneous logging disable new false positives
| /** | ||
| * Base class for all Java tests in HTSJDK. | ||
| */ | ||
| public class HtsjdkTest extends TestNGSuite { | ||
| public class HtsjdkTest{ |
There was a problem hiding this comment.
This class seems pointless now, although maybe it will accrue some convenient functionality in time.
There was a problem hiding this comment.
Yeah, it is pointless now, I wasn't sure about deleting it or just letting it hang out in case we decide to move some functionality there.
| final Map<String, Integer> readGroupMap = new HashMap<String, Integer>() {{ | ||
| readGroupMap.put("chr1", 1); | ||
| readGroupMap.put("chr2", 2); | ||
| }}; |
gradle/spotbugs-exclude.xml
Outdated
| @@ -71,5 +75,12 @@ | |||
| <Class name="htsjdk.samtools.util.BlockCompressedInputStreamTest" /> | |||
| <Bug pattern="ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD" /> | |||
| </Match> | |||
| <Match> | |||
| <!--This seems to be a false positive due to a bug in how spotify handles breaks with gotos --> | |||
There was a problem hiding this comment.
haha spotify breaks us really ?
| } | ||
| } | ||
|
|
||
| spotbugsTest { |
There was a problem hiding this comment.
Kind of a nit, but this seems like a step backwards.
There was a problem hiding this comment.
I know. There's some weird bug where the newer plugin versions don't work with the task.withType construct i know how to use. There might be a workaround but I tried a few things and couldn't get it to work and this seemed easiest.
|
|
||
| task findScalaAndJavaTypes(type: Exec) { | ||
| description = "Check that Scala files only exist in the scala test dir and that java files do not reside in the scala test dir." | ||
| commandLine './scripts/checkScalaAndJavaFiles.sh' |
There was a problem hiding this comment.
This script can be deleted now ?
| testCompile "com.google.jimfs:jimfs:1.1" | ||
| testCompile "com.google.guava:guava:26.0-jre" | ||
| testCompile "org.apache.commons:commons-lang3:3.7" | ||
| api "gov.nih.nlm.ncbi:ngs-java:2.9.0" |
There was a problem hiding this comment.
What is ngs-java for - SRA ?
|
@lbergelson Were we previously exporting all of our dependencies, and if so, are we going to break clients now by having a narrower surface area ? |
|
@cmnbroad I'm hoping we don't.... I don't believe we export any of the newly implementationed ones in our public API but I don't know how to check that automatically. It might cause issues for people who are depending on ours for their own cod without realizing it, but that should be an easy fix for them. If know how to check the API in a sane way that would be very useful, it's definitely something a tool could exist for. |
|
@yash-puligundla This PR might fix the "no test events" issues you were previously seeing (I seem to remember thinking that scala-test was implicated for that). You might rebase on this once it goes in. |
It did complicate the testing infrastructure and reporting considerably and
caused complication in the build.
This leaves in place the now defunct scala tests:
These will need to be back ported to java. This allows us to move to a new version of gradle and eventually to new jvm versions in a much simplified way. It also looks like it may speed up the tests.