Closed Bug 782242 Opened 7 years ago Closed 7 years ago

Robocop: reflection problems

Categories

(Testing :: General, defect)

x86
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: gbrown, Assigned: gbrown)

Details

Attachments

(1 file, 1 obsolete file)

Robocop uses reflection to invoke various Java methods and classes. Reflection is prone to breakage because changes in the product may change method signatures without breaking the build. Further, some of our test code catches and ignores reflection exceptions, so these failures persist silently. Let's review the code, ensure that such exceptions cause test failures, and fix any failures we find.
Here's one problem, normally silently ignored:

08-12 21:41:06.671 W/System.err( 4227): java.lang.IllegalArgumentException: object is not an instance of the class
08-12 21:41:06.681 W/System.err( 4227): 	at java.lang.reflect.Method.invokeNative(Native Method)
08-12 21:41:06.681 W/System.err( 4227): 	at java.lang.reflect.Method.invoke(Method.java:521)
08-12 21:41:06.681 W/System.err( 4227): 	at org.mozilla.fennec_mozdev.tests.ContentProviderTest.tearDown(ContentProviderTest.java:241)
08-12 21:41:06.683 W/System.err( 4227): 	at org.mozilla.fennec_mozdev.tests.testMigration.tearDown(testMigration.java:353)
The method being invoked here is:

public java.lang.String org.mozilla.fennec_mozdev.db.BrowserProvider.getDatabasePath(java.lang.String,boolean)

mProvider is of type:

org.mozilla.fennec_mozdev.tests.ContentProviderTest$DelegatingTestContentProvider@4832ddd0
Lucas - There's more to this bug, but I thought I'd check in with you about this bit for the ContentProviderTest...does it look right? Removing the try/catch exposes the exception, causing the test to fail and report the exception. Then I just changed mProvider to targetProvider. I verified that this allows getDatabasePath to succeed -- it returned "browser.db" for me.
Attachment #651386 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 651386 [details] [diff] [review]
fix for ContentProviderTest tearDown

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

Makes sense.
Attachment #651386 - Flags: feedback?(lucasr.at.mozilla) → feedback+
A slight update to the previous patch; this one ensures that test failures are reported if reflection fails in a couple of additional places.
Attachment #651386 - Attachment is obsolete: true
Attachment #653754 - Flags: review?(jmaher)
Comment on attachment 653754 [details] [diff] [review]
expose reflection failures and fix reflection failure in ContentProviderTest tearDown

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

overall this is a simple patch, lets just address the 1 comment below.

::: mobile/android/base/tests/ContentProviderTest.java.in
@@ +237,4 @@
>  
> +        String defaultProfile = "default";
> +        ContentProvider targetProvider = (ContentProvider) mProviderClass.newInstance();
> +        databaseName = (String) getDatabasePath.invoke(targetProvider, defaultProfile, true /* is test */);

removing the try/catch from here seems like the wrong thing todo.
Attachment #653754 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #7)
> removing the try/catch from here seems like the wrong thing todo.

I hear you, but...the alternative is something like

try { ...
} catch (Exception e) { mAsserter.ok(false, "caught an exception", e); }

and I am reluctant to use mAsserter in tearDown (it's technically valid, but is just about to be closed in super.tearDown).

The Robotium tearDown is allowed to throw an exception, and when it does, the exception is reported and causes a test failure -- that's all we need.
ok, lets land this then.  I didn't spend enough time thinking about what else we could do and if leaving it in made sense.
https://hg.mozilla.org/mozilla-central/rev/fcf13879b587
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.