Closed Bug 1254605 Opened 8 years ago Closed 8 years ago

Split lint and unittest TaskCluster jobs

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: nalexander, Assigned: mcomella)

References

Details

Attachments

(2 files)

Bug 1238678 added a unittest TaskCluster job.  That job runs lint, and then runs the unittests.  When it landed, we didn't burn the tree if lint failed; post Bug 1238788, we do.

This ticket tracks duplicating the unittest job and making it run only lint, so we get independent signals.  That is, we can see when we burn lint but *don't* burn unittests.
The two commands are defined at https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/builds/releng_sub_android_configs/64_api_15_frontend.py#9.

I think this ticket looks like a big copy-paste exercise, with strategic changes of "frontend" to "lint".

mcomella: I'd like somebody other than me to know some of the TaskCluster setup.  Maybe you're interested in trying this?
Flags: needinfo?(michael.l.comella)
Yes, I'm interested – I'll try to get to it this week.
Nick, the "postflight_mach_build_commands" indicates to me that after the build completes, we run some commands. However, the treeherder symbol appears to be associated with the task [1], not the post-flight commands (which is where we currently do unit testing and lint). Provided, I don't know much about the system, I think a strategic copy paste will lead us to two tasks, each doing a full build of the Java source and then running one of lint or unit tests. This seems inefficient – I'd think we want a set-up where we run the full build in a task, then have two dependency tasks run, one for lint and one for unit test, as opposed to post-flight commands, in order to get the Treeherder job separation.

Am I understanding this correctly?

[1]: https://mxr.mozilla.org/mozilla-central/source/testing/taskcluster/tasks/builds/android_api_15_frontend.yml#77
Assignee: nobody → michael.l.comella
Flags: needinfo?(michael.l.comella) → needinfo?(nalexander)
That being said, the patch in comment 5 seems to work (see push in comment 3) with two separate tasks that (I think) each do a full build. I'll need to check it over for correctness, however, when my brain is refreshed.

