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

Strict mode still requires an answer for void-returning Java methods #91

Closed
TheWishWithin opened this Issue Jun 16, 2018 · 14 comments

Comments

5 participants
@TheWishWithin
Contributor

TheWishWithin commented Jun 16, 2018

  • I am running the latest version
  • I checked the documentation and found no answer
  • I checked to make sure that this issue has not already been filed

Expected Behavior

I find it strange that in strict mode, even void returning Java methods require an answer.
At least in most of my cases, I really don't do anything with mocked void-methods, except for verifying that they've been called with the expected arguments.
So it seems pointless to me that I absolutely have to either enable relaxed mode or declare the behaviour with every {...}.

I think in general I get it, that you'd have to provide an answer for a method, that does return something since it keeps you from forgetting to define a clear return value (which is most likely required somewhere). Of course, you could also say that same for void methods, where you'd forget about defined the mocked behaviour, but here I think that in a good design, you wouldn't have to define a lot of logic in most cases.

Steps to Reproduce

Below is a very simply example to see what I mean (yes, I'm mocking a List to verify that clear() was called, but I would never do that in actual code ;)).

Context

  • MockK version: 1.8.3
  • OS: Windows 10
  • Kotlin version: 1.2.50
  • JDK version: 1.7
  • Type of test: unit test

Stack trace

io.mockk.MockKException: no answer found for: List(#1).clear()

	at io.mockk.impl.stub.MockKStub.defaultAnswer(MockKStub.kt:91)
	at io.mockk.impl.stub.MockKStub.answer(MockKStub.kt:43)
	at io.mockk.impl.recording.states.AnsweringState.call(AnsweringState.kt:15)
	at io.mockk.impl.recording.CommonCallRecorder.call(CommonCallRecorder.kt:48)
	at io.mockk.impl.stub.MockKStub.handleInvocation(MockKStub.kt:199)
	at io.mockk.impl.instantiation.JvmMockFactoryHelper$mockHandler$1.invocation(JvmMockFactoryHelper.kt:23)
	at io.mockk.proxy.jvm.advice.Interceptor.call(Interceptor.kt:20)
	at io.mockk.proxy.jvm.advice.BaseAdvice.handle(BaseAdvice.kt:42)
	at io.mockk.proxy.jvm.advice.jvm.JvmMockKProxyInterceptor.interceptNoSuper(JvmMockKProxyInterceptor.java:45)
	at net.bytebuddy.renamed.java.util.List$ByteBuddy$CrWJvDWe.clear(Unknown Source)
	at org.example.mockk.FooTest$Foo.clearList(FooTest.kt:23)
	at org.example.mockk.FooTest.shouldClearList(FooTest.kt:15)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:389)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:115)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$6(TestMethodTestDescriptor.java:167)
	at org.junit.jupiter.engine.execution.ThrowableCollector.execute(ThrowableCollector.java:40)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:163)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:110)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:57)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.lambda$execute$3(HierarchicalTestExecutor.java:83)
	at org.junit.platform.engine.support.hierarchical.SingleTestExecutor.executeSafely(SingleTestExecutor.java:66)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:77)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.lambda$null$2(HierarchicalTestExecutor.java:92)
	at java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184)
	at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:175)
	at java.util.Iterator.forEachRemaining(Iterator.java:116)
	at java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
	at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151)
	at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174)
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:418)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.lambda$execute$3(HierarchicalTestExecutor.java:92)
	at org.junit.platform.engine.support.hierarchical.SingleTestExecutor.executeSafely(SingleTestExecutor.java:66)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:77)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.lambda$null$2(HierarchicalTestExecutor.java:92)
	at java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184)
	at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:175)
	at java.util.Iterator.forEachRemaining(Iterator.java:116)
	at java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
	at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151)
	at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174)
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:418)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.lambda$execute$3(HierarchicalTestExecutor.java:92)
	at org.junit.platform.engine.support.hierarchical.SingleTestExecutor.executeSafely(SingleTestExecutor.java:66)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:77)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:51)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:43)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:170)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:154)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:90)
	at com.intellij.junit5.JUnit5IdeaTestRunner.startRunnerWithArgs(JUnit5IdeaTestRunner.java:65)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.intellij.rt.execution.application.AppMainV2.main(AppMainV2.java:131)

Minimal reproducible code (the gist of this issue)

package org.example.mockk

import io.mockk.mockk
import io.mockk.verify
import org.junit.jupiter.api.Test

class FooTest {

    @Test
    fun shouldClearList() {
        val list = mockk<java.util.List<Any>>(relaxed = true) // works
        //val list = mockk<java.util.List<Any>>() // requires answer even for void methods
        val foo = Foo(list)

        foo.clearList()

        verify { list.clear() }
    }

    class Foo(private val list: java.util.List<Any>) {

        fun clearList() {
            list.clear() // clearly returns void
        }

    }
}
@Kerooker

This comment has been minimized.

Collaborator

Kerooker commented Jun 16, 2018

I think it is necessary.

