CSS Transitions: Fix flaky test unprefixed-transform.html

CSS Transitions: Fix flaky test unprefixed-transform.html

This test now uses testharness. It no longer relies on event timing.

BUG=664859
Committed: https://crrev.com/5e87dd527ae72f6128c08d78a3ece920276ef73e
Cr-Commit-Position: refs/heads/master@{#439674}

[Eric Willigers, 2016-12-16 08:03:16]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[Eric Willigers, 2016-12-16 08:03:30]

ericwilligers@chromium.org changed reviewers:
+ suzyh@chromium.org

[commit-bot: I haz the power, 2016-12-16 08:03:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 09:15:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 09:15:14]

Dry run: This issue passed the CQ dry run.

[suzyh_UTC10 (ex-contributor), 2016-12-18 23:47:27]

Thanks for cleaning all these up! :)

https://codereview.chromium.org/2578143003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/transitions/unprefixed-transform.html
(right):

https://codereview.chromium.org/2578143003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/transitions/unprefixed-transform.html:23:
assert_equals(e.propertyName, 'transform');
Any other checks it would make sense to do on the event, or do we simply want to
test that the event fired?

https://codereview.chromium.org/2578143003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/transitions/unprefixed-transform.html:26: if
(!remaining)
What happens if we don't get the wrong number of events (too many or too few)?
Too few, I guess the test just times out. Too many might be more problematic. Is
there an extra assertion we can make to verify we don't hit either of these
cases?

https://codereview.chromium.org/2578143003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/transitions/unprefixed-transform.html:40: },
'transitionend event for unprefixed transform');
If this test is targeted at the unprefixed transform, perhaps it would make
sense to separate out the parts that are to do with the prefixed property. I can
imagine we want to test the various combinations, but I wonder if it would be
best to have one test set only element.style.webkitTransform or
element.style.transform, not both. What do you think?

Optional: If this file continues to have both prefixed and unprefixed
transforms, and is mostly testing the transitionend event, consider renaming the
file.

[Eric Willigers, 2016-12-19 06:04:38]

https://codereview.chromium.org/2578143003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/transitions/unprefixed-transform.html
(right):

https://codereview.chromium.org/2578143003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/transitions/unprefixed-transform.html:23:
assert_equals(e.propertyName, 'transform');
On 2016/12/18 23:47:27, suzyh wrote:
> Any other checks it would make sense to do on the event, or do we simply want
to
> test that the event fired?

Added a few more asserts on event properties.

https://codereview.chromium.org/2578143003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/transitions/unprefixed-transform.html:26: if
(!remaining)
On 2016/12/18 23:47:27, suzyh wrote:
> What happens if we don't get the wrong number of events (too many or too few)?
> Too few, I guess the test just times out. Too many might be more problematic.
Is
> there an extra assertion we can make to verify we don't hit either of these
> cases?

Done, by waiting a short time so we can detect excess events.

https://codereview.chromium.org/2578143003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/transitions/unprefixed-transform.html:40: },
'transitionend event for unprefixed transform');
On 2016/12/18 23:47:27, suzyh wrote:
> If this test is targeted at the unprefixed transform, perhaps it would make
> sense to separate out the parts that are to do with the prefixed property. I
can
> imagine we want to test the various combinations, but I wonder if it would be
> best to have one test set only element.style.webkitTransform or
> element.style.transform, not both. What do you think?
> 
> Optional: If this file continues to have both prefixed and unprefixed
> transforms, and is mostly testing the transitionend event, consider renaming
the
> file.

No, the point of this test is that the page might have transitions on prefixed
and/or unprefixed transforms, and there will always be one event for the
transition, on unprefixed transform.

Many public web pages intentionally have 'transform ..., -webkit-transform ...'
to support multiple browser versions.

[Eric Willigers, 2016-12-19 06:07:08]

Patchset #2 (id:20001) has been deleted

[suzyh_UTC10 (ex-contributor), 2016-12-19 23:10:52]

lgtm after one minor change (making the test description more explicit). The
other point is optional.

https://codereview.chromium.org/2578143003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/transitions/unprefixed-transform.html
(right):

https://codereview.chromium.org/2578143003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/transitions/unprefixed-transform.html:40: },
'transitionend event for unprefixed transform');
On 2016/12/19 at 06:04:37, Eric Willigers wrote:
> On 2016/12/18 23:47:27, suzyh wrote:
> > If this test is targeted at the unprefixed transform, perhaps it would make
> > sense to separate out the parts that are to do with the prefixed property. I
can
> > imagine we want to test the various combinations, but I wonder if it would
be
> > best to have one test set only element.style.webkitTransform or
> > element.style.transform, not both. What do you think?
> > 
> > Optional: If this file continues to have both prefixed and unprefixed
> > transforms, and is mostly testing the transitionend event, consider renaming
the
> > file.
> 
> No, the point of this test is that the page might have transitions on prefixed
and/or unprefixed transforms, and there will always be one event for the
transition, on unprefixed transform.
> 
> Many public web pages intentionally have 'transform ..., -webkit-transform
...' to support multiple browser versions.

