Factor out a GATKBaseTest for separate test resources from test utilities in BaseTest #3475

magicDGS · 2017-08-22T15:35:47Z

Changes to the testing framework to remove references to the test resources, keeping them into the src/test package. This changes include:

Factor out a GATKBaseTest for separate test resources from test utilities in BaseTest
Remove duplicated CleanSamIntegrationTest
Repackage CommandLineProgramTest to be in the test sources, and use it's interface in testers
Move some testers to the src/test package because they are tool-specific (added TODO to other ones that aren't that clear)
Refactor TargetsToolsTestUtils to use a provided reference

Closes #3029
Closes #2125

magicDGS · 2017-08-22T15:36:45Z

Here is a new PR for the issues related with the test code. Back to you @droazen.

magicDGS · 2017-08-23T11:01:02Z

@droazen, I updated the PR message to describe the changes, because I tried to get all the code referring to test resources packaged in src/test. Now it should pass the tests too.

codecov-io · 2017-08-23T11:17:44Z

Codecov Report

Merging #3475 into master will decrease coverage by 0.54%.
The diff coverage is 85.714%.

@@              Coverage Diff               @@
##              master     #3475      +/-   ##
==============================================
- Coverage     79.554%   79.014%   -0.54%     
+ Complexity     17738     17588     -150     
==============================================
  Files           1154      1151       -3     
  Lines          64092     63666     -426     
  Branches        9757      9748       -9     
==============================================
- Hits           50988     50305     -683     
- Misses          9214      9486     +272     
+ Partials        3890      3875      -15

Impacted Files	Coverage Δ	Complexity Δ
...broadinstitute/hellbender/utils/test/BaseTest.java	`70.707% <ø> (-13.116%)`	`27 <0> (-9)`
...e/hellbender/utils/test/testers/SamFileTester.java	`90.217% <ø> (ø)`	`30 <0> (ø)`	⬇️
.../hellbender/utils/test/testers/CleanSamTester.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...llbender/utils/test/SimpleIntervalTestFactory.java	`100% <100%> (ø)`	`5 <2> (?)`
...ute/hellbender/utils/test/GenomicsDBTestUtils.java	`91.667% <100%> (-0.641%)`	`3 <0> (ø)`
...ils/test/testers/AbstractMarkDuplicatesTester.java	`79.487% <100%> (ø)`	`17 <1> (ø)`	⬇️
...s/spark/ParallelCopyGCSDirectoryIntoHDFSSpark.java	`0% <0%> (-75.51%)`	`0% <0%> (-17%)`
...nder/tools/spark/pipelines/PrintVariantsSpark.java	`0% <0%> (-66.667%)`	`0% <0%> (-2%)`
...institute/hellbender/utils/gcs/GATKGCSOptions.java	`0% <0%> (-66.667%)`	`0% <0%> (ø)`
...lbender/engine/datasources/ReferenceAPISource.java	`22.013% <0%> (-62.264%)`	`8% <0%> (-26%)`
... and 16 more

jamesemery · 2017-10-23T15:28:41Z

@magicDGS Could you rebase this and update newer references to baseTest that have been added since you created this branch?

magicDGS · 2017-10-24T13:31:02Z

@jamesemery - I think that the rebase is done. I'd like to have this in as soon as it can be, to avoid the extra-work of rebasing due to new tests or refactoring of them.... Thank you in advance!

jamesemery

There are a couple things you need to change here, and before this gets merged we should make sure to let people know that this will probably cause merge conflicts in their test methods.

jamesemery · 2017-10-30T20:07:10Z

src/main/java/org/broadinstitute/hellbender/utils/test/BaseTest.java

-            locs.add(hg19GenomeLocParser.parseGenomeLoc(interval));
-        return Collections.unmodifiableList(locs);
-    }
-


For purity sake I think getToolTestDataDir() should be left abstract in BaseTest and instantiated in GATKBaseTest.

This can be overridden, but I think that it is a good practice to have a standard folder structure for test data directories. In addition, this is not project specific, because the java source structure (src/test/resources) is more or less an standard.

No action for this comment.

jamesemery · 2017-10-30T20:17:36Z

src/main/java/org/broadinstitute/hellbender/utils/test/TargetsToolsTestUtils.java

