Add code coverage for A(test) Java unit tests

RESOLVED FIXED in Firefox 63

Status

RESOLVED FIXED
9 months ago
8 months ago

People

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

Tracking

Version 3
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
Implement a task named A(test-ccov) that reports code coverage for the A(test) unit tests.


Current experiment: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d49dcccfd3b239803c92b6782195ec23d3dff78a


Current problems include:

- hardcoded flavor combination for running the test suite
- changes to the build.gradle file will slow down the A(test) task too
(Assignee)

Updated

9 months ago
Assignee: nobody → tvijiala
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Depends on: 1472236
Comment hidden (mozreview-request)
Comment on attachment 8988250 [details]
Bug 1471660 - Integrate code coverage for A(test) junit test suite via JaCoCo plugin.

https://reviewboard.mozilla.org/r/253500/#review261610


Code analysis found 3 defects in this patch:
 - 3 defects 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


::: mobile/android/gradle.configure:182
(Diff revision 3)
>          'app:test{app.variant.name}UnitTest'.format(app=build_config.app),
>          'geckoview:test{geckoview.variant.name}UnitTest'.format(
>              geckoview=build_config.geckoview),
>      ]
>  
> +@dependable

Error: Expected 2 blank lines, found 1 [flake8: E302]

::: mobile/android/gradle.configure:182
(Diff revision 3)
>          'app:test{app.variant.name}UnitTest'.format(app=build_config.app),
>          'geckoview:test{geckoview.variant.name}UnitTest'.format(
>              geckoview=build_config.geckoview),
>      ]
>  
> +@dependable

Error: Undefined name 'dependable' [flake8: F821]

::: mobile/android/gradle.configure:191
(Diff revision 3)
> +        'app:jacocoTestReport',
> +        'geckoview:jacocoTestReport',
> +    ]
> +
>  set_config('GRADLE_ANDROID_TEST_TASKS', gradle_android_test_tasks)
> +set_config('GRADLE_ANDROID_TEST_CCOV_REPORT_TASKS', gradle_android_test_ccov_report_tasks)

Error: Undefined name 'set_config' [flake8: F821]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

9 months ago
mozreview-review
Comment on attachment 8988250 [details]
Bug 1471660 - Integrate code coverage for A(test) junit test suite via JaCoCo plugin.

https://reviewboard.mozilla.org/r/253500/#review261632


Code analysis found 3 defects in this patch:
 - 3 defects 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


::: mobile/android/gradle.configure:182
(Diff revision 4)
>          'app:test{app.variant.name}UnitTest'.format(app=build_config.app),
>          'geckoview:test{geckoview.variant.name}UnitTest'.format(
>              geckoview=build_config.geckoview),
>      ]
>  
> +@dependable

Error: Expected 2 blank lines, found 1 [flake8: E302]

::: mobile/android/gradle.configure:182
(Diff revision 4)
>          'app:test{app.variant.name}UnitTest'.format(app=build_config.app),
>          'geckoview:test{geckoview.variant.name}UnitTest'.format(
>              geckoview=build_config.geckoview),
>      ]
>  
> +@dependable

Error: Undefined name 'dependable' [flake8: F821]

::: mobile/android/gradle.configure:191
(Diff revision 4)
> +        'app:jacocoTestReport',
> +        'geckoview:jacocoTestReport',
> +    ]
> +
>  set_config('GRADLE_ANDROID_TEST_TASKS', gradle_android_test_tasks)
> +set_config('GRADLE_ANDROID_TEST_CCOV_REPORT_TASKS', gradle_android_test_ccov_report_tasks)

Error: Undefined name 'set_config' [flake8: F821]

Comment 8

9 months ago
mozreview-review
Comment on attachment 8988250 [details]
Bug 1471660 - Integrate code coverage for A(test) junit test suite via JaCoCo plugin.

https://reviewboard.mozilla.org/r/253500/#review261642


Code analysis found 3 defects in this patch:
 - 3 defects 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


::: mobile/android/gradle.configure:182
(Diff revision 5)
>          'app:test{app.variant.name}UnitTest'.format(app=build_config.app),
>          'geckoview:test{geckoview.variant.name}UnitTest'.format(
>              geckoview=build_config.geckoview),
>      ]
>  
> +@dependable

Error: Expected 2 blank lines, found 1 [flake8: E302]

