Closed Bug 1473313 Opened 6 years ago Closed 6 years ago

Add code coverage for geckoview-junit emulator unit tests

Categories

(Testing :: Code Coverage, enhancement)

Product:
Testing
Component:
Code Coverage
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: gabriel-v, Assigned: gabriel-v)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

Bug 1473313 - Part 1: Set up geckoview build config for androidTest coverage runs.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1473313 - Part 2: Support running emulator junit tests with code coverage support.
59 bytes, text/x-review-board-request
gbrown
: review+
Details
Bug 1473313 - Part 3: Extend CodeCoverageMixin to handle java code coverage tools.
59 bytes, text/x-review-board-request
gbrown
: review+
marco
: review+
Details
Bug 1473313 - Part 4: Add build and test platforms for android emulator code coverage.
59 bytes, text/x-review-board-request
gbrown
: review+
Details
Bug 1473313 - Part 5: Add in-tree documentation page for Android code coverage.
59 bytes, text/x-review-board-request
nalexander
: review+
Details 
      No description provided.
First idea: use gradle's built-in code coverage plugin
------------------------------------------------------

android {
    buildTypes {
        debug {
            testCoverageEnabled true
        }
    }
}

This should be the simplest solution, as gradle will handle getting the *.ec files out of the emulator and into the build directory.

This results in the method count exceeding 64k. This can be fixed by changing the application type to MultiDex, but enabling it is not trivial.


Second idea: use "-e emma true" with "am instrument"
--------------------------------------------------

See https://developer.android.com/studio/test/command-line (ctrl-f for "code coverage").
>This option requires an EMMA-instrumented build of the test application, which you can generate with the coverage target.

I had no success in finding and triggering that coverage target.

JaCoCo can parse *.ec files without any problem, so the report generation could remain unchanged.


Third idea: use JaCoCo with offline instrumentation
---------------------------------------------------

As hinted by the JaCoCo documentation, this is the way to get JaCoCo running on the Android Dalvik vm. See https://www.eclemma.org/jacoco/trunk/doc/offline.html for details.

The JaCoCo Gradle plugin does not support offline instrumentation: https://github.com/gradle/gradle/issues/2429. But there is a workaround that wraps the JaCoCo ant plugin that does support offline instrumentation.
First idea update: gradle's built-in code coverage plugin
---------------------------------------------------------

MultiDex is simple to set up, but then I ran into this issue: https://issuetracker.google.com/issues/37116868 (JaCoCo version conflicts with our specific gradle plugin version). This can be solved by forcing the use of another version of JaCoCo with gradle's internal code coverage plugin, as described in the link above.
 


Third idea update: instrument the classes ourselves
---------------------------------------------------

I managed to instrument the classes for the "app" and "geckoview" projects by using the JaCoCo ant plugin. Somehow, the resulting classes do not exceed the 64k limit, but come pretty close to doing so.

I didn't manage to find the .exec files in the advertised locations. I'll try to configure a non-default location and see if they show up. Another possible issue is the fact that I didn't let the tests finish, so maybe hitting ctrl-c on the running tests doesn't do a clean shutdown of the app (and it never gets to write the files).
I've managed to get gradle's built-in code coverage plugin (which is actually JaCoCo) to work with our setup. We don't need MultiDex, I was doing something stupid (no need to touch app when we're testing geckoview).

This means we can run the geckoview tests with 

    ./mach gradle geckoview:connectedOfficialWithGeckoBinariesNoMinApiDebugAndroidTest

and then create the 'coverage.ec' file with:

    ./mach gradle geckoview:createOfficialWithGeckoBinariesNoMinApiDebugCoverageReport

The coverage file will show up at:

    gradle/build/mobile/android/geckoview/outputs/code-coverage/connected/flavors/OFFICIALWITHGECKOBINARIESNOMINAPI/mozemulator-7.0(AVD) - 7.0-coverage.ec


For the WithoutGeckoBinaries flavors, running the tests in this way does not work, as some .so file is not found.
Gradle error output for this: https://pastebin.mozilla.org/9089448.
Relevant logcat section: https://pastebin.mozilla.org/9089447.
Because https://issuetracker.google.com/issues/37116868 affects the Gradle plugin version we are using, I needed to override the JaCoCo library version. I found a workaround in the above issue and it works locally, but busts the gradle-dependencies task.

The workaround involves adding 'classpath' entries to the top level build.gradle's buildscript block, then using resolutionStrategy.force to override the JaCoCo package versions. This affects all subprojects and configurations, not just the :geckoview project in the Debug configurations.

Here is the patch with the workaround: https://pastebin.mozilla.org/9089539. See changes to build.gradle.
Here is how it's failing: https://taskcluster-artifacts.net/M5sWhUPHRdqKLXx08LaOXQ/0/public/logs/live_backing.log.