-    public static File getTestDataDir(){
-        return new File(CommandLineProgramTest.getTestDataDir(),"exome");
+    /** Initialize the reference file and dictionary to use for creating intervals. */
+    public TargetsToolsTestUtils(final File referenceFile){


It should not be the case that you are ever instantiating a utils class.

I think if you are trying to avoid calling = new File(getTestDataDir(),"test_reference.fasta"), create a getter which farms out to GATKBaseTest getTestDataDir() to get these files and leave it to external tool authors to avoid using these files.

More specifically, refactor the other methods in this class to take the reference dictionary as an argument and have the REFERENCE_FILE just be a string pointer to the file. Utils classes should not have state.

This was before an utility class to create SimpleIntervals with the reference in the exome test source. It is true that it is not a utils class, but a factory/builder class.

I think that I prefer in this case to hold the dictionary and the refererence to be sure that it correspond to the same one. I renamed to SimpleIntervalTestFactory to be clear that it keeps state and it is a factory.

@magicDGS Thinking about it, this still doesn't seem right, the methods contiained in this class fall under the purview of what IntervalUtils is doing. Its probably better to factor this class out entirely and move its methods over IntervalUtils as static and enforce that they take a referenceDictionary as an argument. Then for all the tests where you instantiate this class just instantiate a dictionary instead.

Can we left this to a different PR? The main problem with this issue is the BaseTest and GATKBaseTest, and removing this class by adding new methods to IntervalUtils will delay even more the reviewing process...

I open an issue to remove it (#3771) and I promise to prepare a PR for it once this is in...

jamesemery · 2017-10-30T20:26:41Z

src/main/java/org/broadinstitute/hellbender/utils/test/TargetsToolsTestUtils.java

@@ -77,7 +72,8 @@ public static SimpleInterval createOverEntireContig(final String contig) {
     * @return never {@code null}.
     * @throws UserException if there was some problem when creating the location.
     */
-    public static SimpleInterval createInterval(final String contig, final int start) {
+    public SimpleInterval createInterval(final String contig, final int start) {
+        // TODO: should this really be createInterval(contig, start, start) instead of using the constructor supplied here?


I agree with you, this method is kindof pointless, it seems to only be called in one place and furthermore it seems to be used in one place, and given the the other overload it seems confusing to just use SimpleInterval

I removed the TODO and call the createInterval(contig, start, start) instead, to be sure that it is using the sequence dictionary.

jamesemery · 2017-10-30T20:34:40Z

src/main/java/org/broadinstitute/hellbender/utils/test/testers/CleanSamIntegrationTest.java

-/**
- * This class is the extension of the SamFileTester to test CleanSam with SAM files generated on the fly.
- */
-public final class CleanSamIntegrationTest extends SamFileTester {


Nice catch on the duplicated code

jamesemery · 2017-10-30T20:42:41Z

...main/java/org/broadinstitute/hellbender/utils/test/testers/AbstractMarkDuplicatesTester.java

@@ -50,7 +53,7 @@ public File getMetricsFile() {
    }

    @Override
-    public String getTestedClassName() { return getProgram().getClass().getSimpleName(); }
+    public final String getTestedToolName() { return getProgram().getClass().getSimpleName(); }


Why did you make this change in a few places?

This classes were implementing CommandLineProgramTest, which is the CommandLineProgramTester implementation for GATK (extending also GATKBaseTest).

Because this classes are in the testing framework, and they are only tester classes, they do not inherit from BaseTest anymore. The method for returning the tool name is called getTestedToolName in CommandLineProgramTester and thus here it should be the one overridden.

Thus, this is one of the results of finally separating the gatk-testing framework (for re-use with downstream projects) and the test from GATK.

jamesemery · 2017-10-30T20:49:12Z

...main/java/org/broadinstitute/hellbender/utils/test/testers/AbstractMarkDuplicatesTester.java

@@ -19,6 +19,9 @@
 * This class is an extension of SamFileTester used to test AbstractMarkDuplicatesCommandLineProgram's with SAM files generated on the fly.
 * This performs the underlying tests defined by classes such as AbstractMarkDuplicatesCommandLineProgramTest.
 */
+// TODO: is this really necessary for the packaged testing framework?
+// TODO: it looks like that this is a SamFileTester exclusive for MarkDuplicates with the parameter in GATK/Picard
+// TODO: and thus, it should live in the test resources
 public abstract class AbstractMarkDuplicatesTester extends SamFileTester {


I agree, though I would leave it for a separate pull request to make that change for the testers more broadly.

Changed the TODO to a clear procedure and open a new ticket for move it to the test sources (#3762)

magicDGS · 2017-10-31T10:40:40Z

@jamesemery - we should get this merge as soon as possible to avoid conflicts that pop up in every round of comments. Once this is in, I can go to the open PRs to point out the conflicts and the new structure (e.g., change the new tests to extend GATKBaseTest).

I added a new commit addressing the issues and I will rebase to resolve conflicts again.

jamesemery · 2017-10-31T17:18:58Z

@magicDGS You need to resolve the conflicts yet again and respond to the comments I made about the SimpleIntervalTestFactory then this could probably be merged

…ties in BaseTest

…th custom references

magicDGS · 2017-11-01T12:30:13Z

@jamesemery - I rebased to solve the conflict and I open an issue regarding the SimpleIntervalTestFactory to do not block this work any longer, because this is already a big change in the testing framework.

Because the changes are big and we are working in different timezones, conflicts pop up everyday when another PR is accepted before this if they modify any of the test files (which is often the case). If we continue delaying this, it would never be possible to merge...

jamesemery · 2017-11-01T15:36:34Z

@magicDGS It looks like you have triggered a few new compiler errors in the last branch, namely in the following places:

/gatk/src/test/java/org/broadinstitute/hellbender/tools/spark/sv/discovery/SVDiscoveryTestDataProvider.java:33: error: cannot find symbol
            BaseTest.b38_reference_20_21, ReferenceWindowFunctions.IDENTITY_FUNCTION);
            ^
  symbol:   variable BaseTest
  location: class SVDiscoveryTestDataProvider
/gatk/src/test/java/org/broadinstitute/hellbender/tools/copynumber/formats/SampleLocatableCollectionUnitTest.java:30: error: cannot find symbol
    private static final String TEST_SUB_DIR = toolsTestDir + "copynumber/formats";
                                               ^
  symbol:   variable toolsTestDir
  location: class SampleLocatableCollectionUnitTest
/gatk/src/test/java/org/broadinstitute/hellbender/tools/copynumber/utils/annotatedregion/SimpleAnnotatedGenomicRegionUnitTest.java:18: error: cannot find symbol
    private static final String TEST_FILE = publicTestDir + "org/broadinstitute/hellbender/tools/copynumber/utils/combine-segment-breakpoints-with-legacy-header-learning-combined-copy-number.tsv";

jamesemery · 2017-11-01T15:48:55Z

@magicDGS Once these conflicts are resolved (which appear to be quick fixes) I will merge this branch.

magicDGS · 2017-11-01T16:16:08Z

Thanks @jamesemery - that's the complication of this big PR. I hope that the tests pass after my last commit and that we can get this in before another PR gets in.

Thanks a lot for reviewing!

Conflicts are resolved, tests are passing.

lbergelson · 2017-11-01T19:51:53Z

@magicDGS merged! Thank you for your this.

magicDGS · 2017-11-02T09:44:03Z

Thanks to all of you!

magicDGS · 2017-11-02T10:24:08Z

I tried to warn in every affected open PR as promised @jamesemery

… (#3475) Projects depending on GATK have had trouble using BaseTest because it tries to load specific files from the gatk test resources. If the project didn't include these files, the tests would crash. This separates out a new GATK only GATKBaseTest which contains all of the file references but leaves the useful utilities in BaseTest for downstream use. It makes similar changes in other test classes that had related issues. fixes #2125 fixes #3029 Introduces #3771 but due to the pain of rebasing the entire test suite repeatedly, this will be addressed in a follow up PR. * Factor out a GATKBaseTest for separate test resources from test utilities in BaseTest * Repackage CommandLineProgramTest due to project-specific paths * Refactor TargetsToolsTestUtils to allow downstream projects to use with custom references

magicDGS mentioned this pull request Aug 22, 2017

Create TestResources for GATK-specific file constants and GenomeLocParser #3176

Closed

droazen self-assigned this Aug 22, 2017

droazen self-requested a review August 22, 2017 15:45

magicDGS force-pushed the dgs_refactor_basetest branch from 984696e to ee1d7f2 Compare September 15, 2017 13:09

droazen requested review from jamesemery and removed request for droazen October 23, 2017 15:25

droazen assigned jamesemery and unassigned droazen Oct 23, 2017

magicDGS force-pushed the dgs_refactor_basetest branch from ee1d7f2 to bede8ef Compare October 24, 2017 09:57

jamesemery previously requested changes Oct 30, 2017

View reviewed changes

magicDGS force-pushed the dgs_refactor_basetest branch from 6ba1f25 to 7a02606 Compare October 31, 2017 14:49

magicDGS mentioned this pull request Nov 1, 2017

adding tests which run directly on a dataproc cluster #3767

Merged

magicDGS added 5 commits November 1, 2017 13:19

Factor out a GATKBaseTest for separate test resources from test utili…

1b1ca38

…ties in BaseTest

Repackage CommandLineProgramTest due to project-specific paths

2bf1cdf

Refactor TargetsToolsTestUtils to allow downstream projects to use wi…

dcf84c1

…th custom references

Fix rebase issues

198baa8

Address comments

dddffd0

magicDGS mentioned this pull request Nov 1, 2017

Remove TargetsToolsTestUtils/SimpleIntervalTestFactory #3771

Closed

magicDGS force-pushed the dgs_refactor_basetest branch from 7a02606 to dddffd0 Compare November 1, 2017 12:26

Fix rebase issues (again)

c16e73c

lbergelson merged commit 9283269 into broadinstitute:master Nov 1, 2017

magicDGS deleted the dgs_refactor_basetest branch November 1, 2017 20:28

magicDGS mentioned this pull request Nov 2, 2017

Basic Validator #3755

Merged

magicDGS mentioned this pull request Nov 2, 2017

Move SimpleIntervalTestFactory methods to IntervalUtils #3778

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor out a GATKBaseTest for separate test resources from test utilities in BaseTest #3475

Factor out a GATKBaseTest for separate test resources from test utilities in BaseTest #3475

magicDGS commented Aug 22, 2017 •

edited

Loading

magicDGS commented Aug 22, 2017

magicDGS commented Aug 23, 2017

codecov-io commented Aug 23, 2017 •

edited

Loading

jamesemery commented Oct 23, 2017

magicDGS commented Oct 24, 2017

jamesemery left a comment

jamesemery Oct 30, 2017

magicDGS Oct 31, 2017

jamesemery Oct 30, 2017

jamesemery Oct 30, 2017

jamesemery Oct 30, 2017

magicDGS Oct 31, 2017

jamesemery Oct 31, 2017 •

edited

Loading

magicDGS Nov 1, 2017

jamesemery Oct 30, 2017

magicDGS Oct 31, 2017

jamesemery Oct 30, 2017

magicDGS Oct 31, 2017

jamesemery Oct 30, 2017

magicDGS Oct 31, 2017

jamesemery Oct 30, 2017

magicDGS Oct 31, 2017

magicDGS commented Oct 31, 2017

jamesemery commented Oct 31, 2017

magicDGS commented Nov 1, 2017

jamesemery commented Nov 1, 2017

jamesemery commented Nov 1, 2017

magicDGS commented Nov 1, 2017

lbergelson commented Nov 1, 2017

magicDGS commented Nov 2, 2017

magicDGS commented Nov 2, 2017

Factor out a GATKBaseTest for separate test resources from test utilities in BaseTest #3475

Factor out a GATKBaseTest for separate test resources from test utilities in BaseTest #3475

Conversation

magicDGS commented Aug 22, 2017 • edited Loading

magicDGS commented Aug 22, 2017

magicDGS commented Aug 23, 2017

codecov-io commented Aug 23, 2017 • edited Loading

Codecov Report

jamesemery commented Oct 23, 2017

magicDGS commented Oct 24, 2017

jamesemery 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

jamesemery Oct 31, 2017 • edited Loading

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

magicDGS commented Oct 31, 2017

jamesemery commented Oct 31, 2017

magicDGS commented Nov 1, 2017

jamesemery commented Nov 1, 2017

jamesemery commented Nov 1, 2017

magicDGS commented Nov 1, 2017

lbergelson commented Nov 1, 2017

magicDGS commented Nov 2, 2017

magicDGS commented Nov 2, 2017

magicDGS commented Aug 22, 2017 •

edited

Loading

codecov-io commented Aug 23, 2017 •

edited

Loading

jamesemery Oct 31, 2017 •

edited

Loading