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
css-grid: Use documents.font.ready() for tests with Ahem font. #22347
css-grid: Use documents.font.ready() for tests with Ahem font. #22347
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, thanks for doing this.
Note that there are still tests out of css-grid that I guess have the same problem:
- css-align/baseline-rules/synthesized-baseline-flexbox-001.html
- css-align/baseline-rules/synthesized-baseline-grid-001.html
- css-align/baseline-rules/synthesized-baseline-inline-block-001.html
- css-contain/contain-size-grid-003.html
- css-multicol/multicol-gap-percentage-001.html
Could you please update those too? (as part of this PR or in a different one, I don't really mind). Thanks!
Mmmm, some tests are becoming unstable after this change, dunno if they need |
Sure, I was planing to do that on other PR. Let's keep this one for the css-grid related tests update |
That was it.. there were 41 tests calling done() but not setting explicit_done.
I will upload a new commit to this PR fixing them |
I'm fine if you do a different PR for the other folders, thanks. About the |
The documentation about testharness-api says this about when a test finish: ## Determining when all tests are complete ##
By default the test harness will assume there are no more results to come
when:
1. There are no `Test` objects that have been created but not completed
2. The load event on the document has fired
This behaviour can be overridden by setting the `explicit_done` property to
true in a call to `setup()`. If `explicit_done` is true, the test harness will
not assume it is done until the global `done()` function is called. Once `done()`
is called, the two conditions above apply like normal. So I understand that before this PR the tests started testing with window.onload event and stopped testing with document.onload. That doesn't seems correct to me. |
it seems |
The flakiness doesn't seem related with
On the first run the two subtest passes, on the second the two subtest give a failure (but they run) |
I'd suggest to leave these tests with issues out of this PR and we could investigate them properly and fix them in a different CL. What do you thing? |
I think I understand what its happening, the tests that are flaky now depend on some image to load. |
…eb-platform-tests#22347) web-fonts may not be ready when the window.onload signal is triggered, so to avoid flakiness or unexpected failures on the css-grid tests that use the Ahem font we should wait for the fonts to be ready before starting the test. However, that is not enough, we should also waiting for window.load before starting the tests because some tests depend on the document images to load (which may happen before or after the fonts are ready) So this patch changes the tests using Ahem to start after the load event has fired and after the fonts are ready. The tests now need to use "explicit_done: true" to avoid race conditions, so this commit also sets this tests to be explicit about when they finish.
I'm pushing a new commit over the branch (forced: replacing the previous ones) that changes the tests to start when both the fonts are ready and the document has loaded. It also sets the tests to be explicit about when they finish. |
3dd303a
to
e874811
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for investigating the issue further!
On WebKit based browsers, web-fonts may not be ready when the document.onload signal is triggered, so to avoid flakiness or unexpected failures on the css-grid tests that use the Ahem font this patch changes those tests to start with document.font.ready().
Most of the changes were done in a batch with sed, but there were also some manual edits on the tests below: