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

Load Ahem as a web font in css-shapes tests. #19577

Merged

Conversation

LukeZielinski
Copy link
Contributor

@LukeZielinski LukeZielinski commented Oct 8, 2019

This makes font setup/restoration explicit rather than per-subtest.
This allows us to setup Ahem usage, then wait for fonts to load, and
then run all computed tests as a discrete block.

The previous method (which dynamically inserted Ahem prior to a subtest
and removed it immediately after) didn't allow us to wait for the web
fonts to be loaded because the test page would have no Ahem usage
present at the time we waited for the fonts.ready event.

Also make font setup/restoration explicit rather than per-subtest.
This allows us to setup Ahem usage, then wait for fonts to load, and
then run all computed tests as a discrete block.

The previous method (which dynamically inserted Ahem prior to a subtest
and removed it immediately after) didn't allow us to wait for the web
fonts to be loaded because the test page would have no Ahem usage
present at the time we waited for the fonts.ready event.
@LukeZielinski
Copy link
Contributor Author

@foolip Hoping to get a sanity check on this approach before diving into inlining a bunch more tests -- thoughts?

@foolip
Copy link
Member

foolip commented Oct 8, 2019

Looks reasonable, but you might also find the people who wrote/reviewed these tests and ask for their feedback before doing it to a lot of tests.

@astearns
Copy link
Contributor

astearns commented Oct 8, 2019

@LukeZielinski what's the motivation for inlining? Would it be possible to get the Ahem improvements while still using generate_tests?

@LukeZielinski
Copy link
Contributor Author

@astearns The motivation to inline was that generate_tests is deprecated, and it helped in debugging what the problem with these tests were.

I've combined font setup changes with generate_tests and it seems to work, and it does appear cleaner this way. @foolip I don't recall why generate_tests got deprecated but unless there are concerns I'll probably move forward with this combination.

@astearns
Copy link
Contributor

astearns commented Oct 9, 2019

Thanks! I think it's much more readable and maintainable this way.

Looks like there's a preferred way of doing data-driven tests:

[generate_tests] 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.

Now that you've improved the Ahem situation, we can probably leave the generate_tests usage in until/unless we see a new problem in running these tests.

@LukeZielinski
Copy link
Contributor Author

Oops, should have read my own link! Ok, I'll move ahead with updating the rest of the tests in this directory. Thanks.

@LukeZielinski LukeZielinski marked this pull request as ready for review October 9, 2019 15:00
@LukeZielinski
Copy link
Contributor Author

For the record, this PR only updates a handful of css-shapes tests. These are ones that were either broken by the parsing-utils.js change in this PR, or identified as regressions when running all tests with --install-fonts disabled (eg: firefox regressions)

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.

None yet

5 participants