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

css-grid: Use documents.font.ready() for tests with Ahem font. #22347

Merged
merged 1 commit into from Mar 24, 2020

Conversation

@clopez
Copy link
Contributor

@clopez clopez commented Mar 20, 2020

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:

css/css-grid/grid-definition/grid-change-fit-content-argument-001.html
css/css-grid/grid-definition/grid-inline-support-grid-template-areas-001.html
css/css-grid/grid-definition/grid-support-grid-template-areas-001.html
css/css-grid/grid-definition/grid-support-repeat-002.html
css/css-grid/grid-items/grid-minimum-size-grid-items-021.html
@mrego
mrego approved these changes Mar 20, 2020
Copy link
Member

@mrego mrego left a comment

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!

@mrego
Copy link
Member

@mrego mrego commented Mar 20, 2020

Mmmm, some tests are becoming unstable after this change, dunno if they need setup({explicit_done: true}); or the issue is something else.

@clopez
Copy link
Contributor Author

@clopez clopez commented Mar 20, 2020

Could you please update those too? (as part of this PR or in a different one, I don't really mind).

Sure, I was planing to do that on other PR. Let's keep this one for the css-grid related tests update

@clopez
Copy link
Contributor Author

@clopez clopez commented Mar 20, 2020

Mmmm, some tests are becoming unstable after this change, dunno if they need setup({explicit_done: true}); or the issue is something else.

That was it.. there were 41 tests calling done() but not setting explicit_done.

css/css-grid/alignment/grid-alignment-implies-size-change-001.html
css/css-grid/alignment/grid-alignment-implies-size-change-002.html
css/css-grid/alignment/grid-alignment-implies-size-change-003.html
css/css-grid/alignment/grid-alignment-implies-size-change-004.html
css/css-grid/alignment/grid-alignment-implies-size-change-005.html
css/css-grid/alignment/grid-alignment-implies-size-change-006.html
css/css-grid/alignment/grid-alignment-implies-size-change-007.html
css/css-grid/alignment/grid-alignment-implies-size-change-008.html
css/css-grid/alignment/grid-alignment-implies-size-change-009.html
css/css-grid/alignment/grid-alignment-implies-size-change-010.html
css/css-grid/alignment/grid-alignment-implies-size-change-011.html
css/css-grid/alignment/grid-alignment-implies-size-change-012.html
css/css-grid/alignment/grid-alignment-implies-size-change-013.html
css/css-grid/alignment/grid-alignment-implies-size-change-014.html
css/css-grid/alignment/grid-alignment-implies-size-change-015.html
css/css-grid/alignment/grid-alignment-implies-size-change-016.html
css/css-grid/alignment/grid-alignment-implies-size-change-017.html
css/css-grid/alignment/grid-alignment-implies-size-change-018.html
css/css-grid/alignment/grid-alignment-implies-size-change-019.html
css/css-grid/alignment/grid-alignment-implies-size-change-020.html
css/css-grid/alignment/grid-alignment-implies-size-change-021.html
css/css-grid/alignment/grid-alignment-implies-size-change-022.html
css/css-grid/alignment/grid-alignment-implies-size-change-023.html
css/css-grid/alignment/grid-alignment-implies-size-change-024.html
css/css-grid/alignment/grid-alignment-implies-size-change-025.html
css/css-grid/alignment/grid-alignment-implies-size-change-026.html
css/css-grid/alignment/grid-alignment-implies-size-change-027.html
css/css-grid/alignment/grid-alignment-implies-size-change-028.html
css/css-grid/alignment/grid-alignment-implies-size-change-029.html
css/css-grid/alignment/grid-alignment-implies-size-change-030.html
css/css-grid/alignment/grid-alignment-implies-size-change-031.html
css/css-grid/alignment/grid-alignment-implies-size-change-032.html
css/css-grid/alignment/grid-alignment-implies-size-change-033.html
css/css-grid/alignment/grid-alignment-implies-size-change-034.html
css/css-grid/alignment/grid-alignment-implies-size-change-035.html
css/css-grid/alignment/grid-alignment-implies-size-change-036.html
css/css-grid/alignment/grid-alignment-style-changes-005.html
css/css-grid/alignment/grid-alignment-style-changes-006.html
css/css-grid/alignment/grid-alignment-style-changes-007.html
css/css-grid/alignment/grid-alignment-style-changes-008.html
css/css-grid/grid-definition/grid-change-fit-content-argument-001.html

I will upload a new commit to this PR fixing them

@mrego
Copy link
Member

@mrego mrego commented Mar 20, 2020

I'm fine if you do a different PR for the other folders, thanks.

About the setup({explicit_done: true}); I'm not sure if this is something new or was not required in the past, or maybe these tests have just been wrong the whole time. Anyway good to know that the issue is fixed. Thanks again.

@clopez
Copy link
Contributor Author

@clopez clopez commented Mar 20, 2020

About the setup({explicit_done: true}); I'm not sure if this is something new or was not required in the past, or maybe these tests have just been wrong the whole time. Anyway good to know that the issue is fixed. Thanks again.

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.
Now that I changed the tests to start with the document.font.ready event the race condition to finish before document.onload was worse and cause more flakiness.

@clopez
Copy link
Contributor Author

@clopez clopez commented Mar 20, 2020

it seems explicit_done: true didn't fixed the flakiness issue ... i'm puzzled :?

@clopez
Copy link
Contributor Author

@clopez clopez commented Mar 20, 2020

The flakiness doesn't seem related with expicit_done: true, the test its consistently running all the subtests, it is just that sometimes them give different results. For example:

 1:49.80 TEST_START: /css/css-grid/alignment/grid-alignment-implies-size-change-012.html
 1:49.80 INFO Run 1/10
 1:50.07 TEST_END: Test OK. Subtests passed 2/2. Unexpected 0
 1:50.07 TEST_START: /css/css-grid/alignment/grid-alignment-implies-size-change-012.html
 1:50.07 INFO Run 2/10
 1:50.33 TEST_END: Test OK. Subtests passed 0/2. Unexpected 2
FAIL .before 1 - assert_equals: 
<img data-expected-width="100" id="item" src="support/100x100-green.png" class=" before" data-expected-height="100">
width expected 100 but got 0
    at assert_tolerance (http://web-platform.test:8000/resources/check-layout-th.js:24:9)
    at checkExpectedValues (http://web-platform.test:8000/resources/check-layout-th.js:34:9)
    at checkSubtreeExpectedValues (http://web-platform.test:8000/resources/check-layout-th.js:7:25)
    at Test.<anonymous> (http://web-platform.test:8000/resources/check-layout-th.js:182:34)
    at Test.step (http://web-platform.test:8000/resources/testharness.js:1958:25)
    at test (http://web-platform.test:8000/resources/testharness.js:548:30)
FAIL .after 2 - assert_equals: 
<img data-expected-width="100" id="item" src="support/100x100-green.png" class=" before after" data-expected-height="200">
width expected 100 but got 0
    at assert_tolerance (http://web-platform.test:8000/resources/check-layout-th.js:24:9)
    at checkExpectedValues (http://web-platform.test:8000/resources/check-layout-th.js:34:9)
    at checkSubtreeExpectedValues (http://web-platform.test:8000/resources/check-layout-th.js:7:25)
    at Test.<anonymous> (http://web-platform.test:8000/resources/check-layout-th.js:182:34)
    at Test.step (http://web-platform.test:8000/resources/testharness.js:1958:25)
    at test (http://web-platform.test:8000/resources/testharness.js:548:30)

On the first run the two subtest passes, on the second the two subtest give a failure (but they run)

@mrego
Copy link
Member

@mrego mrego commented Mar 20, 2020

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?

@clopez
Copy link
Contributor Author

@clopez clopez commented Mar 23, 2020

I think I understand what its happening, the tests that are flaky now depend on some image to load.
The tests were starting before with window.onload (which triggers after the initial images on the document load), but this PR changes the tests to start with documents.fonts.ready which may happen before the image loads.
A possible fix its to instead of just starting the tests when the fonts are ready, to do also when the webpage has loaded

…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.
@clopez
Copy link
Contributor Author

@clopez clopez commented Mar 23, 2020

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.

@clopez clopez force-pushed the clopez:css-grid_fontsready_ahem branch 2 times, most recently from 3dd303a to e874811 Mar 23, 2020
@mrego
mrego approved these changes Mar 24, 2020
Copy link
Member

@mrego mrego left a comment

LGTM, thanks for investigating the issue further!

@clopez clopez merged commit ea0533f into web-platform-tests:master Mar 24, 2020
10 checks passed
10 checks passed
Azure Pipelines Build #20200323.70 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
Community-TC (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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants