Closed Bug 1065773 Opened 10 years ago Closed 10 years ago

Bake "Android instrumentation" test APKs and harness files into tests.zip

Categories

(Firefox for Android Graveyard :: Testing, defect)

Product:
Firefox for Android Graveyard
Component:
Testing
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 35

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(4 files)

Part 1: Extract Android release signing bits into config/android-common.mk. r=mshal
3.39 KB, patch
mshal
: review+
Details | Diff | Splinter Review
Part 2: Stop release signing instrumentation test APKs at package time. r=mshal
3.33 KB, patch
mshal
: review+
Details | Diff | Splinter Review
Part 3: Add release signed instrumentation test APKs to instrumentation/ in tests package. r=mshal
5.64 KB, patch
ted
: review+
mshal
: feedback+
Details | Diff | Splinter Review
Post: Add release signed Robocop APK to mochitest/ in tests package. r=mshal
1.90 KB, patch
mshal
: review+
Details | Diff | Splinter Review
To avoid Bug 919627, and to avoid making test-APK specific changes on the buildbots, I intend to bake the instrumentation test APKs into the tests.zip produced by |make package-tests|. This entails adding testing/instrumentation, writing into _tests/instrumentation, signing as part of |make stage-package|, and zipping the new pieces into tests.zip
CC a few relengers for mobile-related heads up, and signing-related heads up. On this work.
How much does this increase the file size of tests.zip?
(In reply to Geoff Brown [:gbrown] (PTO Sept 15 - Oct 7) from comment #3) > How much does this increase the file size of tests.zip? That's a good question. About 1 meg currently, but I would expect that to grow -- rapidly -- to several megs as we develop tests, and then level off as we just add tests for new functionality. /Users/nalexander/Mozilla/gecko-dev/objdir-droid/_tests/instrumentation: total used in directory 1816 available 5104121 drwxr-xr-x 6 nalexander staff 204 Sep 10 17:28 . drwxr-xr-x 11 nalexander staff 374 Sep 10 17:25 .. -rw-r--r-- 1 nalexander staff 312784 Sep 10 17:28 background-junit3.apk -rw-r--r-- 1 nalexander staff 52592 Sep 10 17:28 browser-junit3.apk -rw-r--r-- 1 nalexander staff 554199 Sep 10 17:28 robocop.apk lrwxr-xr-x 1 nalexander staff 81 Sep 10 17:25 runinstrumentation.py -> /Users/nalexander/Mozilla/gecko-dev/testing/instrumentation/runinstrumentation.py
These aren't actually being uploaded (or consumed) anywhere, so this is non-breaking.
Attachment #8488077 - Flags: review?(mshal)
mshal: I'd like your build peer feedback on this approach for adding a new test harness. Ignore the actual contents of runinstrumentation.py; it's a placeholder only. You can see that I followed web-platform-tests |make package-test| logic, but I write to instrumentation/. By including these APKs in the tests package, we can avoid special buildbot and mozharness APK downloading logic.
Attachment #8488079 - Flags: review?(mshal)
This isn't strictly necessary, but if we start fishing these APKs from the tests package, we can phase out the buildbot and mozharness logic that handles robocop.apk specially. We can't stop signing and uploading robocop.apk yet, though, 'cuz the buildbot handling would need to be updated, etc.
Attachment #8488082 - Flags: review?(mshal)
mshal, releng peeps: this makes Android release signing happen as part of |make package-tests| as well as |make package|. I see no reason to not do this: there's no technical blocker, AFAICT, and I am not aware of any policy restriction -- but I would like to call it out.
For the record, the robocop.apk signed at |make package-tests| time has the release signature that it should have (so we're not failing to release sign at package-tests time). In fact, it's byte identical to the uploaded robocop APK. See: M Filemode Length Date Time File - ---------- -------- ----------- -------- -------------------------- -rw-rw-rw- 583 10-Sep-2014 18:01:04 meta-inf/manifest.mf -rw-rw-rw- 704 10-Sep-2014 18:01:04 meta-inf/nightly.sf -rw-rw-rw- 1090 10-Sep-2014 18:01:04 meta-inf/nightly.dsa -rw-rw-rw- 520 10-Sep-2014 20:55:38 res/layout/main.xml -rw-rw-rw- 2264 10-Sep-2014 20:55:38 AndroidManifest.xml -rw-rw-rw- 1080 10-Sep-2014 20:55:38 resources.arsc -rw-rw-rw- 7542 10-Sep-2014 20:55:38 res/drawable-hdpi/icon.png -rw-rw-rw- 3068 10-Sep-2014 20:55:38 res/drawable-ldpi/icon.png -rw-rw-rw- 4516 10-Sep-2014 20:55:38 res/drawable-mdpi/icon.png -rw-rw-rw- 32220 10-Sep-2014 20:55:40 classes.dex - ---------- -------- ----------- -------- -------------------------- 53587 10 files
Attachment #8488074 - Flags: review?(mshal) → review+
Comment on attachment 8488077 [details] [diff] [review] Part 2: Stop release signing instrumentation test APKs at package time. r=mshal >-INNER_ROBOCOP_PACKAGE=echo I think you still want this line to set the default value, since "$(INNER_ROBOCOP_PACKAGE) &&" is used in the command, and it isn't set in all if-conditions.
Attachment #8488077 - Flags: review?(mshal) → review+
Comment on attachment 8488079 [details] [diff] [review] Part 3: Add release signed instrumentation test APKs to instrumentation/ in tests package. r=mshal This looks fine to me in that it seems to work & the build logic looks reasonable (I'm assuming the boilerplate of the Makefile.in and runinstrumentation.py are unavoidable :). However, I'd like to kick review over to ted because I'm not sure if there should be concerns over adding a new test harness. >+BACKGROUND_TESTS_PATH = $(abspath $(DIST)/../mobile/android/tests/background/junit3) >+BROWSER_TESTS_PATH = $(abspath $(DIST)/../mobile/android/tests/browser/junit3) Why "$(DIST)/.." instead of just "$(DEPTH)" here?
Attachment #8488079 - Flags: review?(ted)
Attachment #8488079 - Flags: review?(mshal)
Attachment #8488079 - Flags: feedback+
Comment on attachment 8488082 [details] [diff] [review] Post: Add release signed Robocop APK to mochitest/ in tests package. r=mshal >+ROBOCOP_PATH = $(abspath $(DIST)/../build/mobile/robocop) Similar DIST vs. DEPTH here.
Attachment #8488082 - Flags: review?(mshal) → review+
bhearsum, is #c9 anything we should be concerned about?
Flags: needinfo?(bhearsum)
(In reply to Michael Shal [:mshal] from comment #14) > bhearsum, is #c9 anything we should be concerned about? Which files are getting (re)signed in package-tests? The actual APK, or the tests package? It's worse if it's the tests package, but either way I think we should try to avoid this -- each file that gets signed adds a ton of network traffic, as well as load on the signing servers.
Flags: needinfo?(bhearsum)
(In reply to Ben Hearsum [:bhearsum] from comment #15) > (In reply to Michael Shal [:mshal] from comment #14) > > bhearsum, is #c9 anything we should be concerned about? > > Which files are getting (re)signed in package-tests? The actual APK, or the > tests package? The files are all signed before being included in the test package itself. All the APKs are already being signed as part of |make package|; we're just shuffling this around. The APKs that get signed are (so far): -rw-r--r-- 1 nalexander staff 312784 Sep 10 17:28 background-junit3.apk -rw-r--r-- 1 nalexander staff 52592 Sep 10 17:28 browser-junit3.apk -rw-r--r-- 1 nalexander staff 554199 Sep 10 17:28 robocop.apk I can imagine we might grow... 2-5 more. Say tests for the Fennec Search Activity, tests for the Fennec Stumbler, and a few more projects I can't predict. > It's worse if it's the tests package, but either way I think we should try > to avoid this -- each file that gets signed adds a ton of network traffic, > as well as load on the signing servers. The files are very small. I'd be surprised if an instrumentation APK ever passes, say, 4 megs. It's just Java bytecode.
Okay. It would be good to stop signing all of the things we don't ship (ie. Robocop and Junit APKs) at some point, but that's clearly not a blocker for this bug. Carry on!
(In reply to Ben Hearsum [:bhearsum] from comment #17) > Okay. It would be good to stop signing all of the things we don't ship (ie. > Robocop and Junit APKs) at some point, but that's clearly not a blocker for > this bug. Carry on! This is not possible in the current scheme: Android instrumentation APKs must be signed with the same keys as the APK under test. At the moment, we only build (and sign, and ship) one APK. I would very much prefer to build two APKs: a release build (not debuggable, signed, shipped) and a debug build (debuggable, not signed, not shipped). Then we could do the right thing and not sign these instrumentation APKs either. Besides minimizing the Mozilla-signed code in the wild (we have way, way too much!), this has other benefits: we can Proguard more aggressively in our release builds, for example.
Comment on attachment 8488079 [details] [diff] [review] Part 3: Add release signed instrumentation test APKs to instrumentation/ in tests package. r=mshal Review of attachment 8488079 [details] [diff] [review]: ----------------------------------------------------------------- I don't have any major qualms about this, just some minor bits. ::: testing/instrumentation/Makefile.in @@ +8,5 @@ > +INSTRUMENTATION_FILES = \ > + runinstrumentation.py \ > + $(NULL) > +INSTRUMENTATION_DEST = $(_DEST_DIR) > +INSTALL_TARGETS += INSTRUMENTATION Is there a reason this needs to get copied to $objdir/_tests/instrumentation? Our existing suites do that for historical reasons, but it seems like you should be able to just run this Python file from the srcdir instead (and copy it into the test package in the stage-package target below). @@ +24,5 @@ > + > +stage-package: > + $(NSINSTALL) -D $(_DEST_DIR) > + $(call RELEASE_SIGN_ANDROID_APK,$(BACKGROUND_TESTS_PATH)/background-junit3-debug-unsigned-unaligned.apk,$(_DEST_DIR)/background-junit3.apk) > + $(call RELEASE_SIGN_ANDROID_APK,$(BROWSER_TESTS_PATH)/browser-junit3-debug-unsigned-unaligned.apk,$(_DEST_DIR)/browser-junit3.apk) We'll need to figure out how to handle this properly when we move this stuff to moz.build. ::: testing/instrumentation/runinstrumentation.py @@ +10,5 @@ > + > +if __name__ == "__main__": > + success = wptrunner.main() > + if not success: > + sys.exit(1) You could just sys.exit(not success)
Attachment #8488079 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #19) > Comment on attachment 8488079 [details] [diff] [review] > Part 3: Add release signed instrumentation test APKs to instrumentation/ in > tests package. r=mshal > > Review of attachment 8488079 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't have any major qualms about this, just some minor bits. > > ::: testing/instrumentation/Makefile.in > @@ +8,5 @@ > > +INSTRUMENTATION_FILES = \ > > + runinstrumentation.py \ > > + $(NULL) > > +INSTRUMENTATION_DEST = $(_DEST_DIR) > > +INSTALL_TARGETS += INSTRUMENTATION > > Is there a reason this needs to get copied to > $objdir/_tests/instrumentation? Our existing suites do that for historical > reasons, but it seems like you should be able to just run this Python file > from the srcdir instead (and copy it into the test package in the > stage-package target below). No, there's no real reason it needs to get into _tests/instrumentation. In fact, I'm going to land with the equivalent, but using the new moz.build TEST_HARNESS_FILES (which adds an INSTALL_TARGET, so gets processed by libs:: just the same). I think doing that is more future-proof than adding a one-off copy in stage-package.
Woops, forgot to bump the r=mshal to r=ted for Part 3. I think with f+ and r+ from two peers, we're okay.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Blocks: 1443246
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: