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

Include testharness in lazy loading reftest-wait tests #25513

Closed
wants to merge 1 commit into from

Conversation

rwlbuis
Copy link
Contributor

@rwlbuis rwlbuis commented Sep 14, 2020

Include testharness in lazy loading reftest-wait tests since WebKit
uses testharness to enable lazy image loading.

Include testharness in lazy loading reftest-wait tests since WebKit
uses testharness to enable lazy image loading.
@zcorpan
Copy link
Member

zcorpan commented Sep 14, 2020

WebKit uses testharness to enable lazy image loading.

Why?

@rwlbuis
Copy link
Contributor Author

rwlbuis commented Sep 14, 2020

WebKit uses testharness to enable lazy image loading.

Why?

WebKit has been making a move from runtime enabled feature flags to settings. In order to toggle settings per test JS API is exposed on internals.settings. So far this has been used for WebAudio but since a few weeks also for lazy loading in WebKit. To be clear, this is controlled by the vendor specific testharnessreport.js, not the generic one, but it has to be included in order for lazy loading to be enabled.

Note that even in the time of runtime enabled feature flags, a WebKit specific tweak was needed (example ), which was not upstreamed to WPT repo, and was a hassle.

@annevk
Copy link
Member

annevk commented Sep 14, 2020

I think we should discuss this in https://github.com/web-platform-tests/rfcs as this would affect all reftests and it's not clear to me this is the solution to configuration for those tests. And if it is, perhaps only the second script file should be included.

Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on an RFC or similar for this.

Including testharness.js itself won't work because the harness will detect the tests as testharness tests; see e.g. https://github.com/web-platform-tests/wpt/pull/25513/checks?check_run_id=1111154398 where we end up with 0 tests after this patch.

@rwlbuis
Copy link
Contributor Author

rwlbuis commented Sep 15, 2020

It turns out this is not needed after all. Sorry for the noise!

@rwlbuis rwlbuis closed this Sep 15, 2020
@annevk annevk deleted the loading_lazy_testharnessreport branch September 16, 2020 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants