Fixes #583 fix bug when trying to capture null vararg #606
Conversation
Current coverage is 88.24% (diff: 100%)@@ master #606 diff @@
==========================================
Files 266 266
Lines 5161 5163 +2
Methods 0 0
Messages 0 0
Branches 847 848 +1
==========================================
+ Hits 4554 4556 +2
Misses 429 429
Partials 178 178
|
| @@ -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++) { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
true,
however code with if-else will be longer - but you know - there are no perfect solutions.
|
Small implementation comment, other than that LGTM. Thanks for debugging and fixing this issue! |
| // then | ||
| verify(mock).varargs(argumentCaptor.capture()); | ||
| Assertions.assertThat(argumentCaptor.getValue()).isNull(); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Take a look at: should_capture_null_array_in_method_vararg. Is it sufficient ?
| @@ -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() { | |||
There was a problem hiding this comment.
This one is not explicit about casting.
Sorry I'm picky and on a mobile phone ;)
There was a problem hiding this comment.
take a look at second test method I have added : should_capture_null_array_in_method_vararg
|
Regardless if we accept this PR or not, shall we postpone merging for 2.2 since we have a code freeze right now for the release candidate? I think our focus right now must be documentation to release ASAP. |
|
I'm postponing this one with the rest of the Argument captor refactoring that will happen after mockito. |
Should fix just one issue - described in #583