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

infrastructure/assumptions/ahem.html is failing in Safari #218

Closed
foolip opened this Issue Nov 8, 2017 · 32 comments

Comments

8 participants
@foolip
Collaborator

foolip commented Nov 8, 2017

https://wpt.fyi/infrastructure/assumptions/ahem.html

Failing this test could mean that lots of other tests are also failing, which is very bad.

@mattl, can you look into why these are failing? If they're problem in Edge or Safari, let's track those bugs here until it's fixed.

Reported by @FremyCompany.

@mattl

This comment has been minimized.

Contributor

mattl commented Nov 8, 2017

I'll take a look shortly.

@foolip

This comment has been minimized.

Collaborator

foolip commented Nov 8, 2017

@mattl I've added urgent and roadmap labels to this repo so that I could add the roadmap one.

@mattl

This comment has been minimized.

Contributor

mattl commented Nov 8, 2017

Sounds good.

@rwaldron

This comment has been minimized.

Contributor

rwaldron commented Nov 21, 2017

Important!! I don't think that the following findings completely address the issue filed here.


@foolip with the following command:

$ ./wpt run chrome infrastructure/assumptions/ahem.html

I'm seeing Chrome fail:

Summary
=======

Ran 1 tests
Expected results: 0
Unexpected results: 1 (FAIL: 1)

Unexpected Results
==================

FAIL /infrastructure/assumptions/ahem.html

And I captured this:

I went to the history and the path to fonts/Ahem.ttf was changed here: https://github.com/w3c/web-platform-tests/blob/master/infrastructure/assumptions/ahem.html; when I changed it from ../../fonts/Ahem.ttf to ./fonts/Ahem.ttf, the test passes:

Summary
=======

Ran 1 tests
Expected results: 1
Unexpected results: 0

OK

And now ahem-ref.html looks like this:

I'm going to open a PR on web-platform-tests, with a ref back to this comment.

rwaldron added a commit to rwaldron/web-platform-tests that referenced this issue Nov 21, 2017

Update path to fonts/Ahem.ttf: "../../fonts/Ahem.ttf" => "./fonts/Ahe…
…m.ttf"

Details originally posted here: web-platform-tests/results-collection#218 (comment)

--------------------------------------------

With the following command:

```
$ ./wpt run chrome infrastructure/assumptions/ahem.html
```

I'm seeing Chrome fail:

```
Summary
=======

Ran 1 tests
Expected results: 0
Unexpected results: 1 (FAIL: 1)

Unexpected Results
==================

FAIL /infrastructure/assumptions/ahem.html
```

And I captured this: https://i.gyazo.com/b6a376ec510f89275746c976d60c6eb3.png

I went to the history and the path to `fonts/Ahem.ttf` was changed here: https://github.com/w3c/web-platform-tests/blob/master/infrastructure/assumptions/ahem.html; when I changed it from `../../fonts/Ahem.ttf` to `./fonts/Ahem.ttf`, the test passes:

```
Summary
=======

Ran 1 tests
Expected results: 1
Unexpected results: 0

OK
```

And now ahem-ref.html looks like this: https://i.gyazo.com/362bd16d1d8c44773365106e8f24ed36.png
@rwaldron

This comment has been minimized.

Contributor

rwaldron commented Nov 21, 2017

lol @ me... Ignore all of that—somehow this change allows the test to pass, but it's also not actually loading the font anymore

@rwaldron

This comment has been minimized.

Contributor

rwaldron commented Nov 21, 2017

I've learned something useful: the tests only pass because they are both rendering with the fallback font. I'm sure everyone else already knew that, but I didn't so I'm sharing ;)

@rwaldron

This comment has been minimized.

Contributor

rwaldron commented Nov 21, 2017

This did lead me to write and open web-platform-tests/wpt#8396

@csnardi

This comment has been minimized.

Member

csnardi commented Jan 1, 2018

It looks like this is because we don't install Ahem on Sauce instances -- only on the local machine that then runs Sauce Connect. To fix this would likely involve adding installing Ahem into the prerun scripts -- I played around with this for a little bit but I couldn't get it to work.

This also means that all Sauce builds (including CI) do not have Ahem properly installed.

@foolip

This comment has been minimized.

Collaborator

foolip commented Jan 14, 2018

Thanks for looking into this @csnardi! Do you think it's at all likely that this could be made to work with Sauce, or would we have to run on machines with the fonts installed?

