1265274 - Make Animatable.animate() tests and KeyframeEffect() constructor tests that use the common test data in keyframe-utils.js

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

Bug 1244591 will add a file that extracts common sets of keyframe test data. The data is taken from the tests for the KeyframeEffectReadOnly constructor and is subsequently used to test KeyframeEffect.setFrames().

We should use that same data to test:

* Animatable.animate(), and
* the KeyframeEffect constructor.

Comment 1

[Ryo Kato [:r_kato]]

Is anyone else handling this bug now? If not, I'd like to take this on! :)

Comment 2

[Brian Birtles (:birtles)]

(In reply to Ryo Kato [:r_kato] from comment #1)
> Is anyone else handling this bug now? If not, I'd like to take this on! :)

Yes, please take this! Thank you!

Comment 3

[Ryo Kato [:r_kato]]

Attachment: MozReview Request: Bug 1265274 - Part 1: Extract tests for single-valued keyframes r?birtles

We also add a new test for single-valued keyframes.

Review commit: https://reviewboard.mozilla.org/r/48993/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48993/

Comment 4

[Ryo Kato [:r_kato]]

Attachment: MozReview Request: Bug 1265274 - Make tests to use the common test data in keyframe-utils.js r=birtles

Meta files are also updated so that we can use the new test which is added in Part 1.

Review commit: https://reviewboard.mozilla.org/r/48995/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48995/

Comment 5

[Brian Birtles (:birtles)]

Kato-san, I don't understand part 1. Why do we want to separate the tests out?

Comment 6

[Brian Birtles (:birtles)]

Comment on attachment 8745343 [details]
MozReview Request: Bug 1265274 - Make tests to use the common test data in keyframe-utils.js r=birtles

https://reviewboard.mozilla.org/r/48995/#review45939

Thanks for doing this.

I'd like to have another look at this with:

* Part 1 removed
* Going through all the values in
  gPropertyIndexedKeyframesTests/gKeyframeSequenceTests not just [0] when that's
  appropriate
* Not using gPropertyIndexedKeyframesTests[0] but simply leaving the tests as-is
  (or, if we don't need keyframes at all--just using either 'null' or '{}' for
  the frames argument)

I know normally we try to re-use code, but I think that for tests, referring to gPropertyIndexedKeyframesTests[0] makes the tests harder to read and debug. (Also, because we're testing a Web standard, I don't think we'll ever stop supporting something like '{ opacity: [ 0, 1 ] }', and, if we do, we can just do search-and-replace to fix it. Furthermore, as this is a shared test suite, I think it's best to optimize for readability and simplicity.)

::: testing/web-platform/tests/web-animations/animatable/animate.html:15
(Diff revision 1)
> +  var sample = gPropertyIndexedKeyframesTests[0];
>    var div = createDiv(t);
> -  var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);
> +  var anim = div.animate(sample.input, 2000);

I think this makes the test harder to read.

If we think the actual keyframes don't matter, we should just make this:

  var anim = div.animate(null, 2000);

Or, for this particular test, just:

  var anim = div.animate(null);

::: testing/web-platform/tests/web-animations/animatable/animate.html:22
(Diff revision 1)
> +  var sample = gPropertyIndexedKeyframesTests[0];
>    var div = createDiv(t);
> -  var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);
> +  var anim = div.animate(sample.input, 2000);

Likewise here:

  var anim = div.animate(null);

should be fine.

::: testing/web-platform/tests/web-animations/animatable/animate.html:34
(Diff revision 1)
>  test(function(t) {
> +  var sample = gPropertyIndexedKeyframesTests[0];
>    var div = createDiv(t);
> -  var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);
> -  assert_equals(anim.effect.getFrames().length, 2);
> +  var anim = div.animate(sample.input, 2000);
> +  assert_frame_lists_equal(anim.effect.getFrames(), sample.output);
> -  assert_equals(anim.effect.getFrames()[0].opacity, '0');
> -  assert_equals(anim.effect.getFrames()[1].opacity, '1');
>  }, 'Element.animate() accepts a property-indexed keyframe specification');

