Closed Bug 1302192 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla53

People

(Reporter: dustin, Assigned: dustin)

References

Details

Attachments

(7 files)

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.
Putting this in my pocket, probably will work on it on q4.
Assignee: nobody → wcosta
Status: NEW → ASSIGNED
Wander, is this still on your list?
Hopefully you don't mind if I take this back?
Assignee: wcosta → dustin
Depends on: 1325465
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.
Flags: needinfo?(wcosta)
Flags: needinfo?(jmaher)
Hrm, I don't think we can do much better than treating Android as another platform and splitting paths where things diverge.
Flags: needinfo?(wcosta)
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.
Flags: needinfo?(jmaher)
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!
I created bug 1325701 to deal with creating job descriptions instead of test descriptions.
Attachment #8821661 - Flags: review?(hammad13060)
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.
Attachment #8821661 - Flags: review?(hammad13060) → review+
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.
Attachment #8821656 - Flags: review?(jmaher) → review-
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.*
Attachment #8821660 - Flags: review?(jmaher) → review-
Comment on attachment 8821657 [details]
Bug 1302192: ensure that only one option matches by-*;

https://reviewboard.mozilla.org/r/100858/#review101490
Attachment #8821657 - Flags: review?(wcosta) → review+
Comment on attachment 8821659 [details]
Bug 1302192: combine desktop_test and android_test transforms;

https://reviewboard.mozilla.org/r/100862/#review101530
Attachment #8821659 - Flags: review?(wcosta) → review+
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
Attachment #8821660 - Flags: review?(wcosta) → review+
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 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 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 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.
Attachment #8821661 - Flags: review?(hammad13060) → review+
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.
Attachment #8821656 - Flags: review?(jmaher) → review+
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 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.
Attachment #8821660 - Flags: review?(jmaher) → review+
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.
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.
macosx-engine is not stable yet, let's hide it in TH.
Attachment #8822204 - Flags: review?(dustin)
Comment on attachment 8822204 [details] [diff] [review]
Set macosx engine tests Tier 3. r=dustin

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

Looks good!
Attachment #8822204 - Flags: review?(dustin) → review+
Keywords: checkin-needed
Comment on attachment 8821661 [details]
Bug 1302192: require headings to begin a line; +583205

https://reviewboard.mozilla.org/r/100866/#review101458
Attachment #8821661 - Flags: review+
Depends on: 1326232
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: