Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Revert UpdateVcfSequenceDictionary handles stdout logic and repair test - DO NOT MERGE #867

Merged
merged 1 commit into from
Sep 4, 2019
Merged

Revert UpdateVcfSequenceDictionary handles stdout logic and repair test - DO NOT MERGE #867

merged 1 commit into from
Sep 4, 2019

Conversation

ronlevine
Copy link
Contributor

Description

Fixes #863
DO NOT MERGE until using a version of htsjdk with samtools/htsjdk#933. samtools/htsjdk#933 fixes an NPE when writing to stdout and UpdateVcfSequenceDictionaryTest.testUpdateVcfSequenceDictionary will no longer fail. Reverted the code changes and fixed the test from #741. This PR did not properly implement the outputting to stdout and and interpreted a .vcf.gz as a VCF rather than a BLOCK_COMPRESSED_VCF. The stdout testing was fixed because file descriptor was interpreted as a VCF rather than VCF_STREAM by VariantContextBuilder.determineOutputTypeFromFile.

Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

  • Added or modified tests to cover changes and any new functionality
  • Edited the README / documentation (if applicable)
  • All tests passing on Travis

Review

  • Final thumbs-up from reviewer
  • Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

@ronlevine ronlevine self-assigned this Jul 12, 2017
@ronlevine ronlevine requested a review from nh13 July 12, 2017 15:18
@ronlevine
Copy link
Contributor Author

@nh13 Please review.

@ronlevine
Copy link
Contributor Author

I successfully tested this fix as follows:

  • Build the htsjdk branch and get the version: ./gradlew install printVersion
  • Build Picard: /gradlew shadowJar -Dhtsjdk.version=2.10.1-21-g309914c-SNAPSHOT
    where 2.10.1-21-g309914c-SNAPSHOT is the htsjdk version
  • Run test in the Picard branch: . ./gradlew test -Dtest.single=UpdateVcfSequenceDictionaryTest -Dhtsjdk.version=2.10.1-21-g309914c-SNAPSHOT

nh13
nh13 requested changes Jul 12, 2017
View reviewed changes
Copy link
Collaborator

@nh13 nh13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor test change requested.

src/main/java/picard/vcf/UpdateVcfSequenceDictionary.java Outdated
@@ -59,7 +56,7 @@
programGroup = VcfOrBcf.class
)
public class UpdateVcfSequenceDictionary extends CommandLineProgram {
@Option(shortName = StandardOptionDefinitions.INPUT_SHORT_NAME, doc = "Input VCF")
@Option(shortName = StandardOptionDefinitions.INPUT_SHORT_NAME, doc = "Input VCF")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra space added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

src/test/java/picard/vcf/UpdateVcfSequenceDictionaryTest.java
@@ -43,6 +39,8 @@
private static final File TEST_DATA_PATH = new File("testdata/picard/vcf/");
private static final File OUTPUT_DATA_PATH = IOUtil.createTempDir("UpdateVcfSequenceDictionaryTest", null);
private static final File STD_OUT_FILE = new File(OUTPUT_DATA_PATH, "stdout.vcf");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used anymore I don't think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

src/test/java/picard/vcf/UpdateVcfSequenceDictionaryTest.java Outdated
@Test(dataProvider = "OutputFiles")
public void testUpdateVcfSequenceDictionary(final String outputFileName) throws IOException, NoSuchFieldException, IllegalAccessException {
File input = new File(TEST_DATA_PATH, "vcfFormatTest.vcf");
File inputFile = new File(TEST_DATA_PATH, "vcfFormatTest.vcf");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your doing, but could you make this final?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/test/java/picard/vcf/UpdateVcfSequenceDictionaryTest.java Outdated
Assert.assertEquals(SAMSequenceDictionaryExtractor.extractDictionary(samSequenceDictionaryVcf).size(), 82);
Assert.assertEquals(SAMSequenceDictionaryExtractor.extractDictionary(outputFile).size(), 82);
// Check that the output is equal to the input file if the output is not going to stdout
if ( outputFileName != STD_OUT_NAME ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra spaces

Copy link
Contributor Author

@ronlevine ronlevine Jul 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

src/test/java/picard/vcf/UpdateVcfSequenceDictionaryTest.java
@@ -54,51 +52,57 @@ public void teardown() {
public static Object[][] outputFies() {

return new Object[][] {
{OUTPUT_DATA_PATH + "updateVcfSequenceDictionaryTest-delete-me.vcf"},
{OUTPUT_DATA_PATH + "updateVcfSequenceDictionaryTest-delete-me" + VcfUtils.COMPRESSED_VCF_ENDING},
{OUTPUT_DATA_PATH + "updateVcfSequenceDictionaryTest-delete-me" + VcfUtils.UNCOMPRESSED_VCF_ENDING},
{STD_OUT_NAME}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a test of the output:

final ByteArrayOutputStream myOut = new ByteArrayOutputStream();
System.setOut(new PrintStream(myOut));

@nh13
Copy link
Collaborator

nh13 commented Oct 16, 2017

@ronlevine any chance you could take a look at this one?

@yfarjoun
Copy link
Contributor

Someone else will have to take over this PR. perhaps @cmnbroad ?

@cmnbroad
Copy link
Contributor

Sure why not.

@cmnbroad cmnbroad assigned cmnbroad and unassigned ronlevine Oct 16, 2017
@yfarjoun
Copy link
Contributor

yfarjoun commented Jun 4, 2018

@cmnbroad can you look at this soon or should we look for another person to take over?

@cmnbroad
Copy link
Contributor

cmnbroad commented Jun 5, 2018

@yfarjoun I have a pretty long backlog at this point so if someone else can take this, feel free.

@yfarjoun yfarjoun assigned gbggrant and unassigned cmnbroad Jun 8, 2018
@gbggrant
Copy link
Contributor

@ronlevine sorry for the delay in getting to this. samtools/htsjdk#933 has been merged - can you update this PR and we can try to get it merged up?

@yfarjoun
Copy link
Contributor

@gbggrant I think that we have to find someone to take over this PR...AFAIK @ronlevine will not fix this.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 81.672% when pulling cf1bea6 on rhl_uvsd_fix_stdout into 4a0179d on master.

@coveralls
Copy link

coveralls commented Jun 13, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.006%) to 81.672% when pulling 56bc18b on rhl_uvsd_fix_stdout into 4a0179d on master.

@gbggrant
Copy link
Contributor

@nh13 I think I've addressed all of your comments on this PR and added a separate test for verifying output to stdout. Can you re-review.

@yfarjoun
Copy link
Contributor

with a minor request, I think that we can assume a 👍 ....

@ronlevine @gbggrant
Revert UpdateVcfSequenceDictionary handles stdout logic and repair test
24593ac 
Mostly cosmetic cleanup

Have test verify outputs to stdout, vcf, and vcf.gz

Added test to better capture stdout and verify
@gbggrant gbggrant merged commit 2221b9a into master Sep 4, 2019
@gbggrant gbggrant deleted the rhl_uvsd_fix_stdout branch September 4, 2019 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nh13 nh13 nh13 requested changes

Assignees

@gbggrant gbggrant

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UpdateVcfSequenceDictionary cannot write index
6 participants
@ronlevine @nh13 @yfarjoun @cmnbroad @gbggrant @coveralls