I don't think it makes sense to just test one value from gPropertyIndexedKeyframesTests. Better just to iterate over the whole lot (like, e.g. keyframe-effect/constructor.html)

::: testing/web-platform/tests/web-animations/animatable/animate.html:41
(Diff revision 1)
>  test(function(t) {
> +  var sample = gKeyframeSequenceTests[0];
>    var div = createDiv(t);
> -  var anim = div.animate([ { opacity: 0 }, { opacity: 1 } ], 2000);
> -  assert_equals(anim.effect.getFrames().length, 2);
> +  var anim = div.animate(sample.input, 2000);
> +  assert_frame_lists_equal(anim.effect.getFrames(), sample.output);
> -  assert_equals(anim.effect.getFrames()[0].opacity, '0');
> -  assert_equals(anim.effect.getFrames()[1].opacity, '1');
>  }, 'Element.animate() accepts a frame-indexed keyframe specification');

Likewise here, we should test all the values.

::: testing/web-platform/tests/web-animations/animatable/animate.html:42
(Diff revision 1)
> +  var sample = gKeyframeSequenceTests[0];
>    var div = createDiv(t);
> -  var anim = div.animate([ { opacity: 0 }, { opacity: 1 } ], 2000);
> -  assert_equals(anim.effect.getFrames().length, 2);
> +  var anim = div.animate(sample.input, 2000);
> +  assert_frame_lists_equal(anim.effect.getFrames(), sample.output);

Likewise here.

::: testing/web-platform/tests/web-animations/animatable/animate.html:48
(Diff revision 1)
>  test(function(t) {
> +  var sample = gSingleValuedKeyframesTests[0];
>    var div = createDiv(t);
> -  var anim = div.animate({ opacity: 0 }, 2000);
> -  assert_equals(anim.effect.getFrames().length, 1);
> +  var anim = div.animate(sample.input, 2000);
> +  assert_frame_lists_equal(anim.effect.getFrames(), sample.output);
> -  assert_equals(anim.effect.getFrames()[0].opacity, '0');
>  }, 'Element.animate() accepts a single-valued keyframe specification');

If we test all the values in gPropertyIndexedKeyframesTests and gKeyframeSequenceTests and drop part 1 from this patch series, we can drop this test.

::: testing/web-platform/tests/web-animations/animatable/animate.html:55
(Diff revision 1)
> +test(function(t) {
> +  var sample = gInvalidKeyframesTests[0];
> +  var div = createDiv(t);
> +  assert_throws(sample.expected, function() {
> +    div.animate(sample.input, 2000);
> +  });
> +}, 'Element.animate() doesn\'t accept an invalid keyframe specification');

Likewise here, I think we should test all gInvalidKeyframesTests, not just one.

::: testing/web-platform/tests/web-animations/animatable/animate.html:70
(Diff revision 1)
> +  var sample = gPropertyIndexedKeyframesTests[0];
>    var div = createDiv(t);
> -  var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);
> +  var anim = div.animate(sample.input, 2000);

I don't think it helps to use gPropertyIndexedKeyframesTests here--I think it just makes the test harder to read.

Likewise for the other places below where we are using gPropertyIndexedKeyframesTests[0].

Comment 7

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #5)
> Kato-san, I don't understand part 1. Why do we want to separate the tests
> out?

Nevermind, I can see in part 2 why this was added, but as per my review feedback on part 2, I think it's better if we don't do this.

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 8745342 [details]
MozReview Request: Bug 1265274 - Part 1: Extract tests for single-valued keyframes r?birtles

https://reviewboard.mozilla.org/r/48993/#review45937

Comment 9

[Ryo Kato [:r_kato]]

https://reviewboard.mozilla.org/r/48995/#review45939

