Closed Bug 1410250 Opened 6 years ago Closed 6 years ago

Further simplify the 'test' kind configs

Categories

(Firefox Build System :: Task Configuration, task)

Product:
Firefox Build System
Component:
Task Configuration
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla58

People

(Reporter: ahal, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1410250 - Use transform for appending --mochitest-suite=<suite> et al,
59 bytes, text/x-review-board-request
dustin
: review+
Details
Bug 1410250 - Stop keying entire 'mozharness' section by-test-platform,
59 bytes, text/x-review-board-request
dustin
: review+
Details 
Bug 1388478 split up tests.yml into several smaller files, but we can still do better.

The motivation here is to make fixing bug 1391130 less painful. For context, that bug is going to define a substantial amount of test harness arguments in taskcluster configs instead of mozharness configs. The way many of the test kinds are set up now means these new arguments would get duplicated for every task, once again ballooning up the size of these files.

The main problem is that we often use 'by-test-platform' to key the entire mozharness dict. This is bad because:

A) It means shared mozharness configs between platforms are duplicated
B) We can't use 'job-defaults' for anything under the 'mozharness' section since the merging is naive.

This bug is going to disallow keying the entire mozharness section, and instead require tasks to key individual keys under this section. This will result in substantially cleaner configs, and make fixing bug 1391130 much simpler.
Comment on attachment 8920389 [details]
Bug 1410250 - Use transform for appending --mochitest-suite=<suite> et al,

https://reviewboard.mozilla.org/r/191358/#review196530

::: taskcluster/ci/test/xpcshell.yml:5
(Diff revision 1)
>  xpcshell:
>      description: "xpcshell test run"
>      suite:
>          by-test-platform:
> -            linux64-jsdcov/opt: xpcshell-coverage
> +            linux64-jsdcov/opt: xpcshell/xpcshell-coverage

This looks like a risky patch, but I've tested this by diffing the entire JSON taskgraph with and without this patch.

This change yields the only difference between the two graphs. I talked to the code coverage people (:gmierz, :ekyle, :jmaher) and they agree it looks safe to make. I also have a fairly comprehensive try run in the works that should alert us to issues this might cause.
Comment on attachment 8920390 [details]
Bug 1410250 - Stop keying entire 'mozharness' section by-test-platform,

https://reviewboard.mozilla.org/r/191360/#review196532

Like the first commit, this was also tested by diffing taskgraphs. Nothing has changed between the two save the order in which mozharness configs get specified for certain tasks (which doesn't matter).
Comment on attachment 8920389 [details]
Bug 1410250 - Use transform for appending --mochitest-suite=<suite> et al,

https://reviewboard.mozilla.org/r/191358/#review196536

::: commit-message-31af3:8
(Diff revision 1)
> +Every task that uses the desktop_unittest.py or android_emulator_unittest.py
> +mozharness scripts needs to pass in either --<suite>-suite=<flavor>, or
> +--test-suite=<flavor> respectively.
> +
> +In almost all cases, <suite> and <flavor> are identical to the value that is
> +already specified under the test['suite'] key. This means we can use a

'cuz that's where I copied it from way back when ;)

::: taskcluster/taskgraph/transforms/tests.py:647
(Diff revision 1)
> +        if suite == 'test-verification':
> +            pass
> +        elif script == 'android_emulator_unittest.py':
> +            category_arg = '--test-suite'
> +        elif script == 'desktop_unittest.py':
> +            category_arg = '--{}-suite'.format(suite)

Is this special-casing sometihng you see growing over time, or going away?
Attachment #8920389 - Flags: review?(dustin) → review+
Comment on attachment 8920390 [details]
Bug 1410250 - Stop keying entire 'mozharness' section by-test-platform,

https://reviewboard.mozilla.org/r/191360/#review196540

Diffing is really a zen way to do this kind of refactor, isn't it?
Attachment #8920390 - Flags: review?(dustin) → review+
Comment on attachment 8920389 [details]
Bug 1410250 - Use transform for appending --mochitest-suite=<suite> et al,

https://reviewboard.mozilla.org/r/191358/#review196536

> Is this special-casing sometihng you see growing over time, or going away?

Definitely going away, as I see mozharness in general going away :)

I did look into making these two config options consistent across mozharness scripts, but it would have been way more complicated than it was worth.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c522ce0d378
Use transform for appending --mochitest-suite=<suite> et al, r=dustin
https://hg.mozilla.org/integration/autoland/rev/3829051139a8
Stop keying entire 'mozharness' section by-test-platform, r=dustin
https://hg.mozilla.org/mozilla-central/rev/7c522ce0d378
https://hg.mozilla.org/mozilla-central/rev/3829051139a8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: TaskCluster → Firefox Build System
Depends on: 1442783
You need to log in before you can comment on or make changes to this bug.