Closed
Bug 1380053
Opened 7 years ago
Closed 7 years ago
stylo: Enable Stylo tests on Mac
Categories
(Core :: CSS Parsing and Computation, enhancement, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: cpeterson, Assigned: jryans)
References
Details
Attachments
(3 files)
Now that we have Stylo support in Nightly builds for Linux64, Mac, and Windows, we should run our Stylo tests on those platforms. We should run all the same test configurations as we do for linux64-stylo on inbound, autoland, and central: https://docs.google.com/spreadsheets/d/1HmBsxrEpFfCgpLBG5Mvj5wEA9oEq5N8m4cxQYsSo4S8/edit#gid=1737640065 We don't need to run the Mac and Windows tests in Stylo sequential mode, just parallel mode. Because we don't have custom Stylo-enabled builds for Mac, Windows, or Linux32 like we do for linux64-stylo, this bug depends on bug 1374748 to enable automation to run Stylo tests using regular builds and the STYLO_FORCE_ENABLED=1 environment variable.
Assignee | ||
Comment 1•7 years ago
|
||
Here's a first attempt to see how green things are running the regular desktop test configurations, but with Stylo parallel forced on: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaf2a5c4c116336d4c632ec6c94390a78b4b4d8c
Assignee | ||
Comment 2•7 years ago
|
||
New attempt after landing bug 1380082 so that test annotations are respected: https://treeherder.mozilla.org/#/jobs?repo=try&revision=94b3ff1cbae742e73cb0f6537dd8dc0d40bc32ad
Assignee | ||
Comment 3•7 years ago
|
||
Previous attempt was too noisy with too many extra test jobs we don't yet want for Stylo. Here's a more focused approach with same test set we're using on Linux: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7213937697abb7f2b7f807afbb31b08488acda5
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
Looks like we got valid results from the Mac run, but for Windows that build crashed quite early when Stylo is used. The latest central build appears to work for Stylo on Windows, so I'll rebase and try again. This run is Windows only, since try is closed for Mac currently: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f096a3bd8433501a75c62f210ba3ce4f003620a4
Comment 5•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4) > Looks like we got valid results from the Mac run, but for Windows that build > crashed quite early when Stylo is used. Perhaps that was bug 1382019?
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4) > > Looks like we got valid results from the Mac run, but for Windows that build > > crashed quite early when Stylo is used. > > Perhaps that was bug 1382019? Right, that was my primary suspect. I didn't realize until just now that that crash was Windows only, but since it appears to be, it seems like the right bug here.
Depends on: 1382019
Assignee | ||
Comment 7•7 years ago
|
||
Okay, making progress. Windows 64 is returning good data, but something's still busted for Windows 32, so I'll check that soon. I also noticed WPT is failing for these new platforms because the annotations are Linux specific. Filed bug 1382322 to clean that up.
Assignee | ||
Comment 8•7 years ago
|
||
Windows 32-bit is hitting a new crash (bug 1382190), so I'll ignore it in the testing runs for now.
Depends on: 1382190
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=723dfa95bd41e7217ed0ed333d52a6e25801f839
Assignee | ||
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74dab813af8b25642919c747cd0cef9b3f25500e
Assignee | ||
Comment 11•7 years ago
|
||
This bug is focused on unit / integration tests only, so it will ignore Talos for the moment. See related bug 1383789 about expanding Talos coverage.
See Also: → 1383789
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=688105b261aa301c6c26268a05360b951be0ad2e
Assignee | ||
Comment 15•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c23ad7ebfd28fc5870445d36b706391c681d89be
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8889537 [details] Bug 1380053 - Stylo reftest annotations for all OSes. https://reviewboard.mozilla.org/r/160558/#review166476
Attachment #8889537 -
Flags: review?(manishearth) → review+
Assignee | ||
Comment 20•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6813000f2a777de4ed8d410522ef9dc4bf8f6de5
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8889538 [details] Bug 1380053 - Add Stylo tests for other desktop platforms. https://reviewboard.mozilla.org/r/160560/#review166522 ::: taskcluster/taskgraph/transforms/tests.py:495 (Diff revision 2) > + # Ignore the table above for stylo. Otherwise we'd have conflicts for > + # macOS stylo vs. regular test platforms. > + if '-stylo' in test['test-platform']: > + test['treeherder-machine-platform'] = test['test-platform'] > + yield test > + continue it took a long time to understand this chunk. it looks like in general we're using the build platform's translation for treeherder, except for stylo where we're using the test-platform? also, the yield/continue could be more clearly expressed as yield/else: ?
Attachment #8889538 -
Flags: review?(catlee) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8890073 [details] Bug 1380053 - Default to parallel Stylo traversal in tests. https://reviewboard.mozilla.org/r/161140/#review166524 ::: taskcluster/taskgraph/transforms/tests.py:780 (Diff revision 1) > if not e10s: > yield test > continue > > # Bug 1356122 - Run Stylo tests in sequential mode > if test['test-platform'].startswith('linux64-stylo-sequential/'): this if condition is redundant I think?
Attachment #8890073 -
Flags: review?(catlee) → review+
Assignee | ||
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8890073 [details] Bug 1380053 - Default to parallel Stylo traversal in tests. https://reviewboard.mozilla.org/r/161140/#review166876 ::: taskcluster/taskgraph/transforms/tests.py:772 (Diff revision 1) > - if (not test['test-platform'].startswith('linux64-stylo/')) and \ > + if not test['test-platform'].startswith('linux64-stylo-sequential/'): > - (not test['test-platform'].startswith('linux64-stylo-sequential/')): > yield test > continue > > e10s = test['e10s'] Just realized that this `e10s` bit here is just causing trouble. It seems like some confusion between plans in bug 1318187 vs. bug 1356122 a few weeks later. Originally e10s status was used to decide parallel vs. seq, but then later a separate test platform was added. So the current state without this patch is that e10s-off runs on linux64-stylo are actually sequential by accident. I'm going to remove the e10s parts here.
Assignee | ||
Comment 24•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8890073 [details] Bug 1380053 - Default to parallel Stylo traversal in tests. https://reviewboard.mozilla.org/r/161140/#review166524 > this if condition is redundant I think? Right, I'll remove it, thanks!
Assignee | ||
Comment 25•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8889538 [details] Bug 1380053 - Add Stylo tests for other desktop platforms. https://reviewboard.mozilla.org/r/160560/#review166522 > it took a long time to understand this chunk. > > it looks like in general we're using the build platform's translation for treeherder, except for stylo where we're using the test-platform? > > also, the yield/continue could be more clearly expressed as yield/else: ? Well, the default in many cases is actually the test platform, unless the build platform appears in the table: ``` translation.get(test['build-platform'], test['test-platform']) ``` For Windows and Linux, only the asan and pgo build platforms are in the table. So all "regular" builds for Windows and Linux aren't in the table either, and so use the test platform. For macOS, the regular build platforms _are_ in the table, so it causes which means the new stylo test runs on macOS would get the same value for 'treeherder-machine-platform' as regular runs without this patch. This triggers an error in `verify_task_graph_symbol` which wants the machine platform to be unique. I don't know the history of why the regular macOS builds are in this table, but other desktop platforms are not that way, so I didn't feel safe to just remove the two macOS lines. I'll update my comment to explain in more detail and change to yield / else.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09d9e25a5b9d8faf2882faaf2ad51306f76fe311 Depending on how this next run looks, I might try to enable macOS first and then come back to Windows in a separate step, since there are still a few crashing issues with Windows.
Assignee | ||
Comment 30•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9bcfaedadd828aff619ef3953dfd37a1e880922
Assignee | ||
Comment 31•7 years ago
|
||
Okay, I'm going to slim this one down to enable just what's working now, which is macOS. See bug 1385025 for Linux32 and bug 1385027 for Windows. We also agreed it's okay to disable the failing Linux64 debug parallel e10s-off config for now. Bug 1385021 filed to re-enable.
Assignee | ||
Comment 32•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c35afee7f9b979341c69ea9916ae582baef0d924
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 36•7 years ago
|
||
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/951aac3f7728 Stylo reftest annotations for all OSes. r=manishearth https://hg.mozilla.org/integration/autoland/rev/8f1268aef844 Default to parallel Stylo traversal in tests. r=catlee https://hg.mozilla.org/integration/autoland/rev/30fb5403e45c Add Stylo tests for other desktop platforms. r=catlee
Comment 37•7 years ago
|
||
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d2ae1aa2d341 Skip ua-style-sheet-input-number-1.html for Stylo. r=me
Comment 38•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/951aac3f7728 https://hg.mozilla.org/mozilla-central/rev/8f1268aef844 https://hg.mozilla.org/mozilla-central/rev/30fb5403e45c https://hg.mozilla.org/mozilla-central/rev/d2ae1aa2d341
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•