Remove debugging, remove obsolete runSystemCommand() method, final re…

…factoring
broadinstitute · Dec 11, 2023 · 5934691 · 5934691
1 parent 636d139
commit 5934691
Show file tree

Hide file tree

Showing 5 changed files with 65 additions and 76 deletions.
diff --git a/...tute/hellbender/tools/walkers/vqsr/scalable/ExtractVariantAnnotationsIntegrationTest.java b/...tute/hellbender/tools/walkers/vqsr/scalable/ExtractVariantAnnotationsIntegrationTest.java
@@ -158,17 +158,18 @@ public void testValidInputs(final String tag,
     private static void assertOutputs(final String tag,
                                       final String outputPrefix) {
         // vcf.idx files are not reproducible
-        SystemCommandUtilsTest.runSystemCommand(String.format("h5diff %s/%s %s",
-                EXPECTED_TEST_FILES_DIR,
-                tag + LabeledVariantAnnotationsWalker.ANNOTATIONS_HDF5_SUFFIX,
-                outputPrefix + LabeledVariantAnnotationsWalker.ANNOTATIONS_HDF5_SUFFIX));
-        SystemCommandUtilsTest.runSystemCommand(String.format("diff %s/%s.vcf %s.vcf",
-                EXPECTED_TEST_FILES_DIR, tag, outputPrefix));
+        SystemCommandUtilsTest.runH5Diff(
+                String.format("%s/%s", EXPECTED_TEST_FILES_DIR, tag + LabeledVariantAnnotationsWalker.ANNOTATIONS_HDF5_SUFFIX),
+                String.format("%s", outputPrefix + LabeledVariantAnnotationsWalker.ANNOTATIONS_HDF5_SUFFIX));
+
+        SystemCommandUtilsTest.runDiff(
+                String.format("%s/%s.vcf", EXPECTED_TEST_FILES_DIR, tag),
+                String.format("%s.vcf", outputPrefix));
+
         if (tag.contains("posUn")) {
-            SystemCommandUtilsTest.runSystemCommand(String.format("h5diff %s/%s %s",
-                    EXPECTED_TEST_FILES_DIR,
-                    tag + ExtractVariantAnnotations.UNLABELED_TAG + ExtractVariantAnnotations.ANNOTATIONS_HDF5_SUFFIX,
-                    outputPrefix + ExtractVariantAnnotations.UNLABELED_TAG + ExtractVariantAnnotations.ANNOTATIONS_HDF5_SUFFIX));
+            SystemCommandUtilsTest.runH5Diff(
+                    String.format("%s/%s", EXPECTED_TEST_FILES_DIR, tag + ExtractVariantAnnotations.UNLABELED_TAG + ExtractVariantAnnotations.ANNOTATIONS_HDF5_SUFFIX),
+                    String.format("%s", outputPrefix + ExtractVariantAnnotations.UNLABELED_TAG + ExtractVariantAnnotations.ANNOTATIONS_HDF5_SUFFIX));
         } else {
             Assert.assertFalse(new File(outputPrefix + ExtractVariantAnnotations.UNLABELED_TAG + ExtractVariantAnnotations.ANNOTATIONS_HDF5_SUFFIX).exists());
         }

diff --git a/...titute/hellbender/tools/walkers/vqsr/scalable/ScoreVariantAnnotationsIntegrationTest.java b/...titute/hellbender/tools/walkers/vqsr/scalable/ScoreVariantAnnotationsIntegrationTest.java
@@ -37,17 +37,6 @@ public final class ScoreVariantAnnotationsIntegrationTest extends CommandLinePro
     // diffs in the new expected outputs in git to confirm that they are consistent with expectations.
     public static final boolean UPDATE_EXACT_MATCH_EXPECTED_OUTPUTS = false;
 
-    public static void runH5Diff(final String expected, final String actual) {
-        final ProcessController controller = ProcessController.getThreadLocal();
-
-        // -r: Report mode. Print the differences.
-        // --use-system-epsilon: Return a difference if and only if the difference between two data values exceeds
-        //                       the system value for epsilon.
-        final String[] command = new String[] { "h5diff", "-r", "--use-system-epsilon", expected, actual };
-
-        runProcessAndCaptureOutputInExceptionMessage(controller, command);
-    }
-
     /**
      * Make sure that someone didn't leave the UPDATE_EXACT_MATCH_EXPECTED_OUTPUTS toggle turned on.
      */
@@ -175,11 +164,16 @@ public void testValidInputs(final String tag,
     private static void assertExpectedOutputs(final String tag,
                                               final String outputPrefix) {
         // vcf.idx files are not reproducible
-        SystemCommandUtilsTest.runSystemCommand(String.format("diff %s/%s.vcf %s.vcf",
-                EXPECTED_TEST_FILES_DIR, tag, outputPrefix));
-        runH5Diff(String.format("%s/%s.annot.hdf5", EXPECTED_TEST_FILES_DIR, tag),
-                  String.format("%s.annot.hdf5", outputPrefix));
-        runH5Diff(String.format("%s/%s.scores.hdf5", EXPECTED_TEST_FILES_DIR, tag),
+        SystemCommandUtilsTest.runDiff(
+                String.format("%s/%s.vcf", EXPECTED_TEST_FILES_DIR, tag),
+                String.format("%s.vcf", outputPrefix));
+
+        SystemCommandUtilsTest.runH5Diff(
+                String.format("%s/%s.annot.hdf5", EXPECTED_TEST_FILES_DIR, tag),
+                String.format("%s.annot.hdf5", outputPrefix));
+
+        SystemCommandUtilsTest.runH5Diff(
+                String.format("%s/%s.scores.hdf5", EXPECTED_TEST_FILES_DIR, tag),
                 String.format("%s.scores.hdf5", outputPrefix));
     }
 

diff --git a/...ava/org/broadinstitute/hellbender/tools/walkers/vqsr/scalable/SystemCommandUtilsTest.java b/...ava/org/broadinstitute/hellbender/tools/walkers/vqsr/scalable/SystemCommandUtilsTest.java
@@ -2,6 +2,7 @@
 
 import org.broadinstitute.hellbender.GATKBaseTest;
 import org.broadinstitute.hellbender.exceptions.GATKException;
+import org.broadinstitute.hellbender.utils.runtime.ProcessController;
 import org.testng.Assert;
 import org.testng.annotations.Test;
 
@@ -16,47 +17,50 @@ public final class SystemCommandUtilsTest extends GATKBaseTest {
             "org/broadinstitute/hellbender/tools/walkers/vqsr/scalable/extract");
     private static final File EXPECTED_TEST_FILES_DIR = new File(TEST_FILES_DIR, "expected");
 
-    static void runSystemCommand(final String command) {
-        logger.debug(String.format("Testing command: %s", command));
-        try {
-            final ProcessBuilder processBuilder = new ProcessBuilder("sh", "-c", command).redirectErrorStream(true);
-            final Process process = processBuilder.start();
-
-            final BufferedReader stdInReader = new BufferedReader(new InputStreamReader(process.getInputStream()));
-            String stdInLine;
-            while ((stdInLine = stdInReader.readLine()) != null) {
-                Assert.fail(String.format("The command \"%s\" resulted in: %s", command, stdInLine));
-            }
-            stdInReader.close();
-
-        } catch (final IOException e) {
-            throw new GATKException.ShouldNeverReachHereException(e.getMessage());
-        }
+    static void runH5Diff(final String expected, final String actual) {
+        final ProcessController controller = ProcessController.getThreadLocal();
+
+        // -r: Report mode. Print the differences.
+        // --use-system-epsilon: Return a difference if and only if the difference between two data values exceeds
+        //                       the system value for epsilon.
+        final String[] command = new String[] { "h5diff", "-r", "--use-system-epsilon", expected, actual };
+
+        runProcessAndCaptureOutputInExceptionMessage(controller, command);
+    }
+
+    static void runDiff(final String expected, final String actual) {
+        final ProcessController controller = ProcessController.getThreadLocal();
+        final String[] command = new String[] { "diff", expected, actual };
+        runProcessAndCaptureOutputInExceptionMessage(controller, command);
     }
 
     @Test(groups = {"python"}) // python environment is required to use h5diff
-    public void testRunSystemCommand() {
-        runSystemCommand(String.format("h5diff %s/extract.AS.indel.pos.annot.hdf5 %s/extract.AS.indel.pos.annot.hdf5",
-                EXPECTED_TEST_FILES_DIR, EXPECTED_TEST_FILES_DIR));
-        runSystemCommand(String.format("diff %s/extract.AS.indel.pos.vcf %s/extract.AS.indel.pos.vcf",
-                EXPECTED_TEST_FILES_DIR, EXPECTED_TEST_FILES_DIR));
+    public void testRunH5DiffIdenticalFile() {
+        runH5Diff(String.format("%s/extract.AS.indel.pos.annot.hdf5", EXPECTED_TEST_FILES_DIR),
+                  String.format("%s/extract.AS.indel.pos.annot.hdf5", EXPECTED_TEST_FILES_DIR));
+    }
+
+    @Test
+    public void testRunDiffIdenticalFile() {
+        runDiff(String.format("%s/extract.AS.indel.pos.vcf", EXPECTED_TEST_FILES_DIR),
+                String.format("%s/extract.AS.indel.pos.vcf", EXPECTED_TEST_FILES_DIR));
     }
 
     @Test(expectedExceptions = AssertionError.class, groups = {"python"}) // python environment is required to use h5diff
-    public void testRunSystemCommandH5diffException() {
-        runSystemCommand(String.format("h5diff %s/extract.AS.indel.pos.annot.hdf5 %s/extract.AS.snp.pos.annot.hdf5",
-                EXPECTED_TEST_FILES_DIR, EXPECTED_TEST_FILES_DIR));
+    public void testRunH5DiffException() {
+        runH5Diff(String.format("%s/extract.AS.indel.pos.annot.hdf5", EXPECTED_TEST_FILES_DIR),
+                  String.format("%s/extract.AS.snp.pos.annot.hdf5", EXPECTED_TEST_FILES_DIR));
     }
 
     @Test(expectedExceptions = AssertionError.class)
-    public void testRunSystemCommandDiffException() {
-        runSystemCommand(String.format("diff %s/extract.AS.indel.pos.vcf %s/extract.AS.snp.pos.vcf",
-                EXPECTED_TEST_FILES_DIR, EXPECTED_TEST_FILES_DIR));
+    public void testRunDiffException() {
+        runDiff(String.format("%s/extract.AS.indel.pos.vcf", EXPECTED_TEST_FILES_DIR),
+                String.format("%s/extract.AS.snp.pos.vcf", EXPECTED_TEST_FILES_DIR));
     }
 
     @Test(expectedExceptions = AssertionError.class)
-    public void testRunSystemCommandDiffNoSuchFileException() {
-        runSystemCommand(String.format("diff %s/blahblah %s/extract.AS.snp.pos.vcf",
-                EXPECTED_TEST_FILES_DIR, EXPECTED_TEST_FILES_DIR));
+    public void testRunDiffNoSuchFile() {
+        runDiff(String.format("%s/blahblah", EXPECTED_TEST_FILES_DIR),
+                String.format("%s/extract.AS.snp.pos.vcf", EXPECTED_TEST_FILES_DIR));
     }
 }
diff --git a/...e/hellbender/tools/walkers/vqsr/scalable/TrainVariantAnnotationsModelIntegrationTest.java b/...e/hellbender/tools/walkers/vqsr/scalable/TrainVariantAnnotationsModelIntegrationTest.java
@@ -39,17 +39,6 @@ public final class TrainVariantAnnotationsModelIntegrationTest extends CommandLi
     // diffs in the new expected outputs in git to confirm that they are consistent with expectations.
     public static final boolean UPDATE_EXACT_MATCH_EXPECTED_OUTPUTS = false;
 
-    public static void runH5Diff(final String expected, final String actual) {
-        final ProcessController controller = ProcessController.getThreadLocal();
-
-        // -r: Report mode. Print the differences.
-        // --use-system-epsilon: Return a difference if and only if the difference between two data values exceeds
-        //                       the system value for epsilon.
-        final String[] command = new String[] { "h5diff", "-r", "--use-system-epsilon", expected, actual };
-
-        runProcessAndCaptureOutputInExceptionMessage(controller, command);
-    }
-
     /**
      * Make sure that someone didn't leave the UPDATE_EXACT_MATCH_EXPECTED_OUTPUTS toggle turned on.
      */
@@ -191,11 +180,13 @@ private static void assertExpectedOutputsForVariantType(final String tag,
         final String tagAndVariantType = String.format("%s.%s", tag, variantType);
         final String outputPrefixAndVariantType = String.format("%s.%s", outputPrefix, variantType);
 
-        runH5Diff(String.format("%s/%s", EXPECTED_TEST_FILES_DIR, tagAndVariantType + TrainVariantAnnotationsModel.TRAINING_SCORES_HDF5_SUFFIX),
-                  String.format("%s", outputPrefixAndVariantType + TrainVariantAnnotationsModel.TRAINING_SCORES_HDF5_SUFFIX));
+        SystemCommandUtilsTest.runH5Diff(
+                String.format("%s/%s", EXPECTED_TEST_FILES_DIR, tagAndVariantType + TrainVariantAnnotationsModel.TRAINING_SCORES_HDF5_SUFFIX),
+                String.format("%s", outputPrefixAndVariantType + TrainVariantAnnotationsModel.TRAINING_SCORES_HDF5_SUFFIX));
 
-        runH5Diff(String.format("%s/%s", EXPECTED_TEST_FILES_DIR, tagAndVariantType + TrainVariantAnnotationsModel.CALIBRATION_SCORES_HDF5_SUFFIX),
-                  String.format("%s", outputPrefixAndVariantType + TrainVariantAnnotationsModel.CALIBRATION_SCORES_HDF5_SUFFIX));
+        SystemCommandUtilsTest.runH5Diff(
+                String.format("%s/%s", EXPECTED_TEST_FILES_DIR, tagAndVariantType + TrainVariantAnnotationsModel.CALIBRATION_SCORES_HDF5_SUFFIX),
+                String.format("%s", outputPrefixAndVariantType + TrainVariantAnnotationsModel.CALIBRATION_SCORES_HDF5_SUFFIX));
 
         assertScorerExpectedOutputs(tagAndVariantType, outputPrefixAndVariantType);
 
@@ -261,10 +252,12 @@ public void testSNPOnlyModelsFromSNPOnlyAndSNPPlusIndelAnnotationsAreIdentical()
                 .apply(argsBuilderSNPPlusIndel);
         runCommandLine(argsBuilderSNPPlusIndel);
 
-        runH5Diff(outputPrefixSNPOnly + ".snp" + TrainVariantAnnotationsModel.TRAINING_SCORES_HDF5_SUFFIX,
+        SystemCommandUtilsTest.runH5Diff(
+                outputPrefixSNPOnly + ".snp" + TrainVariantAnnotationsModel.TRAINING_SCORES_HDF5_SUFFIX,
                 outputPrefixSNPPlusIndel + ".snp" + TrainVariantAnnotationsModel.TRAINING_SCORES_HDF5_SUFFIX);
 
-        runH5Diff(outputPrefixSNPOnly + ".snp" + TrainVariantAnnotationsModel.CALIBRATION_SCORES_HDF5_SUFFIX,
+        SystemCommandUtilsTest.runH5Diff(
+                outputPrefixSNPOnly + ".snp" + TrainVariantAnnotationsModel.CALIBRATION_SCORES_HDF5_SUFFIX,
                 outputPrefixSNPPlusIndel + ".snp" + TrainVariantAnnotationsModel.CALIBRATION_SCORES_HDF5_SUFFIX);
     }
 

diff --git a/src/testUtils/java/org/broadinstitute/hellbender/testutils/BaseTest.java b/src/testUtils/java/org/broadinstitute/hellbender/testutils/BaseTest.java
@@ -112,9 +112,6 @@ public static void runProcess(final ProcessController processController, final S
                 message + "\n***Process stdout:***\n" + output.getStdout() + "\n***Process stderr:***\n" + output.getStderr() :
                 message;
 
-        // Temporarily print the stdout/stderr unconditionally, for debugging
-        System.err.println(fullMessage);
-
         Assert.assertEquals(output.getExitValue(), 0, fullMessage);
     }