Fix SpyStatic() with an interaction closure throws NullPointerException

[AndreasTu]

The usage of

SpyStatic(Type){
}

had thrown an NullPointerException , because the closure got automatically converted into IMockMakerSettings.

This adds a meaningful error message for the user to use SpyStatic(Type) without a Closure instead of SpyStatic(Type){}.

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.07%. Comparing base (ac06a53) to head (f3747e6).

Files with missing lines	Patch %	Lines
...spockframework/mock/runtime/MockMakerRegistry.java	91.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #2254   +/-   ##
=========================================
  Coverage     82.07%   82.07%           
- Complexity     4753     4757    +4     
=========================================
  Files           465      465           
  Lines         14872    14882   +10     
  Branches       1877     1877           
=========================================
+ Hits          12206    12215    +9     
  Misses         1978     1978           
- Partials        688      689    +1     

Files with missing lines	Coverage Δ
...e/src/main/java/spock/mock/IMockMakerSettings.java	100.00% <ø> (ø)
...spockframework/mock/runtime/MockMakerRegistry.java	97.24% <91.66%> (-0.74%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[leonard84]

Isn't this misleading? IMO, this adds a feature that didn't exist before.

[AndreasTu]

Yes it does. I am not really convinced that a user should use the new syntax:

SpyStatic(Type){
  1 * Type.method() >> true
}

because it does not work so great, due to the Class instance issues, e.g. IDE support.

The better syntax is the documented one, without any closure.

SpyStatic(Type)
1 * Type.method() >> true

As I said, I can also remove the new syntax and add a second method to IMockMakerSettings to prevent the auto-conversion, if you like that better.

[AndreasTu]

@leonard84 I have changed PR to not use new API, you can have a look what you like better :).

[AndreasTu]

I changed it again, because the new method to IMockMakerSettings would break contribution API for external MockMakers.

[Vampire]

And why do we not support something like

SpyStatic(Type) {
  1 * method() >> true
}

like we do for all other mock objects?
Imho it should be up to the user whether he does that or

SpyStatic(Type)
1 * Type.method() >> true

like is supported for all other mock objects.
And it should solve the problem, as with a Closure overload, that should be used.

[Vampire]

Ah, I see.
But if you have that version readily available already, why do you think it could not make it in that variant into M7?
The changes are not super intrusive, are they?

[AndreasTu]

Leonard sounded that he was not so convinced about that new API, especially the lack of IDE support.
So I made a non-API change bugfix.

[Vampire]

Just treat it like an instance is given in?
Yes, you then get also instance methods completed, but as you can also call static methods on instances, you also get the static methods.
On the other mock closures you also get both while calling the static does not really make sense.
So I'd say just add the @ClosureParams and @DelegatesTo annotations and add an ?: enclosingCall('SpyStatic') to the gdsl and whatever is necessary for the Eclipse-version and it should be fine enough?

[AndreasTu]

Wouldn't you get IDE warnings that static methods are called on an instance?

Actually before this, I would perfer the nothing at all as in my commit and the user shall use the class syntax inside the closure, which works fine and completes correctly.

SpyStatic(Type){
  1 * Type.method() >> true
}

[Vampire]

Wouldn't you get IDE warnings that static methods are called on an instance?

I don't think the IDE can warn about this, because due to the duck-typing of Groovy, the IDE cannot know whether the instance might also have an instance method with that name at runtime. If it complains, I'd say that is an IDE bug.

But actually I do not see a warning even with

@DelegatesTo(strategy = Closure.DELEGATE_FIRST, genericTypeIndex = 0)
@ClosureParams(FirstParam.FirstGenericType.class)

But you can also do

@DelegatesTo(strategy = Closure.DELEGATE_FIRST)
@ClosureParams(FirstParam.class)

and then the type is correctly considered to be Class<StaticClass> and the static methods are completed as expected too.

And the ?: enclosingCall('StaticSpy') in the spock.gdsl should also work as expected, as it should also deliver Class<StaticClass> as type and thus be correct.

For the spock_tests.dsld I'd expect the same.