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

This comment has been minimized.

Copy link
Contributor Author

LukeZielinski commented Oct 8, 2019

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

@foolip

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

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?

changes to font setup
@LukeZielinski

This comment has been minimized.

Copy link
Contributor Author

LukeZielinski commented Oct 9, 2019

@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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

LukeZielinski commented Oct 9, 2019

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 Oct 9, 2019
@LukeZielinski

This comment has been minimized.

Copy link
Contributor Author

LukeZielinski commented Oct 9, 2019

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)

@LukeZielinski LukeZielinski merged commit 4c1bbd4 into web-platform-tests:master Oct 9, 2019
10 checks passed
10 checks passed
Azure Pipelines Build #20191009.95 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 (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
Taskcluster (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
@LukeZielinski LukeZielinski deleted the LukeZielinski:ahem-css-shapes branch Oct 9, 2019
LukeZielinski added a commit to LukeZielinski/wpt that referenced this pull request Oct 16, 2019
LukeZielinski added a commit that referenced this pull request Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.