Last Comment Bug 734177 - Add tests for BrowserProvider
: Add tests for BrowserProvider
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: Firefox 14
Assigned To: Lucas Rocha (:lucasr)
:
Mentors:
Depends on:
Blocks: 735064
  Show dependency treegraph
 
Reported: 2012-03-08 10:32 PST by Lucas Rocha (:lucasr)
Modified: 2012-03-24 09:15 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Add ContentProvider test infrastructure (46.56 KB, patch)
2012-03-08 11:34 PST, Lucas Rocha (:lucasr)
gbrown: review-
Details | Diff | Splinter Review
Enable foreign keys support in sqlite (1.28 KB, patch)
2012-03-08 11:35 PST, Lucas Rocha (:lucasr)
gpascutto: review+
Details | Diff | Splinter Review
Add method to get path used by the sqlite database (1.15 KB, patch)
2012-03-08 11:35 PST, Lucas Rocha (:lucasr)
gpascutto: review+
Details | Diff | Splinter Review
Only include profile is set when cleaning up records (2.29 KB, patch)
2012-03-12 08:07 PDT, Lucas Rocha (:lucasr)
gpascutto: review+
Details | Diff | Splinter Review
Add PARAM_IS_TEST to BrowserProvider (6.51 KB, patch)
2012-03-12 08:08 PDT, Lucas Rocha (:lucasr)
gpascutto: review+
Details | Diff | Splinter Review
Make BrowserProvider's DATABASE_NAME public (1.15 KB, patch)
2012-03-12 08:08 PDT, Lucas Rocha (:lucasr)
gpascutto: review-
Details | Diff | Splinter Review
Add ContentProvider test infrastructure (46.84 KB, patch)
2012-03-12 08:09 PDT, Lucas Rocha (:lucasr)
gbrown: review-
Details | Diff | Splinter Review
Enable foreign keys support in sqlite (1.67 KB, patch)
2012-03-12 14:42 PDT, Lucas Rocha (:lucasr)
gpascutto: review+
Details | Diff | Splinter Review
Add ContentProvider test infrastructure (47.08 KB, patch)
2012-03-12 14:44 PDT, Lucas Rocha (:lucasr)
gbrown: review+
Details | Diff | Splinter Review
Make BrowserProvider's getDatabasePath() public (1.29 KB, patch)
2012-03-14 11:37 PDT, Lucas Rocha (:lucasr)
gpascutto: review+
Details | Diff | Splinter Review

Description Lucas Rocha (:lucasr) 2012-03-08 10:32:56 PST
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) 2012-03-08 11:34:06 PST
Created attachment 604138 [details] [diff] [review]
Add ContentProvider test infrastructure
Comment 2 Lucas Rocha (:lucasr) 2012-03-08 11:35:00 PST
Created attachment 604139 [details] [diff] [review]
Enable foreign keys support in sqlite
Comment 3 Lucas Rocha (:lucasr) 2012-03-08 11:35:48 PST
Created attachment 604140 [details] [diff] [review]
Add method to get path used by the sqlite database
Comment 4 Gian-Carlo Pascutto [:gcp] 2012-03-08 11:40:08 PST
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] 2012-03-08 11:43:25 PST
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] 2012-03-08 16:34:55 PST
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] 2012-03-09 09:02:30 PST
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) 2012-03-12 08:07:56 PDT
Created attachment 604944 [details] [diff] [review]
Only include profile is set when cleaning up records
Comment 9 Lucas Rocha (:lucasr) 2012-03-12 08:08:13 PDT
Created attachment 604945 [details] [diff] [review]
Add PARAM_IS_TEST to BrowserProvider
Comment 10 Lucas Rocha (:lucasr) 2012-03-12 08:08:39 PDT
Created attachment 604948 [details] [diff] [review]
Make BrowserProvider's DATABASE_NAME public
Comment 11 Lucas Rocha (:lucasr) 2012-03-12 08:09:06 PDT
Created attachment 604949 [details] [diff] [review]
Add ContentProvider test infrastructure
Comment 12 Lucas Rocha (:lucasr) 2012-03-12 08:12:00 PDT
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] 2012-03-12 11:03:12 PDT
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) 2012-03-12 14:42:30 PDT
Created attachment 605138 [details] [diff] [review]
Enable foreign keys support in sqlite
Comment 15 Lucas Rocha (:lucasr) 2012-03-12 14:44:16 PDT
Created attachment 605139 [details] [diff] [review]
Add ContentProvider test infrastructure
Comment 16 Gian-Carlo Pascutto [:gcp] 2012-03-12 14:44:37 PDT
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] 2012-03-12 16:04:45 PDT
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) 2012-03-13 09:30:44 PDT
(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] 2012-03-14 10:20:05 PDT
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) 2012-03-14 11:37:41 PDT
Created attachment 605869 [details] [diff] [review]
Make BrowserProvider's getDatabasePath() public

Note You need to log in before you can comment on or make changes to this bug.