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

RESOLVED FIXED in Firefox 35

Status

()

Firefox for Android
Testing
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 35
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

4 years ago
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?
(Assignee)

Comment 4

4 years ago
(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
(Assignee)

Comment 5

4 years ago
Created attachment 8488074 [details] [diff] [review]
Part 1: Extract Android release signing bits into config/android-common.mk. r=mshal
Attachment #8488074 - Flags: review?(mshal)
(Assignee)

Comment 6

4 years ago
Created attachment 8488077 [details] [diff] [review]
Part 2: Stop release signing instrumentation test APKs at package time. r=mshal

These aren't actually being uploaded (or consumed) anywhere, so this
is non-breaking.
Attachment #8488077 - Flags: review?(mshal)
(Assignee)

Comment 7

4 years ago
Created attachment 8488079 [details] [diff] [review]
Part 3: Add release signed instrumentation test APKs to instrumentation/ in tests package. r=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)
(Assignee)

Comment 8

4 years ago
Created attachment 8488082 [details] [diff] [review]
Post: Add release signed Robocop APK to mochitest/ in tests package. r=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)
(Assignee)

Comment 9

4 years ago
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.
(Assignee)

Comment 10

4 years ago
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

Updated

4 years ago
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)
(Assignee)

Comment 16

4 years ago
(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!
(Assignee)

Comment 18

4 years ago
(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+
(Assignee)

Comment 20

4 years ago
(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.
(Assignee)

Comment 22

4 years ago
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
(Assignee)

Updated

2 months ago
Blocks: 1443246
You need to log in before you can comment on or make changes to this bug.