yahoo / squidb

SquiDB is a SQLite database library for Android and iOS
https://github.com/yahoo/squidb/wiki
Apache License 2.0
1.31k stars 132 forks source link

Getting "null not found in model" when using a SquiDB generated class in unit tests #100

Closed zsoltk closed 9 years ago

zsoltk commented 9 years ago

Hello,

I'm writing a unit test for a mapper class, which maps a SquiDB generated class (let's call it DataFoo) to a presentation entity (Foo) and vice versa.

Problem is, the getters of the generated class don't seem to work in unit tests. My mapper calls DataFoo.setName(someValue), and when testing said value in the unit test by calling DataFoo.getName() in the unit test, I get this exception:

java.lang.UnsupportedOperationException: null not found in model. Make sure the value was set explicitly, read from a cursor, or that the model has a default value for this property.
    at com.yahoo.squidb.data.AbstractModel.get(AbstractModel.java:275)
    at com.example.foobar.data.entity.DataFoo.getName(DataFoo.java:86)

The interesting part is: "null not found in model", where null should be the name of the property I'm trying to read, i.e. the NAME property. Digging deeper, the name of generated property really is null:

assertThat(DataFoo.NAME.getName(), is(not(nullValue()))); // fails

If I have any other generated properties, they also fail, except for ID, which comes from the base classes. That works.

Source for DataFoo, really nothing special:

// DataFooSpec.class

package com.example.foobar.data.entity;

import com.yahoo.squidb.annotations.TableModelSpec;

@TableModelSpec(className="DataFoo", tableName="foo")
public class DataFooSpec {
    String name;
    // remainder omitted
}

Running the app on the device works fine, I'm having this problem in unit tests only. Any idea what causes this?

Thanks!

jdkoren commented 9 years ago

Are you positive setName() was called on your DataFoo instance before your test calls getName() on it?

assertThat(DataFoo.NAME.getName(), is(not(nullValue()))); // fails

I'm not sure how this is possible. Our unit tests include ones that check that generated names for properties are not null. I also tried in a new test project that only had a model spec and had a single test that passed when I ran it:

@TableModelSpec(className = "Foo", tableName = "foo")
public class FooSpec {
    String name;
}

public void testFoo() {
    Foo foo = new Foo();
    assertNotNull(Foo.NAME.getName()); // property name is not null
    foo.setName("Cookie Monster");
    assertNotNull(foo.getName()); // stored name is not null
}

Perhaps you can create a sample project with just the minimal amount of code that reproduces the failure so we can take a deeper look?

sbosley commented 9 years ago

@jdkoren even if no value was set, getName() should always return the name of the property it's called on, so I don't think setting a value first would make a difference. The fact that that test is failing is indeed puzzling!

Thanks for the detailed report @zsoltk. Am I right in assuming you're using Mockito? Maybe there's something about Mockito that is messing with or mocking the generated model classes rather than using their full versions? A minimal project that reproduces the error would be super helpful if you're willing to put it together.

jdkoren commented 9 years ago

@sbosley right, that's why the test above checks both Foo.NAME.getName() and foo.getName().

zsoltk commented 9 years ago

Thanks for the quick reply!

@jdkoren Yes, I'm positive. Also, it's not the problem, since Foo.NAME.getName() is null, yes. @sbosley In this test I'm not using Mockito.

I put together a minimal test project, stripped everything unrelated from it: https://github.com/zsoltk/squidb-issue-100

Just run ./gradlew test after cloning and you should see:

:app:testDebugUnitTest

com.example.foobar.data.mapper.DataFooMapperTest > testMap FAILED
    java.lang.UnsupportedOperationException at DataFooMapperTest.java:35

com.example.foobar.data.mapper.DataFooMapperTest > testDebug FAILED
    java.lang.AssertionError at DataFooMapperTest.java:44

2 tests completed, 2 failed
:app:testDebugUnitTest FAILED

The file in question is located in /app/src/test/java/com/example/foobar/data/mapper/DataFooMapperTest.java

Thanks!

sbosley commented 9 years ago

Great, thanks for putting this together! We'll take a look and see if we can figure out what's going on.

sbosley commented 9 years ago

Ok, new information: I modified the test to log information by throwing an exception with relevant data.

assertThat(DataFoo.NAME, is(not(nullValue())));
assertThat(DataFoo.NAME.getExpression(), is("name"));
String expression = DataFoo.NAME.getExpression();
String name = DataFoo.NAME.getName();
assertThat(name, is(nullValue()));
boolean hasAlias = DataFoo.NAME.hasAlias();
throw new RuntimeException("DataFoo.NAME has alias? " + hasAlias + ", expression: " + expression + ", name: " +
                name + ", TextUtils name check (should be true): " + TextUtils.isEmpty(name));

The exception logs the following: "DataFoo has alias? true, expression: name, name: null, TextUtils name check (should be true): false"

From this I'm gathering that TextUtils.isEmpty is somehow broken in the unit testing environment; it returns false for a null value when it should return true. (Incidentally this is why hasAlias is returning true, when it too should be false). I don't know enough about the unit testing environment to know why this is happening, but I don't think it's an error on the squidb side.

sbosley commented 9 years ago

More simply, to confirm for sanity's sake, this fails:

assertThat(TextUtils.isEmpty(null), is(true));
zsoltk commented 9 years ago

You mean this, right?

Caused by: java.lang.RuntimeException: Method isEmpty in android.text.TextUtils not mocked. See http://g.co/androidstudio/not-mocked for details.

I've seen it, and didn't give it much thought (I didn't notice this came from inside of SquiDB), and used the gradle config the documentation suggests at the url given above:

testOptions {
    unitTests.returnDefaultValues = true
}

So SquiDB depends on TextUtils working? Because that class will only be working instrumented if I'm correct, which means SquiDB classes cannot be tested in simple unit tests. Right? Or wrong?

Looking at SquiDB source, TextUtils is only used for checking isEmpty(), which could be replicated without relying on the Android class itself. What do you think?

sbosley commented 9 years ago

Probably true. We only ever use instrumentation tests for SquiDB which is why we haven't seen this issue before. I think you're right though that it makes sense to duplicate the TextUtils.isEmpty functionality and use that instead.

Thanks for reporting this! We'll get a fix out soon.

zsoltk commented 9 years ago

Great, thanks! :+1:

sbosley commented 9 years ago

Uh oh, I spoke too soon. We could theoretically replace the TextUtils dependency, but SquiDB also depends on ContentValues, which is also apparently mocked in the basic test environment. That dependency we really can't replace, as we leverage ContentValues for things like making model objects Parcelable. So I think if you're trying to directly test SquiDB models, it will need to be in an environment where such dependencies are available.

zsoltk commented 9 years ago

Is ContentValues essential for getting the getters / setters in a generated class to work, or only for the more complex things?

If the latter, then this snippet can come again handy on the client side:

testOptions {
    unitTests.returnDefaultValues = true
}

ContentValues can be left in, and it will not break, just return... null I guess? (Can't test it right now).

If only the getters would be working (that is, the original problem with TextUtils dependency solved), testing would be ok. Again, I'm not testing SquiDB itself, nor any complex behavior of the generated class, just simply asking whether a value was set correctly by my mapper class.

sbosley commented 9 years ago

ContentValues are essential for the getters/setters in a generated to model to work. In fact, model objects are really just big glorified wrappers around a ContentValues. So if ContentValues is mocked, no values would ever be stored or be readable from a model object -- because under the hood, setters call values.set(name, value) and getters call values.get(name).

sbosley commented 9 years ago

I'm not an expert on dealing with mocks in unit tests, but what if your unit test, instead of calling the getter to check the value, merely did some kind of verify() call that the setter method was called with the correct arguments on the model object?

zsoltk commented 9 years ago

Well, that's a possibility (however, a bit awkward here).

Another is using Robolectric for the particular unit test involving calling the getters / setters. I've successfully run my test with annotating the class:

@RunWith(RobolectricGradleTestRunner.class)
@Config(constants = BuildConfig.class, sdk = Build.VERSION_CODES.JELLY_BEAN)
public class DataFooMapperTest {
    // remainder omitted

Changed nothing else. Works.

Care to write a wiki page about this all to help others? :)

sbosley commented 9 years ago

Great! I was just idly wondering if robolectric would help with this problem; thanks for looking into that pre-emptively :) I definitely think a wiki page would be helpful for this. We'll compile all the things we've learned from this issue and post something soon.

Thanks again for helping us look into this! Definitely super helpful to know about.

zsoltk commented 9 years ago

Great, thanks! :)

sbosley commented 9 years ago

Wrote a wiki page summarizing the issue and detailing the fix you discovered for us. Thanks again for raising the issue! I'm going to close this ticket but if you have any other comments that you think would make the wiki page better feel free to reply here.