-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fixes #583 fix bug when trying to capture null vararg #606
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -317,7 +317,7 @@ public void captures_correctly_when_captor_used_on_pure_vararg_method() throws E | |
} | ||
|
||
@Test | ||
public void should_capture_null_vararg() { | ||
public void should_capture_null_single_argument_in_vararg_method() { | ||
// given | ||
ArgumentCaptor<String> argumentCaptor = ArgumentCaptor.forClass(String.class); | ||
|
||
|
@@ -328,4 +328,18 @@ public void should_capture_null_vararg() { | |
verify(mock).varargs(argumentCaptor.capture()); | ||
Assertions.assertThat(argumentCaptor.getValue()).isNull(); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 This allows thorough testing and documentation of each caseof. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case noting should be captured, see my comment above. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lukasz-szewc
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. |
||
} | ||
} |
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.
This one is not explicit about casting.
Sorry I'm picky and on a mobile phone ;)
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.
take a look at second test method I have added : should_capture_null_array_in_method_vararg