search

web-platform-tests / rfcs

web-platform-tests RFCs
75 stars 63 forks source link

Add TestRendered event #34

Closed jgraham closed 4 years ago

foolip commented 4 years ago

Can you add an example of how a test would use this? What is the event target, and does it bubble?

Ms2ger commented 4 years ago

Is this something that can be implemented in a browser-agnostic way, or do we need browser-internal support? If the latter, would it be hard to implement in non-Gecko browsers?

jgraham commented 4 years ago

I don't think there's anything magic here; just at the point at which we would take a screenshot we instead fire an event. So unless I'm missing something this is browser-agnostic.

zcorpan commented 4 years ago

The "instead" semantic isn't clear from the RFC details. That the event is only fired when the reftest-wait class is present (and the test is a reftest?), seems good to point out.

Is there a need for an event like this for load tests?

jgraham commented 4 years ago

I think it's sort of a moot point whether we also fire the event in the case where there isn't a reftest-wait; although you could do something in response to the event before the screenshot was taken there would be no guarantee that the changes would be reflected in the rendering before the screenshot, so any test doing that would be buggy. But sure, in practice I think we won't fire this except when reftest-wait is present to stop people doing the buggy thing. I'll update the PR.

Is there a need for an event like this for load tests?

Good question. It looks like there are some users of this in m-c: https://searchfox.org/mozilla-central/search?q=MozReftestInvalidate&path=crashtest

jgraham commented 4 years ago

I updated the proposal to rename the event WptTestRendered which seems like a better name if we're also going to use it for load tests. I also added the detailed steps and prepared a patch which implements this for webdriver and marionette (that uses an older iteration of the name without the Wpt prefix).

foolip commented 4 years ago

Given the capitalization name conflicts with the web platform are virtually impossible, I'd like it better without the Wpt prefix :)

jgraham commented 4 years ago

Given the capitalization name conflicts with the web platform are virtually impossible, I'd like it better without the Wpt prefix :)

OK, changed.

jgraham commented 4 years ago

I don't think this has substantive disagreement, so unless something comes up in the next day and a half I'm going to merge the issue and make a PR implementing the feature.

foolip commented 4 years ago

I think this is ready to go, I'll go ahead and merge.

foolip commented 4 years ago

No, actually, the file will and PR will need to be renamed, so I'll leave it you, @jgraham!