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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Chris Kitching [:ckitching]]

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)

Comment 1

[Chris Kitching [:ckitching]]

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.

Comment 2

[Nick Alexander :nalexander [he/him]]

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 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Adjust robocop Makefile to allow importing of org.mozilla.gecko classes.

Comment 4

[Nick Alexander :nalexander [he/him]]

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!

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Adjust robocop Makefile to allow importing of org.mozilla.gecko classes. v2

Removed the "via..." line.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 8

[Nick Alexander :nalexander [he/him]]

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

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 3: Remove reflection from DatabaseHelper.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 831997 [details] [diff] [review]
Part 3: Remove reflection from DatabaseHelper.

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

lgtm.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 4: Remove reflection from tests.

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.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 13

[Nick Alexander :nalexander [he/him]]

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.

Comment 14

[Nick Alexander :nalexander [he/him]]

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 15

[Nick Alexander :nalexander [he/him]]

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.

Comment 16

[Nick Alexander :nalexander [he/him]]

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

Comment 17

[Nick Alexander :nalexander [he/him]]

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

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Remove reflection from BaseTest.

Rebase.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Move r+.

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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