You might want your void method to throw an exception, for example, or to just run. You might want your void method to add a value to, perhaps, a list while doing it's logic, and not only run. How is it supposed to know either case?

Mockk shouldn't guess what you meant to do (what if you forgot to throw an Exception, but it's your test behaviour? What if the list should be populated?), unless you explicitly said "I want you to just run", needing the behaviour block.

However, as you said, most cases you won't be needing this logic. Anyway, Mockk shouldn't make assumptions based on usual-case-scenario, that's why you have relaxed mock for

@oleksiyp oleksiyp added bug and removed enhancement labels Jun 17, 2018

@TheWishWithin

This comment has been minimized.

Contributor

TheWishWithin commented Jun 17, 2018

Well, maybe it's better to keep things like they are right now. Just to be consistent in general and to be on the safe side.

@oleksiyp

This comment has been minimized.

Collaborator

oleksiyp commented Jun 17, 2018

Sorry. Give me some time. I still not tried to get to the main point. From description it seems to me as a bug that should be fixed, so reopening

@oleksiyp oleksiyp reopened this Jun 17, 2018

@TheWishWithin

This comment has been minimized.

Contributor

TheWishWithin commented Jun 17, 2018

The main point is that methods implemented in Java that return Void still require an answer. This seems weird at first, but now I guess it makes sense to still force the developer to define how that method should be mocked in strict mode.

@oleksiyp

This comment has been minimized.

Collaborator

oleksiyp commented Jun 17, 2018

I see two cases.

For strict mock it is of course required to specify answer, for relaxed should work without it.

So I assumed it is not working as it should for relaxed.

        val list = mockk<java.util.List<Any>>(relaxed = true) // fails

but for strict it is okay to "fail" and have this exception. Right?

@TheWishWithin

This comment has been minimized.

Contributor

TheWishWithin commented Jun 17, 2018

Ah, crap, I made the wrong comment.

val list = mockk<java.util.List<Any>>(relaxed = true)

works as expected.

But
val list = mockk<java.util.List<Any>>()
requires an answer, even for void-method, which I thought would be weird.

@Kerooker Kerooker added enhancement and removed bug labels Jun 17, 2018

@TheWishWithin

This comment has been minimized.

Contributor

TheWishWithin commented Jun 18, 2018

I thought about it, at first it seems really weird that you'd have to define the answer of a void method. This is just because I'm used to the Mockito way of having default return values.

Now after thinking about it, it seems to me like this is actually a very good feature and it can keep you from making mistakes of forgetting about mocking some behaviour or forgetting that a certain function call needs to be made and checked as well.

So please, DO NOT fix anything! Everything works as expected:

  • Relaxed mode just lets you execute and VOID-method without defining the behaviour
  • Strict mode requires you to at least use at least just Runs
@oshamahue

This comment has been minimized.

oshamahue commented Jun 21, 2018

Hey. I just started this library and I tried to use Mockito and I had some problems. So I am changing to MockK.
While I am converting to code I am having problems with void returning methods at the mocked classes since Mockito doesn't have this restriction. So right now I have two choices the first one is the write answers for void returning methods which is not possible for me for the large codebase. Or convert all mocks to relaxed which I would feel like I am missing out on some cool functionality. I think the answer requirement for void returning type should be optional within the strict mode. I see your point on having the restriction but it introduces a lot of unnecessary code. I would rather use verify then just Runs to verify the call I made.

@Kerooker Kerooker reopened this Jun 21, 2018

@oleksiyp

This comment has been minimized.

Collaborator

oleksiyp commented Jun 21, 2018

I anyway not able to change public API, so maximum I can is introduce some additional flag alike relaxUnitReturningFunctions=true which will include void case.

@oshamahue

This comment has been minimized.

oshamahue commented Jun 22, 2018

Sounds good to me. Thanks for looking into it. It would be good to be able to add the flag to MockKAnnotations.init(this,relaxUnitReturningFunctions=true ) to have default behavior on objects mocked with annotation.

@raharrison

This comment has been minimized.

raharrison commented Jun 22, 2018

+1 this would be very handy

oleksiyp added a commit that referenced this issue Jun 23, 2018

@oleksiyp

This comment has been minimized.

Collaborator

oleksiyp commented Jun 23, 2018

Added support for this feature with following API:

Function:

mockk<MockCls>(relaxUnitFun = true)

Annotation:

@MockK(relaxUnitFun = true)
lateinit var mock1: RurfMockCls
init {
    MockKAnnotations.init(this)
}

MockKAnnotations.init:

@MockK
lateinit var mock2: RurfMockCls
init {
    MockKAnnotations.init(this, relaxUnitFun = true)
}
@oleksiyp

This comment has been minimized.

Collaborator

oleksiyp commented Jun 23, 2018

Version 1.8.4 released, please check if everything ok and close the issue.

@oshamahue

This comment has been minimized.

oshamahue commented Jun 23, 2018

@oleksiyp you rock. I just tested it and works as expected. Thank you for quick fix. I didn't created the issue so someone else should close it.

@oleksiyp oleksiyp closed this Jun 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment