Do not strip type information from arguments (#871) by Vampire · Pull Request #1134 · spockframework/spock

Vampire · 2020-03-30T03:26:56Z

Fixes #871

This change is

codecov · 2020-03-31T13:07:29Z

Codecov Report

Merging #1134 into master will decrease coverage by 0.00%.
The diff coverage is 77.41%.

@@             Coverage Diff              @@
##             master    #1134      +/-   ##
============================================
- Coverage     75.01%   75.00%   -0.01%     
- Complexity     3526     3528       +2     
============================================
  Files           388      388              
  Lines         10872    10876       +4     
  Branches       1347     1348       +1     
============================================
+ Hits           8156     8158       +2     
- Misses         2245     2247       +2     
  Partials        471      471

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/org/spockframework/util/RenderUtil.java	`85.00% <50.00%> (-6.18%)`	`19.00 <0.00> (+1.00)`	⬇️
...kframework/mock/runtime/GroovyMockInterceptor.java	`75.75% <66.66%> (ø)`	`9.00 <0.00> (ø)`
...ockframework/mock/runtime/GroovyMockMetaClass.java	`86.04% <88.88%> (+0.33%)`	`21.00 <10.00> (ø)`
...ockframework/mock/runtime/JavaMockInterceptor.java	`92.30% <100.00%> (ø)`	`16.00 <0.00> (ø)`
.../org/spockframework/runtime/GroovyRuntimeUtil.java	`70.23% <100.00%> (ø)`	`41.00 <1.00> (ø)`
...n/java/org/spockframework/util/ReflectionUtil.java	`79.64% <100.00%> (-0.53%)`	`65.00 <1.00> (-1.00)`
...rg/spockframework/mock/runtime/MockInvocation.java	`82.14% <0.00%> (+3.57%)`	`12.00% <0.00%> (+1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e30462a...da70701. Read the comment docs.

leonard84

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Vampire)

spock-core/src/main/java/org/spockframework/mock/runtime/GroovyMockInterceptor.java, line 48 at r1 (raw file):

    // we do not need the cast information from the wrappers here, the method selection
    // is already done, so unwrap the argument array if there are still wrappers present
    arguments = GroovyRuntimeUtil.asUnwrappedArgumentArray(arguments);

Parameters should be treated as final, even if we omit the final everywhere to avoid clutter. Even if some old code did that, we should clean it up.

spock-core/src/main/java/org/spockframework/mock/runtime/GroovyMockMetaClass.java, line 65 at r1 (raw file):

asArgumentArray

Isn't this redundant here? arguments are already an array.

spock-core/src/main/java/org/spockframework/mock/runtime/JavaMockInterceptor.java, line 53 at r1 (raw file):

asArgumentArray

Isn't this redundant here? arguments are already an array.

spock-core/src/main/java/org/spockframework/runtime/GroovyRuntimeUtil.java, line 228 at r1 (raw file):

  public static Object[] asUnwrappedArgumentArray(Object args) {
    return InvokerHelper.asUnwrappedArray(InvokerHelper.asArray(args).clone());

Maybe document that it does clone the array.

spock-specs/src/test/groovy/org/spockframework/smoke/mock/GroovySpiesThatAreGlobal.groovy, line 244 at r1 (raw file):

    when:
    PersonWithOverloadedMethods.perform(_) >> "Foo"

Maybe split here into a new test, as the result changes and it is not directly obvious.

Can we do this data-driven, or does it not work/reproduce the problem?

where:
action | result
{ PersonWithOverloadedMethods.perform("null string" as String) } | 'String'

Vampire

Reviewable status: 3 of 8 files reviewed, 5 unresolved discussions (waiting on @leonard84)

spock-core/src/main/java/org/spockframework/mock/runtime/GroovyMockInterceptor.java, line 48 at r1 (raw file):