In the above log, proguard warns about missing classes (from java.lang.instrument, java.lang.management, javax.management) referenced from JaCoCo classes. I'll try adding -keep arguments to proguard-rules.txt for those classes, but this doesn't feel like the right solution (as they'll be kept in Release builds too). Judging from the warnings and the failing task name, I'm sure this is what's causing the build failure.

Is it possible to implement the above workaround only for the :geckoview target in the Debug configuration?
Flags: needinfo?(nalexander)
I tried adding JaCoCo's dependencies to geckoview's proguard-rules.txt: https://hg.mozilla.org/try/rev/22e37d38b3fa726351535182fc2dd5af434b4696. The result looks exactly the same: warnings are still there and the build fails in the same way.
> For the WithoutGeckoBinaries flavors, running the tests in this way does not
> work, as some .so file is not found.
> Gradle error output for this: https://pastebin.mozilla.org/9089448.
> Relevant logcat section: https://pastebin.mozilla.org/9089447.

This is what "withoutGeckoBinaries" means -- there are no Gecko binaries (libxul.so, etc) in the APK.  For actually doing things, you always want "withGeckoBinaries".  (There is an option because the Gecko binaries are produced by the build process rather late, so we have to stage things.  Yes, this is very confusing.)

(In reply to Tudor-Gabriel Vijiala [:tvijiala] from comment #4)
> Because https://issuetracker.google.com/issues/37116868 affects the Gradle
> plugin version we are using, I needed to override the JaCoCo library
> version. I found a workaround in the above issue and it works locally, but
> busts the gradle-dependencies task.
> 
> The workaround involves adding 'classpath' entries to the top level
> build.gradle's buildscript block, then using resolutionStrategy.force to
> override the JaCoCo package versions. This affects all subprojects and
> configurations, not just the :geckoview project in the Debug configurations.
> 
> Here is the patch with the workaround: https://pastebin.mozilla.org/9089539.
> See changes to build.gradle.
> Here is how it's failing:
> https://taskcluster-artifacts.net/M5sWhUPHRdqKLXx08LaOXQ/0/public/logs/
> live_backing.log.
> 
> In the above log, proguard warns about missing classes (from
> java.lang.instrument, java.lang.management, javax.management) referenced
> from JaCoCo classes. I'll try adding -keep arguments to proguard-rules.txt
> for those classes, but this doesn't feel like the right solution (as they'll
> be kept in Release builds too). Judging from the warnings and the failing
> task name, I'm sure this is what's causing the build failure.
> 
> Is it possible to implement the above workaround only for the :geckoview
> target in the Debug configuration?

This is quite complicated.  Fundamentally, our dependency fetching task (`android-gradle-dependencies`) is global, and you're trying to make it local (per-task).  Your solution is to push the JaCoCo dependencies into GeckoView in every configuration, which means they get picked up by Fennec (the `app` Gradle project), which for historical reasons is _both_ a debug build _and_ has ProGuard run against it.  (Yes, that's 100% bananas.)  JaCoCo and PG don't mix; hence, sadness.

I see two issues.  The first is fetching the dependencies.  That can either be as you have it (i.e., always enabled); or we can investigate a new, special Gradle configuration that extends a GeckoView androidTest configuration, like at https://docs.gradle.org/current/userguide/managing_dependency_configurations.html, and then make some dummy task that can be a Gradle dependency task and ensures JaCoCo deps get download in `android-gradle-dependencies`.  The former is easier but slows down all builds, sometimes a lot (see, e.g., https://jeroenmols.com/blog/2016/09/01/coveragecost/), and presumably has _some_ runtime cost.

The second is that we do some annotation processing, which isn't playing nicely with JaCoCo.  I tried to make the former work by changing your `api` line to `androidTestApi` -- but now I think you really want the API in the library code and not just the test code.  Google, in their infinite wisdom, made this depend on `buildType` and not something flexible, which makes it bloody difficult to make this work conditionally.  Try adding a build flag (like `-Pcoverage`) to conditionally include the dependencies, and then see if you can a) keep |mach build| running without that flag and b) get |./mach gradle -Pcoverage geckoview:connected...| to work with that flag.

I don't have a lot of bandwidth to help with this, sorry.  Good luck!
Flags: needinfo?(nalexander)
(In reply to Nick Alexander :nalexander from comment #6)
> This is what "withoutGeckoBinaries" means -- there are no Gecko binaries
> (libxul.so, etc) in the APK.  For actually doing things, you always want
> "withGeckoBinaries".  (There is an option because the Gecko binaries are
> produced by the build process rather late, so we have to stage things.  Yes,
> this is very confusing.)

This is what I suspected, I just wanted to make sure I'm not missing anything obvious related to this. Thanks for the clarification.

> This is quite complicated.  Fundamentally, our dependency fetching task
> (`android-gradle-dependencies`) is global, and you're trying to make it
> local (per-task).  Your solution is to push the JaCoCo dependencies into
> GeckoView in every configuration, which means they get picked up by Fennec
> (the `app` Gradle project), which for historical reasons is _both_ a debug
> build _and_ has ProGuard run against it.  (Yes, that's 100% bananas.) 

> JaCoCo and PG don't mix; hence, sadness.

Locally I got './mach gradle geckoview:create...CoverageReport' (which runs './mach gradle geckoview:connected...AndroidTest' and then gets the files with 'adb pull') to work without touching the ProGuard configuration. I still have to check if the coverage reports are any good. Either way, I'm sure Proguard can be configured to work nicely with JaCoCo (like this: https://android.googlesource.com/platform/build/+/master/core/proguard.jacoco.flags).

> I see two issues.  The first is fetching the dependencies.  That can either
> be as you have it (i.e., always enabled); or we can investigate a new,
> special Gradle configuration that extends a GeckoView androidTest
> configuration, like at
> https://docs.gradle.org/current/userguide/managing_dependency_configurations.
> html, and then make some dummy task that can be a Gradle dependency task and
> ensures JaCoCo deps get download in `android-gradle-dependencies`.  The
> former is easier but slows down all builds, sometimes a lot (see, e.g.,
> https://jeroenmols.com/blog/2016/09/01/coveragecost/), and presumably has
> _some_ runtime cost.

I'll look into extending the androidTest configuration. The JaCoCo dependencies are being downloaded anyway because of https://bugzilla.mozilla.org/show_bug.cgi?id=1471660, so what I'd need is to prepend them to the classpath and use 'resolutionStrategy.force' to select their versions, only when some -Pcoverage argument is present. 

> The second is that we do some annotation processing, which isn't playing
> nicely with JaCoCo.  I tried to make the former work by changing your `api`
> line to `androidTestApi` -- but now I think you really want the API in the
> library code and not just the test code.  Google, in their infinite wisdom,
> made this depend on `buildType` and not something flexible, which makes it
> bloody difficult to make this work conditionally.  Try adding a build flag
> (like `-Pcoverage`) to conditionally include the dependencies, and then see
> if you can a) keep |mach build| running without that flag and b) get |./mach
> gradle -Pcoverage geckoview:connected...| to work with that flag.

The annotation processing didn't conflict with JaCoCo while running the tests locally, but I'll watch out for any weird stuff related to this.

Yes, we need the JaCoCo API available in the instrumented library code. We want coverage data for the library, coverage for the test code is irrelevant (and always 100%).

> I don't have a lot of bandwidth to help with this, sorry.  Good luck!

Thank you for your guidance.
(In reply to Tudor-Gabriel Vijiala [:tvijiala] from comment #7)
> (In reply to Nick Alexander :nalexander from comment #6)
> > This is what "withoutGeckoBinaries" means -- there are no Gecko binaries
> > (libxul.so, etc) in the APK.  For actually doing things, you always want
> > "withGeckoBinaries".  (There is an option because the Gecko binaries are
> > produced by the build process rather late, so we have to stage things.  Yes,
> > this is very confusing.)
> 
> This is what I suspected, I just wanted to make sure I'm not missing
> anything obvious related to this. Thanks for the clarification.

Very good.

> > This is quite complicated.  Fundamentally, our dependency fetching task
> > (`android-gradle-dependencies`) is global, and you're trying to make it
> > local (per-task).  Your solution is to push the JaCoCo dependencies into
> > GeckoView in every configuration, which means they get picked up by Fennec
> > (the `app` Gradle project), which for historical reasons is _both_ a debug
> > build _and_ has ProGuard run against it.  (Yes, that's 100% bananas.) 
> 
> > JaCoCo and PG don't mix; hence, sadness.
> 
> Locally I got './mach gradle geckoview:create...CoverageReport' (which runs
> './mach gradle geckoview:connected...AndroidTest' and then gets the files
> with 'adb pull') to work without touching the ProGuard configuration. I
> still have to check if the coverage reports are any good. Either way, I'm
> sure Proguard can be configured to work nicely with JaCoCo (like this:
> https://android.googlesource.com/platform/build/+/master/core/proguard.
> jacoco.flags).

Yes, this makes sense: that is, with `geckoview { dependencies { api 'jacoco' }}`, you get GeckoView instrumented, regardless of where it's consumed.

> > I see two issues.  The first is fetching the dependencies.  That can either
> > be as you have it (i.e., always enabled); or we can investigate a new,
> > special Gradle configuration that extends a GeckoView androidTest
> > configuration, like at
> > https://docs.gradle.org/current/userguide/managing_dependency_configurations.
> > html, and then make some dummy task that can be a Gradle dependency task and
> > ensures JaCoCo deps get download in `android-gradle-dependencies`.  The
> > former is easier but slows down all builds, sometimes a lot (see, e.g.,
> > https://jeroenmols.com/blog/2016/09/01/coveragecost/), and presumably has
> > _some_ runtime cost.
> 
> I'll look into extending the androidTest configuration. The JaCoCo
> dependencies are being downloaded anyway because of
> https://bugzilla.mozilla.org/show_bug.cgi?id=1471660, so what I'd need is to
> prepend them to the classpath and use 'resolutionStrategy.force' to select
> their versions, only when some -Pcoverage argument is present. 
> 
> > The second is that we do some annotation processing, which isn't playing
> > nicely with JaCoCo.  I tried to make the former work by changing your `api`
> > line to `androidTestApi` -- but now I think you really want the API in the
> > library code and not just the test code.  Google, in their infinite wisdom,
> > made this depend on `buildType` and not something flexible, which makes it
> > bloody difficult to make this work conditionally.  Try adding a build flag
> > (like `-Pcoverage`) to conditionally include the dependencies, and then see
> > if you can a) keep |mach build| running without that flag and b) get |./mach
> > gradle -Pcoverage geckoview:connected...| to work with that flag.
> 
> The annotation processing didn't conflict with JaCoCo while running the
> tests locally, but I'll watch out for any weird stuff related to this.

Mmm, right, I think I crossed a wire.  I was finding that if I tried to restrict the `api` to `androidTestApi` then our special annotation processing was failing.  But that's not relevant here.

> Yes, we need the JaCoCo API available in the instrumented library code. We
> want coverage data for the library, coverage for the test code is irrelevant
> (and always 100%).
> 
> > I don't have a lot of bandwidth to help with this, sorry.  Good luck!
> 
> Thank you for your guidance.

yw.  I see that we're using 3.0.1 of the Android-Gradle plugin, but that 3.1.0 has been released (from http://google.github.io/android-gradle-dsl/current/ today).  It's probably easy to upgrade.  Would that address your issues?   If yes, try it locally and push a try build?
> Yes, this makes sense: that is, with `geckoview { dependencies { api
> 'jacoco' }}`, you get GeckoView instrumented, regardless of where it's
> consumed.

No, that line just adds JaCoCo as a dependency, it does not instrument anything. What instruments the classes in this case is '
android { buildTypes { debug { testCoverageEnabled true } } }', which adds a 'geckoview:transformClassesWithJacoco...' task at the right place.


I finally got the emulator code coverage working on try. My initial patch (the one with ProGuard warnings) was very close. There is this task I have no idea where it comes from: ':app:transformClassesAndResourcesWithProguardForOfficialWithoutGeckoBinariesNoMinApiPhotonDebug'. This task fails the build if there are ProGuard warnings, so removing those warnings resulted in a green build. As the warnings were about JaCoCo's dependencies missing from the 'app' build, we can safely ignore them, as we're not using JaCoCo on that build.

> yw.  I see that we're using 3.0.1 of the Android-Gradle plugin, but that
> 3.1.0 has been released (from
> http://google.github.io/android-gradle-dsl/current/ today).  It's probably
> easy to upgrade.  Would that address your issues?   If yes, try it locally
> and push a try build?

The android-gradle 3.1.0 plugin needs Gradle 4.4+, but we're using 4.1. After changing both of those versions, building and running works locally, but the gradle-dependencies task fails with 

    com.android.builder.dexing.DexArchiveBuilderException: Failed to process /builds/worker/workspace/build/src/mobile/android/gradle/dotgradle-online/caches/modules-2/files-2.1/org.ow2.asm/asm/6.0/bc6fa6b19424bb9592fe43bbc20178f92d403105/asm-6.0.jar

which probably means I have to update that dependency's version too.


Upgrading the plugin is the right way to do this, because it'll allow for 'andoid { jacoco { toolVersion xxx } }' to work as expected, which means I won't have to add JaCoCo as a dependency to ':app' too.
Depends on: 1476165
(In reply to Tudor-Gabriel Vijiala [:tvijiala] from comment #9)
> > Yes, this makes sense: that is, with `geckoview { dependencies { api
> > 'jacoco' }}`, you get GeckoView instrumented, regardless of where it's
> > consumed.
> 
> No, that line just adds JaCoCo as a dependency, it does not instrument
> anything. What instruments the classes in this case is '
> android { buildTypes { debug { testCoverageEnabled true } } }', which adds a
> 'geckoview:transformClassesWithJacoco...' task at the right place.

Thanks for clarifying.

> I finally got the emulator code coverage working on try. My initial patch
> (the one with ProGuard warnings) was very close. There is this task I have
> no idea where it comes from:
> ':app:
> transformClassesAndResourcesWithProguardForOfficialWithoutGeckoBinariesNoMinA
> piPhotonDebug'. This task fails the build if there are ProGuard warnings, so
> removing those warnings resulted in a green build. As the warnings were
> about JaCoCo's dependencies missing from the 'app' build, we can safely
> ignore them, as we're not using JaCoCo on that build.

I concur.  I'm pretty worried that we're working around JaCoCo in *non* code-coverage builds, though.

> > yw.  I see that we're using 3.0.1 of the Android-Gradle plugin, but that
> > 3.1.0 has been released (from
> > http://google.github.io/android-gradle-dsl/current/ today).  It's probably
> > easy to upgrade.  Would that address your issues?   If yes, try it locally
> > and push a try build?
> 
> The android-gradle 3.1.0 plugin needs Gradle 4.4+, but we're using 4.1.
> After changing both of those versions, building and running works locally,
> but the gradle-dependencies task fails with 
> 
>     com.android.builder.dexing.DexArchiveBuilderException: Failed to process
> /builds/worker/workspace/build/src/mobile/android/gradle/dotgradle-online/
> caches/modules-2/files-2.1/org.ow2.asm/asm/6.0/
> bc6fa6b19424bb9592fe43bbc20178f92d403105/asm-6.0.jar
> 
> which probably means I have to update that dependency's version too.
> 
> 
> Upgrading the plugin is the right way to do this, because it'll allow for
> 'andoid { jacoco { toolVersion xxx } }' to work as expected, which means I
> won't have to add JaCoCo as a dependency to ':app' too.

Link to your investigations and a try build showing the unexpected behaviour if you want me to look at it.
Comment on attachment 8994468 [details]
Bug 1473313 - Part 1: Set up geckoview build config for androidTest coverage runs.

https://reviewboard.mozilla.org/r/259024/#review266044


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: build/moz.configure/java.configure:72
(Diff revision 1)
>      except subprocess.CalledProcessError as e:
>          die('Failed to get javac version: %s', e.output)
> +
> +# Java Code Coverage
> +# ========================================================
> +option('--enable-java-coverage', env='MOZ_JAVA_CODE_COVERAGE', help='Enable Java code coverage')

Error: Expected 2 blank lines after class or function definition, found 1 [flake8: E305]
Try push with all patches applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a739fdf74fe61a7091369b0af963d5e3e8616909

I'll run one try push per each patch and report the results.
Comment on attachment 8994470 [details]
Bug 1473313 - Part 3: Extend CodeCoverageMixin to handle java code coverage tools.

https://reviewboard.mozilla.org/r/259028/#review266126

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:495
(Diff revision 2)
> +        if not self.java_code_coverage_enabled:
> +            return
> +
> +        dirs = self.query_abs_dirs()
> +        xml_path = tempfile.mkdtemp()
> +        jacoco_command = ['java', '-jar', self.jacoco_jar, 'report',

We have `JAVA` defined in the build configuration, and we also have to do some things to set the file encoding globally -- see https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/mobile/android/mach_commands.py#537-573.

Can you not do this with a Gradle command?  That's the most robust approach.
Comment on attachment 8994468 [details]
Bug 1473313 - Part 1: Set up geckoview build config for androidTest coverage runs.

https://reviewboard.mozilla.org/r/259024/#review266124

This is looking very close.  r- just so that I get to see you addressing the product flavors issue.  If it's too hard, I'd accept you having only one Gradle task but fishing some parameter from the mozconfig (set in `gradle.configure`) that tells you which product flavor/path to get the files from.

::: mobile/android/geckoview/build.gradle:391
(Diff revision 2)
>      dependsOn project(':annotations').jar
>  }
>  
>  apply from: "${topsrcdir}/mobile/android/gradle/jacoco_dependencies.gradle"
> -if (project.hasProperty('enable_code_coverage')) {
> +// TODO: also use substs.MOZ_JAVA_CODE_COVERAGE for A(test-ccov), instead of -Penable_code_coverage
> +if (project.hasProperty('enable_code_coverage') || mozconfig.substs.MOZ_JAVA_CODE_COVERAGE) {

It looks like you changed horses mid stream -- I don't see you setting `-Penable_code_coverage` any more.

::: mobile/android/geckoview/build.gradle:402
(Diff revision 2)
> +        reports {
> +            xml.enabled = true
> +            html.enabled = false
> +            csv.enabled = false
> +        }
> +        def fileFilter = ['**/androidTest/**', '**/test/**', '**/R.class', '**/R$*.class',

nit: newlines for each entry, please.

::: mobile/android/geckoview/build.gradle:404
(Diff revision 2)
> +            html.enabled = false
> +            csv.enabled = false
> +        }
> +        def fileFilter = ['**/androidTest/**', '**/test/**', '**/R.class', '**/R$*.class',
> +                '**/BuildConfig.*', '**/Manifest*.*', '**/*Test*.*', 'android/**/*.*']
> +        def debugTree = fileTree(dir: "${buildDir}/intermediates/classes/officialWithGeckoBinariesNoMinApi/debug/", excludes: fileFilter)

Encoding the build paths like this (`officialWithGeckoBinariesNoMinApi`) isn't robust as these things evolve.

The best way to do this is to:
- make one JaCoCo task per product flavor (like at https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/mobile/android/gradle/with_gecko_binaries.gradle#48-62)
- use the product flavor to configure paths like this
- push the choice of which flavor to actually invoke into `mobile/android/gradle.configure` (like all the other, similar, choices)
- make the actual invocation a specific |mach android| command in `mobile/android/mach_commands.py`
Attachment #8994468 - Flags: review?(nalexander) → review-
Comment on attachment 8994469 [details]
Bug 1473313 - Part 2: Support running emulator junit tests with code coverage support.

https://reviewboard.mozilla.org/r/259026/#review266400

No serious concerns here, but let's run through another review with these minor issues addressed.

::: testing/mochitest/runjunit.py:54
(Diff revision 2)
>                                   verbose=verbose)
>          self.options = options
>          self.log.debug("options=%s" % vars(options))
>          update_mozinfo()
>          self.remote_profile = posixpath.join(self.device.test_root, 'junit-profile')
> +        self.coverage_output_path = posixpath.join(self.device.test_root,

nit: I'd prefer to see 'coverage_output_path' named 'remote_coverage_output_path' or similar.

::: testing/mochitest/runjunit.py:55
(Diff revision 2)
>          self.options = options
>          self.log.debug("options=%s" % vars(options))
>          update_mozinfo()
>          self.remote_profile = posixpath.join(self.device.test_root, 'junit-profile')
> +        self.coverage_output_path = posixpath.join(self.device.test_root,
> +                                                   'geckoview-junit-coverage.ec')

nit: For consistency with 'junit-profile', and to keep the path shorter and simpler, can we use 'junit-coverage.ec' instead, or even just 'junit-coverage'?

::: testing/mochitest/runjunit.py:271
(Diff revision 2)
>              self.log.suite_end()
>  
>          if self.check_for_crashes():
>              self.fail_count = 1
>  
> +        if self.options.coverage:

What if a caller has set options.coverage but did not specify options.coverage_output_path?
Attachment #8994469 - Flags: review?(gbrown) → review-
Comment on attachment 8994470 [details]
Bug 1473313 - Part 3: Extend CodeCoverageMixin to handle java code coverage tools.

https://reviewboard.mozilla.org/r/259028/#review266414

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:220
(Diff revision 2)
>                  if suite not in self.suites:
>                      self.suites[suite] = []
>                  if test not in self.suites[suite]:
>                      self.suites[suite].append(test)
>  
> +    @PostScriptAction('download-and-extract')

What determines the order of execution when there are 2 PostScriptActions for the same step? Are you confident this is safe? (If not, consider having one PostScriptAction that calls 2 functions.)

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:245
(Diff revision 2)
> +        self.java_coverage_output_path = os.path.join(tempfile.mkdtemp(),
> +                                                      'geckoview-junit-coverage.ec')
> +
> +        # TODO: set up jacoco as a fetch task from the android build
> +        self.jacoco_jar = os.path.join(tempfile.mkdtemp(), 'org.jacoco.cli-0.8.1-nodeps.jar')
> +        self.download_file('http://repo1.maven.org/maven2/org/jacoco/org.jacoco.cli/0.8.1/org.jacoco.cli-0.8.1-nodeps.jar', self.jacoco_jar)  # NOQA: E501

I'd really prefer not to add a dependency on an external resource like this -- it never ends well. Is there an ETA for the TODO?
Attachment #8994470 - Flags: review?(gbrown)
Comment on attachment 8994471 [details]
Bug 1473313 - Part 4: Add build and test platforms for android emulator code coverage.

https://reviewboard.mozilla.org/r/259022/#review266406

::: taskcluster/ci/test/misc.yml:36
(Diff revision 2)
>      e10s: true
>      target: geckoview-androidTest.apk
>      max-run-time: 3600
>      chunks:
>          by-test-platform:
> +            android-em-4.3-arm7-api-16-ccov/debug: 8

The comment 20 try push shows one chunk running in 16 minutes. With chunks=4, would tests run in about 30 minutes? If so, I'd recommend 4 chunks.

::: taskcluster/ci/test/test-platforms.yml:319
(Diff revision 2)
>      build-platform: android-api-16/debug
>      test-sets:
>          - android-common-tests
>          - android-gradle-tests
>  
> +# Treeherder platform names should not use dots '.' in their name. The other

To make this work 'properly', keep this entry just as it is (but let's remove this comment), and add an entry to treeherder, around 

https://github.com/mozilla/treeherder/blob/master/ui/js/constants.js#L87

(Use a separate bug in Tree Management::Treeherder).

Optionally (I don't think you need to), add another translation entry in tests.py.
Attachment #8994471 - Flags: review?(gbrown) → review-
Comment on attachment 8994470 [details]
Bug 1473313 - Part 3: Extend CodeCoverageMixin to handle java code coverage tools.

https://reviewboard.mozilla.org/r/259028/#review266430

Just a couple of issues for the code coverage part.

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:48
(Diff revision 2)
>       {"action": "store_true",
>        "dest": "jsd_code_coverage",
>        "default": False,
>        "help": "Whether JSDebugger code coverage should be run."
>        }],
> +    [["--java-code-coverage"],

At some point I'd like to merge this option with the "--code-coverage" option, could you file a follow-up bug?

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:140
(Diff revision 2)
>  
>          # Download the chrome-map.json file from the build machine.
>          self.download_file(self.url_to_chrome_map, parent_dir=self.grcov_dir)
>  
> +    def _download_artifacts(self, destination):
> +        # TODO: use the fetch-content script to download artifacts.

You'll need to rebase this after https://bugzilla.mozilla.org/show_bug.cgi?id=1468812.

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:243
(Diff revision 2)
> +
> +        # Create the directory where the emulator coverage file will be placed.
> +        self.java_coverage_output_path = os.path.join(tempfile.mkdtemp(),
> +                                                      'geckoview-junit-coverage.ec')
> +
> +        # TODO: set up jacoco as a fetch task from the android build

It would be nice to do this now since it should already be feasible, especially after https://bugzilla.mozilla.org/show_bug.cgi?id=1468812 (unless you manage to add this to the gradle file directly like Nick suggested).
Attachment #8994470 - Flags: review?(mcastelluccio) → review+
Comment on attachment 8994468 [details]
Bug 1473313 - Part 1: Set up geckoview build config for androidTest coverage runs.

https://reviewboard.mozilla.org/r/259024/#review266986

::: mobile/android/geckoview/build.gradle:391
(Diff revision 2)
>      dependsOn project(':annotations').jar
>  }
>  
>  apply from: "${topsrcdir}/mobile/android/gradle/jacoco_dependencies.gradle"
> -if (project.hasProperty('enable_code_coverage')) {
> +// TODO: also use substs.MOZ_JAVA_CODE_COVERAGE for A(test-ccov), instead of -Penable_code_coverage
> +if (project.hasProperty('enable_code_coverage') || mozconfig.substs.MOZ_JAVA_CODE_COVERAGE) {

That is true. I switched to using the '--enable-java-coverage' mozconfigure flag, as setting that will also show up when running the tests with mochitest in the geckoview-junit task.

I will separate the A(test-ccov) and geckoview-junit cases here.

::: mobile/android/geckoview/build.gradle:404
(Diff revision 2)
> +            html.enabled = false
> +            csv.enabled = false
> +        }
> +        def fileFilter = ['**/androidTest/**', '**/test/**', '**/R.class', '**/R$*.class',
> +                '**/BuildConfig.*', '**/Manifest*.*', '**/*Test*.*', 'android/**/*.*']
> +        def debugTree = fileTree(dir: "${buildDir}/intermediates/classes/officialWithGeckoBinariesNoMinApi/debug/", excludes: fileFilter)

Actually, the 'jacocoAndroidTestReport' task is unused code: we can not generate the report this way, as the geckoview-junit test task does not have a clone of gecko available for this. In contrast, A(test-ccov) is a build task, so it has a clone of gecko available.

Your feedback applies to the 'zipClassfiles' task below. I will make a 'mach android archive-classfiles' subcommand for it.
Comment on attachment 8994469 [details]
Bug 1473313 - Part 2: Support running emulator junit tests with code coverage support.

https://reviewboard.mozilla.org/r/259026/#review266400

> nit: For consistency with 'junit-profile', and to keep the path shorter and simpler, can we use 'junit-coverage.ec' instead, or even just 'junit-coverage'?

I'd like to keep the file extension for clarity. All the related documentation uses that extension in examples.

> What if a caller has set options.coverage but did not specify options.coverage_output_path?

Right, I'll make it throw.
Comment on attachment 8994470 [details]
Bug 1473313 - Part 3: Extend CodeCoverageMixin to handle java code coverage tools.

https://reviewboard.mozilla.org/r/259028/#review266126

> We have `JAVA` defined in the build configuration, and we also have to do some things to set the file encoding globally -- see https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/mobile/android/mach_commands.py#537-573.
> 
> Can you not do this with a Gradle command?  That's the most robust approach.

This code is running on the geckoview-junit task, it's being run using this script[1] which then this script[2]. This task does not have a gecko clone available. That means no mach, no gradle.

I can change that and clone gecko for this specific platform/test combination, but I think avoiding the overhead of cloning gecko and setting everything up again is preferrable.

[1] https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/testing/mozharness/scripts/android_emulator_unittest.py
[2] https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/testing/mochitest/runjunit.py

To generate the coverage report, we need three things:
- the classfiles before instrumentation (in this patch, archived as a public artifact during build time, and downloaded to the testing machine)
- the coverage.ec file with coverage counters. This is generated while running the tests on the emulator.
- jacoco-report, the JaCoCo component that takes the classfiles and the coverage counters as input, generating XML reports as output.

The current revision of this patch uses jacoco-cli.jar to generate the report. The jacoco-cli.jar file will probably come from the gradle-dependencies task, as soon as I figure out how to reference that task here.
Comment on attachment 8994470 [details]
Bug 1473313 - Part 3: Extend CodeCoverageMixin to handle java code coverage tools.

https://reviewboard.mozilla.org/r/259028/#review266414

> What determines the order of execution when there are 2 PostScriptActions for the same step? Are you confident this is safe? (If not, consider having one PostScriptAction that calls 2 functions.)

The order these run in should be irrelevant, but there is another problem: if both run, grcov is downloaded twice. I will change this to have only one PostScriptAction.

> I'd really prefer not to add a dependency on an external resource like this -- it never ends well. Is there an ETA for the TODO?

I agree. I will fix this in this patch and remove the TODO.
(In reply to Tudor-Gabriel Vijiala [:tvijiala] from comment #29)
> Comment on attachment 8994470 [details]
> Bug 1473313 - Part 3: Extend CodeCoverageMixin to handle java code coverage
> tools.
> 
> https://reviewboard.mozilla.org/r/259028/#review266126
> 
> > We have `JAVA` defined in the build configuration, and we also have to do some things to set the file encoding globally -- see https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/mobile/android/mach_commands.py#537-573.
> > 
> > Can you not do this with a Gradle command?  That's the most robust approach.
> 
> This code is running on the geckoview-junit task, it's being run using this
> script[1] which then this script[2]. This task does not have a gecko clone
> available. That means no mach, no gradle.
> 
> I can change that and clone gecko for this specific platform/test
> combination, but I think avoiding the overhead of cloning gecko and setting
> everything up again is preferrable.

No, don't do this.  I understand why you're doing what you're doing; just comment about it someplace, 'cuz it's subtle.

> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 085cdfb90903d4985f0de1dc7786522d9fb45596/testing/mozharness/scripts/
> android_emulator_unittest.py
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> 085cdfb90903d4985f0de1dc7786522d9fb45596/testing/mochitest/runjunit.py
> 
> To generate the coverage report, we need three things:
> - the classfiles before instrumentation (in this patch, archived as a public
> artifact during build time, and downloaded to the testing machine)
> - the coverage.ec file with coverage counters. This is generated while
> running the tests on the emulator.
> - jacoco-report, the JaCoCo component that takes the classfiles and the
> coverage counters as input, generating XML reports as output.

This is helpful.  Make sure it gets into the comments/documentation somewhere.

> The current revision of this patch uses jacoco-cli.jar to generate the
> report. The jacoco-cli.jar file will probably come from the
> gradle-dependencies task, as soon as I figure out how to reference that task
> here.
Attachment #8994469 - Attachment is obsolete: true
Attachment #8994470 - Attachment is obsolete: true
Attachment #8994471 - Attachment is obsolete: true
Comment on attachment 8994468 [details]
Bug 1473313 - Part 1: Set up geckoview build config for androidTest coverage runs.

https://reviewboard.mozilla.org/r/259024/#review267330

If this works for you, it works for me!  Well done!
Attachment #8994468 - Flags: review?(nalexander) → review+
Blocks: 1480462
(In reply to Nick Alexander :nalexander from comment #33)
> Comment on attachment 8994468 [details]
> Bug 1473313 - Part 1: Set up geckoview build config for androidTest coverage
> runs.
> 
> https://reviewboard.mozilla.org/r/259024/#review267330
> 
> If this works for you, it works for me!  Well done!

Carrying r+ with further changes:

- updated test for java.configure (python/mozbuild/mozbuild/test/configure/test_checks_configure.py) to include MOZ_JAVA_CODE_COVERAGE. This seems a bit fishy (as java.configure only looks for java tool locations), but I did not succeed in adding the --enable-java-coverage flag in toolchain.configure, to mimic what --enable-coverage is doing.

- updated gradle.configure: added gradle_android_archive_geckoview_coverage_artifacts_tasks as a dependency for gradle_android_dependencies_tasks, so the jar files get downloaded when running 'mach android android-dependencies'

There's a follow up filed (bug 1480462) to remove --enable-java-coverage and merge it with --enable-coverage.
Comment on attachment 8994471 [details]
Bug 1473313 - Part 4: Add build and test platforms for android emulator code coverage.

https://reviewboard.mozilla.org/r/259022/#review266406

> To make this work 'properly', keep this entry just as it is (but let's remove this comment), and add an entry to treeherder, around 
> 
> https://github.com/mozilla/treeherder/blob/master/ui/js/constants.js#L87
> 
> (Use a separate bug in Tree Management::Treeherder).
> 
> Optionally (I don't think you need to), add another translation entry in tests.py.

For consistency, I used a name with a dot '.' and added another translation entry in tests.py.
The entry has been added to treeherder: https://github.com/mozilla/treeherder/commit/9fb4c965e58a3ae79c80dd55223cf09634445216
Comment on attachment 8997049 [details]
Bug 1473313 - Part 3: Extend CodeCoverageMixin to handle java code coverage tools.

https://reviewboard.mozilla.org/r/260996/#review268082

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:485
(Diff revision 1)
>          shutil.rmtree(self.grcov_dir)
>  
> +    @PostScriptAction('run-tests')
> +    def process_java_coverage_data(self, action, success=None):
> +        '''
> +        Run JaCoCo on the coverage.exec file in order to get a XML report.

Nit: `coverage.ec`, right?

::: testing/mozharness/mozharness/mozilla/testing/codecoverage.py:492
(Diff revision 1)
> +        Finally, archive the lcov file and upload it, as process_coverage_data is doing.
> +        '''
> +        if not self.java_code_coverage_enabled:
> +            return
> +
> +        # Don't try to get reports after a failed test run, as the coverage

We do get reports after failed test runs for C++/JS. It would be nice to have the same behavior here if possible.
As discussed on IRC, let's return here only in the case where `not success` and the coverage.ec file doesn't exist (so we don't return when test fails and the emulator terminated gracefully, but we do return when the emulator doesn't terminate gracefully and let Treeherder retry automatically).
Attachment #8997049 - Flags: review?(mcastelluccio) → review+
Attachment #8997051 - Attachment is obsolete: true
Attachment #8997051 - Flags: review?(nalexander)
Comment on attachment 8997116 [details]
Bug 1473313 - Part 5: Add in-tree documentation page for Android code coverage.

https://reviewboard.mozilla.org/r/261012/#review268100

This looks great, thank you!

::: mobile/android/docs/testcoverage.rst:26
(Diff revision 1)
> +instrumentation to record execution coverage data. It has two operating modes:
> +
> +- `online instrumentation`_: Class files are instrumented on-the-fly using a so
> +  called Java agent. The JaCoCo agent collects execution information and dumps
> +  it on request or when the JVM exits. This method is used for test suites
> +  which run on the JVM.

add: ... on the JVM (generally, `*UnitTest` Gradle tasks).

::: mobile/android/docs/testcoverage.rst:30
(Diff revision 1)
> +  it on request or when the JVM exits. This method is used for test suites
> +  which run on the JVM.
> +- `offline instrumentation`_: At runtime the pre-instrumented classes needs be
> +  on the classpath instead of the original classes. In addition,
> +  `jacocoagent.jar` must be put on the classpath. This method is used for test
> +  suites with run on the Android emulator.

nit: WHICH run ...

Add: ... Android emulator (generally, `*AndroidTest` Gradle tasks, including `robocop`).

::: mobile/android/docs/testcoverage.rst:54
(Diff revision 1)
> +----------------------------------------
> +
> +All tasks that output code coverage information do so by exporting an artifact
> +named ``code-coverage-grcov.zip`` which contains a single file named
> +``grcov_lcov_output.info``. The ``grcov_lcov_output.info`` file should contain
> +covearge information in the lcov format. The artifact, once uploaded, is picked

nit: COVERAGE.

::: mobile/android/docs/testcoverage.rst:72
(Diff revision 1)
> +===============================
> +
> +The ``android-test`` suite is a JUnit test suite that runs locally on the
> +host's JVM. It can be run with ``mach android test``. The test suite is
> +implemented as a build task, defined at
> +`taskcluster/ci/build/android-stuff.yml`.

Are the single quotes intentional?
Attachment #8997116 - Flags: review?(nalexander) → review+
Comment on attachment 8997048 [details]
Bug 1473313 - Part 2: Support running emulator junit tests with code coverage support.

https://reviewboard.mozilla.org/r/260994/#review268104
Attachment #8997048 - Flags: review?(gbrown) → review+
Comment on attachment 8997049 [details]
Bug 1473313 - Part 3: Extend CodeCoverageMixin to handle java code coverage tools.

https://reviewboard.mozilla.org/r/260996/#review268108

Nice!
Attachment #8997049 - Flags: review?(gbrown) → review+
Comment on attachment 8997050 [details]
Bug 1473313 - Part 4: Add build and test platforms for android emulator code coverage.

https://reviewboard.mozilla.org/r/260998/#review268110
Attachment #8997050 - Flags: review?(gbrown) → review+
Comment on attachment 8997116 [details]
Bug 1473313 - Part 5: Add in-tree documentation page for Android code coverage.

https://reviewboard.mozilla.org/r/261012/#review268152

::: mobile/android/docs/testcoverage.rst:72
(Diff revision 1)
> +===============================
> +
> +The ``android-test`` suite is a JUnit test suite that runs locally on the
> +host's JVM. It can be run with ``mach android test``. The test suite is
> +implemented as a build task, defined at
> +`taskcluster/ci/build/android-stuff.yml`.

No, good catch.

Thank you for going over this.
Keywords: checkin-needed
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92b8a54b4ad4
Part 1: Set up geckoview build config for androidTest coverage runs. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/0ae5ffe1725a
Part 2: Support running emulator junit tests with code coverage support. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/f346c4e9cd7c
Part 3: Extend CodeCoverageMixin to handle java code coverage tools. r=gbrown,marco
https://hg.mozilla.org/integration/autoland/rev/da58e1d55821
Part 4: Add build and test platforms for android emulator code coverage. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/3f140c222e02
Part 5: Add in-tree documentation page for Android code coverage. r=nalexander
Keywords: checkin-needed
Depends on: 1480875
Depends on: 1481856
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: