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

Fix css-shapes tests to load ahem as a web font and wait for fonts ready #19048

Conversation

LukeZielinski
Copy link
Contributor

The font-related code is in a shared utility file for these tests. Updated it to load Ahem as a web font and wait for fonts.ready before running tests.

Adding these changes to the tests themselves didn't work, presumably because generate_tests is used and it interferes somehow.

@LukeZielinski LukeZielinski changed the title Fix css-grid tests to load ahem as a web font and wait for fonts ready Fix css-shapes tests to load ahem as a web font and wait for fonts ready Sep 13, 2019
@LukeZielinski LukeZielinski marked this pull request as ready for review September 13, 2019 19:02
@LukeZielinski LukeZielinski assigned gsnedders and foolip and unassigned plinss Sep 13, 2019
@LukeZielinski
Copy link
Contributor Author

@foolip @gsnedders Could you please take a look?

Scanning over the results, this looks like a general improvement for both Chrome and Firefox. Safari is worse, likely because of the known document.fonts.ready timing issue on WebKit.

One thing that is wrong with this change is that I see some test assertion failures getting returned as harness statuses (example). Any advice on how to address that?

@foolip
Copy link
Member

foolip commented Sep 14, 2019

@LukeZielinski a failed assert turning into a harness error means that there's some code running outside of a test "step function". This is usually fixed by adding t.step_func somewhere.

However, here generate_tests is used. To quote the docs:

This is deprecated because it runs all the tests outside of the test functions and as a result any test
throwing an exception will result in no tests being run. In almost all cases, you should simply call test within the loop you would use to generate the parameter list array.

Can you rewrite to avoid generate_tests?

@foolip
Copy link
Member

foolip commented Sep 14, 2019

@LukeZielinski also note that Ahem is still installed as a system font in our Safari config, so the document.fonts.ready bug shouldn't affect this, it'll kick in only if we remove the system font.

@foolip
Copy link
Member

foolip commented Oct 18, 2019

@LukeZielinski this is conflicting now, can you rebase if this wasn't already fixed in another PR?

@LukeZielinski
Copy link
Contributor Author

Abandoning, this was fixed properly in #19577. Thanks for the ping.

@LukeZielinski LukeZielinski deleted the ahem-hidden-failures branch October 18, 2019 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants