rebase Dgs configure plugin packages #5573
Conversation
|
@lbergelson Looks like the build failed with exception... |
There was a problem hiding this comment.
After you fix the compiler error, you should really add a test for this plugin, maybe create a dummy annotation and/or read/filter in the test utils, then write a test that asserts that a tool can indeed detect both those annotations and the default annotations when you ask for a list of the annotations present.
lbergelson
force-pushed
the
dgs_configure_plugin_packages
branch
2 times, most recently
from
152f123
to
6dbc1c5
Compare
|
@jamesemery I think this is actually ready for review this time... |
Codecov Report
@@ Coverage Diff @@
## master #5573 +/- ##
===============================================
- Coverage 87.046% 87.043% -0.003%
- Complexity 31524 31533 +9
===============================================
Files 1928 1930 +2
Lines 145340 145387 +47
Branches 16089 16088 -1
===============================================
+ Hits 126513 126549 +36
- Misses 12966 12981 +15
+ Partials 5861 5857 -4
|
* added tests to show that the Annotation /ReadFilter loading is controlled by the config file * added a new method to base test runToolInNewJvm which lets you run a gatk tool in a clean jvm so you can alter static state like which config file was used. Co-authored-by: Louis Bergelson <louisb@broadinstitute.org>
lbergelson
force-pushed
the
dgs_configure_plugin_packages
branch
from
89ab300
to
3225f8a
Compare
|
|
||
| import org.broadinstitute.hellbender.engine.filters.ReadFilter; | ||
| import org.broadinstitute.hellbender.utils.read.GATKRead; | ||
|
|
There was a problem hiding this comment.
Hmm... Is there an issue with putting these explicitly in the test package?
There was a problem hiding this comment.
They're in the test package specifically so they're not discoverable under normal circumstances. They aren't found by the plugin unless you explicitly tell it to look in this package.
| package org.broadinstitute.hellbender.cmdline.GATKPlugin.testpluggables; | ||
|
|
||
| import org.broadinstitute.hellbender.tools.walkers.annotator.Annotation; | ||
|
|
There was a problem hiding this comment.
Add a comment explaining that these are just for testing purposes and maybe even a link to the exact test so nobody is confused in the future, as these are discovered by reflection and its not easy to track down their purpose.
| @@ -1,5 +1,6 @@ | |||
| package org.broadinstitute.hellbender.utils.config; | |||
|
|
|||
| import com.netflix.servo.util.VisibleForTesting; | |||
There was a problem hiding this comment.
Its nice to think that the gatk is dependent on netflix....
There was a problem hiding this comment.
fixed, it now depends on myspace
| @Test | ||
| public void testConfigFileControlsAnnotationPackages() throws IOException { | ||
| final File output = createTempFile("annotations", "txt"); | ||
| final File configFile = new File("src/test/resources/org/broadinstitute/hellbender/cmdline/GATKPlugin/changePluginPackages.properties"); |
There was a problem hiding this comment.
should probably use the default test dir constants.
| @Test | ||
| public void testConfigFileControlsReadFiltersPackages() throws IOException { | ||
| final File output = createTempFile("annotations", "txt"); | ||
| final File configFile = new File("src/test/resources/org/broadinstitute/hellbender/cmdline/GATKPlugin/changePluginPackages.properties"); |
There was a problem hiding this comment.
Same comment as above.
There was a problem hiding this comment.
same comment as above
| * @param arguments arguments to provide to the tool | ||
| */ | ||
| public static void runToolInNewJVM(String toolName, ArgumentsBuilder arguments){ | ||
| final String javaHome = System.getProperty("java.home"); |
There was a problem hiding this comment.
This will be useful to have
rebase of #4611 with fixes