Skip to content
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

org.mockito.exceptions.misusing.WrongTypeOfReturnValue while stubbing a function from a "spy" object with mockito in groovy #2226

Open
mahmoud-ashi opened this issue Mar 13, 2021 · 14 comments

Comments

@mahmoud-ashi
Copy link

I'm having this issue which has taken enough time from me so I decided to ask the experts here.
Some context.. I have a Jenkins shared library which with some groovy classes and I'm writing some unit tests using the JenkinsPipelineUnit framework. I'm using normal junit for testing so nothing special there.
Basically I have a groovy class as following:

class MyClass {
    // In reality, `run` is object from the final class WorkflowRun but in the tests, I pass here a dummy map with the important fields
    def run

    MyClass(def run) {
        this.run = run
    }

    boolean somethingExists() {
        // some code here that does some stuff on `run` and return a boolean
    }

    String doSomething() {
        if(somethingExists()) {
            // do something with it.
            return "foo"
        } else {
            // do something else
            return "bar"
        }
    }
}

The code above is simple enough and I am writing the following test for the doSomething() function without having to mock all the internals of somethingExists().

@Test
void testDoSomething() {
    Map run = [number: 1]
    MyClass mc = spy(new MyClass(run))
    // The problem is with the next line.
    doReturn(true).when(mc).somethingExists()

    assertTrue(mc.somethingExists())
    assertEquals("foo", mc.doSomething())
}

Note that I cannot mock the WorkflowRun class since it is a final class and honestly I don't care about it since I need to test the logic inside doSomething() rather than what somethingExists() does which is checking some Jenkins internals on the run object. Therefore, I decided to "spy" on the object under test mc and make it return a simple true or false when I call somethingExists() to avoid too much mocking of the run object.
According to Mockito's documentation, when I use the spy function, I should use the stubbing format like I did doReturn(true).when(mc).somethingExists() unlike the other format of when(mc.somethingExists()).thenReturn(true) since this will actually call the real function on the class while attempting to stub it.

Error message:
When I call the doReturn(true).when(mc).somethingExists() I get the following error

org.mockito.exceptions.misusing.WrongTypeOfReturnValue: 
Boolean cannot be returned by getMetaClass()
getMetaClass() should return MetaClass
***
If you're unsure why you're getting above error read on.
Due to the nature of the syntax above problem might occur because:
1. This exception *might* occur in wrongly written multi-threaded tests.
   Please refer to Mockito FAQ on limitations of concurrency testing.
2. A spy is stubbed using when(spy.foo()).then() syntax. It is safer to stub spies - 
   - with doReturn|Throw() family of methods. More in javadocs for Mockito.spy() method.

	at org.codehaus.groovy.runtime.callsite.CallSiteArray.createPogoSite(CallSiteArray.java:140)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.createCallSite(CallSiteArray.java:158)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:47)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:125)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:130)
	at MyClassTest.testDoSomething(MyClassTest.groovy:37)
...

Note that I'm not running tests in parallel (actually while reproducing the issue, I was running only this test) and I'm following the stubbing rule suggested.
At first I thought it is because of the doReturn(true) which is returning a primitive type instead of an object so I replaced it with doReturn(Boolean.FALSE) but I still got the same error.
When I replace the line doReturn(true).when(mc).somethingExists() with when(mc.somethingExists()).thenReturn(true) then it fails because it is calling the real function mc.somethingExists() which is expected in my opinion. On the other hand, when I replace the entire content of the somethingExists() function with a simple return false, surprisingly, the format doReturn(true).when(mc).somethingExists() still failed however the other format when(mc.somethingExists()).thenReturn(true) worked which I'm surprised because this format should not work for spy objects.

Any clues what I might be doing wrong? I have spent a lot of time trying to find the problem.
Thanks in advance.

@HonorKnight
Copy link

@TimvdLippe and @raphw could I trouble you to look at this issue? I've been stuck on the same issue reported here for months and I had thought it would be fixed with #2618 that you two collaborated on, but it wasn't. Hopefully it's a similar enough issue that you might have some insight into it?

The current behavior makes no sense:

1: If you call mockito in what seems to be the "right" way, like doReturn(true).when(mc).somethingExists(), then it blows up due to improperly trying to apply the mock to the call to getMetaClass(), which seems to be related to the behavior you were fixing in that other pull request, perhaps a different path through the code that blows up due to yet another place it stumbles on that getMetaClass groovy pattern.

2: If you call mockito in what seems to be the "wrong" way, like when(mc.somethingExists()).thenReturn(true) then that does 2 things, first it immediately calls that somethingExists method which is obviously not what a developer writing this code intended for it to do, but then it also somehow correctly mocks this method for all future calls. I have no idea why Mockito is behaving in this way, but I'm trying to get by with using this approach for now, since it does ultimately do the right mocking... but it's problematic that it also calls the real method, so it has unwanted side effects depending on what the method in question may do.

@raphw
Copy link
Member

raphw commented May 11, 2022

We had fixed some meta class Groovy issues just recently. Are you sure that this is still an issue? This should use both for the regular and inline mock makers. We have also tests here: https://github.com/mockito/mockito/tree/main/subprojects

Could you attempt a reproducer in these projects? That might make it easier to point to the problem. Fair warning, though, I am not a Groovy expert, I have no idea how easy or hard this might be.

@HonorKnight
Copy link

HonorKnight commented May 12, 2022

Well this is frustrating. I tried to make a reproducer for this, and the error didn't happen. I stepped through the code in my full project versus this reproducer project, and they diverged in behavior in the middle of a Groovy class, CallSiteArray.createPogoSite, within the call there to .getMetaClass(). The error happens for me several layers down the stack within that call (the first step being bytebuddy's MockMethodDispatcher.get). However when I try to step into that call in the reproducer, it skips over it to the next line as though there was no method call there to step in to, and even if I put a breakpoint at MockMethodDispatcher.get it's never hit. I can't tell why this behavior diverges here and one project steps into that bytebuddy code while the other doesn't. With the IDE refusing to step into the method, and without any knowledge of how Mockito and Bytebuddy work I'm not sure how to trace this down any further and I can't create a viable reproducer on my own.

@HonorKnight
Copy link

And another test of mine that's failing is again at the exact same line in Groovy's CallSiteArray, but this time byteBuddy intercepts the call with MockMethodAdvice instead of MockMethodDispatcher. I don't understand what is triggering byteBuddy to take the wheel like this, so I don't know how to reproduce it under a simple controlled test. The byteBuddy methods that keep intercepting the calls are in Mockito, do you have any inkling as to what causes method calls to be redirected by this byteBuddy logic, @raphw ?

@raphw
Copy link
Member

raphw commented May 14, 2022

Are you sure that you are using the same mock maker? Java 17 imposes some limitations that we can only address with the inline mock maker. The reason your breakpoints do not always match up is due to Mockito instrumenting code and adding new "mock" branches to methods where you cannot see the generated code in your sources. The redirection effectively means that Mockito took control of a method call.

@HonorKnight
Copy link

I imported mockito-core and mockito-inline 4.5.1 into my gradle build and confirmed they’re on my classpath as those versions. Is there something else involved in determining the mock maker to be used? I am on openjdk 11 though.

@HonorKnight
Copy link

mockito seems to be trying to instrument that getMetaClass method, “taking control” of it in some manner, so it mistakes the method I’m actually trying to mock for the getMetaClass method itself - the metaClass is a stepping stone groovy uses for getting to the method I’m really trying to mock - and because my intended method vs getMetaClass have different numbers of parameters, zero vs nonzero, it kicks out in some validation logic.

@raphw
Copy link
Member

raphw commented May 14, 2022

In this case, the inline mock maker is used. It is true that the get meta class method is not mocked as it breaks many things. Did you declare your own method of this signature but with arguments? You find the exclusion rule in org.mockito.internal.creation.bytebuddy.BytecodeGenerator. Could you prepare a PR that demonstrates this error and maybe even adjust the matcher to fix your problem?

@solvingj
Copy link

solvingj commented Dec 12, 2022

@raphw here's a reproduction of a common Groovy testing situation with JUnit 5 and Mockito 4.9.0 which reproduces the error:

cannot be returned by getMetaClass()
getMetaClass() should return MetaClass

ExampleClass.groovy

import org.junit.jupiter.api.Test

import static org.mockito.Mockito.*

class ExampleClass {
    String exampleMethod(){
        return "ExampleClass.exampleMethod_returned"
    }
}


class ExampleTest {
    
    @Test
    void "getMetaClass error Mockito 490"() {

        given:
        String groovyScriptContents = """
            import ExampleClass 
            
            ExampleClass getExampleClass(){
                println("getExampleClass called")
                ExampleClass exampleClass = new ExampleClass()
                exampleClass.exampleMethod()
                return exampleClass
            }
            
            String helperMethod(){
                println("helperMethod called")
                ExampleClass exampleClass = getExampleClass()
                return exampleClass.exampleMethod()
            }
        """

        Script helperObject = new GroovyShell().parse(groovyScriptContents)

        when: "Running the method without any mocks."
        String result1 = helperObject.helperMethod()

        then: "Everything works fine."
        assert result1 == "ExampleClass.exampleMethod_returned"

        when: "Trying to spy/mock on the helper object with Mockito"
        Script helperMock = spy(helperObject)
        ExampleClass mockExampleClass = mock(ExampleClass)
        doReturn("mockReturn").when(mockExampleClass).exampleMethod()
        when(helperMock.getExampleClass()).thenReturn(mockExampleClass)
        String result2 = helperObject.helperMethod()

        then: "This code is never reached because we fail with error String cannot be returned by getMetaClass() "
        println(result2)
        assert result2 == "mockReturn"

    }

}

Output

helperMethod called
getExampleClass called


String cannot be returned by getMetaClass()
getMetaClass() should return MetaClass

Of note, one common use-case for this is "Jenkins Shared Libraries". A significant majority of non-trivial build pipelines used in Jenkins make use of Jenkins Shared Pipelines, and Groovy is the supported scripting language, and testing those scripts continues to be one of the biggest challenges in the entire Jenkins ecosystem. There are multiple custom testing tools for that use case, but none are silver bullets, all have drawbacks, but many of us would prefer to use our standard java dev tools of Mockito/JUnit. We could if you can make this work.

@raphw
Copy link
Member

raphw commented Dec 20, 2022

This class determines what methods to skip:

static ElementMatcher<MethodDescription> isGroovyMethod(boolean inline) {

Can you check the byte code of the Groovy class' and see if there is any new method that should be added? Ideally, submit a PR with that test case and groovyInlineTest. I can see if I find the time after the New Year's if you cannot.

@solvingj
Copy link

Here's the byte code, not sure if there is any new method to be added but maybe it helps you to see.
Thanks so much for looking into it:

//
// Source code recreated from a .class file by IntelliJ IDEA
// (powered by FernFlower decompiler)
//

import groovy.lang.GroovyObject;
import groovy.lang.MetaClass;
import org.codehaus.groovy.runtime.callsite.CallSite;

public class ExampleClass implements GroovyObject {
    public ExampleClass() {
        CallSite[] var1 = $getCallSiteArray();
        super();
        MetaClass var2 = this.$getStaticMetaClass();
        this.metaClass = var2;
    }

    public String exampleMethod() {
        CallSite[] var1 = $getCallSiteArray();
        return "ExampleClass.exampleMethod_returned";
    }
}

@eric-milles
Copy link

What groovy version was used to create this class? Groovy 2.5+ should be writing @groovy.transform.Generated or @groovy.transform.Internal on methods like getMetaClass(). Mockito's BytecodeGenerator#isGroovyMethod() referenced above looks for internal tagging.

I am able to run your example successfully with Groovy 4.0.13 and Mockito 4.9.0.

@cahi
Copy link

cahi commented Dec 4, 2023

I can still reproduce this error with the current bundles groovy version in Jenkins, which is 2.4.21. It fails with the described behavior regardless which mockito version I try to use, currently I'm using 5.2.0 for mockito-core and mockito-inline. Is there any possible way to work around this?

@eric-milles
Copy link

Groovy 2.5 is required to get the annotations on the generated methods for Mockito to handle properly. You can add these annotations in Groovy 2.4, although it is probably easier to supply a mocker.

This was tested against Mockito 1.10.19 -- I don't know if these hooks remain in Mockito 5.

<srcdir>/mockito-extensions/org.mockito.plugins.MockMaker

org.mockito.internal.creation.cglib.GroovyCglibMockMaker

<srcdir>/org/mockito/internal/creation/cglib/GroovyCglibMockMaker.java

package org.mockito.internal.creation.cglib;

import java.lang.reflect.Method;

import org.mockito.cglib.proxy.Factory;
import org.mockito.cglib.proxy.MethodInterceptor;
import org.mockito.cglib.proxy.MethodProxy;
import org.mockito.internal.InternalMockHandler;
import org.mockito.internal.creation.instance.InstantiatorProvider;
import org.mockito.invocation.MockHandler;
import org.mockito.mock.MockCreationSettings;
import org.mockito.plugins.MockMaker;

public class GroovyCglibMockMaker implements MockMaker
{
    @Override
    public MockHandler getHandler(final Object mock)
    {
        return new CglibMockMaker().getHandler(mock);
    }

    @Override
    public <T> T createMock(final MockCreationSettings<T> settings, final MockHandler handler)
    {
        new AcrossJVMSerializationFeature().enableSerializationAcrossJVM(settings);
        return new ClassImposterizer(new InstantiatorProvider().getInstantiator(settings))
            .imposterise(newMethodInterceptor(settings, handler), settings.getTypeToMock(), settings.getExtraInterfaces());
    }

    @Override
    public void resetMock(final Object mock, final MockHandler handler, final MockCreationSettings settings)
    {
        ((Factory) mock).setCallback(0, newMethodInterceptor(settings, handler));
    }

    private static MethodInterceptor newMethodInterceptor(final MockCreationSettings<?> settings, final MockHandler handler)
    {
        if (!groovy.lang.GroovyObject.class.isAssignableFrom(settings.getTypeToMock()))
            return new MethodInterceptorFilter((InternalMockHandler<?>) handler, settings) ;
        else
            return new MethodInterceptorFilter((InternalMockHandler<?>) handler, settings) {
                @Override
                public Object intercept(final Object proxy, final Method method, final Object[] args, final MethodProxy methodProxy)
                    throws Throwable
                {
                    if (method.getParameterCount() == 0 && method.getName().equals("getMetaClass"))
                        return groovy.lang.GroovySystem.getMetaClassRegistry().getMetaClass(settings.getTypeToMock());

                    return super.intercept(proxy, method, args, methodProxy);
                }
            };
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants