1302192 - Merge android-test and desktop-test into a "test" kind; base on jobs

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

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

We have two test-related kinds, `android-test` and `desktop-test`.  As things have evolved, I've come to think it's a bad idea to have platform-specific kinds.  While there are differences between the Android and Linux (and Mac and Windows) test suites, those differences are relatively minor and can be overcome with some good design.  In fact, some of that design is already taken care of in the "job description" defined in bug 1286075.

So the task here is to fold these two existing types into a single new type that will generate the same set of tasks.

Comment 1

[Wander Lairson Costa]

Putting this in my pocket, probably will work on it on q4.

Comment 2

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

Wander, is this still on your list?

Comment 3

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

Hopefully you don't mind if I take this back?

Comment 4

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

The tests that are common between Android and Desktop are starting to get long:

https://hg.mozilla.org/users/dmitchell_mozilla.com/mozilla-central/file/7bf59934cf47/taskcluster/ci/test/tests.yml#l40
crashtest:
    description: "Crashtest run"
    suite: reftest/crashtest
    treeherder-symbol: tc-R(C)
    instance-size:
        by-test-platform-phylum:
            android: xlarge
            default: default
    docker-image:
        by-test-platform-phylum:
            android: {'in-tree': 'desktop-test'}
            default: {"in-tree": 'desktop1604-test'}
    chunks:
        by-test-platform:
            android-4.3-arm7-api-15/debug: 10
            android-.*: 4
            default: 1
    e10s:
        by-test-platform-phylum:
            # Bug 1304435
            windows: false
            android: false
            default: both
    mozharness:
        by-test-platform-phylum:
            android:
                script: android_emulator_unittest.py
                no-read-buildbot-config: true
                config:
                    - android/androidarm_4_3.py
                    - remove_executables.py
                    - android/androidarm_4_3-tc.py
                extra-options:
                    - --test-suite=crashtest
            default:
                script: desktop_unittest.py
                chunked: true
                no-read-buildbot-config: true
                config:
                    by-test-platform-phylum:
                        windows:
                            - unittests/win_taskcluster_unittest.py
                        macosx:
                            - remove_executables.py
                            - unittests/mac_unittest.py
                        linux:
                            - unittests/linux_unittest.py
                            - remove_executables.py
                extra-options:
                    - --reftest-suite=crashtest

Joel, wander, do you think there's a better way to represent this?  They do, in fact, differ in a number of characteristics.

Comment 5

[Wander Lairson Costa]

Hrm, I don't think we can do much better than treating Android as another platform and splitting paths where things diverge.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

I think there is a lot we can cleanup in what is above:
the remove_executables.py should be on windows and we should make it part of the *unittest.py script- it is too bad we have to split that config data out by platform.

moving up, we specify android, that has a remove_executables.py as well- again, merge this in.  Maybe if we have android_unittest.py as a name, we could make it all the same, then find a way to support: [platform]_unittest.py.

The above to recommendations are to cleanup our filenames and options for mozharness.

we don't do e10s at all on android, so could that be a transform?  That is one small simplification, but every line helps here.

Outside of that, I think we end up with something similar to what you have outlined.

Comment 7

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

Thanks for the feedback!  I think we can use this as the base for further improvements -- it will, at least, make the redundancies pretty clear from inspection of tests.yml.  I'll keep going then!

Comment 8

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

I created bug 1325701 to deal with creating job descriptions instead of test descriptions.

Comment 9

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

Attachment: Bug 1302192: update android test suites to match desktop;

From grepping the source, the suite and flavor seem only used to set
attributes, which are only used in a few target task methods.  Notably, the try
option syntax target task method does *not* use these attributes, and those
methods which do use the attributes are not looking at these particular suites.
So, I think these changes are completely harmless.

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

Comment 10

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

Attachment: Bug 1302192: ensure that only one option matches by-*;

If an item matches exactly, it is always selected.  This will catch the case
where multiple regular expressions match (the previous version would silently
pick one based on dictionary order).

I was worried this was occurring, but because exact matches items are selected,
it is not; still, best to be sure.

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

Comment 11

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

Attachment: Bug 1302192: (DO NOT LAND) graph-diffing script

This was necessary since I was changing labels -- without it, the diffs were
meaningless because the order of the tasks changed.

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

Comment 12

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

Attachment: Bug 1302192: combine desktop_test and android_test transforms;

This combines all of the transforms into a single file, `tests.py`.

It's not quite copy/paste: the transforms have been combined in several cases.
The generated taskgraph does not change with this commit, though.

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

Comment 13

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

Attachment: Bug 1302192: combine the android-test and desktop-test kinds into the test kind

This makes heavy use of by-test-platform to distinguish the various platforms, since Android
mozharness invocations differ substantially from desktop.

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

Comment 14

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

Attachment: Bug 1302192: require headings to begin a line; +583205

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

Comment 15

[HAMMAD AKHTAR]

Comment on attachment 8821661 [details]
Bug 1302192: require headings to begin a line; +583205

