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)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: cpeterson, Assigned: jryans)

References

Details

Attachments

(3 files)

Bug 1380053 - Stylo reftest annotations for all OSes.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1380053 - Add Stylo tests for other desktop platforms.
59 bytes, text/x-review-board-request
catlee
: review+
Details
Bug 1380053 - Default to parallel Stylo traversal in tests.
59 bytes, text/x-review-board-request
catlee
: review+
Details 
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.
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
Blocks: 1380086
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: nobody → jryans
Status: NEW → ASSIGNED
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
(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?
(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
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.
Windows 32-bit is hitting a new crash (bug 1382190), so I'll ignore it in the testing runs for now.
Depends on: 1382190
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
Blocks: 1383845
No longer depends on: 1383845
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+
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 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+
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.
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!
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.
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.
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.
No longer depends on: 1366050, 1384701, 1384724
Summary: stylo: Enable Stylo tests on all desktop platforms: Linux32, Linux64, Win32, Win64, Mac → stylo: Enable Stylo tests on Mac
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
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: