734177 - Add tests for BrowserProvider

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

We have zero tests for our database layer at the moment. This is a core part of Fennec that needs proper testing coverage.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Add ContentProvider test infrastructure

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Enable foreign keys support in sqlite

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Add method to get path used by the sqlite database

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 604139 [details] [diff] [review]
Enable foreign keys support in sqlite

How much of our stuff breaks if we actually enable this? :-)

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Ahem, I can't see enough context in this patch, but I think you need to pull it out one more level. Right now it won't be enabled if the SDK >= 11?

Comment 6

[Geoff Brown [:gbrown]]

This test runs fine on my Galaxy S / Froyo, but fails consistently on the Galaxy Nexus / ICS:

INFO | runtests.py | Running tests: start.


org.mozilla.fennec_mozdev.tests.testBrowserProvider:
Error in testBrowserProvider:
java.lang.reflect.InvocationTargetException
	at java.lang.reflect.Method.invokeNative(Native Method)
	at org.mozilla.fennec_mozdev.tests.testBrowserProvider.getDatabasePath(testBrowserProvider.java:150)
	at org.mozilla.fennec_mozdev.tests.testBrowserProvider.tearDown(testBrowserProvider.java:278)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:537)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)
Caused by: java.lang.IllegalArgumentException: File testBrowserProvider./data/data/org.mozilla.fennec_mozdev/files/mozilla/pe4tfvnl.default/browser.db contains a path separator
	at android.app.ContextImpl.makeFilename(ContextImpl.java:1596)
	at android.app.ContextImpl.validateFilePath(ContextImpl.java:1580)
	at android.app.ContextImpl.openOrCreateDatabase(ContextImpl.java:769)
	at android.test.RenamingDelegatingContext.openOrCreateDatabase(RenamingDelegatingContext.java:159)
	at android.content.ContextWrapper.openOrCreateDatabase(ContextWrapper.java:221)
	at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:157)
	at android.database.sqlite.SQLiteOpenHelper.getReadableDatabase(SQLiteOpenHelper.java:231)
	at org.mozilla.fennec_mozdev.db.BrowserProvider.getDatabasePathForProfile(BrowserProvider.java:652)
	... 13 more

Error in testAndroidTestCaseSetupProperly:
java.lang.reflect.InvocationTargetException
	at java.lang.reflect.Method.invokeNative(Native Method)
	at org.mozilla.fennec_mozdev.tests.testBrowserProvider.getDatabasePath(testBrowserProvider.java:150)
	at org.mozilla.fennec_mozdev.tests.testBrowserProvider.tearDown(testBrowserProvider.java:278)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:537)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)
Caused by: java.lang.IllegalArgumentException: File testBrowserProvider./data/data/org.mozilla.fennec_mozdev/files/mozilla/pe4tfvnl.default/browser.db contains a path separator
	at android.app.ContextImpl.makeFilename(ContextImpl.java:1596)
	at android.app.ContextImpl.validateFilePath(ContextImpl.java:1580)
	at android.app.ContextImpl.openOrCreateDatabase(ContextImpl.java:769)
	at android.test.RenamingDelegatingContext.openOrCreateDatabase(RenamingDelegatingContext.java:159)
	at android.content.ContextWrapper.openOrCreateDatabase(ContextWrapper.java:221)
	at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:157)
	at android.database.sqlite.SQLiteOpenHelper.getReadableDatabase(SQLiteOpenHelper.java:231)
	at org.mozilla.fennec_mozdev.db.BrowserProvider.getDatabasePathForProfile(BrowserProvider.java:652)
	... 13 more

Test results for InstrumentationTestRunner=.E.E
Time: 0.071

FAILURES!!!
Tests run: 2,  Failures: 0,  Errors: 2

Comment 7

[Geoff Brown [:gbrown]]

Comment on attachment 604138 [details] [diff] [review]
Add ContentProvider test infrastructure

Review of attachment 604138 [details] [diff] [review]:
-----------------------------------------------------------------

r- only because of the test failure on ICS (see previous comment).

