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)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1371445 - Pre: Produce and upload XML and HTML findbugs reports.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1371445 - Pre: Fail unit tests if unlimited strength JCE is not installed.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1371445 - Pre: Upload HTML checkstyle report.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1371445 - Add |mach android {lint,findbugs,checkstyle,test}| commands for running Android-specific test suites.
59 bytes, text/x-review-board-request
gps
: review+
Details 
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.
(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 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 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 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 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 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 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.
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.
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
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: 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. :)
(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)
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
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: