Skip to content
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

Merged
merged 1 commit into from Dec 12, 2019
Merged

Conversation

@foolip
Copy link
Member

foolip commented Sep 9, 2019

Installing Ahem a system font no longer works on Mojave, so drop the
install step. This will probably cause a handful of regressions.

@foolip
Copy link
Member Author

foolip commented Sep 9, 2019

@wpt-pr-bot wpt-pr-bot added the infra label Sep 9, 2019
@wpt-pr-bot wpt-pr-bot requested a review from jgraham Sep 9, 2019
@foolip foolip force-pushed the foolip/safari-stable-mojave branch from 426f5b7 to fe1196f Sep 9, 2019
@foolip
Copy link
Member Author

foolip commented Sep 9, 2019

This did not fail due to safaridriver, so my guess is that safardriver --enable for STP has regressed or maybe never worked on Mojave.

@burg is it a known issue with safaridriver --enable? Would getting the verbose safaridriver logs for this help debug? We can't upgrade the version of STP tested until this is resolved :/

@gsnedders

@foolip foolip added this to To do in Reliable Safari results via automation Sep 9, 2019
@foolip
Copy link
Member Author

foolip commented Sep 9, 2019

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.

@gsnedders
Copy link
Contributor

gsnedders commented Sep 10, 2019

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.

@foolip
Copy link
Member Author

foolip commented Sep 10, 2019

Yeah, we should leave Safari stable on High Sierra for as long we can I think.

@foolip
Copy link
Member Author

foolip commented Sep 11, 2019

As such, closing this. Need to take care to not upgrade stable when doing STP and all infra test jobs.

@foolip foolip closed this Sep 11, 2019
Reliable Safari results automation moved this from To do to Done Sep 11, 2019
@foolip foolip deleted the foolip/safari-stable-mojave branch Sep 11, 2019
@foolip foolip restored the foolip/safari-stable-mojave branch Nov 20, 2019
@foolip foolip reopened this Nov 20, 2019
Reliable Safari results automation moved this from Done to To do Nov 20, 2019
@foolip foolip force-pushed the foolip/safari-stable-mojave branch from fe1196f to 924578b Nov 20, 2019
@foolip
Copy link
Member Author

foolip commented Nov 20, 2019

@foolip
Copy link
Member Author

foolip commented Nov 20, 2019

@gsnedders @youennf do you think we could work around the document.fonts.ready bug in reftest-wait_webdriver.js somehow to allow us to upgrade to 10.14? That might resolve #20324.

@foolip
Copy link
Member Author

foolip commented Nov 20, 2019

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 document.fonts.ready the first time?

@fred-wang who reviewed that might have ideas.

@fred-wang
Copy link
Contributor

fred-wang commented Nov 20, 2019

@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

@foolip
Copy link
Member Author

foolip commented Nov 20, 2019

@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...

@fred-wang
Copy link
Contributor

fred-wang commented Nov 20, 2019

@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.

Oh, I see.

@gsnedders
Copy link
Contributor

gsnedders commented Nov 20, 2019

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...

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?

@gsnedders
Copy link
Contributor

gsnedders commented Nov 20, 2019

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 document.fonts.ready the first time?

rAF is only paint, though, right? We could force layout (through CSSOM), but that might hide other bugs…

@foolip
Copy link
Member Author

foolip commented Nov 20, 2019

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.

@foolip foolip force-pushed the foolip/safari-stable-mojave branch from 924578b to ab21d0b Nov 21, 2019
@foolip
Copy link
Member Author

foolip commented Nov 21, 2019

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

@foolip
Copy link
Member Author

foolip commented Nov 21, 2019

https://wpt.fyi/results/?q=is%3Adifferent&run_id=378110002&run_id=353030001 is a diff of results with a root.offsetTop; // force layout hack applied. It's actually very similar to the differences without the hack, mostly making more tests pass.

I'm not sure why it is that the document.fonts.ready problem doesn't manifest, however...

@foolip foolip force-pushed the foolip/safari-stable-mojave branch from ab21d0b to a0c1beb Nov 27, 2019
@foolip foolip marked this pull request as ready for review Nov 27, 2019
@foolip foolip requested review from stephenmcgruer and removed request for jgraham Nov 27, 2019
@foolip
Copy link
Member Author

foolip commented Nov 27, 2019

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.

@gsnedders
Copy link
Contributor

gsnedders commented Nov 27, 2019

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.

@foolip
Copy link
Member Author

foolip commented Nov 28, 2019

@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.

@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Dec 4, 2019

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...?

@foolip
Copy link
Member Author

foolip commented Dec 5, 2019

No, a0c1beb 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.

@gsnedders
Copy link
Contributor

gsnedders commented Dec 9, 2019

@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.
@foolip foolip force-pushed the foolip/safari-stable-mojave branch from a0c1beb to 36f3511 Dec 10, 2019
@wpt-pr-bot wpt-pr-bot requested a review from jgraham Dec 10, 2019
@foolip
Copy link
Member Author

foolip commented Dec 10, 2019

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?

@smfr
Copy link
Contributor

smfr commented Dec 10, 2019

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.

@foolip
Copy link
Member Author

foolip commented Dec 10, 2019

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?

@foolip
Copy link
Member Author

foolip commented Dec 10, 2019

@smfr you're of course also welcome to review if you like YAML syntax :)

@foolip foolip merged commit bf5d16e into master Dec 12, 2019
17 checks passed
17 checks passed
Azure Pipelines Build #20191210.29 succeeded
Details
Azure Pipelines #20191210.29 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (all tests: Safari 1) all tests: Safari 1 succeeded
Details
Azure Pipelines (all tests: Safari 2) all tests: Safari 2 succeeded
Details
Azure Pipelines (all tests: Safari 3) all tests: Safari 3 succeeded
Details
Azure Pipelines (all tests: Safari 4) all tests: Safari 4 succeeded
Details
Azure Pipelines (all tests: Safari 5) all tests: Safari 5 succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-results) wpt.fyi hook: safari-results succeeded
Details
Community-TC (pull_request) TaskGroup: success
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
Reliable Safari results automation moved this from To do to Done Dec 12, 2019
@foolip foolip deleted the foolip/safari-stable-mojave branch Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.