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

Mockito.spy(Activity).getBaseContext() returns null on Robolectric 4.4 and Java8 #2040

Closed
ganadist opened this issue Sep 9, 2020 · 38 comments
Assignees

Comments

@ganadist
Copy link

ganadist commented Sep 9, 2020

Description

Since robolectric 4.4, Mockito.spy(Activity).getBaseContext() returns null

Steps to Reproduce

$ git clone -b mockito_spy_robolectric_4_4 https://github.com/ganadist/VersionCodeDemo demo
$ cd demo

// switch to robolectric 4.3.1
$ git checkout HEAD~1
$ ./gradlew :app:testDevelopDebugUnitTest
BUILD SUCCESSFUL in 14s
28 actionable tasks: 28 executed

// switch to robolectric 4.4
$ git checkout mockito_spy_robolectric_4_4
$ ./gradlew :app:testDevelopDebugUnitTest
> Task :app:testDevelopDebugUnitTest FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':app:testDevelopDebugUnitTest'.

Failed unittest result : https://scans.gradle.com/s/wbmmbnzatcyai/tests/:app:testDevelopDebugUnitTest/com.example.myapplication.UnitTest1/test1#1

Robolectric & Android Version

  • Compile Sdk : 29
  • Target Sdk : 28
  • Robolectric : 4.4
  • Mockito : 3.5.10
  • Jvm : OpenJDK 8

Link to a public git repo demonstrating the problem:

https://github.com/ganadist/VersionCodeDemo/blob/mockito_spy_robolectric_4_4/app/src/test/java/com/example/myapplication/UnitTest1.java#L27-L38

This issue is copied from robolectric/robolectric#5916

@hoisie
Copy link

hoisie commented Sep 12, 2020

This seems to work with slightly older versions of mockito-inline, e.g. 3.4.2. I'll try a bisect and see what commit may have broken it.

@hoisie
Copy link

hoisie commented Sep 12, 2020

I did a bisect and this seems to have started failing in 47ef058, Adds support for creating spies from within a mock instance's constructor, thus avoiding reflection on final fields.

The workaround is to use mockito-inline < 3.5.0 I believe.

@raphw
Copy link
Member

raphw commented Sep 13, 2020

I just had a look and the problem is likely with Robolectric. Maybe they can have a look at the problem?
Mockito simply invokes the constructor chain and per class and within the constructor, it copies all values field by field. I am wondering if Robolectric does some explicit initialization where this approach now fails.

I am to foreign to both Android and Robolectric to really understand why this no longer works but it likely is related to the timing of the field copy.

Worst case, we need to offer a switch to create spies using reflection if required but if we can get a hint why this timing is a problem, we might be able to address this.

@hoisie
Copy link

hoisie commented Sep 13, 2020

Thanks for the initial investigation @raphw, I am one of the Robolectric maintainers and can try investigating this. It's odd that on Java 11 this occasionally works (but it is flaky according to robolectric/robolectric#5916), but on Java 8 it seems to always fail.

@raphw
Copy link
Member

raphw commented Sep 13, 2020

I'd like to understand in what field value the two objects differ to produce this result. They should be identical unless some field is not discoverable.

If you could check what state difference is responsible, I can probably find out how it occurred.

@hoisie
Copy link

hoisie commented Sep 14, 2020

Out of curiosity is there some kind of helper/debug util in Mockito to dump an object to a String? In the meantime I'll try to just serialize it to a json-type format.

@raphw
Copy link
Member

raphw commented Sep 14, 2020

No, we can dump class files but you could likely use a serializer such as Jackson for this?

@hoisie
Copy link

hoisie commented Sep 14, 2020

Weird, out of 269 fields, the only difference is a field called 'mBase'. On the spy object it is null.

132c132
< class android.content.ContextWrapper:mBase: android.app.ContextImpl@6d340ec8
---
> class android.content.ContextWrapper:mBase: null
269a270
>

