Closed
Bug 1371445
Opened 7 years ago
Closed 7 years ago
Add |mach android {lint,findbugs,checkstyle,test}| commands for running Android-specific test suites
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: nalexander, Assigned: nalexander)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
The android-* suites are all "build and then run some Gradle targets", but the set of targets is buried in a bunch of configuration files [1]. This ticket tracks adding |mach android-*| to centralize the set of targets for each command, making it easier for automation and local developers to invoke them.
This will also be a good place for us to post-process the XML reports, producing Tree Herder-friendly output, as discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1370539#c16 and friends.
[1] Like http://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/builds/releng_sub_android_configs/64_lint.py#11
Comment 1•7 years ago
|
||
I would strongly prefer we avoid introducing multiple mach commands for this. Please find a way to shoehorn into `mach lint` or `mach test` if possible.
Comment 2•7 years ago
|
||
... because the number of mach commands is growing unwieldy.
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #1)
> I would strongly prefer we avoid introducing multiple mach commands for
> this. Please find a way to shoehorn into `mach lint` or `mach test` if
> possible.
I was thinking they'd be in mobile/android/mach_commands.py, so they wouldn't pollute the global mach command namespace.
But I will try to achieve:
- mach lint android-{lint,findbugs,checkstyle}
- mach test android-test
We'll see how I do!
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8883402 [details]
Bug 1371445 - Pre: Produce and upload XML and HTML findbugs reports.
https://reviewboard.mozilla.org/r/154292/#review160654
Catching up. Thanks, Nick!
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8883402 [details]
Bug 1371445 - Pre: Produce and upload XML and HTML findbugs reports.
https://reviewboard.mozilla.org/r/154296/#review160600
LGTM
Attachment #8883402 -
Flags: review?(max) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8883403 [details]
Bug 1371445 - Pre: Fail unit tests if unlimited strength JCE is not installed.
https://reviewboard.mozilla.org/r/154298/#review160650
Attachment #8883403 -
Flags: review?(max) → review+
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8883404 [details]
Bug 1371445 - Pre: Upload HTML checkstyle report.
https://reviewboard.mozilla.org/r/154300/#review160652
Attachment #8883404 -
Flags: review?(max) → review+
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8883405 [details]
Bug 1371445 - Add |mach android {lint,findbugs,checkstyle,test}| commands for running Android-specific test suites.
https://reviewboard.mozilla.org/r/154294/#review160770
It seems like there may be some copy pasta in here for things like parsing XML. But I think it is sufficiently different that I'm not sure it's worth factoring out into a common function.
::: mobile/android/mach_commands.py:81
(Diff revision 2)
> + reports = ('officialAustralisDebug',
> + 'officialPhotonDebug',)
> + for report in reports:
> + finder = FileFinder(os.path.join(self.topobjdir, 'gradle/build/mobile/android/app/test-results/', report))
> + for p, _ in finder.find('TEST-*.xml'):
> + f = open(os.path.join(finder.base, p), 'rt')
Nit: I think this file should be opened in binary mode (rb), as encoding is handled at the XML layer, not at the I/O layer.
::: mobile/android/mach_commands.py:117
(Diff revision 2)
> + print('TEST-PASS | {}'.format(name))
> +
> + print('SUITE-END | android-test | {} {}'.format(report, root.get('name')))
> +
> + title = report
> + print("TinderboxPrint: report<br/><a href='{}/{}/index.html'>HTML {} report</a>, visit \"Inspect Task\" link for details".format(root_url, report, title))
I'm not sure if we still need this legacy "TinderboxPrint" syntax. You might want to follow up with a Treeherder guru.
Attachment #8883405 -
Flags: review?(gps) → review+
Assignee | ||
Comment 21•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8883403 [details]
Bug 1371445 - Pre: Fail unit tests if unlimited strength JCE is not installed.
https://reviewboard.mozilla.org/r/154298/#review161768
::: mobile/android/app/build.gradle:482
(Diff revision 3)
> if (System.env.TASK_ID && System.env.RUN_ID) {
> def artifactRootUrl = "https://queue.taskcluster.net/v1/task/${System.env.TASK_ID}/runs/${System.env.RUN_ID}/artifacts"
> gradle.addListener(makeTaskExecutionListener(artifactRootUrl))
> }
>
> +if (gradle.startParameter.taskNames.any { it.endsWith('UnitTest') }) {
maliu: I rewrote this patch, 'cuz the previous approach didn't abort the build. The exception thrown was in a listener, which isn't able to stop the test process. This tries to stop the test process earlier, at the risk of being less comprehensive: it's possible to run tests without having a `startParameter` explicitly ask to run tests.
Assignee | ||
Comment 27•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8883405 [details]
Bug 1371445 - Add |mach android {lint,findbugs,checkstyle,test}| commands for running Android-specific test suites.
https://reviewboard.mozilla.org/r/154294/#review160770
Yeah, they're all "similar but different". Thanks for not making me try to find some common framework.
> I'm not sure if we still need this legacy "TinderboxPrint" syntax. You might want to follow up with a Treeherder guru.
I kept this, since I want TH to give a nice link to the report. It doesn't work (yet) due to https://bugzilla.mozilla.org/show_bug.cgi?id=1378242.
Assignee | ||
Updated•7 years ago
|
Summary: Add |mach android-{lint,findbugs,checkstyle,test}| commands for running Android-specific test suites → Add |mach android {lint,findbugs,checkstyle,test}| commands for running Android-specific test suites
Assignee | ||
Comment 28•7 years ago
|
||
RyanVM: kwierso: I'm not sure who to flag for "sheriff review", so could one of you either OK the output or redirect?
The try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5a23b3fa47c67801e6b18eda36bd5e7b325290c should have failing android-{lint,findbugs,checkstyle,test} jobs. The failing output should include (lots of) details about the failing tests. The details are dumps of the XML errors the tools produce -- I see no advantage to massaging that output, at least not at first.
Could one of you confirm that this output meets the "usable logs" part of https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#Usable_job_logs?
Further, could one you confirm that the documentation I put in place at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing (search for "android-") and the detailed documentation at https://developer.mozilla.org/en-US/docs/Mozilla/Android-specific_test_suites meets the "sufficient documentation" part of https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#Has_sufficient_documentation?
Thanks! Once we have the sign off, I'll file a "make them tier 1" ticket and prepare whatever additional emails are required.
Flags: needinfo?(wkocher)
Flags: needinfo?(ryanvm)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
The failure output isn't really conducive to being able to file bugs against the failures.
We could file against the "android-lint | Lint found errors in the project..." lines, but every single failure will look exactly the same, aside from the per-push hash, which will never match any other failure in the future.
The lines containing xml output don't match up with Treeherder's expected output form, so filing bugs for them in a format that will return bug suggestions will be difficult.
I'm not sure if these are important issues, or what the solution for those issues would be. Assuming lint failures are never intermittent (any failures will either require an immediate followup or backout), we shouldn't need to file bugs against these failures. Maybe the failure output could be massaged into something like this?
> TEST-UNEXPECTED-FAIL | <filewithlintissue.ext> | <XX> lint issues detected, see https://path.to/lint-results-foo.html for the full report
Flags: needinfo?(wkocher)
Documentation looks sufficient, though. :)
Assignee | ||
Comment 31•7 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #29)
> The failure output isn't really conducive to being able to file bugs against
> the failures.
>
> We could file against the "android-lint | Lint found errors in the
> project..." lines, but every single failure will look exactly the same,
> aside from the per-push hash, which will never match any other failure in
> the future.
>
> The lines containing xml output don't match up with Treeherder's expected
> output form, so filing bugs for them in a format that will return bug
> suggestions will be difficult.
>
> I'm not sure if these are important issues, or what the solution for those
> issues would be. Assuming lint failures are never intermittent (any failures
> will either require an immediate followup or backout), we shouldn't need to
> file bugs against these failures. Maybe the failure output could be massaged
> into something like this?
> > TEST-UNEXPECTED-FAIL | <filewithlintissue.ext> | <XX> lint issues detected, see https://path.to/lint-results-foo.html for the full report
I had a discussion on IRC with KWierso, where we agreed to land this as is and follow-up to improve the error messages if we see non-deterministic failures. (Only android-test -- unit tests -- should ever have non-deterministic failures.) The improvements would be to move the XML output from TEST-UNEXPECTED-FAIL to TEST-INFO, so it doesn't contribute to the Orange Factor search; and to make the first line include either failing filenames or testnames.
Flags: needinfo?(ryanvm)
Comment 32•7 years ago
|
||
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2fb8cf9807ba
Pre: Produce and upload XML and HTML findbugs reports. r=maliu
https://hg.mozilla.org/integration/autoland/rev/91701101061d
Pre: Fail unit tests if unlimited strength JCE is not installed. r=maliu
https://hg.mozilla.org/integration/autoland/rev/3159a025e57a
Pre: Upload HTML checkstyle report. r=maliu
https://hg.mozilla.org/integration/autoland/rev/485b02d68d8a
Add |mach android {lint,findbugs,checkstyle,test}| commands for running Android-specific test suites. r=gps
Comment 33•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2fb8cf9807ba
https://hg.mozilla.org/mozilla-central/rev/91701101061d
https://hg.mozilla.org/mozilla-central/rev/3159a025e57a
https://hg.mozilla.org/mozilla-central/rev/485b02d68d8a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•