Thank you for reviewing!

I used the only `[0]` test just because there is some comment saying animate.html is a subset of constructor.html. But I also agree to iterate as many tests as possible.

Comment 10

[Ryo Kato [:r_kato]]

Comment on attachment 8745343 [details]
MozReview Request: Bug 1265274 - Make tests to use the common test data in keyframe-utils.js r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48995/diff/1-2/

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 8745343 [details]
MozReview Request: Bug 1265274 - Make tests to use the common test data in keyframe-utils.js r=birtles

https://reviewboard.mozilla.org/r/48995/#review45967

This looks great. Thank you!

::: testing/web-platform/tests/web-animations/animatable/animate.html:51
(Diff revision 2)
> -test(function(t) {
> +  test(function(t) {
> -  var div = createDiv(t);
> +    var div = createDiv(t);
> -  var anim = div.animate({ opacity: 0 }, 2000);
> -  assert_equals(anim.effect.getFrames().length, 1);
> -  assert_equals(anim.effect.getFrames()[0].opacity, '0');
> -}, 'Element.animate() accepts a single-valued keyframe specification');
> +    assert_throws(subtest.expected, function() {
> +      div.animate(subtest.input, 2000);
> +    });
> +  }, 'Element.animate() doesn\'t accept ' + subtest.desc);

Nit: 'does not accept'

::: testing/web-platform/tests/web-animations/animatable/animate.html:120
(Diff revision 2)
>    var div = createDiv(t);
>    var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);
>    assert_equals(anim.playState, 'pending');
>  }, 'Element.animate() calls play on the Animation');
>  
> -// Tests on CSSPseudoElement
> +//  Tests on CSSPseudoElement

Very very very minor nit: Is the extra space needed here?

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html:178
(Diff revision 2)
> +gInvalidKeyframesTests.forEach(function(subtest) {
> +  test(function(t) {
> +    assert_throws(subtest.expected, function() {
> +      new KeyframeEffectReadOnly(target, subtest.input);
> +    });
> +  }, "Invalid KeyframeEffect option by " + subtest.desc);

'KeyframeEffectReadOnly constructor throws with '

(I know this matches the string in setFrames.html but we should probably fix that string too.)

::: testing/web-platform/tests/web-animations/resources/keyframe-utils.js:400
(Diff revision 2)
> +  { desc:   "a one property one keyframe sequence",
> +    input:  [{ left: "10px" }],
> +    output: [{ offset: null, computedOffset: 1, easing: "linear",
> +               left: "10px" }] }

Perhaps we should put this at the start of the list?

Better still, we could add two version of this:

a) where offset: 1.0 is specified (added to the start of the list)
b) where offset is omitted (as it is here, added just before "a one property keyframe sequence with some omitted offsets" ?

What do you think?

Comment 12

[Ryo Kato [:r_kato]]

https://reviewboard.mozilla.org/r/48995/#review45967

> Perhaps we should put this at the start of the list?
> 
> Better still, we could add two version of this:
> 
> a) where offset: 1.0 is specified (added to the start of the list)
> b) where offset is omitted (as it is here, added just before "a one property keyframe sequence with some omitted offsets" ?
> 
> What do you think?

I agree to that. As another option, we could insert b) between "some omitted offsets" and "all omitted offsets". However, I think it would be better to put b) before "some omitted offsets" for consistency with a).

Comment 13

[Ryo Kato [:r_kato]]

Comment on attachment 8745343 [details]
MozReview Request: Bug 1265274 - Make tests to use the common test data in keyframe-utils.js r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48995/diff/2-3/

Comment 14

[Ryo Kato [:r_kato]]

I think this is ready to land. But I don't have the permission, so I would be happy if someone would push these patches to try server.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

I did it!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2940b97b270e

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c69991834c09

Comment 17

[Ryo Kato [:r_kato]]

hiro-san, thank you for landing this commit!

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c69991834c09