1410250 - Further simplify the 'test' kind configs

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Andrew Halberstadt [:ahal]]

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 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1410250 - Use transform for appending --mochitest-suite=<suite> et al,

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
transform to append these command line arguments and reduce the complexity of
the yaml files.

Review commit: https://reviewboard.mozilla.org/r/191358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191358/

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1410250 - Stop keying entire 'mozharness' section by-test-platform,

Many of the 'test' tasks key the entire 'mozharness' section by-test-platform.
This is bad because:

A) Configuration under 'mozharness' can't be shared across platforms
B) We can't use the 'job-defaults' simplification since merging is naive

Instead of keying the entire 'mozharness' section, this change keys only the
specific configuration that needs it.

Review commit: https://reviewboard.mozilla.org/r/191360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191360/

Comment 3

[Andrew Halberstadt [:ahal]]

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 4

[Andrew Halberstadt [:ahal]]

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 5

[Dustin J. Mitchell [:dustin] (he/him)]

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?

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

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?

Comment 7

[Andrew Halberstadt [:ahal]]

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.

Comment 8

[Andrew Halberstadt [:ahal]]

Try run looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcf613dbe43428ebe8756670dcb12b23c4da03a0

The busted Cpp/Gtest/Jittest is because I accidentally used artifact builds. Here's a push for those:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7cae8a8f21625ea571357026d4bbb5029e15f10

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7c522ce0d378
https://hg.mozilla.org/mozilla-central/rev/3829051139a8