Closed Bug 916507 Opened 6 years ago Closed 6 years ago

Make Robocop testing no longer rely on Reflection on our own code.

Categories

(Firefox for Android :: Testing, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: ckitching, Assigned: mcomella)

References

Details

Attachments

(4 files, 2 obsolete files)

Robocop should be used, to the greatest possible extent, for UI blackbox testing - unit testing is better done with JUnit.
Currently, a number of Robocop's tests aren't really UI tests at all, but do unit testing via the Reflection APIs on our classes.
Other Robocop tests are UI tests, but use Reflection to check the results (Which might instead be possible to do by using Reflection on Android classes only to explore the view tree, instead of reflecting upon our own classes - at some cost of complexity)

Filing this to make the dependancies of Proguard explicit. (And in the vague hope someone will address it before I get to it)
To clarify - using Reflection APIs on classes we are writing prevents Proguard from being able to optimise them. This firstly makes making Proguard not break such a test extremely difficult, but also means that the performance gain from Proguard is inversely proportional to our test coverage. Eek.

Reflection on Android API classes is of course fine (And, indeed, necessary to do testing) - since these are untouchable by Proguard.
OS: Linux → Android
Hardware: x86_64 → ARM
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Our long term plan for this probably moves in two pieces.  For Robocop, we want to add the Fennec libraries to the bootclasspath (see mobile/android/tests/background/junit3/Makefile.in) and then remove explicit reflection where possible.  This hits this ticket.

But ckitching's thrust is correct: unit testing is better done with JUnit.  That's Bug 903528.
Comment on attachment 831834 [details] [diff] [review]
Part 1: Adjust robocop Makefile to allow importing of org.mozilla.gecko classes.

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

r=nalexander, but toss a try build up after the meat.

::: build/mobile/robocop/Makefile.in
@@ +84,5 @@
>  tools:: $(ANDROID_APK_NAME).apk
>  
>  GENERATED_DIRS += $(dir-tests)
> +
> +# via mobile/android/tests/background/junit3/Makefile.in:

Kill this -- it's going to diverge fast.  Or just say, "Like the background JUnit 3 tests, the test APK ..."

@@ +89,5 @@
> +# The test APK needs to know the contents of the target APK while not
> +# being linked against them.  This is a best effort to avoid getting
> +# out of sync with base's build config.
> +JARS_DIR := $(DEPTH)/mobile/android/base
> +JAVA_BOOTCLASSPATH := $(JAVA_BOOTCLASSPATH):$(subst $(NULL) ,:,$(wildcard $(JARS_DIR)/*.jar))

I will be so happy when we can extract this from Fennec's moz.build.  Soon!
Attachment #831834 - Flags: review?(nalexander) → review+
Attached patch Remove reflection from BaseTest. (obsolete) — Splinter Review
There are still a few reflection uses I'm not sure how to remove
(mLauncherActivityClass and mAsserter.setTestName), or if we even can/should
respectively. Please comment on this.
Attachment #831842 - Flags: review?(nalexander)
Attachment #831834 - Attachment is obsolete: true
Comment on attachment 831845 [details] [diff] [review]
Part 1: Adjust robocop Makefile to allow importing of org.mozilla.gecko classes. v2

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

Move r+.
Attachment #831845 - Flags: review+
(In reply to Michael Comella (:mcomella) from comment #5)
> Created attachment 831842 [details] [diff] [review]
> Remove reflection from BaseTest.
> 
> There are still a few reflection uses I'm not sure how to remove
> (mLauncherActivityClass and mAsserter.setTestName), or if we even can/should
> respectively. Please comment on this.

So, this is irritating.  You can just fish the mLauncherActivityClass directly out of org.mozilla.fennec_*, expect we need to preprocess to actually get the damn class name.  Very funny.

So, at or around http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/TestConstants.java.in#10 add something like

public static final Class<Activity> LAUNCHER_ACTIVITY_CLASS = @ANDROID_PACKAGE_NAME@.App.class;

You'll probably need to 'import @ANDROID_PACKAGE_NAME@.App;' first.
Comment on attachment 831997 [details] [diff] [review]
Part 3: Remove reflection from DatabaseHelper.

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

lgtm.
Attachment #831997 - Flags: review?(nalexander) → review+
Final part. In followup bugs:
  * testSearchSuggestions uses reflection to access classes with lower access
rights (e.g. package private)
  * ContentProviderTest requires a bit of restructuring through the setUp
method and inheriting classes.

MotionEventReplayer also uses reflection, but to access built-in, but prviate
Android APIs.
Attachment #832019 - Flags: review?(nalexander)
Comment on attachment 832019 [details] [diff] [review]
Part 4: Remove reflection from tests.

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

fly by again, but nothing looks bad.  Some of these guys are good candidates for JUnit 3 tests (e.g., testJarReader.java).  Wouldn't hurt to say as much, either as TODO or linked in that other ticket.
Attachment #832019 - Flags: review?(nalexander) → review+
Here's a try with a small patch to get around the Class<> business.  Instead of launching App, launch BrowserApp.  Let's see how that works.

http://tbpl.mozilla.org/?tree=Try&rev=ccd556ba9b36
Comment on attachment 831842 [details] [diff] [review]
Remove reflection from BaseTest.

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

These are fine, but I think this patch needs an iteration to before we land so I'm going to f+ with the expectation of a rubber stamp r+ tomorrow.
Attachment #831842 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #14)
> Here's a try with a small patch to get around the Class<> business.  Instead
> of launching App, launch BrowserApp.  Let's see how that works.
> 
> http://tbpl.mozilla.org/?tree=Try&rev=ccd556ba9b36

Sigh.  There are other ways to do this that require more preprocessing.  Or we could register the intents for BrowserApp, like bnicholson and I discussed in a bug I can't find.
(In reply to Michael Comella (:mcomella) from comment #12)
> Parts 1-4 (so far): https://tbpl.mozilla.org/?tree=Try&rev=d023f0cc2af2

So, I'm kind of surprised this worked.  I thought build/mobile/robocop would be built before mobile/android/base, meaning that Robocop wouldn't find the Fennec JARs.  (And we know the JARs aren't old, since try always clobbers.)  That's interesting.
Component: General → Testing
Attachment #831842 - Attachment is obsolete: true
Comment on attachment 8334015 [details] [diff] [review]
Part 2: Remove reflection from BaseTest.

Move r+.
Attachment #8334015 - Flags: review+
You need to log in before you can comment on or make changes to this bug.