The default bug view has changed. See this FAQ.

Add tests for BrowserProvider

RESOLVED FIXED in Firefox 13

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

Trunk
Firefox 14
All
Android
Points:
---

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed)

Details

Attachments

(5 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 604138 [details] [diff] [review]
Add ContentProvider test infrastructure
Attachment #604138 - Flags: review?(gbrown)
(Assignee)

Comment 2

5 years ago
Created attachment 604139 [details] [diff] [review]
Enable foreign keys support in sqlite
Attachment #604139 - Flags: review?(gpascutto)
(Assignee)

Comment 3

5 years ago
Created attachment 604140 [details] [diff] [review]
Add method to get path used by the sqlite database
Attachment #604140 - Flags: review?(gpascutto)
Comment on attachment 604139 [details] [diff] [review]
Enable foreign keys support in sqlite

How much of our stuff breaks if we actually enable this? :-)
Attachment #604139 - Flags: review?(gpascutto) → review+
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?
Attachment #604140 - Flags: review?(gpascutto) → review+
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 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/
Attachment #604138 - Flags: review?(gbrown) → review-
(Assignee)

Comment 8

5 years ago
Created attachment 604944 [details] [diff] [review]
Only include profile is set when cleaning up records
Attachment #604944 - Flags: review?(gpascutto)
(Assignee)

Comment 9

5 years ago
Created attachment 604945 [details] [diff] [review]
Add PARAM_IS_TEST to BrowserProvider
Attachment #604945 - Flags: review?(gpascutto)
(Assignee)

Comment 10

5 years ago
Created attachment 604948 [details] [diff] [review]
Make BrowserProvider's DATABASE_NAME public
Attachment #604948 - Flags: review?(gpascutto)
(Assignee)

Comment 11

5 years ago
Created attachment 604949 [details] [diff] [review]
Add ContentProvider test infrastructure
Attachment #604949 - Flags: review?(gbrown)
(Assignee)

Updated

5 years ago
Attachment #604138 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #604140 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
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 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?
Attachment #604949 - Flags: review?(gbrown) → review-
(Assignee)

Comment 14

5 years ago
Created attachment 605138 [details] [diff] [review]
Enable foreign keys support in sqlite
Attachment #605138 - Flags: review?(gpascutto)
(Assignee)

Updated

5 years ago
Attachment #604139 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 605139 [details] [diff] [review]
Add ContentProvider test infrastructure
Attachment #605139 - Flags: review?(gbrown)
(Assignee)

Updated

5 years ago
Attachment #604949 - Attachment is obsolete: true
Comment on attachment 605138 [details] [diff] [review]
Enable foreign keys support in sqlite

I don't think you need the ; at the end.
Attachment #605138 - Flags: review?(gpascutto) → review+
Attachment #604944 - Flags: review?(gpascutto) → review+
Blocks: 735064
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.
Attachment #605139 - Flags: review?(gbrown) → review+
(Assignee)

Comment 18

5 years ago
(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.
Attachment #604945 - Flags: review?(gpascutto) → review+
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.
Attachment #604948 - Flags: review?(gpascutto) → review-
(Assignee)

Comment 20

5 years ago
Created attachment 605869 [details] [diff] [review]
Make BrowserProvider's getDatabasePath() public
Attachment #605869 - Flags: review?(gpascutto)
(Assignee)

Updated

5 years ago
Attachment #604948 - Attachment is obsolete: true
Attachment #605869 - Flags: review?(gpascutto) → review+
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
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
status-firefox13: --- → fixed
status-firefox14: --- → fixed
You need to log in before you can comment on or make changes to this bug.