Rebasing and updating, adding tests.

* 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>
broadinstitute · Jan 18, 2019 · 6dbc1c5 · 6dbc1c5
1 parent f07cef9
commit 6dbc1c5
Show file tree

Hide file tree

Showing 11 changed files with 164 additions and 29 deletions.
diff --git a/...java/org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKAnnotationPluginDescriptor.java b/...java/org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKAnnotationPluginDescriptor.java
@@ -34,7 +34,7 @@
  * NOTE: this class enforces that annotations with required arguments must see their arguments, yet this is not currently tested
  *       as no such annotations exist in the GATK.
  */
-public class GATKAnnotationPluginDescriptor  extends CommandLinePluginDescriptor<Annotation> {
+public class GATKAnnotationPluginDescriptor extends CommandLinePluginDescriptor<Annotation> {
     /**
      * At startup, set the plugin package name to the one(s) in the configuration file.
      */
@@ -43,10 +43,10 @@ public class GATKAnnotationPluginDescriptor  extends CommandLinePluginDescriptor
         // Get our configuration:
         final GATKConfig config = ConfigFactory.getInstance().getGATKConfig();
         // Exclude abstract classes and interfaces from the list of discovered codec classes
-        PLUGIN_PACKAGE_NAMES = Collections.unmodifiableList(config.codec_packages());
+        PLUGIN_PACKAGE_NAMES = Collections.unmodifiableList(config.annotation_packages());
     }
 
-    private static final Class<?> pluginBaseClass = org.broadinstitute.hellbender.tools.walkers.annotator.Annotation.class;
+    private static final Class<?> PLUGIN_BASE_CLASS = org.broadinstitute.hellbender.tools.walkers.annotator.Annotation.class;
 
     protected transient Logger logger = LogManager.getLogger(this.getClass());
 
@@ -85,7 +85,7 @@ public class GATKAnnotationPluginDescriptor  extends CommandLinePluginDescriptor
      */
     @Override
     public Class<?> getPluginBaseClass() {
-        return pluginBaseClass;
+        return PLUGIN_BASE_CLASS;
     }
 
     /**
@@ -230,7 +230,7 @@ private void populateAnnotationGroups(final String simpleName, final Annotation
             // extend Annotation, groups are discovered by interrogating annotations for their interfaces and
             // associating the discovered annotations with their defined groups.
             // If a duplicate annotation is added, the group will opt to keep the old instantiation around
-            if ((inter != pluginBaseClass) && (pluginBaseClass.isAssignableFrom(inter))) {
+            if ((inter != PLUGIN_BASE_CLASS) && (PLUGIN_BASE_CLASS.isAssignableFrom(inter))) {
                 Map<String, Annotation> groupIdentity = (discoveredGroups.containsKey(inter.getSimpleName()) ? discoveredGroups.get(inter.getSimpleName()) : new HashMap<>());
                 groupIdentity.putIfAbsent(simpleName, annot);
                 discoveredGroups.put(inter.getSimpleName(), groupIdentity);

diff --git a/...java/org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKReadFilterPluginDescriptor.java b/...java/org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKReadFilterPluginDescriptor.java
@@ -5,7 +5,6 @@
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.broadinstitute.barclay.argparser.ArgumentCollection;
-import org.broadinstitute.barclay.argparser.ClassFinder;
 import org.broadinstitute.barclay.argparser.CommandLineException;
 import org.broadinstitute.barclay.argparser.CommandLinePluginDescriptor;
 import org.broadinstitute.hellbender.cmdline.ReadFilterArgumentDefinitions;
@@ -38,11 +37,10 @@ private void readObject(java.io.ObjectInputStream in)
      */
     private static final List<String> PLUGIN_PACKAGE_NAMES;
     static {
-
         // Get our configuration:
         final GATKConfig config = ConfigFactory.getInstance().getGATKConfig();
         // Exclude abstract classes and interfaces from the list of discovered codec classes
-        PLUGIN_PACKAGE_NAMES = Collections.unmodifiableList(config.codec_packages());
+        PLUGIN_PACKAGE_NAMES = Collections.unmodifiableList(config.read_filter_packages());
     }
 
     private static final Class<ReadFilter> pluginBaseClass = org.broadinstitute.hellbender.engine.filters.ReadFilter.class;

diff --git a/src/main/java/org/broadinstitute/hellbender/utils/config/GATKConfig.java b/src/main/java/org/broadinstitute/hellbender/utils/config/GATKConfig.java
@@ -1,5 +1,6 @@
 package org.broadinstitute.hellbender.utils.config;
 
+import com.netflix.servo.util.VisibleForTesting;
 import org.aeonbits.owner.Accessible;
 import org.aeonbits.owner.Mutable;
 import org.aeonbits.owner.Config.LoadPolicy;
@@ -52,6 +53,9 @@ public interface GATKConfig extends Mutable, Accessible {
      */
     String CONFIG_FILE_VARIABLE_CLASS_PATH = "GATKConfig.classPathToGatkConfig";
 
+    @VisibleForTesting
+    public String DEFAULT_ANNOTATION_PACKAGES = "org.broadinstitute.hellbender.tools.walkers.annotator";
+
     // =================================================================================================================
     // =================================================================================================================
     // System Options:
@@ -147,7 +151,7 @@ public interface GATKConfig extends Mutable, Accessible {
     @DefaultValue("org.broadinstitute.hellbender.engine.filters")
     List<String> read_filter_packages();
 
-    @DefaultValue("org.broadinstitute.hellbender.tools.walkers.annotator")
+    @DefaultValue(DEFAULT_ANNOTATION_PACKAGES)
     List<String> annotation_packages();
 
     // ----------------------------------------------------------

diff --git a/src/main/resources/org/broadinstitute/hellbender/utils/config/GATKConfig.properties b/src/main/resources/org/broadinstitute/hellbender/utils/config/GATKConfig.properties
@@ -73,8 +73,8 @@ spark.executor.extraJavaOptions =
 # ---------------------------------------------------------------------------------
 
 codec_packages = htsjdk.variant, htsjdk.tribble, org.broadinstitute.hellbender.utils.codecs
-read_filter_packages = "org.broadinstitute.hellbender.engine.filters"
-annotation_packages = "org.broadinstitute.hellbender.tools.walkers.annotator"
+read_filter_packages = org.broadinstitute.hellbender.engine.filters
+annotation_packages = org.broadinstitute.hellbender.tools.walkers.annotator
 
 # ---------------------------------------------------------------------------------
 # GATKTool Options:

diff --git a/.../broadinstitute/hellbender/cmdline/GATKPlugin/GATKAnnotationPluginDescriptorUnitTest.java b/.../broadinstitute/hellbender/cmdline/GATKPlugin/GATKAnnotationPluginDescriptorUnitTest.java
@@ -1,28 +1,34 @@
 package org.broadinstitute.hellbender.cmdline.GATKPlugin;
 
-import org.broadinstitute.barclay.argparser.ClassFinder;
 import htsjdk.variant.variantcontext.*;
 import org.apache.commons.io.output.NullOutputStream;
-import org.broadinstitute.barclay.argparser.CommandLineArgumentParser;
-import org.broadinstitute.barclay.argparser.CommandLineException;
-import org.broadinstitute.barclay.argparser.CommandLineParser;
+import org.broadinstitute.barclay.argparser.*;
 import org.broadinstitute.hellbender.GATKBaseTest;
+import org.broadinstitute.hellbender.cmdline.GATKPlugin.testpluggables.TestAnnotation;
 import org.broadinstitute.hellbender.cmdline.StandardArgumentDefinitions;
+import org.broadinstitute.hellbender.cmdline.TestProgramGroup;
 import org.broadinstitute.hellbender.engine.FeatureContext;
+import org.broadinstitute.hellbender.engine.GATKTool;
 import org.broadinstitute.hellbender.engine.ReferenceContext;
 import org.broadinstitute.hellbender.exceptions.GATKException;
+import org.broadinstitute.hellbender.testutils.ArgumentsBuilder;
 import org.broadinstitute.hellbender.tools.walkers.annotator.*;
 import org.broadinstitute.hellbender.tools.walkers.annotator.allelespecific.AS_RMSMappingQuality;
 import org.broadinstitute.hellbender.tools.walkers.annotator.allelespecific.AS_StandardAnnotation;
+import org.broadinstitute.hellbender.utils.config.GATKConfig;
 import org.broadinstitute.hellbender.utils.genotyper.ReadLikelihoods;
 import org.broadinstitute.hellbender.utils.variant.GATKVCFConstants;
 import org.mockito.internal.util.collections.Sets;
+import org.mockito.internal.util.io.IOUtil;
 import org.testng.Assert;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 import org.testng.collections.Lists;
 
+import java.io.File;
+import java.io.IOException;
 import java.io.PrintStream;
+import java.nio.file.Files;
 import java.util.*;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -456,7 +462,7 @@ public void testUseAllAnnotations(){
         List<Annotation> annots = instantiateAnnotations(clp);
 
         ClassFinder finder = new ClassFinder();
-        finder.find(GATKAnnotationPluginDescriptor.pluginPackageName, Annotation.class);
+        finder.find(GATKConfig.DEFAULT_ANNOTATION_PACKAGES, Annotation.class);
 
         Set<Class<?>> classes = finder.getConcreteClasses();
         Assert.assertFalse(classes.isEmpty());
@@ -477,7 +483,7 @@ public void testUseAllAnnotationsDisableToolDefaultAnnotaiotns(){
         List<Annotation> annots = instantiateAnnotations(clp);
 
         ClassFinder finder = new ClassFinder();
-        finder.find(GATKAnnotationPluginDescriptor.pluginPackageName, Annotation.class);
+        finder.find(GATKConfig.DEFAULT_ANNOTATION_PACKAGES, Annotation.class);
 
         Set<Class<?>> classes = finder.getConcreteClasses();
         Assert.assertFalse(classes.isEmpty());
@@ -502,7 +508,7 @@ public void testIncludeAllExcludeIndividual(){
         Assert.assertFalse(annots.stream().anyMatch(a -> a.getClass()==AS_StandardAnnotation.class));
 
         ClassFinder finder = new ClassFinder();
-        finder.find(GATKAnnotationPluginDescriptor.pluginPackageName, Annotation.class);
+        finder.find(GATKConfig.DEFAULT_ANNOTATION_PACKAGES, Annotation.class);
 
         Set<Class<?>> classes = finder.getConcreteClasses();
         classes.remove(Coverage.class);
@@ -628,6 +634,7 @@ public List<String> getKeyNames() {
             return Collections.singletonList("Test");
         }
     }
+
     static class testParentAnnotation extends InfoFieldAnnotation implements ParentAnnotationGroup  {
         boolean dontAnnotate = false;
 
@@ -651,4 +658,37 @@ public List<String> getKeyNames() {
         }
     }
 
+    @CommandLineProgramProperties(summary="test tool to check annotation loading",
+            oneLineSummary = "test tool to check annotation loading",
+            programGroup = TestProgramGroup.class,
+            omitFromCommandLine = true)
+    public static class TestAnnotationsTool extends GATKTool {
+        @Argument(fullName = StandardArgumentDefinitions.OUTPUT_LONG_NAME)
+        String output;
+
+        @Override
+        public boolean useVariantAnnotations(){
+            return true;
+        }
+
+        @Override
+        public void traverse() {
+            String annotations = makeVariantAnnotations().stream()
+                    .map(a -> a.getClass().getSimpleName())
+                    .collect(Collectors.joining("\n"));
+            IOUtil.writeText(annotations, new File(output));
+        }
+    }
+
+    @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");
+        ArgumentsBuilder args = new ArgumentsBuilder();
+        args.addArgument(StandardArgumentDefinitions.ANNOTATION_LONG_NAME, TestAnnotation.class.getSimpleName())
+                .addFileArgument(StandardArgumentDefinitions.GATK_CONFIG_FILE_OPTION, configFile)
+                .addOutput(output);
+        runToolInNewJVM("TestAnnotationsTool", args);
+        Assert.assertEquals(Files.readAllLines(output.toPath()), Collections.singletonList(TestAnnotation.class.getSimpleName()));
+    }
 }
diff --git a/.../org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKReadFilterPluginDescriptorTest.java b/.../org/broadinstitute/hellbender/cmdline/GATKPlugin/GATKReadFilterPluginDescriptorTest.java
@@ -4,19 +4,26 @@
 import htsjdk.samtools.SAMFileHeader;
 import htsjdk.samtools.TextCigarCodec;
 import org.apache.commons.io.output.NullOutputStream;
-import org.broadinstitute.barclay.argparser.CommandLineArgumentParser;
-import org.broadinstitute.barclay.argparser.CommandLineException;
-import org.broadinstitute.barclay.argparser.CommandLineParser;
+import org.broadinstitute.barclay.argparser.*;
 import org.broadinstitute.hellbender.GATKBaseTest;
+import org.broadinstitute.hellbender.cmdline.GATKPlugin.testpluggables.TestReadFilter;
 import org.broadinstitute.hellbender.cmdline.ReadFilterArgumentDefinitions;
+import org.broadinstitute.hellbender.cmdline.StandardArgumentDefinitions;
+import org.broadinstitute.hellbender.cmdline.TestProgramGroup;
+import org.broadinstitute.hellbender.engine.GATKTool;
 import org.broadinstitute.hellbender.engine.filters.*;
+import org.broadinstitute.hellbender.testutils.ArgumentsBuilder;
 import org.broadinstitute.hellbender.utils.read.ArtificialReadUtils;
 import org.broadinstitute.hellbender.utils.read.GATKRead;
+import org.mockito.internal.util.io.IOUtil;
 import org.testng.Assert;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
+import java.io.File;
+import java.io.IOException;
 import java.io.PrintStream;
+import java.nio.file.Files;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -786,5 +793,37 @@ private void setupReadNameTest(SetupTest setup) {
     private void setupSampleTest(SetupTest setup) {
         setup.hdr.getReadGroup(setup.read.getReadGroup()).setSample(setup.argValue);
     }
+
+    @CommandLineProgramProperties(summary="test tool to check ReadFilter loading",
+            oneLineSummary = "test tool to check ReadFilter loading",
+            programGroup = TestProgramGroup.class,
+            omitFromCommandLine = true)
+    public static class TestReadFiltersTool extends GATKTool {
+        @Argument(fullName = StandardArgumentDefinitions.OUTPUT_LONG_NAME)
+        String output;
+
+        @Override
+        public void traverse() {
+            String readFilters = getCommandLineParser()
+                    .getPluginDescriptor(GATKReadFilterPluginDescriptor.class)
+                    .getResolvedInstances()
+                    .stream()
+                    .map(a -> a.getClass().getSimpleName())
+                    .collect(Collectors.joining("\n"));
+            IOUtil.writeText(readFilters, new File(output));
+        }
+    }
+
+    @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");
+        ArgumentsBuilder args = new ArgumentsBuilder();
+        args.addArgument(ReadFilterArgumentDefinitions.READ_FILTER_LONG_NAME, TestReadFilter.class.getSimpleName())
+                .addFileArgument(StandardArgumentDefinitions.GATK_CONFIG_FILE_OPTION, configFile)
+                .addOutput(output);
+        runToolInNewJVM("TestReadFiltersTool", args);
+        Assert.assertEquals(Files.readAllLines(output.toPath()), Arrays.asList(WellformedReadFilter.class.getSimpleName(), TestReadFilter.class.getSimpleName()));
+    }
 
 }
diff --git a/.../java/org/broadinstitute/hellbender/cmdline/GATKPlugin/testpluggables/TestAnnotation.java b/.../java/org/broadinstitute/hellbender/cmdline/GATKPlugin/testpluggables/TestAnnotation.java
@@ -0,0 +1,6 @@
+package org.broadinstitute.hellbender.cmdline.GATKPlugin.testpluggables;
+
+import org.broadinstitute.hellbender.tools.walkers.annotator.Annotation;
+
+public class TestAnnotation implements Annotation {
+}
diff --git a/.../java/org/broadinstitute/hellbender/cmdline/GATKPlugin/testpluggables/TestReadFilter.java b/.../java/org/broadinstitute/hellbender/cmdline/GATKPlugin/testpluggables/TestReadFilter.java
@@ -0,0 +1,12 @@
+package org.broadinstitute.hellbender.cmdline.GATKPlugin.testpluggables;
+
+import org.broadinstitute.hellbender.engine.filters.ReadFilter;
+import org.broadinstitute.hellbender.utils.read.GATKRead;
+
+public class TestReadFilter extends ReadFilter {
+    private static final long serialVersionUID = 0L;
+    @Override
+    public boolean test(GATKRead read) {
+        return true;
+    }
+}
diff --git a/src/test/java/org/broadinstitute/hellbender/engine/GATKToolUnitTest.java b/src/test/java/org/broadinstitute/hellbender/engine/GATKToolUnitTest.java
@@ -13,18 +13,19 @@
 import htsjdk.variant.vcf.VCFIDHeaderLine;
 import org.broadinstitute.barclay.argparser.*;
 import org.broadinstitute.hellbender.GATKBaseTest;
-import org.broadinstitute.hellbender.cmdline.GATKPlugin.GATKAnnotationPluginDescriptor;
+import org.broadinstitute.hellbender.cmdline.GATKPlugin.testpluggables.TestAnnotation;
 import org.broadinstitute.hellbender.cmdline.StandardArgumentDefinitions;
 import org.broadinstitute.hellbender.cmdline.TestProgramGroup;
 import org.broadinstitute.hellbender.exceptions.UserException;
+import org.broadinstitute.hellbender.testutils.ArgumentsBuilder;
 import org.broadinstitute.hellbender.tools.walkers.annotator.Annotation;
 import org.broadinstitute.hellbender.tools.walkers.annotator.ClippingRankSumTest;
 import org.broadinstitute.hellbender.tools.walkers.annotator.Coverage;
 import org.broadinstitute.hellbender.tools.walkers.annotator.StandardAnnotation;
 import org.broadinstitute.hellbender.utils.SimpleInterval;
+import org.broadinstitute.hellbender.utils.config.GATKConfig;
 import org.broadinstitute.hellbender.utils.read.GATKRead;
 import org.broadinstitute.hellbender.utils.reference.ReferenceUtils;
-import org.broadinstitute.hellbender.testutils.ArgumentsBuilder;
 import org.testng.Assert;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
@@ -631,7 +632,7 @@ public void testGetAllAnnotations() {
         Collection<Annotation> annots = tool.makeVariantAnnotations();
 
         ClassFinder finder = new ClassFinder();
-        finder.find(GATKAnnotationPluginDescriptor.pluginPackageName, Annotation.class);
+        finder.find(GATKConfig.DEFAULT_ANNOTATION_PACKAGES, Annotation.class);
 
         Set<Class<?>> classes = finder.getConcreteClasses();
         Assert.assertFalse(classes.isEmpty());
@@ -641,6 +642,16 @@ public void testGetAllAnnotations() {
         }
     }
 
+    @Test
+    public void testConfigureAnnotationPackages(){
+        String[] args = {"--"+StandardArgumentDefinitions.ENABLE_ALL_ANNOTATIONS,
+                "--"+StandardArgumentDefinitions.GATK_CONFIG_FILE_OPTION, publicTestDir + "org/broadinstitute/hellbender/cmdline/GATKPlugin/changePluginPackages.properties"};
+
+        final TestGATKToolWithVariants tool = createTestVariantTool(args);
+        Collection<Annotation> annots = tool.makeVariantAnnotations();
+        Assert.assertEquals(annots.iterator().next().getClass(), TestAnnotation.class);
+    }
+
     @Test
     public void testExcludeAnnotation(){
         String[] args = {"--"+StandardArgumentDefinitions.ENABLE_ALL_ANNOTATIONS, "-AX", "Coverage"};
@@ -662,7 +673,7 @@ public void testIncludeAnnotationGroups(){
 
         // Asserting that a standard annotation was included but not everything
         ClassFinder finder = new ClassFinder();
-        finder.find(GATKAnnotationPluginDescriptor.pluginPackageName, StandardAnnotation.class);
+        finder.find(GATKConfig.DEFAULT_ANNOTATION_PACKAGES, StandardAnnotation.class);
 
         Set<Class<?>> classes = finder.getConcreteClasses();
         Assert.assertFalse(classes.isEmpty());
@@ -682,7 +693,7 @@ public void testIncludeAnnotation(){
 
         // Asserting coverage was added
         Assert.assertTrue(annots.stream().anyMatch(a -> a.getClass()==Coverage.class));
-        Assert.assertTrue(annots.size() == 1);
+        Assert.assertEquals(annots.size(), 1);
     }
 
     @Test
@@ -694,7 +705,7 @@ public void testMakeDefaultAnnotations() {
 
         // Asserting coverage was added by default
         Assert.assertTrue(annots.stream().anyMatch(a -> a.getClass()==Coverage.class));
-        Assert.assertTrue(annots.size() == 1);
+        Assert.assertEquals(annots.size(), 1);
     }
 
     @Test
@@ -705,7 +716,7 @@ public void testMakeDefaultAnnotationGroups() {
         Collection<Annotation> annots = tool.makeVariantAnnotations();
 
         ClassFinder finder = new ClassFinder();
-        finder.find(GATKAnnotationPluginDescriptor.pluginPackageName, StandardAnnotation.class);
+        finder.find(GATKConfig.DEFAULT_ANNOTATION_PACKAGES, StandardAnnotation.class);
 
         Set<Class<?>> classes = finder.getConcreteClasses();
         Assert.assertFalse(classes.isEmpty());
@@ -752,7 +763,7 @@ public void testHelpWithAllPluginDescriptors() {
         new TestGATKToolWithVariants().instanceMain(args);
     }
 
-    private TestGATKToolWithVariants createTestVariantTool(final String args[]) {
+    public TestGATKToolWithVariants createTestVariantTool(final String args[]) {
        return createTestVariantTool(new TestGATKToolWithVariants(), args);
     }
 

diff --git a/...esources/org/broadinstitute/hellbender/cmdline/GATKPlugin/changePluginPackages.properties b/...esources/org/broadinstitute/hellbender/cmdline/GATKPlugin/changePluginPackages.properties
@@ -0,0 +1,2 @@
+read_filter_packages = org.broadinstitute.hellbender.cmdline.GATKPlugin.testpluggables
+annotation_packages = org.broadinstitute.hellbender.cmdline.GATKPlugin.testpluggables