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.

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!

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.

Removed the "via..." line.

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+.

(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.

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.

Parts 1-4 (so far): https://tbpl.mozilla.org/?tree=Try&rev=d023f0cc2af2

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.

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.

(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.

Rebase.

Comment on attachment 8334015 [details] [diff] [review] Part 2: Remove reflection from BaseTest. Move r+.

remote: https://hg.mozilla.org/integration/fx-team/rev/3dca9763f7a3 remote: https://hg.mozilla.org/integration/fx-team/rev/bb55a9e16dcb remote: https://hg.mozilla.org/integration/fx-team/rev/275253f77f3a remote: https://hg.mozilla.org/integration/fx-team/rev/29ff9f6052b4

https://hg.mozilla.org/mozilla-central/rev/3dca9763f7a3 https://hg.mozilla.org/mozilla-central/rev/bb55a9e16dcb https://hg.mozilla.org/mozilla-central/rev/275253f77f3a https://hg.mozilla.org/mozilla-central/rev/29ff9f6052b4