MainActivity: https://gist.github.com/hoisie/5a7b93acc90a68917fc97de1c8364fc5
SpyActivity: https://gist.github.com/hoisie/76bbd478520f0bd7ec6f89fe74c0e163

Is there a way to turn on debug logs during this field copy to see how 'mBase' is being copied?

@hoisie
Copy link

hoisie commented Sep 14, 2020

This is the class that contains the 'mBase' field:
http://androidxref.com/9.0.0_r3/xref/frameworks/base/core/java/android/content/ContextWrapper.java

FWIW, it's a long class hierarchy, MainActivity > AppCompatActivity > FragmentActivity > ComponentActivity > Activity > ContextThemeWrapper > ContextWrapper > Context

@hoisie
Copy link

hoisie commented Sep 14, 2020

I just noticed ContextThemeWrapper, which is the parent of ContextWrapper, calls the ContextWrapper(null) constructor, which could be setting mBase to null in ContextWrapper:

    public ContextThemeWrapper() {
        super(null);
    }
    public ContextWrapper(Context base) {
        mBase = base;
    }

Could this be causing mBase to be set to null?

@hoisie
Copy link

hoisie commented Sep 14, 2020

How does one dump the class bytecode of the transient spy class created? I am curious what that looks like.

@raphw
Copy link
Member

raphw commented Sep 14, 2020

-Dnet.bytebuddy.dump=/some/folder

@hoisie
Copy link

hoisie commented Sep 14, 2020

Mockito simply invokes the constructor chain and per class and within the constructor, it copies all values field by field.

For some reason, when I do spy(object), I am not seeing the constructors being invoked. What constructors does mockito invoke? Are there some new/hidden constructors being generated?

@raphw
Copy link
Member

raphw commented Sep 14, 2020

No, the constructors are prefixed and then the original chain is shortcut. Basically:

public MyClass { MyClass() {
  // some code
} }

Is turned into:

public MyClass { MyClass() {
  if (Mockito.isMockConstruction()) {
    // Mockito stuff, basically copy all fields of the spied at object
    return;
  }
  // some code
} }

As a consequence, you cannot set a constructor breakpoint. I had a look at the field state and for both activity and mockActivity, ContextWrapper.mBase is set to the same instance but only activity returns the field's value.

@hoisie
Copy link

hoisie commented Sep 14, 2020

Out of curiosity how do you inject print statements to the field copy ByteBuddy logic? I wanted to add some print statements to verify that the 'mBase' before/after values are the same.

I had a look at the field state and for both activity and mockActivity, ContextWrapper.mBase is set to the same instance but only activity returns the field's value.

How can you tell that? From what I see, it seems like 'mBase' is null for the spy activity:

image

Do you think it's initially non-null and then gets set to null?

@raphw
Copy link
Member

raphw commented Sep 15, 2020

You are right, I did not debug properly (IntelliJ refuses to run the sample project for me, I am back on the command line).

When I inspect the transformed ContextWrapper constructor, I do however see that the field is copied within it:

        69: getfield      #40                 // Field mBase:Landroid/content/Context;
        72: aload_0
        73: dup_x1
        74: pop
        75: putfield      #40                 // Field mBase:Landroid/content/Context;

When is the mBase field populated? Is this something done after object construction?

@raphw
Copy link
Member

raphw commented Sep 15, 2020

I created a special debug build (println-constructor, you can run it by using your local Maven repo and the publishToMavenLocal task of Mockito) which shows what is happening by printing all field assignments. It follows the pattern

<class name> // for each in hierarchy
<mock object>
<field name> // for each field
<spied field value>
<mock field value after setting>

This verifies that the field is actually set. It's very strange that the field is null again once it is used. Since this problem occurs only with Robolectric, there must be some mechanism where the value disappears.