::: mobile/android/gradle.configure:182
(Diff revision 5)
>          'app:test{app.variant.name}UnitTest'.format(app=build_config.app),
>          'geckoview:test{geckoview.variant.name}UnitTest'.format(
>              geckoview=build_config.geckoview),
>      ]
>  
> +@dependable

Error: Undefined name 'dependable' [flake8: F821]

::: mobile/android/gradle.configure:191
(Diff revision 5)
> +        'app:jacocoTestReport',
> +        'geckoview:jacocoTestReport',
> +    ]
> +
>  set_config('GRADLE_ANDROID_TEST_TASKS', gradle_android_test_tasks)
> +set_config('GRADLE_ANDROID_TEST_CCOV_REPORT_TASKS', gradle_android_test_ccov_report_tasks)

Error: Undefined name 'set_config' [flake8: F821]
(Assignee)

Comment 9

9 months ago
(please ignore spammy bot)

In order to deduplicate configuration for "app" and "geckoview" unit tests, I moved all JaCoCo-related configuration into a separate gradle file: "jacoco_for_junit.gradle".

To avoid running JaCoCo during the A(test) suite too, that configuration file is imported with "apply from:" only when an environment variable has been set.

Since gradle is being run in offline mode, when the dependencies are listed and archived, said environment variable is not set, and the JaCoCo dependencies are not pulled. 

I tried using gradle in online mode, but that had no effect.

The fix I found was setting the dependencies for both "android test" and "android test --code-coverage".

Could I also set that environment variable when the dependencies are being analyzed? If so, where do I look?
Flags: needinfo?(nalexander)
Comment hidden (mozreview-request)

Comment 12

9 months ago
mozreview-review
Comment on attachment 8988250 [details]
Bug 1471660 - Integrate code coverage for A(test) junit test suite via JaCoCo plugin.

https://reviewboard.mozilla.org/r/253500/#review261818


Code analysis found 3 defects in this patch:
 - 3 defects 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


::: mobile/android/gradle.configure:182
(Diff revision 6)
>          'app:test{app.variant.name}UnitTest'.format(app=build_config.app),
>          'geckoview:test{geckoview.variant.name}UnitTest'.format(
>              geckoview=build_config.geckoview),
>      ]
>  
> +@dependable

Error: Expected 2 blank lines, found 1 [flake8: E302]

::: mobile/android/gradle.configure:182
(Diff revision 6)
>          'app:test{app.variant.name}UnitTest'.format(app=build_config.app),
>          'geckoview:test{geckoview.variant.name}UnitTest'.format(
>              geckoview=build_config.geckoview),
>      ]
>  
> +@dependable

Error: Undefined name 'dependable' [flake8: F821]

::: mobile/android/gradle.configure:191
(Diff revision 6)
> +        'app:jacocoTestReport',
> +        'geckoview:jacocoTestReport',
> +    ]
> +
>  set_config('GRADLE_ANDROID_TEST_TASKS', gradle_android_test_tasks)
> +set_config('GRADLE_ANDROID_TEST_CCOV_REPORT_TASKS', gradle_android_test_ccov_report_tasks)

Error: Undefined name 'set_config' [flake8: F821]

Comment 13

9 months ago
mozreview-review
Comment on attachment 8988250 [details]
Bug 1471660 - Integrate code coverage for A(test) junit test suite via JaCoCo plugin.

https://reviewboard.mozilla.org/r/253500/#review261852

The code coverage parts look good to me. I defer to :nalexander for the Gradle parts.

::: mobile/android/mach_commands.py:139
(Diff revision 6)
> +            args = [grcov_path, input_path, '-t', 'lcov']
> +            return subprocess.check_output(args)
> +
> +        grcov = download_grcov()
> +        xml_dir = tempfile.mkdtemp()
> +        root = get_root_dir()

You always use the objdir, so maybe make this objdir = get_obj_dir().

There might be a better way to get the objdir though, self.topobjdir.

::: mobile/android/mach_commands.py:149
(Diff revision 6)
> +        shutil.copy(app_xml, os.path.join(xml_dir, 'app.xml'))
> +
> +        gv_xml = os.path.join(root, report_xml_template % 'geckoview')
> +        shutil.copy(gv_xml, os.path.join(xml_dir, 'geckoview.xml'))
> +
> +        # merge output files

Maybe "parse output files" is better. Even though you are parsing and merging the inputs into a single output, I think "parse" is more explanatory.

::: taskcluster/ci/build/android-stuff.yml:48
(Diff revision 6)
>              - "mobile/android/config/**"
>              - "mobile/android/gradle.configure"
>              - "mobile/android/tests/background/junit4/**"
>              - "**/*.gradle"
>  
> +android-test-ccov/opt:

We should make sure the coverage artifacts from this build are picked up by ActiveData.
Attachment #8988250 - Flags: review?(mcastelluccio) → review+
(Assignee)

Comment 14

9 months ago
mozreview-review-reply
Comment on attachment 8988250 [details]
Bug 1471660 - Integrate code coverage for A(test) junit test suite via JaCoCo plugin.

https://reviewboard.mozilla.org/r/253500/#review261852

> Maybe "parse output files" is better. Even though you are parsing and merging the inputs into a single output, I think "parse" is more explanatory.

Forgot that comment there after rewriting the logic. Last time it was actually merging the reports.

> We should make sure the coverage artifacts from this build are picked up by ActiveData.

Is a change in ActiveData required?
Comment hidden (mozreview-request)

Comment 16

9 months ago
mozreview-review
Comment on attachment 8988250 [details]
Bug 1471660 - Integrate code coverage for A(test) junit test suite via JaCoCo plugin.

https://reviewboard.mozilla.org/r/253500/#review261896


Code analysis found 4 defects in this patch:
 - 4 defects 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


::: mobile/android/gradle.configure:183
(Diff revision 7)
>          'geckoview:test{geckoview.variant.name}UnitTest'.format(
>              geckoview=build_config.geckoview),
>      ]
>  
> +
> +@dependable

Error: Undefined name 'dependable' [flake8: F821]

::: mobile/android/gradle.configure:192
(Diff revision 7)
> +        'app:jacocoTestReport',
> +        'geckoview:jacocoTestReport',
> +    ]
> +
>  set_config('GRADLE_ANDROID_TEST_TASKS', gradle_android_test_tasks)
> +set_config('GRADLE_ANDROID_TEST_CCOV_REPORT_TASKS', gradle_android_test_ccov_report_tasks)

Error: Undefined name 'set_config' [flake8: F821]

::: mobile/android/mach_commands.py:133
(Diff revision 7)
> +            args = [grcov_path, input_path, '-t', 'lcov']
> +            return subprocess.check_output(args)
> +
> +        grcov = download_grcov()
> +        xml_dir = tempfile.mkdtemp()
> +        root = get_root_dir()

Error: Local variable 'root' is assigned to but never used [flake8: F841]

::: mobile/android/mach_commands.py:133
(Diff revision 7)
> +            args = [grcov_path, input_path, '-t', 'lcov']
> +            return subprocess.check_output(args)
> +
> +        grcov = download_grcov()
> +        xml_dir = tempfile.mkdtemp()
> +        root = get_root_dir()

Error: Undefined name 'get_root_dir' [flake8: F821]
(In reply to Tudor-Gabriel Vijiala from comment #14)
> > We should make sure the coverage artifacts from this build are picked up by ActiveData.
> 
> Is a change in ActiveData required?

It might be required, we'll see with Kyle when it is back up. It doesn't block landing this though, it's just something we have to verify.
Comment hidden (mozreview-request)

Comment 19

9 months ago
mozreview-review
Comment on attachment 8988250 [details]
Bug 1471660 - Integrate code coverage for A(test) junit test suite via JaCoCo plugin.

https://reviewboard.mozilla.org/r/253500/#review261908


Code analysis found 2 defects in this patch:
 - 2 defects 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


::: mobile/android/gradle.configure:183
(Diff revision 8)
>          'geckoview:test{geckoview.variant.name}UnitTest'.format(
>              geckoview=build_config.geckoview),
>      ]
>  
> +
> +@dependable

Error: Undefined name 'dependable' [flake8: F821]

::: mobile/android/gradle.configure:192
(Diff revision 8)
> +        'app:jacocoTestReport',
> +        'geckoview:jacocoTestReport',
> +    ]
> +
>  set_config('GRADLE_ANDROID_TEST_TASKS', gradle_android_test_tasks)
> +set_config('GRADLE_ANDROID_TEST_CCOV_REPORT_TASKS', gradle_android_test_ccov_report_tasks)

Error: Undefined name 'set_config' [flake8: F821]

Comment 20

9 months ago
mozreview-review
Comment on attachment 8988250 [details]
Bug 1471660 - Integrate code coverage for A(test) junit test suite via JaCoCo plugin.

https://reviewboard.mozilla.org/r/253500/#review261910

This is basically fine, but there's a simpler expression to achieve the environment handling that I think is worth pursuing.  Gradle automatically turns parameters with a certain shape into project properties, and there's an example of this in the tree.  The argument is set at https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/mobile/android/mach_commands.py#80 and is consumed in Gradle at https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/mobile/android/geckoview/build.gradle#360-361.

With this change, you can drop the special environment handling, and delegate to `gradle` internally with an additional argument.  (Yay!)  It's cheap to add |mach android ...| commands; you called your automation job "test-ccov" so we want |mach android test-ccov| to be exactly what that job invokes.  (Just like the other jobs.)

r- so I get to take another look.  You hint in the ticket that the dependency-gathering mechanism for |mach android gradle-dependencies| is subtle here (and it is subtle!), so please tell me how this works in practice on try.

::: mobile/android/geckoview/build.gradle:369
(Diff revision 8)
>      workingDir "${topsrcdir}/widget/android/bindings"
>  
>      dependsOn project(':annotations').jar
>  }
> +
> +apply from: "${topsrcdir}/mobile/android/gradle/jacoco_dependencies.gradle"

Why are these dependencies not conditional as well?

::: mobile/android/gradle/jacoco_dependencies.gradle:1
(Diff revision 8)
> +project.ext.jacoco_version = "0.7.8"

License header, here and elsewhere.

::: mobile/android/mach_commands.py:132
(Diff revision 8)
> +        def run_grcov(grcov_path, input_path):
> +            args = [grcov_path, input_path, '-t', 'lcov']
> +            return subprocess.check_output(args)
> +
> +        grcov = download_grcov()
> +        xml_dir = tempfile.mkdtemp()

We have patterns for temporary work like this: https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/vendor_python.py#45.  Can you put everything into such a temporary directory?  Then you don't need to remember stuff and clean up at all.

::: mobile/android/mach_commands.py:144
(Diff revision 8)
> +        grcov_output = run_grcov(grcov, xml_dir)
> +        grcov_zip_path = os.path.join(self.topobjdir, 'code-coverage-grcov.zip')
> +        with zipfile.ZipFile(grcov_zip_path, 'w', zipfile.ZIP_DEFLATED) as z:
> +            z.writestr('grcov_lcov_output.info', grcov_output)
> +
> +        # Cleanup

nit: full sentence.  And if you really care about cleanup, put it in a try:finally: block.

::: testing/mozharness/configs/builds/releng_sub_android_configs/64_test_ccov.py:3
(Diff revision 8)
> +config = {
> +    'base_name': 'Android armv7 unit test code coverage %(branch)s',
> +    'stage_platform': 'android-test',

andoid-test-ccov?

::: testing/mozharness/configs/builds/releng_sub_android_configs/64_test_ccov.py:10
(Diff revision 8)
> +    'multi_locale_config_platform': 'android',
> +    # unit tests don't produce a package. So don't collect package metrics.
> +    'disable_package_metrics': True,
> +    'postflight_build_mach_commands': [
> +        ['android',
> +         'test',

After you make the changes I suggest, this will be |android test-ccov|, parallel to all the others.
Attachment #8988250 - Flags: review?(nalexander) → review-
> In order to deduplicate configuration for "app" and "geckoview" unit tests,
> I moved all JaCoCo-related configuration into a separate gradle file:
> "jacoco_for_junit.gradle".

This is good, thanks.
 
> To avoid running JaCoCo during the A(test) suite too, that configuration
> file is imported with "apply from:" only when an environment variable has
> been set.

I'd prefer an argument, which is equivalent.

> Since gradle is being run in offline mode, when the dependencies are listed
> and archived, said environment variable is not set, and the JaCoCo
> dependencies are not pulled. 

Right.  We list all of the dependency tasks here https://searchfox.org/mozilla-central/source/mobile/android/gradle.configure#244, so you want to make sure that your ccov task is in there.  (Presumably it's not just the test task, but there's also a "testWithCodeCoverage" test somewhere.  I hope!)

> I tried using gradle in online mode, but that had no effect.

Yeah, this isn't how this process works.  The |mach android gradle-dependencies| process collects the dependencies in a toolchain task once; subsequent builds and tests use the collected dependencies in offline mode.  We can't change that calculus (and you should need to).

> The fix I found was setting the dependencies for both "android test" and
> "android test --code-coverage".

I'm fine with this; those dependencies are not a big burden on local testers.  (As long as they're not actually compiled in for most usage.)

> Could I also set that environment variable when the dependencies are being
> analyzed? If so, where do I look?

This is probably not the right approach, especially with the flags.  I can help get a task that actually requires these dependencies at the right times if that's a sticking point.  Process my review, re-flag me with a link to a new try build, and then we can work out this detail together.
Flags: needinfo?(nalexander)
Comment hidden (mozreview-request)

Comment 23

9 months ago
mozreview-review
Comment on attachment 8988250 [details]
Bug 1471660 - Integrate code coverage for A(test) junit test suite via JaCoCo plugin.

https://reviewboard.mozilla.org/r/253500/#review262566


Code analysis found 2 defects in this patch:
 - 2 defects 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


::: mobile/android/gradle.configure:183
(Diff revision 9)
>          'geckoview:test{geckoview.variant.name}UnitTest'.format(
>              geckoview=build_config.geckoview),
>      ]
>  
> +
> +@dependable

Error: Undefined name 'dependable' [flake8: F821]

::: mobile/android/gradle.configure:192
(Diff revision 9)
> +        'app:jacocoTestReport',
> +        'geckoview:jacocoTestReport',
> +    ]
> +
>  set_config('GRADLE_ANDROID_TEST_TASKS', gradle_android_test_tasks)
> +set_config('GRADLE_ANDROID_TEST_CCOV_REPORT_TASKS', gradle_android_test_ccov_report_tasks)

Error: Undefined name 'set_config' [flake8: F821]

Comment 24

9 months ago
mozreview-review
Comment on attachment 8988250 [details]
Bug 1471660 - Integrate code coverage for A(test) junit test suite via JaCoCo plugin.

https://reviewboard.mozilla.org/r/253500/#review262590

This looks good to me.  If it's green on try (and other tasks are too, of course :) then roll on!  Good work!

::: mobile/android/gradle.configure:185
(Diff revision 9)
>      ]
>  
> +
> +@dependable
> +def gradle_android_test_ccov_report_tasks():
> +    '''Additional gradle tasks run by |mach android test --ccov-report|.'''

This should be |mach android test-ccov| now.
Attachment #8988250 - Flags: review?(nalexander) → review+
(Assignee)

Comment 25

9 months ago
(In reply to Nick Alexander :nalexander from comment #24)
> This should be |mach android test-ccov| now.
Missed that, thanks!


try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f4f1a53471ddf73a09b448b6af7f98448c3588a
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Keywords: checkin-needed

Comment 27

9 months ago
mozreview-review
Comment on attachment 8988250 [details]
Bug 1471660 - Integrate code coverage for A(test) junit test suite via JaCoCo plugin.

https://reviewboard.mozilla.org/r/253500/#review262622


Code analysis found 2 defects in this patch:
 - 2 defects 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


::: mobile/android/gradle.configure:183
(Diff revision 10)
>          'geckoview:test{geckoview.variant.name}UnitTest'.format(
>              geckoview=build_config.geckoview),
>      ]
>  
> +
> +@dependable

Error: Undefined name 'dependable' [flake8: F821]

::: mobile/android/gradle.configure:192
(Diff revision 10)
> +        'app:jacocoTestReport',
> +        'geckoview:jacocoTestReport',
> +    ]
> +
>  set_config('GRADLE_ANDROID_TEST_TASKS', gradle_android_test_tasks)
> +set_config('GRADLE_ANDROID_TEST_CCOV_REPORT_TASKS', gradle_android_test_ccov_report_tasks)

Error: Undefined name 'set_config' [flake8: F821]
Gabriel, could you file a follow-up bug to download grcov using fetches for the Android build too?
Please resolve the open issues, so we can land this patch.
Flags: needinfo?(tvijiala)
Keywords: checkin-needed
(Assignee)

Comment 30

8 months ago
The issues were resolved, I just omitted to click the buttons.

Sorry for the mix-up.
Flags: needinfo?(tvijiala)
(Assignee)

Updated

8 months ago
Keywords: checkin-needed

Comment 31

8 months ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1976c96d3a5
Integrate code coverage for A(test) junit test suite via JaCoCo plugin. r=marco,nalexander
Keywords: checkin-needed
(Assignee)

Updated

8 months ago
Blocks: 1474572
(Assignee)

Updated

8 months ago
Blocks: 1474575

Comment 32

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f1976c96d3a5
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.