905703 - Don't pre-process build/mobile/robocop/*java.in and mobile/android/base/tests/*java.in

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

At the moment, the contents of build/mobile/robocop/*java.in are preprocessed before building robocop.apk.  The only preprocessor directive is #filter substitution, and the only substitution is @ANDROID_PACKAGE_NAME@.  I'd like to get rid of this pre-processing step, to simplify the build process.

This discussion is complicated by the use of "package" to mean Android package (what is shown in the Play store, for example) and also to mean Java package (used by the system class loader).

The name of the Java package in robocop.apk does not need to be the same as the Android package under test, or of the Java package providing the code in that Android package.  See, for example, the example test project from http://code.google.com/p/robotium/wiki/Getting_Started:  the Java and Android packages under test are "com.example.android.notepad"; the robotium Android and Java packages are "com.jayway.test", and the relevant test manifest line is

<instrumentation android:targetPackage="com.example.android.notepad" android:name="android.test.InstrumentationTestRunner" />

I think we can simplify this whole build process by making the robocop Java package name match the Android package name: "org.mozilla.roboexample.test".

This will require updating some of the imports in mobile/android/base/tests, but I don't see this as a big issue.

If I were to implement this, I would move the contents of build/mobile/robocop into mobile/android/base/tests.

Comment 1

[Nick Alexander :nalexander [he/him]]

mfinkle: blassey: gbrown: jmaher: this is a scatter-shot call for comments.  Am I missing context here?  Is my understanding of robotium wrong?  I'd rather not dive into this rabbit hole if it's deep.

I should add that simplifying the robocop build process will help pave the way for a mooted sibling test harness (Bug 709353), and provide the Fennec team a clearer view into how robocop is built and operates.

Comment 2

[Geoff Brown [:gbrown]]

I believe the ANDROID_PACKAGE_NAME pre-dates my robocop involvement: It feels like it has always been there and I haven't investigated since it wasn't bothering me. I suspect that it is possible to remove our ANDROID_PACKAGE_NAME dependence; I'm not sure it will be easy.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

I was under the impressions the android security model required the robotium app to have the same name as the application under test.

If it works just fine without the application name, I fully support removing the filter substitution and .in filenames!  luckily we have try server to test this on:)

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Same as joel, if it doesn't break things, feel free to change it.

One scenario to test though, is running robocop tests on nightly and private builds on the same device.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

I can't offer any insight on why ANDROID_PACKAGE_NAME is used in the tests, but would also be happy to see less preprocessing.

Comment 7

[Brian Nicholson (:bnicholson)]

Win. https://tbpl.mozilla.org/?tree=Try&rev=b47f504fc489

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: Rename files and fix imports/packages

This patch simply does the dirty work of renaming *.java.in -> *.java, and changes package declarations/imports from org.mozilla.fennec_X -> org.mozilla.gecko.

Note to self: Check for any new *.java.in files in tests/ that may have been added before landing this.

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Additional changes (to be squashed)

This patch contains the remaining changes. I separated this from the first patch only to aid review; this should be folded with the first patch before landing.

We might want to different packages, especially for the harness classes (which are currently just using org.mozilla.gecko). We can do this in a follow-up, or I can make those changes now if you already have a package structure in mind. Same goes for moving these files somewhere else.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 820057 [details] [diff] [review]
Rename files and fix imports/packages

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

lgtm.  I would like to move this stuff into mobile/android/tests and improve the build system pieces, but that can be follow up.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 820062 [details] [diff] [review]
Additional changes (to be squashed)

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

lgtm.

Comment 12

[Brian Nicholson (:bnicholson)]

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

Comment 13

[Brian Nicholson (:bnicholson)]

Argh, forgot to check talos tests on try. Backed out: https://hg.mozilla.org/integration/fx-team/rev/1b166374191c

Comment 14

[Brian Nicholson (:bnicholson)]

NEEDINFOing Joel since he said he'd look into updating the Talos tests.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

filed bug 930188 to track this for talos.

Comment 16

[Nick Alexander :nalexander [he/him]]

http://tbpl.mozilla.org/?tree=Try&rev=d1c72e65f582

Comment 17

[Nick Alexander :nalexander [he/him]]

I am working up a new try build with the patch from Bug 930188.  If it's green, we land ASAP since this is blocking build improvements left, right, and center.

Comment 18

[Nick Alexander :nalexander [he/him]]

https://tbpl.mozilla.org/?tree=Try&rev=64d2089ac862

Comment 19

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/082c445306e8

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/082c445306e8