New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade Safari stable runs to use macOS Mojave #18927
Conversation
426f5b7
to
fe1196f
Compare
This did not fail due to safaridriver, so my guess is that @burg is it a known issue with |
https://wpt.fyi/results/?diff&filter=ADC&run_id=285710001&run_id=277450003 shows the diff of two full runs. It includes some noise because of unrelated changes between 820f0f8 and 426f5b7, but the important thing is that lots of tests will be regressed because of https://bugs.webkit.org/show_bug.cgi?id=174030. So... I think I'd suggest not upgrading Safari until STP is upgraded and not affected by that bug, or if we have to upgrade because support for High Sierra going away. |
Well, we can't upgrade Safari stable until it reaches Safari stable, no, which will be even longer? Those test runs are Safari stable after all. |
Yeah, we should leave Safari stable on High Sierra for as long we can I think. |
As such, closing this. Need to take care to not upgrade stable when doing STP and all infra test jobs. |
fe1196f
to
924578b
Compare
Trying a full run in https://dev.azure.com/web-platform-tests/wpt/_build/results?buildId=36869. |
@gsnedders @youennf do you think we could work around the |
Would it be possible to mimic what https://trac.webkit.org/changeset/249295/webkit does by first waiting for the first layout (using rAF?) before we access @fred-wang who reviewed that might have ideas. |
@foolip So you are saying that fixing bug 174030 broke the tests? Or that we need to wait for the fix to arrive in STP? Before bug 174030, what I had used for MathML tests using web fonts was something like this https://bugs.webkit.org/show_bug.cgi?id=174030#c15 |
@fred-wang fixing https://bugs.webkit.org/show_bug.cgi?id=174030 (which has reached STP) made it possible to upgrade to 10.14 for STP. The fix hasn't reached Safari stable, however, and so we've been stuck on 10.13 there. https://wpt.fyi/results/?q=is%3Adifferent&run_id=364400009&run_id=372060001 is the diff of the latest full run. The number of differences are actually not that many, so I wonder if something has changed... |
Oh, I see. |
https://opensource.apple.com/source/WebCore/WebCore-7607.2.6.1.1/css/FontFaceSet.cpp.auto.html still doesn't appear to have the patch as of 10.14.5, so potentially just timing causing it to appear better? It is fundamentally a race currently, and if it normally loads from cache aside from the first test in each chunk then we could see it not looking too bad? |
rAF is only paint, though, right? We could force layout (through CSSOM), but that might hide other bugs… |
Potentially hiding bugs is fine, what I'd be looking for is a hack that makes Safari stable results on 10.13 and 10.14 very similar, and then when Safari 13.1 is released the hack would be removed. |
924578b
to
ab21d0b
Compare
I've tried just forcing layout and am doing a trial run in https://dev.azure.com/web-platform-tests/wpt/_build/results?buildId=36959 |
https://wpt.fyi/results/?q=is%3Adifferent&run_id=378110002&run_id=353030001 is a diff of results with a I'm not sure why it is that the |
ab21d0b
to
a0c1beb
Compare
I think this'll work now. I've triggered a full run in https://dev.azure.com/web-platform-tests/wpt/_build/results?buildId=37353 to compare against https://wpt.fyi/results/?run_id=359060007 which is Safari stable on 10.13 with the parent commit of this PR. |
I'm still reluctant to land this until we can be sure we aren't going to get lots of flakiness from web font layout. |
@gsnedders https://wpt.fyi/results/?diff&filter=ADC&q=is%3Adifferent&run_id=359060007&run_id=366850030 are the two latest runs from master. Would two runs on 10.14 showing a similar number of differences suffice? Or 3, 4, ...? Pick any number and I will trigger. https://wpt.fyi/results/?q=is%3Adifferent&run_id=359060007&run_id=362890031 is the 10.13→10.14 diff BTW. |
Is this with the hack mentioned above applied? The differences in WebCryptoAPI are interesting. Similarly in encrypted-media/idlharness.https.html. Would we expect such differences from just a different OS...? |
No, a0c1beba45 didn't have the hack applied, it was just changing the OS version. Differences in crypto algorithms supported and MSE support based on the OS seem pretty plausible, but if you suspect it's bogus it'd be best to loop in someone working on Safari. |
@foolip I'm willing to defer to those on the WebKit team if they think that this isn't going to introduce new flakiness into the results, and think that moving to Mojave is beneficial. |
Installing Ahem as a system font no longer works on Mojave, so drop the install step. This will probably cause a handful of regressions.
a0c1beb
to
36f3511
Compare
I've rebased and triggered https://dev.azure.com/web-platform-tests/wpt/_build/results?buildId=38576 for a more up-to-date diff to make the decision against. @smfr @youennf do either of you have input on this, is there anything in particular we should check in the results when doing this upgrade? Any other reason to stay behind on macOS 10.13? |
There are certainly web-exposed features that are only enabled on newer OSes (CSS conic gradients is one) so in general I would expect Safari to pass more tests on a newer OS. |
https://wpt.fyi/results/?diff&filter=ADC&run_id=384420080&run_id=374750086 is the most recent diff of results, and it's still an overall improvement. @stephenmcgruer for review? |
@smfr you're of course also welcome to review if you like YAML syntax :) |
Installing Ahem a system font no longer works on Mojave, so drop the
install step. This will probably cause a handful of regressions.