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)
Firefox Build System
Task Configuration
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla53
People
(Reporter: dustin, Assigned: dustin)
References
Details
Attachments
(7 files)
59 bytes,
text/x-review-board-request
|
jmaher
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
wcosta
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Details | |
59 bytes,
text/x-review-board-request
|
wcosta
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jmaher
:
review+
wcosta
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
dustin
:
review+
hammad13060
:
review+
|
Details |
1.20 KB,
patch
|
dustin
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
Putting this in my pocket, probably will work on it on q4.
Assignee: nobody → wcosta
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
Wander, is this still on your list?
Assignee | ||
Comment 3•8 years ago
|
||
Hopefully you don't mind if I take this back?
Assignee: wcosta → dustin
Assignee | ||
Comment 4•7 years ago
|
||
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)
Comment 5•7 years ago
|
||
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)
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
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!
Assignee | ||
Comment 8•7 years ago
|
||
I created bug 1325701 to deal with creating job descriptions instead of test descriptions.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8821661 -
Flags: review?(hammad13060)
Comment 15•7 years ago
|
||
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 16•7 years ago
|
||
mozreview-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 17•7 years ago
|
||
mozreview-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 18•7 years ago
|
||
mozreview-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 19•7 years ago
|
||
mozreview-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 20•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 21•7 years ago
|
||
mozreview-review-reply |
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 :)
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 23•7 years ago
|
||
mozreview-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/#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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
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 31•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 32•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
mozreview-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 ::: 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+
Assignee | ||
Comment 34•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
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.
Assignee | ||
Comment 36•7 years ago
|
||
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
Assignee | ||
Comment 37•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed7eb453f864e43f8ec5ff7c557f1c8dba288e3 Bug 1302192: fix python lint; a=me
Comment 38•7 years ago
|
||
macosx-engine is not stable yet, let's hide it in TH.
Updated•7 years ago
|
Attachment #8822204 -
Flags: review?(dustin)
Assignee | ||
Comment 39•7 years ago
|
||
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+
Updated•7 years ago
|
Keywords: checkin-needed
Comment 40•7 years ago
|
||
mozreview-review |
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+
Comment 41•7 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•6 years ago
|
Product: TaskCluster → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•