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

Fixes #583 fix bug when trying to capture null vararg #606

Closed
wants to merge 3 commits into from
Closed

Fixes #583 fix bug when trying to capture null vararg #606

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
672b254
Fixes #583 fix bug when trying to capture null vararg
lukasz-szewc Aug 28, 2016
e112fb7
Fixes #583 rework after PR comments
lukasz-szewc Aug 28, 2016
8a42098
Fixes #583 added one more test to check behavior for null array refer…
lukasz-szewc Aug 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/main/java/org/mockito/internal/invocation/InvocationMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,21 @@ private void captureVarargsPart(Invocation invocation) {
for (ArgumentMatcher m : uniqueMatcherSet(indexOfVararg)) {
if (m instanceof CapturesArguments) {
Object rawArgument = invocation.getRawArguments()[indexOfVararg];
for (int i = 0; i < Array.getLength(rawArgument); i++) {
((CapturesArguments) m).captureFrom(Array.get(rawArgument, i));
for (int i = 0; i < getLength(rawArgument); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of magically changing the length of the array, I would rather introduce an if-else around the for-loop, to make the intent clearer. With this implementation there is a hidden connection between getLength and getArgument

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true,
however code with if-else will be longer - but you know - there are no perfect solutions.

((CapturesArguments) m).captureFrom(getArgument(rawArgument, i));
}
}
}
}

private int getLength(Object rawArgument) {
return rawArgument == null ? 1 : Array.getLength(rawArgument);
}

private Object getArgument(Object rawArgument, int i) {
return rawArgument == null ? null : Array.get(rawArgument, i);
}

private int regularArgumentsSize(Invocation invocation) {
return invocation.getMethod().isVarArgs() ?
invocation.getRawArguments().length - 1 // ignores vararg holder array
Expand Down
13 changes: 13 additions & 0 deletions src/test/java/org/mockitousage/matchers/CapturingArgumentsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,4 +315,17 @@ public void captures_correctly_when_captor_used_on_pure_vararg_method() throws E
verify(mock).varargs(eq(42), argumentCaptor.capture());
Assertions.assertThat(argumentCaptor.getValue()).contains("capturedValue");
}

@Test
public void should_capture_null_vararg() {
// given
ArgumentCaptor<String> argumentCaptor = ArgumentCaptor.forClass(String.class);

// when
mock.varargs(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @lukasz-szewc,

thanks for you PR! Please note this test doesn't test the captoring of an null-array ((String[])null) but an null-argument (new String[]{null}).
Can you add a test with mock.varargs((String[])null) to verfiy #583 is fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ChristianSchwarz

I will add that test and let see what happens. Should work :)


// then
verify(mock).varargs(argumentCaptor.capture());
Assertions.assertThat(argumentCaptor.getValue()).isNull();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add another test with casting?

in the end we will have a test with mock.varargs((Type[]) null) and a test with mock.varargs((Type) null)

This allows thorough testing and documentation of each caseof.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a look at: should_capture_null_array_in_method_vararg. Is it sufficient ?

}