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

VcfToBqTask unit tests #19

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

VcfToBqTask unit tests #19

wants to merge 4 commits into from

Conversation

mjfeigles
Copy link
Collaborator

Unit tests for VcfToBqTask.java:

  • tests VcfToBqTask constructor
  • tests run()
    • verifies that the 3 function calls inside run() are executed
  • tests setPipelineOptions()
    • verifies that setPipelineOptions() is callable
    • does not test static function FileSystems.setDefaultPipelineOptions()
  • does not test static main(), which runs a static function to create injector and get injector instance

@mjfeigles mjfeigles requested a review from tneymanov July 13, 2020 21:23
tneymanov
tneymanov reviewed Jul 15, 2020
View reviewed changes
.gitignore Outdated
@@ -10,3 +10,6 @@ gradlew.bat
# Ignore IntelliJ project files
.idea
gcp-variant-transforms-java.iml

#Ignore VSCode files
.vscode/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wasn't this added in previous PRs? Doesn't matter much, but maybe some commit mishap.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if this commit still affects this PR. Maybe we can take it out

src/test/java/com/google/gcp_variant_transforms/TestEnv.java Outdated
bind(VcfParser.class).to(VcfParserImpl.class);
bind(Task.class).to(VcfToBqTask.class);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add new line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should had been created in Zhiqiang's PR right? Could you sync and update the PR?

src/test/java/com/google/gcp_variant_transforms/task/VcfToBqTaskTest.java Outdated
VcfToBqTask vcfToBqTask;
VcfToBqTask spyVcfToBqTask;

@Mock
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can inline, just to save space stylistically:

@mock PipelineRunner mockedPipelineRunner.

Here and below.

src/test/java/com/google/gcp_variant_transforms/task/VcfToBqTaskTest.java
ImmutableList<String> headerLines = headerLinesBuilder.build();
doReturn(headerLines).when(mockedHeaderReader).getHeaderLines();
doNothing().when(spyVcfToBqTask).setPipelineOptions(argCaptorPipelineOptions.capture());
doNothing().when(mockedVcfToBqContext).setHeaderLines(argCaptorImmutableList.capture());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we, specifically for this case, use headerLines instead of capturing? The problem with PipelineOptions was that the object was changed (type casted into PipelineOptions). Here I don't think we have the same issue.

src/test/java/com/google/gcp_variant_transforms/task/VcfToBqTaskTest.java Outdated
setUpRunBehaviors();
spyVcfToBqTask.run();

verify(mockedVcfToBqContext).setHeaderLines(argCaptorImmutableList.capture());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this part we can just check by checking that it was called with headerLines (since they have not been modified).

src/test/java/com/google/gcp_variant_transforms/task/VcfToBqTaskTest.java Outdated

@Test
public void testVcfTask_whenSetPipelineOptions_thenVerify() throws Exception {
doNothing().when(spyVcfToBqTask).setPipelineOptions(argCaptorPipelineOptions.capture());;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is kinda pointless, tbh. We just call the function here and verify that function has been called. I say we remove it.

src/test/java/com/google/gcp_variant_transforms/task/VcfToBqTaskTest.java Outdated
}

@Test
public void testVcfTask_whenRun_thenVerifySetPipelineOptions() throws IOException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can combine all three tests into one.

src/test/java/com/google/gcp_variant_transforms/task/VcfToBqTaskTest.java Outdated
setUpRunBehaviors();
spyVcfToBqTask.run();

verify(spyVcfToBqTask).setPipelineOptions(mockedVcfToBqOptions);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised this passes... wasn't the whole purpose of using arg captor was that the value was modified on runtime and thus, we couldn't have expected it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tneymanov tneymanov tneymanov left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mjfeigles @tneymanov