949208 - ContentProviderTest crashes during tearDown()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[:Margaret Leibovic]

In `ContentProviderTest.tearDown` we try to delete the databases created by the content provider in order to clean up after ourselves. However, we always fail because of an NPE, but TBPL doesn't seem to notice since this happens after all the assertions pass.

11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): ----- begin exception -----
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145):
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): java.lang.reflect.InvocationTargetException
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at java.lang.reflect.Method.invokeNative(Native Method)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at java.lang.reflect.Method.invoke(Method.java:511)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at org.mozilla.gecko.tests.ContentProviderTest.tearDown(ContentProviderTest.java:257)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at org.mozilla.gecko.tests.testDistribution.tearDown(testDistribution.java:333)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at junit.framework.TestCase.runBare(TestCase.java:130)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at junit.framework.TestResult$1.protect(TestResult.java:106)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at junit.framework.TestResult.runProtected(TestResult.java:124)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at junit.framework.TestResult.run(TestResult.java:109)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at junit.framework.TestCase.run(TestCase.java:118)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:545)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): Caused by: java.lang.NullPointerException
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	at org.mozilla.gecko.db.BrowserProvider.getDatabasePath(BrowserProvider.java:1959)
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): 	... 13 more
11:52:39     INFO -  12-11 11:52:35.820 I/TestRunner( 4145): ----- end exception -----

Looking at TBPL logs from before/after bug 941357 landed, that seems to have introduced this problem :/

I suppose that's because we used to just automatically return the database name, rather than try getting the database helper out of the map first:
https://hg.mozilla.org/mozilla-central/rev/904545658c9b#l1.129

All that being said, I don't think we actually need to delete these databases, since each robocop test runs with a new profile. Maybe that's a bad thing to rely on, but it would be a convenient excuse to remove this bit of reflection, as well as the getDatabasePath method in BrowserProvider.

Comment 1

[:Margaret Leibovic]

Attachment: Don't use reflection to get database name in robocop tests

This fixes the problem for me locally, and I think it's a nice way to get rid of some test-only methods in our code.

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=8094ee3c293e

Comment 2

[:Margaret Leibovic]

Comment on attachment 8346221 [details] [diff] [review]
Don't use reflection to get database name in robocop tests

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

::: mobile/android/base/tests/ContentProviderTest.java
@@ +249,5 @@
>          if (Build.VERSION.SDK_INT >= 11) {
>              mProvider.shutdown();
>          }
>  
> +        if (mDatabaseName != null) {

Note: I kept this null check because theoretically you could want to test a content provider that's not backed by a database.

Comment 3

[:Margaret Leibovic]

Attachment: Don't use reflection to get database name in robocop tests

I actually have no idea why we kept this getDatabasePath method in TabsProvider. TabsProvider doesn't even have any robocop tests!

New try push:
https://tbpl.mozilla.org/?tree=Try&rev=48379686595f

Comment 4

[:Margaret Leibovic]

Comment on attachment 8346225 [details] [diff] [review]
Don't use reflection to get database name in robocop tests

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

Also tagging wesj here in case he might be able to get to this sooner :)

Comment 5

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/35c77e77f73f

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35c77e77f73f