I also added a statement for the spy when it is constructed in its entirety and the field seems to be set correctly:

    DEBUG ROBOLECTICS: android.app.ContextImpl@51ad277e - original android.app.ContextImpl@51ad277e
    class android.content.Context
    com.example.myapplication.MainActivity@6219c42b
    public java.lang.Object android.content.Context.__robo_data__
    null
    null
    class android.content.ContextWrapper
    com.example.myapplication.MainActivity@6219c42b
    public java.lang.Object android.content.ContextWrapper.__robo_data__
    null
    null
    android.content.Context android.content.ContextWrapper.mBase
    android.app.ContextImpl@51ad277e
    android.app.ContextImpl@51ad277e
    ....

@raphw
Copy link
Member

raphw commented Sep 15, 2020

It's very strange, I optimized the example for output now and the fields seem to be set now that I changed the code:

        Field field = Class.forName("android.content.ContextWrapper").getDeclaredField("mBase");
        field.setAccessible(true);

        MainActivity activity = Robolectric.buildActivity(MainActivity.class).setup().get();
        Object fieldBefore = field.get(activity);
        Object methodBefore = activity.getBaseContext();

        MainActivity spyActivity = Mockito.spy(activity);

        throw new AssertionError("activity field: " + field.get(activity)
                        + " - mock activity field: " + field.get(spyActivity)
                        + " - activity getter: " + activity.getBaseContext()
                        + " - mock activity getter: " + spyActivity.getBaseContext()
                        + " - activity field before: " + fieldBefore
                        + " - activity getter before: " + methodBefore
                        + " - original: " + activity
                        + " - mock: " + spyActivity);

My gutt fealing is that that's some sort of initialization issue for the original object that is triggered later then Mockito expects it.

@raphw
Copy link
Member

raphw commented Sep 15, 2020

Maybe I turned blind but simply running:

MainActivity activity = Robolectric.buildActivity(MainActivity.class).setup().get();
MainActivity spyActivity = Mockito.spy(activity);
throw new AssertionError(activity.getBaseContext() + " - " + spyActivity.getBaseContext());

of the current release/3.x branch yields both fields being set correctly. Maybe this is something we already fixed in Mockito by accident?

@raphw
Copy link
Member

raphw commented Sep 15, 2020

Okay, I verified that Mockito creates the spy correctly in any case. But something happens to the object if passed through Robolectics's runner logic.

This test will function correctly:

@Test
public void test1() {
  MainActivity activity = Robolectric.buildActivity(MainActivity.class).setup().get();
  MainActivity mockActivity = spy(activity);
  assertEquals("compare base context between activity and mockActivity",
     activity.getBaseContext(),
     mockActivity.getBaseContext());
    }

While this test will yield yield an empty spy:

private MainActivity activity;
private MainActivity mockActivity;

@Before
public void setup() {
   activity = Robolectric.buildActivity(MainActivity.class).setup().get();
   mockActivity = spy(activity);
}

@Test
public void test1() {
 assertEquals("compare base context between activity and mockActivity",
   activity.getBaseContext(),    
   mockActivity.getBaseContext());
}

How can that be?

@raphw
Copy link
Member

raphw commented Sep 15, 2020

Comparing the output of the println branch: if the mock is created in the @Before method, the ContextWrapper constructor is never invoked what seems dubious. Something must be done differently here.

@hoisie
Copy link

hoisie commented Sep 15, 2020

I added a simpler test case to the Robolectric tree (currently @Ignored).

https://github.com/robolectric/robolectric/blob/master/integration_tests/mockito-experimental/src/test/java/org/robolectric/integrationtests/mockito/experimental/MockitoSpyTest.java

It is very strange, I've also seen situations where the test starts passing randomly, and I usually have to do a clean to get it to start failing again. It seems to fail more consistently when running through Gradle vs. running through intellij. This sometimes indicates a classpath issue.

Thanks for all your investigation and putting up that println branch, I'll try some print debugging as well.

