Support testdriver.js in reftests

gsnedders commented 5 years ago

Previously, when testdriver.js we decided to punt on supporting it in reftests; we now have an actual use for it (#9346), so this needs done.

This will need changes in all the executors: the Python code needs to handle various messages to interact with the document before eventually taking a screenshot, and the JavaScript callbacks needs to deal with passing the testdriver.js messages. The right way to do this is probably by way of comparison of both bits of reftest code with the test harness code.

gsnedders commented 5 years ago

Myself and @jgraham have separately on various occasions remembered that this is going to be somewhat awkward for Firefox the internal Marionette reftest runner, given we'll need to reimplement the testdriver.js implementation within Marionette for that.

Perhaps the right solution here is to just put some "testdriver" flag in the manifest and then use the external reftest runner for those tests.

jgraham commented 5 years ago

Yeah, that seems acceptable for a v1 implemenation; we would probably want to fix this to work inside the harness later, but that could be a seperate project.

gsnedders commented 5 years ago

I'm concerned about the ongoing maintenance cost of having yet another implementation, especially one that wptrunner relies on (and which exists in an external repo). I'm not convinced the benefits of having the internal reftest runner support testdriver.js are actually big enough to outweigh that cost.

jgraham commented 5 years ago

It's probably a bad idea but we definitely could put the code in the wpt repo and ship it over to marionette in the reftest:setup.

foolip commented 5 years ago

@gsnedders any chance of getting this fixed before the end of the quarter?

gsnedders commented 5 years ago

Is there any reason to believe this is so high priority? Does it affect more tests than other untestable issues?

foolip commented 5 years ago

@Hexcles having dug into test writing patterns in Chromium a bit, do you have a hunch for how often this would be useful? I'm inclined to agree with @gsnedders that maybe this isn't very high prio, but don't know.

foolip commented 4 years ago

I've downgraded the priority. We'll probably want this to convert manual tests in the future, but at this point the request for it isn't coming from outside, it's just those of us working on wpt infra that think it's a good idea.

Hexcles commented 4 years ago

My research only looked at existing tests / test history. And of course, there is zero testdriver usage in reftests as it doesn't work, but I don't know how many people attempted to do that.

gsnedders commented 4 years ago

I think there was some discussion at TPAC suggesting this was needed for a whole bunch of tests, but I think it's pretty low priority still.

stephenmcgruer commented 3 years ago

So there is now at least one case of testdriver.js being used in reftests, in css/selectors/remove-hovered-element.html - which a Chromium engineer asked about today as it times out in all browsers in wpt.fyi.

Doing a quick grep, there are half a dozen cases today in the tree (some of which pass!):

$ grep -lr rel.*match  | xargs grep -lr testdriver.js 
css/css-grid/grid-model/grid-layout-stale-001.html
css/css-grid/grid-model/grid-layout-stale-002.html
css/css-shadow-parts/interaction-with-nested-pseudo-class.html
css/selectors/remove-hovered-element.html
pointerevents/pointerevent_releasepointercapture_invalid_pointerid.html
[tools/ entries]
docs/writing-tests/test-templates.md

stephenmcgruer commented 3 years ago

as it times out in all browsers in wpt.fyi.

Note that with the current testrunner implementation in Chromium's CI (which is not wptrunner), this test passes in Chromium's CI, which is why the engineer was surprised to discover the all-timeout status.

josepharhar commented 2 years ago

I am adding some new tests which need this fixed here: https://github.com/web-platform-tests/wpt/pull/34169

foolip commented 7 months ago

I forgot about this issue and wrote tests that don't work: https://github.com/web-platform-tests/wpt/pull/43445

foolip commented 7 months ago

Using git grep -l /resources/testdriver.js | xargs grep -lE 'rel.*match' I found these 10 tests that are currently timing out because this doesn't work:

I also found these two that pass because there's no class="reftest-wait", so the tests probably don't have the intended coverage:

https://wpt.fyi/results/css/css-grid/grid-model/grid-layout-stale-001.html https://wpt.fyi/results/css/css-grid/grid-model/grid-layout-stale-002.html

I fixed two tests that were a bit confused in https://github.com/web-platform-tests/wpt/pull/43937 and https://github.com/web-platform-tests/wpt/pull/43938.

foolip commented 7 months ago

We also have class="test-wait" for crash tests, and I've found one test like that which is also timing out:

https://wpt.fyi/results/accessibility/crashtests/svg-mouse-listener.html

josepharhar commented 3 months ago

I'm working on a bunch of reftests which need testdriver for user activation here: https://github.com/web-platform-tests/wpt/tree/master/html/semantics/forms/the-select-element/stylable-select

WeizhongX commented 3 months ago

I realized we should put such tests in wpt_internal until this issue is fixed. @josepharhar WDYT?

josepharhar commented 3 months ago

I realized we should put such tests in wpt_internal until this issue is fixed. @josepharhar WDYT?

That sounds fine to me

web-platform-tests / wpt

Support testdriver.js in reftests #13183