::: mobile/android/base/tests/ContentProviderTest.java.in
@@ +22,5 @@
> +import java.io.File;
> +import java.util.ArrayList;
> +import java.util.HashMap;
> +
> +abstract class ContentProviderTest extends AndroidTestCase {

AndroidTestCase rather than BaseTest/InstrumentationTestCase: Interesting! 

I think you *could* use BaseTest instead, with these benefits:
* Your code could be used by UI tests that need BaseTest/InstrumentationTestCase capabilities. (eg. manipulate history entries, then verify history UI).
* Duplicate code for setTestType, etc would be eliminated.
On the other hand, the way you have written this is very tidy, and a BaseTest-derived version might not be as clean...so this is just something to consider.

Regardless, add a brief comment here to tell people what a ContentProviderTest is and why/when they might want to re-use it.

@@ +58,5 @@
> +            return "/data";
> +        }
> +    }
> +
> +	private void setUpProviderClassAndAuthority(String providerClassName, String authorityUriField) throws Exception {

fix spacing / formatting

@@ +131,5 @@
> +
> +    public void setUp(String providerClassName, String authorityUriField) throws Exception {
> +        super.setUp();
> +
> +	    mClassLoader = getContext().getClassLoader();

fix spacing

::: mobile/android/base/tests/testBrowserProvider.java.in
@@ +12,5 @@
> +
> +import java.io.File;
> +import java.lang.reflect.Method;
> +
> +public class testBrowserProvider extends ContentProviderTest {

nit - add a brief comment here announcing the purpose/scope of this test

@@ +353,5 @@
> +
> +            mAsserter.is(c.moveToFirst(), true, "Inserted bookmark found");
> +
> +            mAsserter.is(c.getString(c.getColumnIndex(mBookmarksTitleCol)), b.getAsString(mBookmarksTitleCol),
> +                         "Inserted history entry has correct title");

s/history/bookmark/

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Only include profile is set when cleaning up records

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Add PARAM_IS_TEST to BrowserProvider

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: Make BrowserProvider's DATABASE_NAME public

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: Add ContentProvider test infrastructure

Comment 12

[Lucas Rocha (:lucasr)]

I've taken a new approach to handle the db file while running the content provider inside the test case. Hence the new patches. Geoff, I hope the comments I added at the top of the ContentProviderTest clarifies why I'm not using BaseTest/InstrumentationTest here. You have to apply all those patches to get the tests running fine.

Comment 13

[Geoff Brown [:gbrown]]

Comment on attachment 604949 [details] [diff] [review]
Add ContentProvider test infrastructure

Review of attachment 604949 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good. And it works great on my Galaxy S...but it still fails on my Galaxy Nexus:

Failure in testBrowserProvider:
junit.framework.AssertionFailedError: 31 INFO TEST-UNEXPECTED-FAIL | testBrowserProvider - TestInsertBookmarks | Should not be able to insert bookmark with invalid parent - got 7, expected -1
	at org.mozilla.fennec_mozdev.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:134)
	at org.mozilla.fennec_mozdev.FennecMochitestAssert.ok(FennecMochitestAssert.java:164)
	at org.mozilla.fennec_mozdev.FennecMochitestAssert.is(FennecMochitestAssert.java:174)
	at org.mozilla.fennec_mozdev.tests.testBrowserProvider$TestInsertBookmarks.test(testBrowserProvider.java:389)
	at org.mozilla.fennec_mozdev.tests.testBrowserProvider$Test.run(testBrowserProvider.java:285)
	at org.mozilla.fennec_mozdev.tests.testBrowserProvider.testBrowserProvider(testBrowserProvider.java:270)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:537)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)


r- only because of the Galaxy Nexus test failure.

::: mobile/android/base/tests/ContentProviderTest.java.in
@@ +28,5 @@
> + * tests in an controlled/isolated environment, guaranteeing that your tests
> + * will not affect or be affected by any UI-related code. This is basically
> + * a heavily adapted port of Android's ProviderTestCase2 to work on Mozilla's
> + * infrastructure.
> + */

That's great - just what I was looking for.

@@ +58,5 @@
> +            return this;
> +        }
> +    }
> +
> +	private void setUpProviderClassAndAuthority(String providerClassName,

fix the spacing here, and below

@@ +133,5 @@
> +
> +    public void setUp(String providerClassName, String authorityUriField) throws Exception {
> +        super.setUp();
> +
> +	    mClassLoader = getContext().getClassLoader();

spacing again - are these tabs?

::: mobile/android/base/tests/testBrowserProvider.java.in
@@ +271,5 @@
> +        }
> +    }
> +
> +    public void getDatabaseName() throws Exception {
> +        super.tearDown();

what's happening here? copy/paste error?

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: Enable foreign keys support in sqlite

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Add ContentProvider test infrastructure

Comment 16

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 605138 [details] [diff] [review]
Enable foreign keys support in sqlite

I don't think you need the ; at the end.

Comment 17

[Geoff Brown [:gbrown]]

Comment on attachment 605139 [details] [diff] [review]
Add ContentProvider test infrastructure

Review of attachment 605139 [details] [diff] [review]:
-----------------------------------------------------------------

The new tests pass on both Galaxy S and Galaxy Nexus now. A few nits remain -- see comment 13.

Comment 18

[Lucas Rocha (:lucasr)]

(In reply to Geoff Brown [:gbrown] from comment #17)
> Comment on attachment 605139 [details] [diff] [review]
> Add ContentProvider test infrastructure
> 
> Review of attachment 605139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The new tests pass on both Galaxy S and Galaxy Nexus now. A few nits remain
> -- see comment 13.

Fixed, thanks.

Comment 19

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 604948 [details] [diff] [review]
Make BrowserProvider's DATABASE_NAME public

As discussed on IRC, given that you need this to delete the physical database file, it looks slightly cleaner to me to make getDatabasePath public (if that works for the tests) and try to use that.

Comment 20

[Lucas Rocha (:lucasr)]

Attachment: Make BrowserProvider's getDatabasePath() public

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/481a36ea59ec
https://hg.mozilla.org/mozilla-central/rev/b1f692f2df1a
https://hg.mozilla.org/mozilla-central/rev/65bede15d312
https://hg.mozilla.org/mozilla-central/rev/6fad9374a6ca
https://hg.mozilla.org/mozilla-central/rev/9580af5523a9

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5186e1f163e9
https://hg.mozilla.org/releases/mozilla-aurora/rev/e25db51325c0
https://hg.mozilla.org/releases/mozilla-aurora/rev/f6fd040cf716
https://hg.mozilla.org/releases/mozilla-aurora/rev/96559da96bf2
https://hg.mozilla.org/releases/mozilla-aurora/rev/111155f275ab