@hoisie
Copy link

hoisie commented Sep 15, 2020

After adding some additional debug statements, it seems like the mockito instrumentation for ContextWrapper constructor is skipped So ContextThemeWrapper tries to call the ContextWrapper constructor, but it calls a non-instrumented one.
Maybe the default empty constructor is private?

Start of constructor for 'class android.app.Activity'
Invoking super constructor 'class android.app.Activity' -> 'android/view/ContextThemeWrapper'
Start of constructor for 'class android.view.ContextThemeWrapper'
Invoking super constructor 'class android.view.ContextThemeWrapper' -> 'android/content/ContextWrapper'
Start of constructor for 'class android.content.Context'
Invoking super constructor 'class android.content.Context' -> 'java/lang/Object'

@hoisie
Copy link

hoisie commented Sep 15, 2020

From looking at the bytecode, I see that ContextThemeWrapper calls ContextWrapper():

  public android.view.ContextThemeWrapper();
      ...
      31: invokespecial #317                // Method android/content/ContextWrapper."<init>":()V

However, in ContextWrapper, the no-arg constructor is not insrumented:

  public android.content.ContextWrapper();
    Code:
       0: aload_0
       1: invokespecial #816                // Method android/content/Context."<init>":()V
       4: aload_0
       5: invokevirtual #819                // Method $$robo$init:()V
       8: return

The one-arg ContextWrapper constructor is instrumented. though:

  public android.content.ContextWrapper(android.content.Context);

@hoisie
Copy link

hoisie commented Sep 15, 2020

This seems to be a simpler repro of the issue (non-Robolectric):

** EDIT **

NVM, still looking into a non-Robolectric repro:

@hoisie
Copy link

hoisie commented Sep 15, 2020

The issue seems to be that mockito is not instrumenting the no-arg public android.content.ContextWrapper() constructor, yet it is the one being selected to be called from the base class ContextThemeWrapper.

The one-arg public android.content.ContextWrapper(Context) does get instrumented, but the no-arg one is preferred as the one to be called from the base class.

Do you know what might cause Mockito to skip over the no-arg public android.content.ContextWrapper() constructor?

@raphw
Copy link
Member

raphw commented Sep 15, 2020

Is that constructor added later by Robolectrics? Maybe there's some confusion around the creation order. It instruments the constructors in-flight but takes basis on the reflective model. You can set a breakpoint in the ConstructorShortcut's wrap method. Does the no-arg constructor ever arrive here? If not, walk a bit down the stack into Byte Buddy to see if Byte Buddy was able to resolve this constructor to begin with.

@hoisie
Copy link

hoisie commented Sep 15, 2020

Yes, it does seem like Robolectric adds the no-arg constructor:
https://github.com/robolectric/robolectric/blob/master/sandbox/src/main/java/org/robolectric/internal/bytecode/ClassInstrumentor.java#L141-L153

(why does it do this? I cannot say exactly, that code was added many years ago...)

@raphw
Copy link
Member

raphw commented Sep 15, 2020

It's certainly related to that. Byte Buddy uses the reflection API to process the members. How is it even possible that the reflection API is unaware of a constructor? A class cannot add members after its loaded, the JVM does not allow it.

I assume this also explains why it makes a difference where the mock is created. Does Robolectrics transform classes from one class loader into another? I have a feeling this is somewhat related to parallel versions of the same class.

@raphw
Copy link
Member

raphw commented Sep 15, 2020

As a workaround, Mockito could prefer non-synthetic constructors for the time being.

@hoisie
Copy link

hoisie commented Sep 15, 2020

Another possible solution: could Mockito only select super-constructors to call that are known to be instrumented? It should be possible to keep track of that.

@hoisie
Copy link

hoisie commented Sep 15, 2020

ConstructorShortcut.wrap never hits the no-arg ContextWrapper() constructor. I am not sure how to check if ByteBuddy ever resolved it, but I do not believe it did.

@hoisie
Copy link

hoisie commented Sep 15, 2020

I have a feeling this is somewhat related to parallel versions of the same class

Yes, I believe this is happening. When an Android test is run, initially the Android SDK stubs are on the classpath, which contains a small subset of the real Android framework Jar that are all no-ops. Robolectric, during runtime, pulls in the real Android framework JARs and instruments the Android classes to use the real Android code (plus mixes in the shadows, which override the framework method implementations).

@hoisie
Copy link

hoisie commented Sep 16, 2020

BTW I think preferring non-synthetic constructors seems like a fine workaround for now. I will attempt to stop adding the no-arg constructor to Android objects. I'm not sure to what extent it is still being used.

@hoisie
Copy link

hoisie commented Sep 16, 2020

Actually, ByteBuddy does seem to find the no-arg ContextWrapper constructor, but methodDescription is null, so the wrap method does not get invoked:

image

hoisie added a commit to hoisie/mockito that referenced this issue Sep 16, 2020
…tcut

After moving spy creation to instrumenting constructor chains in
ByteBuddyMockMaker, creating spies for Robolectric-instrumented Android
classes started failing in some situations due to fields not being
copied on base classes (specifically, ContextWrapper.mBase). The problem
is that AsmVisitorWrapper.ForDeclaredMethods does not visit the
synthetic constructrs that are added by Robolectric during runtime.
While the visibility issuer is still being explored, a workaround is to
prefer non-synthetic constructors when selecting which parent
constructor to call in MockMethodAdvice.ConstructorShortcut.

Fixes mockito#2040
hoisie added a commit to hoisie/mockito that referenced this issue Sep 16, 2020
…tcut

After moving spy creation to instrumenting constructor chains in
ByteBuddyMockMaker, creating spies for Robolectric-instrumented Android
classes started failing in some situations due to fields not being
copied on base classes (specifically, ContextWrapper.mBase). The problem
is that AsmVisitorWrapper.ForDeclaredMethods does not visit the
synthetic constructrs that are added by Robolectric during runtime.
While the visibility issuer is still being explored, a workaround is to
prefer non-synthetic constructors when selecting which parent
constructor to call in MockMethodAdvice.ConstructorShortcut.

Fixes mockito#2040
hoisie added a commit to hoisie/mockito that referenced this issue Sep 16, 2020
…tcut

After moving spy creation to instrumenting constructor chains in
ByteBuddyMockMaker, creating spies for Robolectric-instrumented Android
classes started failing in some situations due to fields not being
copied on base classes (specifically, ContextWrapper.mBase). The problem
is that AsmVisitorWrapper.ForDeclaredMethods does not visit the
synthetic constructrs that are added by Robolectric during runtime.
While the visibility issuer is still being explored, a workaround is to
prefer non-synthetic constructors when selecting which parent
constructor to call in MockMethodAdvice.ConstructorShortcut.

Fixes mockito#2040
@raphw
Copy link
Member

raphw commented Sep 16, 2020

Yes, it's in the changed class file but I wonder why the reflection API does not contain those constructors, that's why Byte Buddy cannot map them. I assume that the class is reloaded somehow into another class loader what then causes this mismatch.

The problem in short is that ContectWrapper. class.getDeclaredConstructors() does not add the added no argument constructor.

@raphw raphw closed this as completed in 4a40f58 Sep 17, 2020
raphw added a commit that referenced this issue Sep 17, 2020
…ude-synthetic

Do not exclude synthetic constructors from instrumentation. Fixes #2040.
@hoisie
Copy link

hoisie commented Sep 17, 2020

Thanks for all your hard work @raphw! I am glad this is fixed.

@raphw
Copy link
Member

raphw commented Sep 18, 2020

Right back at you! Thanks for your patient debugging help!

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

Successfully merging a pull request may close this issue.

3 participants