Nice ! You are enforcing that heading begins on a new line.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8821656 [details]
Bug 1302192: update android test suites to match desktop;

https://reviewboard.mozilla.org/r/100856/#review101480

::: taskcluster/ci/android-test/tests.yml:144
(Diff revision 1)
>          extra-options:
>              - --test-suite=mochitest-chrome
>  
>  mochitest-clipboard:
>      description: "Mochitest clipboard run"
> -    suite: mochitest/plain-clipboard
> +    suite: mochitest/plain-clipboard,chrome-clipboard,browser-chrome-clipboard,jetpack-package-clipboard

this won't work because these test suites do not run on Android

::: taskcluster/ci/android-test/tests.yml:161
(Diff revision 1)
>          extra-options:
>              - --test-suite=mochitest-plain-clipboard
>  
>  mochitest-gpu:
>      description: "Mochitest gpu run"
> -    suite: mochitest/plain-gpu
> +    suite: mochitest/plain-gpu,chrome-gpu,browser-chrome-gpu

same here, these addtional test suites to not run on android.  In this case there are no tests to run and that causes problems.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8821660 [details]
Bug 1302192: combine the android-test and desktop-test kinds into the test kind

https://reviewboard.mozilla.org/r/100864/#review101482

The cleanup of windows things are not the r-, but that would make things cleaner- it will be done in due time when we land windows config changes :)

The concern I have about the missing .* on macosx and linux is the reason for the r-

::: taskcluster/ci/test/tests.yml:59
(Diff revision 1)
> +            android-4.3-arm7-api-15/debug: 10
> +            android.*: 4
> +            default: 1
>      e10s:
>          by-test-platform:
> -            # Bug 1304435
> +            windows.*: false # Bug 1304435

actually now this runs fine so the need for the windows.* line (and related comment) can be removed.  We just haven't landed config changes yet

::: taskcluster/ci/test/tests.yml:202
(Diff revision 1)
> +            windows.*: 1
>              default: 2
>      e10s:
>          by-test-platform:
>              # Bug 1321782
> -            win.*: false
> +            windows.*: false

same as crashtests, windows e10s works now:)

::: taskcluster/ci/test/tests.yml:1182
(Diff revision 1)
> -        no-read-buildbot-config: true
> +                no-read-buildbot-config: true
> -        config:
> +                config:
> -            by-test-platform:
> +                    by-test-platform:
> -                win.*:
> +                        windows.*:
> -                    - unittests/win_taskcluster_unittest.py
> +                            - unittests/win_taskcluster_unittest.py
> -                macosx.*:
> +                        macosx:

how come we removed the .* here?  I see in previous edits we have macosx.* and linux.*

Comment 18

[Wander Lairson Costa]

Comment on attachment 8821657 [details]
Bug 1302192: ensure that only one option matches by-*;

https://reviewboard.mozilla.org/r/100858/#review101490

Comment 19

[Wander Lairson Costa]

Comment on attachment 8821659 [details]
Bug 1302192: combine desktop_test and android_test transforms;

https://reviewboard.mozilla.org/r/100862/#review101530

Comment 20

[Wander Lairson Costa]

Comment on attachment 8821660 [details]
Bug 1302192: combine the android-test and desktop-test kinds into the test kind

https://reviewboard.mozilla.org/r/100864/#review101532

Comment 21

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

Comment on attachment 8821656 [details]
Bug 1302192: update android test suites to match desktop;

https://reviewboard.mozilla.org/r/100856/#review101480

> same here, these addtional test suites to not run on android.  In this case there are no tests to run and that causes problems.

Looking at the generated task, the value `plain-gpu,chrome-gpu,browser-chrome-gpu` only appears in `attributes.flavor` and `task.task.extra.suite.flavor`, neither of which are available to mozharness.  Nothing that I can find splits on `,` in that string, either.  Am I missing something that would cause these runs to actually fail?

That said, it sounds like this is an inaccurate description of what the test suite does.  As it seems this flavor value isn't actually used anywhere, I think we're fairly free to modify it.  Maybe we should change this case and the previous one you identified to

    suite: mochitest/clipboard

and

    suite: mochitest/gpu
    
What do you think?  The other option is to make it `by-test-platform`, but that seems like overkill for something that is never used.  I suppose the third option is to just delete the suite/flavor :)

Comment 22

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

Comment on attachment 8821660 [details]
Bug 1302192: combine the android-test and desktop-test kinds into the test kind

https://reviewboard.mozilla.org/r/100864/#review101482

> how come we removed the .* here?  I see in previous edits we have macosx.* and linux.*

Yikes!  I had originally switched everything to `by-test-platform-phylum` which was just `linux`, `windows`, etc., then decided that was overkill.  Apparently I missed a thing or two converting back.  I'll grep and fix any omissions I find.  Thanks1

Comment 23

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

Comment on attachment 8821660 [details]
Bug 1302192: combine the android-test and desktop-test kinds into the test kind

https://reviewboard.mozilla.org/r/100864/#review101650

::: taskcluster/ci/test/tests.yml:59
(Diff revision 1)
> +            android-4.3-arm7-api-15/debug: 10
> +            android.*: 4
> +            default: 1
>      e10s:
>          by-test-platform:
> -            # Bug 1304435
> +            windows.*: false # Bug 1304435

I would rather leave this and similar to other bugs - this bug is changing enough already!

Comment 24

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

Comment on attachment 8821656 [details]
Bug 1302192: update android test suites to match desktop;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100856/diff/1-2/

Comment 25

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

Comment on attachment 8821657 [details]
Bug 1302192: ensure that only one option matches by-*;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100858/diff/1-2/

Comment 26

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

Comment on attachment 8821658 [details]
Bug 1302192: (DO NOT LAND) graph-diffing script

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100860/diff/1-2/

Comment 27

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

Comment on attachment 8821659 [details]
Bug 1302192: combine desktop_test and android_test transforms;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100862/diff/1-2/

Comment 28

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

Comment on attachment 8821660 [details]
Bug 1302192: combine the android-test and desktop-test kinds into the test kind

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100864/diff/1-2/

Comment 29

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

Comment on attachment 8821661 [details]
Bug 1302192: require headings to begin a line; +583205

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100866/diff/1-2/

Comment 30

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

Comment on attachment 8821661 [details]
Bug 1302192: require headings to begin a line; +583205

Sorry, hammad.  Mozreview got confused.  The patch hasn't changed, so no need to re-review.

Comment 31

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8821656 [details]
Bug 1302192: update android test suites to match desktop;

https://reviewboard.mozilla.org/r/100856/#review101656

this looks good- keep in mind the mochitest suite options need to be different for android than that of desktop- we don't run certain test types on android and if a suite has zero tests to run it will throw an error.

Comment 32

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

Comment on attachment 8821656 [details]
Bug 1302192: update android test suites to match desktop;

https://reviewboard.mozilla.org/r/100856/#review101656

Thanks, yeah, I added a comment to that effect.

Comment 33

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8821660 [details]
Bug 1302192: combine the android-test and desktop-test kinds into the test kind

https://reviewboard.mozilla.org/r/100864/#review101660

::: taskcluster/ci/test/tests.yml:25
(Diff revision 2)
> +                    - remove_executables.py
> +                    - android/androidarm_4_3-tc.py
> +                extra-options:
> +                    - --test-suite=cppunittest
> +            default:
> -        script: desktop_unittest.py
> +                script: desktop_unittest.py

one other thought here is that we could put the script by-test-platform.

Comment 34

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

Comment on attachment 8821660 [details]
Bug 1302192: combine the android-test and desktop-test kinds into the test kind

https://reviewboard.mozilla.org/r/100864/#review101660

> one other thought here is that we could put the script by-test-platform.

We'd need to do the same with `extra-options`.  That starts to get a bit verbose!  I think there's ample room to clean all of this up with changes to how mozharness runs, later.

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

good point- if mozharness had the same parameters for android/desktop, then we could simplify this greatly- that seems like a worthwhile use of time as we could simplify our configs/debugging more than just incremental.

Comment 36

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6ea04fae66b1fbf1464fa06699470adc092f575
Bug 1302192: update android test suites to match desktop; r=jmaher

https://hg.mozilla.org/integration/mozilla-inbound/rev/61bcf47fea4c496d1fc10f7f8a2b357ca825c343
Bug 1302192: ensure that only one option matches by-*; r=wcosta

https://hg.mozilla.org/integration/mozilla-inbound/rev/124465dfe2c4b1371cc001f1c8370589e69beb29
Bug 1302192: combine desktop_test and android_test transforms; r=wcosta

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb5360c95a9e156a79f2c4a934c609b93e43713
Bug 1302192: combine the android-test and desktop-test kinds into the test kind; r=wcosta r=jmaher

https://hg.mozilla.org/integration/mozilla-inbound/rev/13267e455f0fbfeb3d9e78d1b12b6a553068ae22
Bug 1302192: require headings to begin a line; r=hammad13060+583205

Comment 37

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed7eb453f864e43f8ec5ff7c557f1c8dba288e3
Bug 1302192: fix python lint; a=me

Comment 38

[Wander Lairson Costa]

Attachment: Set macosx engine tests Tier 3. r=dustin

macosx-engine is not stable yet, let's hide it in TH.

Comment 39

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

Comment on attachment 8822204 [details] [diff] [review]
Set macosx engine tests Tier 3. r=dustin

Review of attachment 8822204 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 40

[HAMMAD AKHTAR]

Comment on attachment 8821661 [details]
Bug 1302192: require headings to begin a line; +583205

https://reviewboard.mozilla.org/r/100866/#review101458

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f6ea04fae66b
https://hg.mozilla.org/mozilla-central/rev/61bcf47fea4c
https://hg.mozilla.org/mozilla-central/rev/124465dfe2c4
https://hg.mozilla.org/mozilla-central/rev/dfb5360c95a9
https://hg.mozilla.org/mozilla-central/rev/13267e455f0f
https://hg.mozilla.org/mozilla-central/rev/8ed7eb453f86