jmockit / jmockit1

Advanced Java library for integration testing, mocking, faking, and code coverage
Other
465 stars 240 forks source link

Incorrect dynamically dispatched method using faking API #631

Closed Novemser closed 5 years ago

Novemser commented 5 years ago

Please provide the following information:

Way to reproduce just run the following junit test:

package mockit;

import org.junit.Test;

public class FakingAPITest {
  public class A {
    public void add(Object o) {}
  }

  public class B extends A {
    public void addLocal(Object o) {
      super.add(o);
    }

    @Override
    public void add(Object o) {
      throw new RuntimeException("Should not be here");
    }
  }

  @Test
  public void test() {
    class FakeA extends MockUp<A> {
      @Mock
      public Object $advice(Invocation invocation) {
        return invocation.proceed();
      }
    }
    new FakeA();
    B b = new B();
    b.addLocal("");
  }
}

Produces exception like this:

java.lang.RuntimeException: Should not be here

    at mockit.FakingAPITest$B.add(FakingAPITest.java:17)
    at mockit.FakingAPITest$1FakeA.$advice(FakingAPITest.java:26)
    at mockit.FakingAPITest$B.addLocal(FakingAPITest.java:12)
    at mockit.FakingAPITest.test(FakingAPITest.java:31)

jdk version:

openjdk version "1.8.0_181"
OpenJDK Runtime Environment (build 1.8.0_181-8u181-b13-2~deb9u1-b13)
OpenJDK 64-Bit Server VM (build 25.181-b13, mixed mode)
rliesenfeld commented 5 years ago

Well, proceed doesn't know the method execution is coming from a super.add call, and if addLocal did call add(o) instead, we would expect B#add to be executed, so I wouln't necessarily say this behavior is "wrong". In any case, I don't see an alternative to what proceed does. Do you?

A potential work-around is to write the fake method as follows:

         @Mock
         public Object $advice(Invocation invocation) {
            if (invocation.getInvokedInstance().getClass() == A.class) {
               return invocation.proceed();
            }
            return null;
         }
Novemser commented 5 years ago

Yeah, while the JVM distinguishes super.add via invokespecial and this.add via invokevirtual, proceed itself seems doesn't have enough information to infer this, thus will always be dynamically dispatched based on the invoked instance's type.

In the above work-around, invocation.getInvokedInstance().getClass() will be B.class rather than A.class because the invoked instance is of type B, so this work-around seems does not work?

Currently, I don't come up with an idea that could help me distinguish the two different cases(maybe intercepting the call stack and infer from the stack trace? But seems inefficient and subtle)...Do you have any suggestions?

Many Thanks!

rliesenfeld commented 5 years ago

To clariry, when proceed gets called, it invokes the faked method (A#add) again through Reflection (Method#invoke), after having set an internal flag so that the faked method this times executes its real implementation instead of diverting into the fake method ($advice). Reflection provides no way to execute a super.abc() call, so a solution would have, necessarily, to not make this Reflection invoke call.

So, the only solution I can think of would be add a new kind of "proceed" method to the Invocation class, something like proceedNoReturn() (can't think of a good name at this time), which causes the faked method to complete its execution (and if it returns some value, the fake method would not be able to get it). This would be equivalent to "Before advice", while proceed is equivalent to AOP's "Around advice".

Novemser commented 5 years ago

Got it, thanks!