Note to self: update Unit job to stop linting.
I made the realization that we may not need to build for lint, exception to pre-processed resources, so we could also just not build for the lint job (however, it seems more robust to make the build a separate task, if I understand the terminology correctly).
(In reply to Michael Comella (:mcomella) from comment #4)
> Nick, the "postflight_mach_build_commands" indicates to me that after the
> build completes, we run some commands. However, the treeherder symbol
> appears to be associated with the task [1], not the post-flight commands
> (which is where we currently do unit testing and lint). Provided, I don't
> know much about the system, I think a strategic copy paste will lead us to
> two tasks, each doing a full build of the Java source and then running one
> of lint or unit tests. This seems inefficient – I'd think we want a set-up
> where we run the full build in a task, then have two dependency tasks run,
> one for lint and one for unit test, as opposed to post-flight commands, in
> order to get the Treeherder job separation.

Your logic is flawless, but it's hard to achieve in practice.  |gradle task| does everything a task needs; if that includes building, you either need an in-place build or to be able to build.  Compiling the Java bits (the front-end stuff) is trivial; splitting and Android build into pieces, essentially impossible.  Hence I don't worry about it.  If builds start taking more than a few minutes, which is *very* unlikely, then we can try to be more clever.  Just for the record, most of these Gradle jobs *aren't* using an APK -- unit tests compiles special "mockable JARs", linting does that and a bunch of resource stuff, checkstyle probably uses the mockableJars, etc.

Even if we could convince Gradle to split the build, getting the dependencies and artifacts to flow through Task Cluster is a big open problem: Bug 1247703.
Flags: needinfo?(nalexander)
(In reply to Michael Comella (:mcomella) from comment #6)
> That being said, the patch in comment 5 seems to work (see push in comment
> 3) with two separate tasks that (I think) each do a full build. I'll need to
> check it over for correctness, however, when my brain is refreshed.
> 
> Note to self: update Unit job to stop linting.

This looks pretty great!  I don't have a great way to cut down the copy-paste and boilerplate yet.  Stop linting in Unit and let's start doing this for real.
Review commit: https://reviewboard.mozilla.org/r/42869/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42869/
Attachment #8735054 - Attachment description: MozReview Request: Bug 1254605 - WIP r=na → MozReview Request: Bug 1254605 - Create separate android "lint" task cluster task. r=nalexander
Attachment #8735575 - Flags: review?(nalexander)
Attachment #8735054 - Flags: review?(nalexander)
Comment on attachment 8735054 [details]
MozReview Request: Bug 1254605 - Create separate android "lint" task cluster task. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42537/diff/1-2/
Attachment #8735054 - Flags: review?(nalexander) → review+
Comment on attachment 8735054 [details]
MozReview Request: Bug 1254605 - Create separate android "lint" task cluster task. r=nalexander

https://reviewboard.mozilla.org/r/42537/#review39337

One significant change, but I trust you to get it right :)

::: testing/taskcluster/tasks/branches/base_jobs.yml:121
(Diff revision 2)
>      platforms:
>        - Android
>      types:
>        opt:
>          task: tasks/builds/android_api_15_frontend.yml
> +  android-lint:

Push this down into the `tasks:` sectoin, so you can restrict the set of changed files that it runs against.
Comment on attachment 8735575 [details]
MozReview Request: Bug 1254605 - Don't run lint jobs in Unit task cluster task. r=nalexander

https://reviewboard.mozilla.org/r/42869/#review39339

::: testing/taskcluster/tasks/builds/android_api_15_frontend.yml:70
(Diff revision 1)
>        symbol: Unit
>        tier: 2
>      # Rather then enforcing particular conventions we require that all build
>      # tasks provide the "build" extra field to specify where the build and tests
>      # files are located.
>      locations:

While you're here, consider renaming "Unit" to "test".

We might as well standardize on the Gradle task name, which is "lint" (the "AutomationDebug" is the flavour and configuration combined) in your case, and "test" in this case.  Integration tests would be "androidTest".
Attachment #8735575 - Flags: review?(nalexander) → review+
https://reviewboard.mozilla.org/r/42869/#review39339

> While you're here, consider renaming "Unit" to "test".
> 
> We might as well standardize on the Gradle task name, which is "lint" (the "AutomationDebug" is the flavour and configuration combined) in your case, and "test" in this case.  Integration tests would be "androidTest".

test & androidTest only hint at what their actual purpose is – I'd expect nonAndroidTest/standaloneTest/offDeviceTest & androidTest/onDeviceTest, or something similar. I'll change it for this patch because I think the consistency is a good idea, but I wonder if we should change the gradle task names to make them more explanatory?
Flags: needinfo?(nalexander)
(In reply to Michael Comella (:mcomella) from comment #15)
> https://reviewboard.mozilla.org/r/42869/#review39339
> 
> > While you're here, consider renaming "Unit" to "test".
> > 
> > We might as well standardize on the Gradle task name, which is "lint" (the "AutomationDebug" is the flavour and configuration combined) in your case, and "test" in this case.  Integration tests would be "androidTest".
> 
> test & androidTest only hint at what their actual purpose is – I'd expect
> nonAndroidTest/standaloneTest/offDeviceTest & androidTest/onDeviceTest, or
> something similar. I'll change it for this patch because I think the
> consistency is a good idea, but I wonder if we should change the gradle task
> names to make them more explanatory?

These are Android standard; it's probably not possible to change them, and definitely not a good idea.  That is, they come from the Android Gradle plugin and everybody in the community will expect them.

I'd prefer to keep descriptive TH names and try to make TH tell you what it did (like, I ran |mach gradle ...|) than remap the Gradle names.
Flags: needinfo?(nalexander)
https://hg.mozilla.org/mozilla-central/rev/ac5adb69dc57
https://hg.mozilla.org/mozilla-central/rev/39b92bd04399
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 48 → mozilla48
You need to log in before you can comment on or make changes to this bug.