Ease troubleshooting of unit tests with calls to mock unstubbed methods

[gberche-orange]

Currently, unit tests make heavy use of mocks. When mocks are improperly configured, calls to unstubbed methods return null, which is hard to diagnose with async reactor calls.

After adding Hooks.onOperatorDebug(); stack trace such as the following are displayed, but do not display the actual stubbed method call invocation, to ease comparison with mock setup

25-03-2020 15:38:54.073 [main] ERROR o.s.c.a.w.i.AppDeploymentDeleteServiceInstanceWorkflow.error - Error deleting backing services for service1/plan1 with error 'The mapper returned a null Publisher.'
java.lang.NullPointerException: The mapper returned a null Publisher.
	at java.util.Objects.requireNonNull(Objects.java:228)
	Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: 
Assembly trace from producer [reactor.core.publisher.MonoFlatMapMany] :
	reactor.core.publisher.Mono.flatMapMany(Mono.java:2748)
	org.springframework.cloud.appbroker.workflow.instance.AppDeploymentDeleteServiceInstanceWorkflow.deleteBackingServices(AppDeploymentDeleteServiceInstanceWorkflow.java:71)
Error has been observed at the following site(s):
	|_  Mono.flatMapMany ⇢ at org.springframework.cloud.appbroker.workflow.instance.AppDeploymentDeleteServiceInstanceWorkflow.deleteBackingServices(AppDeploymentDeleteServiceInstanceWorkflow.java:71)
	|_      Flux.flatMap ⇢ at org.springframework.cloud.appbroker.workflow.instance.AppDeploymentDeleteServiceInstanceWorkflow.deleteBackingServices(AppDeploymentDeleteServiceInstanceWorkflow.java:75)
	|_  Flux.doOnRequest ⇢ at org.springframework.cloud.appbroker.workflow.instance.AppDeploymentDeleteServiceInstanceWorkflow.deleteBackingServices(AppDeploymentDeleteServiceInstanceWorkflow.java:76)
	|_ Flux.doOnComplete ⇢ at org.springframework.cloud.appbroker.workflow.instance.AppDeploymentDeleteServiceInstanceWorkflow.deleteBackingServices(AppDeploymentDeleteServiceInstanceWorkflow.java:78)

Possible improvement: systematically throw a exception is friendly message when an unstubbed method call invocation is received:

25-03-2020 15:42:11.499 [main] ERROR o.s.c.a.w.i.AppDeploymentDeleteServiceInstanceWorkflow.error - Error deleting backing services for service1/plan1 with error 'Unexpected call to unstubbed mock method: targetService.addToBackingServices(
    [BackingService{serviceInstanceName='my-service-instance', name='my-service', plan='a-plan', parameters={}, properties={}, parametersTransformers=[], rebindOnUpdate=false}],
    org.springframework.cloud.appbroker.deployer.TargetSpec@e67e89f4,
    "service-instance-id"
);'
java.lang.RuntimeException: Unexpected call to unstubbed mock method: targetService.addToBackingServices(
    [BackingService{serviceInstanceName='my-service-instance', name='my-service', plan='a-plan', parameters={}, properties={}, parametersTransformers=[], rebindOnUpdate=false}],
    org.springframework.cloud.appbroker.deployer.TargetSpec@e67e89f4,
    "service-instance-id"
);
	at org.springframework.cloud.appbroker.workflow.instance.AppDeploymentDeleteServiceInstanceWorkflowTest$RuntimeExceptionAnswer.answer(AppDeploymentDeleteServiceInstanceWorkflowTest.java:352)
	Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: 
Assembly trace from producer [reactor.core.publisher.MonoFlatMapMany] :
	reactor.core.publisher.Mono.flatMapMany(Mono.java:2748)
	org.springframework.cloud.appbroker.workflow.instance.AppDeploymentDeleteServiceInstanceWorkflow.deleteBackingServices(AppDeploymentDeleteServiceInstanceWorkflow.java:71)
Error has been observed at the following site(s):
	|_  Mono.flatMapMany ⇢ at org.springframework.cloud.appbroker.workflow.instance.AppDeploymentDeleteServiceInstanceWorkflow.deleteBackingServices(AppDeploymentDeleteServiceInstanceWorkflow.java:71)
	|_      Flux.flatMap ⇢ at org.springframework.cloud.appbroker.workflow.instance.AppDeploymentDeleteServiceInstanceWorkflow.deleteBackingServices(AppDeploymentDeleteServiceInstanceWorkflow.java:75)
	|_  Flux.doOnRequest ⇢ at org.springframework.cloud.appbroker.workflow.instance.AppDeploymentDeleteServiceInstanceWorkflow.deleteBackingServices(AppDeploymentDeleteServiceInstanceWorkflow.java:76)
	|_ Flux.doOnComplete ⇢ at org.springframework.cloud.appbroker.workflow.instance.AppDeploymentDeleteServiceInstanceWorkflow.deleteBackingServices(AppDeploymentDeleteServiceInstanceWorkflow.java:78)

This is supported through a custom default mockito answer, see https://stackoverflow.com/a/15835255/1484823

I applied this in a sample test in orange-cloudfoundry/osb-cmdb@b2324e4#diff-4a2e062758255b0f47b073e44268b72c as part of #292

[gberche-orange]

Actually, mockito provides smart nulls which supports the @Mockito annotation, and is systematically used in cf-java-client, see https://github.com/cloudfoundry/cf-java-client/blob/8ec06b4cdd61dda0f0ba5e4d546651b880735faa/cloudfoundry-operations/src/test/java/org/cloudfoundry/operations/AbstractOperationsTest.java#L79-L97

    @Mock(answer = Answers.RETURNS_SMART_NULLS)
    private BackingAppDeploymentService backingAppDeploymentService;

It may be less strict than a custom Answer systematically throwing an exception, as it attempts to return first tries to return ordinary values for "empty" objects, thereby possibly still hiding some unstubbed methods

See https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#RETURNS_SMART_NULLS

If your code uses the object returned by an unstubbed call you get a NullPointerException. This implementation of Answer returns SmartNull instead of null. SmartNull gives nicer exception message than NPE because it points out the line where unstubbed method was called. You just click on the stack trace.

ReturnsSmartNulls first tries to return ordinary values (zeros, empty collections, empty string, etc.) then it tries to return SmartNull. If the return type is final then plain null is returned.

ReturnsSmartNulls will be probably the default return values strategy in Mockito 4.0.0

Here is a sample of such exception thrown with line navigation provided by intellij

See refined attempt at orange-cloudfoundry/osb-cmdb@bb8d5e9

The full exception is at https://gist.github.com/gberche-orange/0e113d844c454f6f2579d8e0f2645f26

To apply smart nulls to all app broker tests requires converting tests using BDDMockito (in form of given(object.method()).willReturn(value) into doReturn(value).when(object.method())

As such manual conversion is time consumming, intellij structural search and replace can mostly automate this

Here is an export of this search/replace template which can be imported into intellij.
https://gist.github.com/gberche-orange/f3f8003349d8192c3b4a8237a2079223

If opting for the "use static imports" checkbox, then Mockito class specifier is omitted but also Mono.empty() becomes empty()

I'm happy to give it a try on the whole codebase once my outstanding PRs get merged (as to avoid time consumming conflicts handing)

[royclarkson]

To apply smart nulls to all app broker tests requires converting tests using BDDMockito (in form of given(object.method()).willReturn(value) into doReturn(value).when(object.method())

Is this absolutely necessary? It was an effort to convert everything to BDDMockito. 😅

[gberche-orange]

Is this absolutely necessary? It was an effort to convert everything to BDDMockito.

https://stackoverflow.com/a/15835255/1484823 details why BDDMockito can't be used with small nulls

Note that you cannot use when with this functionality, since the method is called before when (How does mockito when() invocation work?) and it will throw a RuntimeException before the mock goes into stubbing mode.

Therefore, you must use doReturn for this to work.

I'm sorry if this feels like going backwards. I hope that friendly exceptions will save plenty of time to app broker maintainers when tests need change, and will outweight the readability benefits of BDDMockito. Hopefully the "intellij structural search and replace" practice should make it easier to apply future similar syntax changes in used libraries.

[MRamonLeon]

@Mock(answer = Answers.RETURNS_SMART_NULLS)
saved my day 🙇🏽‍♂️