We care about the difference between system fonts and web fonts here, and I wonder if web developers would ever face this same problem. @kereliuk, could it make sense to have WebDriver APIs to install system fonts? It's not a given that such an API could even be made to work on all OSes, but maybe?

@csnardi

This comment has been minimized.

Member

csnardi commented Jan 14, 2018

Theoretically, this shouldn't require a whole lot of work to fix with Sauce. There's a tutorial for this for Linux, and doing a similar thing with our prerun scripts should just work. However, I didn't have a lot of luck doing that; that's likely more due to my lack of experience with Sauce rather than it not being possible though.

@kereliuk

This comment has been minimized.

Collaborator

kereliuk commented Jan 15, 2018

I'm not sure how fonts are installed on in Chromium, or how they're installed in other browsers. I am also skeptic about how hard it would be to make such a command cross-platform :/
If it is somewhat easy we could make this an extension command in ChromeDriver.

@mariestaver mariestaver added this to In progress in WPT Dashboard Jan 22, 2018

@mariestaver mariestaver moved this from In progress to To Do in WPT Dashboard Jan 22, 2018

@mariestaver mariestaver moved this from To Do (this sprint) to Backlog (future sprints) in WPT Dashboard Jan 22, 2018

@gsnedders

This comment has been minimized.

Collaborator

gsnedders commented Apr 10, 2018

This should probably be fixed upstream in wptrunner in the Sauce support there.

@rachelandrew and @gregwhitworth have apparently run into this trying to look at the css-multicol tests.

@mariestaver mariestaver moved this from Backlog (future sprints) to To Do in WPT Dashboard Apr 11, 2018

@jugglinmike

This comment has been minimized.

Collaborator

jugglinmike commented Apr 11, 2018

I agree that the fix here belongs in the WPT CLI. This problem is also being discussed in the WPT repository, though that discussion is driving towards a much larger patch for a longer-term solution.

I'm making it a priority to implement the Sauce Labs-specific solution for now.

@jugglinmike jugglinmike self-assigned this Apr 11, 2018

@foolip

This comment has been minimized.

Collaborator

foolip commented Apr 11, 2018

I'm making it a priority to implement the Sauce Labs-specific solution for now.

Would that go into the WPT CLI, where you think it belong?

@jugglinmike

This comment has been minimized.

Collaborator

jugglinmike commented Apr 12, 2018

Yes. The WPT CLI is already using Sauce Labs' "Pre-Run Executables" feature to disable Edge's popups blocker and to disable Safari's popup blocker. The fix will probably be contained within those two files.

Though this begs the question: now that Sauce Labs is no longer used to validate pull requests for WPT, will the WPT CLI continue to support integration with Sauce Labs?

@jgraham Can you speak to this at all?

@foolip

This comment has been minimized.

Collaborator

foolip commented Apr 12, 2018

Unless we remove it it'll stay around, but when it breaks wpt.fyi will probably be the first to notice.

@jgraham

This comment has been minimized.

Collaborator

jgraham commented Apr 16, 2018

I don't see why we would delete functionality that's being used.

I created a PR for Ahem in Sauce/OSX, but I have no way to test it: web-platform-tests/wpt#10491

For Windows we want to do something similar but of course it's horrific. https://superuser.com/questions/201896/how-do-i-install-a-font-from-the-windows-command-prompt has some suggestions for how to do what we want, but it would really be better if someone with Windows set up could make the patch.

@jugglinmike

This comment has been minimized.

Collaborator

jugglinmike commented Apr 16, 2018

I began work on this last week after self-assigning the issue. Interestingly, the Windows half was relatively painless, and it's the macOS half which is causing me trouble. I've been talking with Sauce Labs support to ensure the VM is configured as we expect (it is), so I've started debugging with a MacBook Pro locally. I'll report here when I know more!

@jugglinmike

This comment has been minimized.

Collaborator

jugglinmike commented Apr 18, 2018

We landed a fix for Edge yesterday evening. At that time, a patch was also merged to address the problem in Safari, but that has not been reported to consistently solve the problem, so this issue should remain open for the time being

@jugglinmike

This comment has been minimized.

Collaborator

jugglinmike commented Apr 30, 2018

Validating the fix for Edge took longer than expected due to last week's kerfuffle. With that resolved, we've successfully collected results using this solution--the proof is visible on https://wpt.fyi today:

screenshot from 2018-04-30 17-59-41

Unfortunately, the fix for Safari still eludes us. Another contributor merged a patch, but it was never expected to solve the problem. We still don't understand why, though. I have a support ticket open with Sauce Labs; it's currently waiting on me to manually test on a local device.