Ah, making sure that there's only one event makes sense. I'm still curious
whether this test helps us be sure that the events are triggered both by setting
"element.style.webkitTransform = 'rotate(360deg)'" and by setting
"element.style.transform = 'rotate(360deg)'", or whether it's only one of these
statements that is responsible. But that point may not be important; I'll leave
that to your discretion.

To more clearly document this explanation, please change the test description
from "transitionend event for unprefixed transform" to something like "A
transition on the prefixed and/or unprefixed transform property causes a single
transitionend event for the unprefixed transform property."

[Eric Willigers, 2016-12-20 01:06:16]

https://codereview.chromium.org/2578143003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/transitions/unprefixed-transform.html
(right):

https://codereview.chromium.org/2578143003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/transitions/unprefixed-transform.html:40: },
'transitionend event for unprefixed transform');
On 2016/12/19 23:10:52, suzyh wrote:
> On 2016/12/19 at 06:04:37, Eric Willigers wrote:
> > On 2016/12/18 23:47:27, suzyh wrote:
> > > If this test is targeted at the unprefixed transform, perhaps it would
make
> > > sense to separate out the parts that are to do with the prefixed property.
I
> can
> > > imagine we want to test the various combinations, but I wonder if it would
> be
> > > best to have one test set only element.style.webkitTransform or
> > > element.style.transform, not both. What do you think?
> > > 
> > > Optional: If this file continues to have both prefixed and unprefixed
> > > transforms, and is mostly testing the transitionend event, consider
renaming
> the
> > > file.
> > 
> > No, the point of this test is that the page might have transitions on
prefixed
> and/or unprefixed transforms, and there will always be one event for the
> transition, on unprefixed transform.
> > 
> > Many public web pages intentionally have 'transform ..., -webkit-transform
> ...' to support multiple browser versions.
> 
> Ah, making sure that there's only one event makes sense. I'm still curious
> whether this test helps us be sure that the events are triggered both by
setting
> "element.style.webkitTransform = 'rotate(360deg)'" and by setting
> "element.style.transform = 'rotate(360deg)'", or whether it's only one of
these
> statements that is responsible. But that point may not be important; I'll
leave
> that to your discretion.
> 
> To more clearly document this explanation, please change the test description
> from "transitionend event for unprefixed transform" to something like "A
> transition on the prefixed and/or unprefixed transform property causes a
single
> transitionend event for the unprefixed transform property."

Test description updated.

[Eric Willigers, 2016-12-20 01:46:33]

The CQ bit was checked by ericwilligers@chromium.org

[Eric Willigers, 2016-12-20 01:46:33]

The patchset sent to the CQ was uploaded after l-g-t-m from suzyh@chromium.org
Link to the patchset: https://codereview.chromium.org/2578143003/#ps60001
(title: "test description")

[commit-bot: I haz the power, 2016-12-20 01:46:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 02:26:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482198393523450,
"parent_rev": "6f286c3cfbe0f7db96c18c3553c93fd9f009e969", "commit_rev":
"f7a55ce13e98269a44154bd6662075fda2717318"}

[commit-bot: I haz the power, 2016-12-20 02:26:57]

Description was changed from

==========
CSS Transitions: Fix flaky test unprefixed-transform.html

This test now uses testharness. It no longer relies on event timing.

BUG=664859
==========

to

==========
CSS Transitions: Fix flaky test unprefixed-transform.html

This test now uses testharness. It no longer relies on event timing.

BUG=664859

Review-Url: https://codereview.chromium.org/2578143003
==========

[commit-bot: I haz the power, 2016-12-20 02:26:58]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-12-20 02:28:28]

Description was changed from

==========
CSS Transitions: Fix flaky test unprefixed-transform.html

This test now uses testharness. It no longer relies on event timing.

BUG=664859

Review-Url: https://codereview.chromium.org/2578143003
==========

to

==========
CSS Transitions: Fix flaky test unprefixed-transform.html

This test now uses testharness. It no longer relies on event timing.

BUG=664859

Committed: https://crrev.com/5e87dd527ae72f6128c08d78a3ece920276ef73e
Cr-Commit-Position: refs/heads/master@{#439674}
==========

[commit-bot: I haz the power, 2016-12-20 02:28:29]

Patchset 3 (id:??) landed as
https://crrev.com/5e87dd527ae72f6128c08d78a3ece920276ef73e
Cr-Commit-Position: refs/heads/master@{#439674}