Closed
Bug 1471660
Opened 6 years ago
Closed 6 years ago
Add code coverage for A(test) Java unit tests
Categories
(Testing :: Code Coverage, enhancement)
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: gabriel-v, Assigned: gabriel-v)
References
Details
Attachments
(1 file)
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•6 years ago
|
Assignee: nobody → tvijiala
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•6 years ago
|
||
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•6 years 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•6 years 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•6 years 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)
Assignee | ||
Comment 10•6 years ago
|
||
try with A(test-ccov) working: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f2fc19607a2697690487e42a8854137bbd2a6d5
Comment hidden (mozreview-request) |
Comment 12•6 years 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•6 years 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•6 years 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•6 years 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]
Comment 17•6 years ago
|
||
(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•6 years 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•6 years 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-
Comment 21•6 years ago
|
||
> 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•6 years 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•6 years 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•6 years 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•6 years ago
|
Keywords: checkin-needed
Comment 27•6 years 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]
Comment 28•6 years ago
|
||
Gabriel, could you file a follow-up bug to download grcov using fetches for the Android build too?
Comment 29•6 years ago
|
||
Please resolve the open issues, so we can land this patch.
Flags: needinfo?(tvijiala)
Keywords: checkin-needed
Assignee | ||
Comment 30•6 years ago
|
||
The issues were resolved, I just omitted to click the buttons.
Sorry for the mix-up.
Flags: needinfo?(tvijiala)
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 31•6 years 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
Comment 32•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years 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.
Description
•