@gsnedders

This comment has been minimized.

Collaborator

gsnedders commented Apr 30, 2018

@jugglinmike

So fonts on macOS have this thing called "auto-activation". We could try various things with atsutil (see its manpage), potentially:

  • Ensuring autoactivation is enabled: atsutil autoactivation -e ~/Library/Fonts
  • Deleting the database and restarting fontd: atsutil databases -removeUser && atsutil server -shutdown (there's some warning about restarting it but…)
@jugglinmike

This comment has been minimized.

Collaborator

jugglinmike commented Apr 30, 2018

That's surprising. Through my limited local testing, macOS has recognized Ahem following the introduction of the file. In other words (and unlike in Windows) no additional configuration has been needed.

Note that the pre-run script does not run with system administrator-level permissions. If those are required to execute the commands you've mentioned, then we may need to file a feature request to Sauce Labs.

@gsnedders

This comment has been minimized.

Collaborator

gsnedders commented May 1, 2018

@jugglinmike Yeah, it should work, but I presume there's some bug somewhere. None of the commands I mentioned should need root, though.

@foolip

This comment has been minimized.

Collaborator

foolip commented May 9, 2018

The Edge fix worked, now it's just failing in Safari. Progress!

@jugglinmike

This comment has been minimized.

Collaborator

jugglinmike commented May 9, 2018

@gsnedders Thanks for the tip about atsutil. No combination of the databases and autoactivation subcommands improve the situation, but re-starting the server does cause the test to pass. You're right to call out the stability of this workaround, though. For those following along, here's the relevant text from the utility's documentation:

server queries the status of fontd or shutdowns the fontd.
Shutting down the fontd will spawn a new fontd.  Shutting down the
server is NOT recom- mended and will likely lead to misrendered
text or application crashes.

It's possible that this only concerns running processes and is therefor not a concern for us (the warning is a little vague), but I'd rather not be responsible for additional problems resulting from a change I don't fully understand.

An important detail of the apparent fix is that the ~/Library/Fonts directory is not initially present on the VM and must be created before fontd is re-started. Given that the machines that I've tested locally have that directory by default, I'm wondering if Sauce Labs could fix this simply by making sure that directory is available at system startup time.

I've been discussing this with Sauce Labs technical support, and they have agreed to try this in their next update. That's scheduled for today, so I should have more information to report soon.

@gsnedders

This comment has been minimized.

Collaborator

gsnedders commented May 10, 2018

@jugglinmike I assume you've tried it, but: create the directory, autoactivate it, then install the font?

@jugglinmike

This comment has been minimized.

Collaborator

jugglinmike commented May 10, 2018

I did, but no dice.

@jugglinmike

This comment has been minimized.

Collaborator

jugglinmike commented May 16, 2018

Unfortunately, Sauce Labs support tells me that the proposed fix was not included in their latest VM update. They do not have an estimate for the date of the next update. They have offered to tell me when that occurs; I'll post here when I hear from them.

@gsnedders gsnedders changed the title from infrastructure/assumptions/ahem.html is failing in Edge and Safari to infrastructure/assumptions/ahem.html is failing in Safari Jul 9, 2018

@foolip

This comment has been minimized.

Collaborator

foolip commented Oct 10, 2018

I may have run into this again in https://github.com/foolip/wpt/pull/5/checks?check_run_id=22723523 where I'm trying to run Safari on Azure.

@jgraham

This comment has been minimized.

Collaborator

jgraham commented Oct 10, 2018

The related bug for Gecko is https://bugzilla.mozilla.org/show_bug.cgi?id=1425698

It seems like font availability is pretty deeply tied to the way the user session is created. Do we have enough access on Azure to install the font globally rather than as a user font?

@foolip

This comment has been minimized.

Collaborator

foolip commented Oct 10, 2018

Do we have enough access on Azure to install the font globally rather than as a user font?

Yeah, just putting it in /Library/Fonts worked. However, I'm not certain that's robust, and it's possible there's just a bug with --install-fonts that I should fix. This is solvable in any event.

@jugglinmike

This comment has been minimized.

Collaborator

jugglinmike commented Nov 16, 2018

We resolved this by collecting from Safari running on a locally-manage Mac Mini (see gh-112 for Safari Technology Preview and gh-613 for the "stable" release of the browser). This is demonstrated by the latest results published to wpt.fyi:

WPT Dashboard automation moved this from To Do to Done Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment