Skip to content

Add TestRendered event#34

Merged
jgraham merged 10 commits intomasterfrom
reftest_ready
Nov 11, 2019
Merged

Add TestRendered event#34
jgraham merged 10 commits intomasterfrom
reftest_ready

Conversation

@jgraham
Copy link
Contributor

@jgraham jgraham commented Oct 29, 2019

No description provided.

Co-Authored-By: Simon Pieters <zcorpan@gmail.com>
@foolip
Copy link
Member

foolip commented Oct 30, 2019

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

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 30, 2019

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
Copy link
Contributor Author

jgraham commented Oct 30, 2019

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
Copy link
Member

zcorpan commented Oct 30, 2019

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?

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

Makes sense!

Co-Authored-By: Philip Jägenstedt <philip@foolip.org>
@jgraham
Copy link
Contributor Author

jgraham commented Oct 30, 2019

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
Copy link
Contributor Author

jgraham commented Oct 31, 2019

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
Copy link
Member

foolip commented Oct 31, 2019

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

@jgraham
Copy link
Contributor Author

jgraham commented Oct 31, 2019

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

OK, changed.

Copy link
Member

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

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

LGTM % nits

jgraham and others added 2 commits October 31, 2019 16:26
Co-Authored-By: Simon Pieters <zcorpan@gmail.com>
Co-Authored-By: Simon Pieters <zcorpan@gmail.com>
@jgraham
Copy link
Contributor Author

jgraham commented Nov 4, 2019

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
Copy link
Member

foolip commented Nov 7, 2019

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

@foolip
Copy link
Member

foolip commented Nov 7, 2019

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

@jgraham jgraham changed the title Add PR for ReftestReady event #34 Add TestRendered event Nov 11, 2019
@jgraham jgraham changed the title #34 Add TestRendered event Add TestRendered event Nov 11, 2019
@jgraham jgraham merged commit e645410 into master Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants