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 all commits
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
8 changes: 6 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,8 +141,12 @@ 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));
if (rawArgument != null) {
for (int i = 0; i < Array.getLength(rawArgument); i++) {
((CapturesArguments) m).captureFrom(Array.get(rawArgument, i));
}
} else {
((CapturesArguments) m).captureFrom(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we capture null in case an null-array was passed? How can it be distinguished from an null-element?
IMO it should be ignored cause null is strictly speaken an <Type[]> but the captor bound to <Type>.

}
}
}
Expand Down
27 changes: 27 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,31 @@ 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_single_argument_in_vararg_method() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is not explicit about casting.

Sorry I'm picky and on a mobile phone ;)

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 second test method I have added : should_capture_null_array_in_method_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 ?


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

// when
String[] nullArray = null;
mock.varargs(nullArray);

// 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.

In this case noting should be captured, see my comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChristianSchwarz not sure about that, because 'null' has been passed to method, hasn't it ?

If 'noting should be captured' then what getValue method should return in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that null can be the vararg array it self or an element of the vararg array. In this case we talk about the vararg array.

getValues() must return an empty List in this case, getValue() throws an exception if nothing was captured.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we capture a null vararg-array it might block #584 and #565, so I vote to keep it out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we keep it out there will be NPE in the code right now.
What is better: allow NPE or return null (even if that is not desired value) ?
I vote: in.

Copy link
Contributor

Choose a reason for hiding this comment

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

My expectation as a user would be that the captor does capture null, just like any other null invocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lukasz-szewc
I think we speak about two different things here. I mean we should remove the capturing part in case the vararg-array is null not the vararg element that is null. It should work if you simply remove the else part in line 149. This change should not cause an NPE.

@TimvdLippe

My expectation as a user would be that the captor does capture null, just like any other null invocation.

My expectation is that a ArgumentCaptor only capture Strings, but if Ian String[] is passed even if it is null then it should capture nothing, this relates to #565.

}
}