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

Add WPT tests for webkit-prefixed animation/transition events #19099

Merged
merged 1 commit into from Jan 13, 2020

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Sep 16, 2019

Bug: 999895
Change-Id: I5f3679561e923d0e24809ebe982f0672e7769392
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1807190
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730629}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

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

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1807190 branch 5 times, most recently from c8c635f to 845b889 Compare December 24, 2019 11:49
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1807190 branch 7 times, most recently from e39681a to de0c541 Compare January 3, 2020 18:07
@chromium-wpt-export-bot chromium-wpt-export-bot changed the title Compat-spec tests for webkit-prefixed animation/transition events Add WPT tests for webkit-prefixed animation/transition events Jan 8, 2020
@wpt-pr-bot wpt-pr-bot added the dom label Jan 8, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1807190 branch 3 times, most recently from 4a5e9a3 to cae049e Compare January 10, 2020 14:44
@bzbarsky
Copy link
Contributor

@stephenmcgruer it might be good to also test content attributes here, because the behavior for those is weird with these events: the content attribute is all-lowercase (like the IDL attribute) but the event name is not all-lowercase (unlike most events). This showed up in some Gecko-internal tests when I was working on the casing issue in Firefox...

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1807190 branch 2 times, most recently from eb3120a to dddfe02 Compare January 13, 2020 15:24
@stephenmcgruer
Copy link
Contributor

@bzbarsky I agree! I've started looking into it, but it seems non-trivial to add support for so I would like to punt on it till a follow-up PR. I will file an issue to look into it.

@bzbarsky
Copy link
Contributor

@stephenmcgruer That sounds great, thank you!

Bug: 999895
Change-Id: I5f3679561e923d0e24809ebe982f0672e7769392
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1807190
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730629}
@bzbarsky
Copy link
Contributor

@stephenmcgruer
Copy link
Contributor

@bzbarsky Ah, this is sort of good news to me. I saw some timeouts in FF during development, but they then went away when I wanted to report them in a bug...

Is it only webkit-animation-iteration-event.html that is randomly timing out? (Because thats what I saw in testing). I suspect my choice of 50ms, 2 iterations in https://github.com/web-platform-tests/wpt/blob/master/dom/events/webkit-animation-iteration-event.html#L18 is too fast for slow machines, and sometimes there isn't an iteration (because the animation is over by the time the next frame fires).

The problem is that increasing that animation length delays the tests by the iteration length (since they all wait for the iteration event). So its iteration_time * num_tests (currently 25ms * 12).

I could try upping it to, say, 75ms iteration time and see how that goes?

@bzbarsky
Copy link
Contributor

Good catch; it looks like it's only webkit-animation-iteration-event.html, but in different parts of the test. And yes, given that your diagnosis seems pretty likely.

We could try 75ms and see what happens, sure. I'll give that a shot on the Firefox side and see what our CI says.

@stephenmcgruer
Copy link
Contributor

So, to be clear, that means the test should be changed to say 150ms 2 (so that each iteration is 75ms)

@bzbarsky
Copy link
Contributor

Ah, that is a very useful clarification! ;)

@birtles
Copy link
Contributor

birtles commented Jan 22, 2020

Generally we don't use animations that are less than 100s (seconds, not milliseconds) long in Firefox CI since we have had some really slow machines in CI. If we're waiting on iteration events, we would typically use iterations of 100s and then use a negative delay to start the animation just before the first iteration ends.

@bzbarsky
Copy link
Contributor

OK, so I did a push with the existing 50ms number and another with 150ms. On the first one, 15 out of 40 test runs failed. On the second, 7 out of 40. So probably a lot better (or I got really lucky/unlucky), but still pretty bad... :(

@stephenmcgruer
Copy link
Contributor

Generally we don't use animations that are less than 100s (seconds, not milliseconds) long in Firefox CI since we have had some really slow machines in CI. If we're waiting on iteration events, we would typically use iterations of 100s and then use a negative delay to start the animation just before the first iteration ends.

... I should have thought of that idea (since we've used it before as well -_-). Yeah, we should try that here; @bzbarsky are you ok to try that change or shall I? (Will require a little more change than just tweaking the animation length).

@bzbarsky
Copy link
Contributor

I'd appreciate it if someone who knows animations better than I created the diff, but I'm happy to do some test runs. :)

@stephenmcgruer
Copy link
Contributor

Ack, I'll try to find time to do it this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants