Add |mach android {lint,findbugs,checkstyle,test}| commands for running Android-specific test suites

RESOLVED FIXED in Firefox 56

Status

Firefox Build System
General
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

a year ago
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
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.
... because the number of mach commands is growing unwieldy.
(Assignee)

Comment 3

a year 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!
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)

Comment 16

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

11 months 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

11 months 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

11 months 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

11 months 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

11 months ago
Assignee: nobody → nalexander
Status: NEW → ASSIGNED

Comment 29

11 months ago
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)

Comment 30

11 months ago
Documentation looks sufficient, though. :)
(Assignee)

Comment 31

11 months 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

11 months 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

11 months 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
Last Resolved: 11 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

8 months ago
Blocks: 1408140

Updated

3 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.