-
Notifications
You must be signed in to change notification settings - Fork 587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add CreateVariantIngestFiles integration test #7071
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
private static final String sample_map_file = "test_sample_map.tsv"; | ||
private static final File outputDir = createTempDir("output_dir"); | ||
|
||
// TODO why can't I run Collections.sort() here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't run anything other than variable definitions here (ie no blocks of code). In Java you can run code here by saying:
static {
// your code
}
but that's generally bad form since you don't really know when that code will run. The better place to put this would be in the constructor for this class. Although in your case I would just sort the three things by hand since they're literals anyway
|
||
public class CreateVariantIngestFilesIntegrationTest extends CommandLineProgramTest { | ||
|
||
private static final String input_vcf_file = "NA12878.haplotypeCalls.reblocked.chr20.100k.vcf.gz"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kcibul is it better practice to move all of these inside the test? I'd thought eventually we may have other tests that would use them, but for now there's just the one.
OR if these should stay here, should I also move the "expected" file names out here as well? it's not consistent right now and I don't have any real rationale for their being defined one place or another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah -- either way seems reasonable to me. FWIW I think I would put them inside the test for now... but that's just personal preference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, moved them inside the test - can always refactor if/when we add more tests if they use the same files
* add CreateVariantIngestFiles integration test * clean up * define all needed files inside test
* add CreateVariantIngestFiles integration test * clean up * define all needed files inside test
* add CreateVariantIngestFiles integration test * clean up * define all needed files inside test
dsp-spec-ops issue #214 (https://github.com/broadinstitute/dsp-spec-ops/issues/214)