Splitting out gatk-testUtils as a separate artifact #5112

lbergelson · 2018-08-14T20:39:31Z

this allows us to remove the compile time dependencies minicluster and testng and convert them to dependencies of this new artifact
moving utils.test package to testutils and a new source root

the new dependency structure looks like

main <- testUtils
^ ^
test

one side effect was that commons.math is no longer imported, we were accidentally using this in some places instead of commons.math3 which is what we wanted.

lbergelson · 2018-08-14T20:50:38Z

build.gradle

@@ -325,6 +336,12 @@ dependencies {
    // Required for SV Discovery machine learning
    compile group: 'biz.k11i', name: 'xgboost-predictor', version: '0.3.0'

+    testUtilsCompile sourceSets.main.output
+    testUtilsCompile 'org.testng:testng:' + testNGVersion //compile instead of testCompile because it is needed for test infrastructure that needs to be packaged


need to update this comment...

lbergelson · 2018-08-14T20:51:22Z

@jamesemery @cmnbroad This fixes a personal pet peeve of mine, which is that we're packaging a bunch of test classes as a runtime dependency. It splits out a new test artifact which contains just the classes in utils.test and allows it to be published and consumed separately from the rest of the gatk. This allows us to make testng and minicluster no longer compile dependencies

codecov-io · 2018-08-14T22:14:34Z

Codecov Report

Merging #5112 into master will increase coverage by 0.145%.
The diff coverage is 50%.

@@               Coverage Diff               @@
##              master     #5112       +/-   ##
===============================================
+ Coverage     86.507%   86.652%   +0.145%     
+ Complexity     29260     28982      -278     
===============================================
  Files           1814      1799       -15     
  Lines         135635    134495     -1140     
  Branches       15063     14924      -139     
===============================================
- Hits          117334    116543      -791     
+ Misses         12833     12546      -287     
+ Partials        5468      5406       -62

Impacted Files	Coverage Δ	Complexity Δ
...ollectQualityYieldMetricsSparkIntegrationTest.java	`100% <ø> (ø)`	`5 <0> (ø)`	⬇️
...hellbender/tools/CountVariantsIntegrationTest.java	`100% <ø> (ø)`	`4 <0> (ø)`	⬇️
...ne/spark/datasources/ReadsSparkSourceUnitTest.java	`91.852% <ø> (ø)`	`25 <0> (ø)`	⬇️
...ExampleReadWalkerWithReferenceIntegrationTest.java	`100% <ø> (ø)`	`3 <0> (ø)`	⬇️
...park/pipelines/PrintReadsSparkIntegrationTest.java	`88.293% <ø> (ø)`	`30 <0> (ø)`	⬇️
...ls/genomicsdb/GenomicsDBImportIntegrationTest.java	`89.356% <ø> (ø)`	`73 <0> (ø)`	⬇️
...ls/CalculateGenotypePosteriorsIntegrationTest.java	`95.082% <ø> (ø)`	`13 <0> (ø)`	⬇️
...er/tools/funcotator/FuncotatorIntegrationTest.java	`84.305% <ø> (ø)`	`72 <0> (ø)`	⬇️
...bender/tools/walkers/qc/PileupIntegrationTest.java	`90.476% <ø> (ø)`	`6 <0> (ø)`	⬇️
.../PathSeqBuildReferenceTaxonomyIntegrationTest.java	`87.273% <ø> (ø)`	`8 <0> (ø)`	⬇️
... and 210 more

cmnbroad

Glad we're doing this - just a couple of questions/suggestions.

cmnbroad · 2018-08-15T12:44:41Z

build.gradle

+    testUtils
+}
+
+configurations {


Minor question - is there any significance to the fact that this configurations closure is separate from the other one ? Can they be consolidated ?

I don't think there is any significance, I'll combine them.

cmnbroad · 2018-08-15T12:52:09Z

src/main/java/org/broadinstitute/hellbender/utils/python/StreamingPythonScriptExecutor.java

@@ -293,7 +295,9 @@ protected Process getProcess() {
    public void terminate() {
        if (dataTransferFIFOWriter != null) {
            if (asyncWriter != null) {
-                Assert.assertTrue(asyncWriter.terminate());
+                if(!asyncWriter.terminate()){
+                    throw new GATKException("failed to close asynWriter");


Two comments:
1 Wow.
2) asynWriter -> asyncWriter.

Fixed. It's easy to end up accidentally depending on things you don't want to be when they're in your classpath...

cmnbroad · 2018-08-15T13:25:13Z

build.gradle

+            addFilter('gatk-test-utils') {artifact, file ->
+                artifact.name == 'gatk-test-utils'
+            }
+


Can we factor this out into a named (def =) closure so it can be shared ?

Yes, done, although it doesn't save that much typing.

cmnbroad · 2018-08-15T13:33:37Z

build.gradle

+    testUtilsCompile 'org.apache.hadoop:hadoop-minicluster:' + hadoopVersion
+
+    testCompile sourceSets.testUtils.output
+
    testCompile "org.mockito:mockito-core:2.10.0"


Can/should mockito be moved to testUtilsCompile ?

It can, but there's no reason to do so. Nothing in testUtils uses it. We can move it when / if we need it. Best to keep dependencies scoped as narrowly as possible I think. Similar issue with jimfs. I suspect your ArgumentsBuilder branch will require jimfs to move to testUtils.

magicDGS

This is so cool!! Finally cleaning up the dependencies for test classes. Some comments and this should close #1481 and #3567

magicDGS · 2018-08-16T06:16:25Z

build.gradle

@@ -279,8 +290,6 @@ dependencies {
    }

    compile 'org.jgrapht:jgrapht-core:0.9.1'
-    compile 'org.testng:testng:' + testNGVersion //compile instead of testCompile because it is needed for test infrastructure that needs to be packaged
-    compile 'org.apache.hadoop:hadoop-minicluster:' + hadoopVersion


Finally these two out of my toolkit dependencies!

magicDGS · 2018-08-16T06:17:12Z

...roadinstitute/hellbender/tools/funcotator/dataSources/gencode/GencodeFuncotationFactory.java

@@ -22,7 +23,6 @@
 import org.broadinstitute.hellbender.utils.param.ParamUtils;
 import org.broadinstitute.hellbender.utils.read.ReadUtils;
 import org.broadinstitute.hellbender.utils.variant.GATKVariantContextUtils;
-import org.testng.collections.Sets;


magicDGS · 2018-08-16T06:26:45Z

...est/java/org/broadinstitute/hellbender/testutils/DocumentationGenerationIntegrationTest.java

@@ -1,4 +1,4 @@
-package org.broadinstitute.hellbender.utils.test;
+package org.broadinstitute.hellbender.testutils;


This shouldn't be here, but should be a test in living in org.broadinstitute.hellbender.utils.help (no idea why it was here in the first place)

Oh, good call, I didn't actually audit what these things are.

magicDGS · 2018-08-16T06:29:10Z

src/test/java/org/broadinstitute/hellbender/testutils/FuncotatorReferenceTestUtils.java

@@ -1,4 +1,4 @@
-package org.broadinstitute.hellbender.utils.test;
+package org.broadinstitute.hellbender.testutils;


Should this be better moved to the funcotator package, as it isn't really related with testutils?

It has one method that's generally useful, maybe that should move but I'm going to leave it here for now.

magicDGS · 2018-08-16T06:34:03Z

src/test/java/org/broadinstitute/hellbender/testutils/TabCompletionIntegrationTest.java

@@ -1,4 +1,4 @@
-package org.broadinstitute.hellbender.utils.test;
+package org.broadinstitute.hellbender.testutils;


Integration test for help package. Same as DocumentationGenerationIntegrationTest

magicDGS · 2018-08-16T06:38:30Z

src/test/java/org/broadinstitute/hellbender/testutils/testers/AbstractMarkDuplicatesTester.java

@@ -1,4 +1,4 @@
-package org.broadinstitute.hellbender.utils.test.testers;
+package org.broadinstitute.hellbender.testutils.testers;


Should this live in testutils or better in the markduplicate's related package?

Well, if anyone wants to make a new version of mark duplicates this might help. I'm going to leave it here for now.

magicDGS · 2018-08-16T06:46:46Z

src/testUtils/java/org/broadinstitute/hellbender/testutils/testers/CleanSamTester.java

@@ -1,4 +1,4 @@
-package org.broadinstitute.hellbender.utils.test.testers;
+package org.broadinstitute.hellbender.testutils.testers;


This PR is a good place to fix the TODO on top of the class: this only tests one class and it is not part of the framework; it should be added to the GATK-test-sources. And actually it is testing a picard tool, so I am not sure if it is needed at all.

I think we can just delete this, it's unused.

this allows us to remove the compile time dependencies minicluster and testng and convert them to dependencies of this new artifact moving utils.test package to testutils and a new source root the new dependency structure looks like main <- testUtils ^ ^ test

jamesemery

Can you check and see that all of the tests are being run now? I'm not sure i'm convinced these test classes have made it into the docker.

lbergelson · 2018-08-16T19:24:14Z

Tests are run.

jamesemery

Can you check and see that all of the tests are being run now? I'm not sure i'm convinced these test classes have made it into the docker. 👍

lbergelson requested a review from jamesemery August 14, 2018 20:41

lbergelson changed the title ~~Splitting out gatk-test-utils as a separate artifact~~ Splitting out gatk-testUtils as a separate artifact Aug 14, 2018

lbergelson requested a review from cmnbroad August 14, 2018 20:48

lbergelson commented Aug 14, 2018

View reviewed changes

cmnbroad reviewed Aug 15, 2018

View reviewed changes

magicDGS reviewed Aug 16, 2018

View reviewed changes

magicDGS mentioned this pull request Aug 16, 2018

Add jitpack support for SNAPSHOTS #5056

Merged

droazen assigned jamesemery Aug 16, 2018

lbergelson added 5 commits August 16, 2018 12:46

fixes after rebasing

d7f450c

deleting because it was moved to test instead of test utils

73bc6eb

responding to comments

a532ed2

responding to comments

38945dc

lbergelson force-pushed the lb_split_testutils branch from 36cb9b8 to 38945dc Compare August 16, 2018 16:54

jamesemery reviewed Aug 16, 2018

View reviewed changes

lbergelson merged commit e6e6bd3 into master Aug 16, 2018

lbergelson deleted the lb_split_testutils branch August 16, 2018 19:46

magicDGS mentioned this pull request Sep 9, 2018

Decide how to package/organize test-helper sources samtools/htsjdk-next-beta#23

Open

lbergelson mentioned this pull request Sep 21, 2018

Fix maven release and perform manual release of 4.0.9.0 #5212

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Splitting out gatk-testUtils as a separate artifact #5112

Splitting out gatk-testUtils as a separate artifact #5112

lbergelson commented Aug 14, 2018

lbergelson Aug 14, 2018

lbergelson Aug 15, 2018

lbergelson commented Aug 14, 2018

codecov-io commented Aug 14, 2018 •

edited

Loading

cmnbroad left a comment

cmnbroad Aug 15, 2018

lbergelson Aug 15, 2018

cmnbroad Aug 15, 2018

lbergelson Aug 15, 2018

cmnbroad Aug 15, 2018

lbergelson Aug 15, 2018

cmnbroad Aug 15, 2018

lbergelson Aug 15, 2018

magicDGS left a comment

magicDGS Aug 16, 2018

lbergelson Aug 16, 2018

magicDGS Aug 16, 2018

magicDGS Aug 16, 2018

lbergelson Aug 16, 2018

magicDGS Aug 16, 2018

lbergelson Aug 16, 2018

magicDGS Aug 16, 2018

lbergelson Aug 16, 2018

magicDGS Aug 16, 2018

lbergelson Aug 16, 2018

magicDGS Aug 16, 2018

lbergelson Aug 16, 2018

jamesemery left a comment

lbergelson commented Aug 16, 2018

jamesemery left a comment

		@@ -1,4 +1,4 @@
		package org.broadinstitute.hellbender.utils.test;
		package org.broadinstitute.hellbender.testutils;

Splitting out gatk-testUtils as a separate artifact #5112

Splitting out gatk-testUtils as a separate artifact #5112

Conversation

lbergelson commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson commented Aug 14, 2018

codecov-io commented Aug 14, 2018 • edited Loading

Codecov Report

cmnbroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicDGS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesemery left a comment

Choose a reason for hiding this comment

lbergelson commented Aug 16, 2018

jamesemery left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 14, 2018 •

edited

Loading