Closed Bug 1415448 Opened 2 years ago Closed 2 years ago

Tidy up and modernize web-platform-tests for Web Animations

Categories

(Core :: DOM: Animation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 18 obsolete files)

59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
For a number of bugs blocking bug 1398037 I've been gradually tidying up the associated web-platform-tests. However, rather than doing that test-by-test as I need to change them, it's probably better to go through all the tests and fix up the common issues. That should be easier to review and make the result more consistent.

In this bug I plan to:

* Update the titles of all the tests
* Move/rename a couple of test files that appear to be in the wrong place
* Replace var with const/let as appropriate
* Replace double quotes with single quotes as appropriate
* Use template strings were appropriate
* Use arrow functions
I started working on this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a60d9d3259e4b525a5eb15cd05881570f698212

I've only fixed the first two items from comment 0 since I encountered so many other problems with the tests I looked at. I might put this first set of patches up for review soon-ish since if there is any review feedback from them, fixing it is likely to bitrot all the other patches in the series badly.
I'm going to put up the first set of patches for review but there will be a few more (very simply ones) to come.
Comment on attachment 8927144 [details]
Bug 1415448 - Update web-platform-tests manifest;

https://reviewboard.mozilla.org/r/198396/#review203590
Attachment #8927144 - Flags: review?(hikezoe) → review+
Comment on attachment 8927145 [details]
Bug 1415448 - Tidy up titles of Web Animations web-platform-tests;

https://reviewboard.mozilla.org/r/198398/#review203598

::: testing/web-platform/tests/web-animations/animation-model/keyframe-effects/effect-value-overlapping-keyframes.html:3
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>Effect value computation tests when keyframes overlap</title>
> +<title>The effect value of a keyframe animation effect: Overlapping keyframes</title>

I am curious what the difference between 'keyframe animation effect' and 'keyframe effect' is.  The spec link is 'keyframe-animatioin-effect' but the spec itself says 'keyframe effect'.

::: testing/web-platform/tests/web-animations/interfaces/Animation/constructor.html:4
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>Animation constructor tests</title>
> +<title>Animation constructor</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#dom-animation-animation">

Nit: https.

::: testing/web-platform/tests/web-animations/interfaces/Animation/constructor.html:5
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>Animation constructor tests</title>
> +<title>Animation constructor</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#dom-animation-animation">
>  <link rel="author" title="Hiroyuki Ikezoe" href="mailto:hiikezoe@mozilla-japan.org">

Nit: drop this.

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/duration.html:4
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>duration tests</title>
> +<title>AnimationEffectTiming.duration</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#dom-animationeffecttiming-duration">

Nit: https.

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/endDelay.html:4
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>endDelay tests</title>
> +<title>AnimationEffectTiming.endDelay</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#dom-animationeffecttiming-enddelay">

Nit: https.

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/endDelay.html:5
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>endDelay tests</title>
> +<title>AnimationEffectTiming.endDelay</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#dom-animationeffecttiming-enddelay">
>  <link rel="author" title="Ryo Motozawa" href="mailto:motozawa@mozilla-japan.org">

Nit: drop this.

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/getAnimations.html:4
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>Element.getAnimations tests</title>
> +<title>Element.getAnimations</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#animationeffecttiming">

Nit: https.

I thought the spec link is incorrect but we already have test files for Animatable.getAnimations(), so the title is incorrect?  AnimationEffectTiming?

After I looked subsequent changes, the title seems to be correct, at least it's not incorrect.  I don't think it's really good but it's OK as it is.

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/getComputedStyle.html:4
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>getComputedStyle tests</title>
> +<title>getComputedStyle</title>
>  <link rel="help" href="http://w3c.github.io/web-animations/#animationeffecttiming">

Nit: https.

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/iterationStart.html:5
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>iterationStart tests</title>
> +<title>AnimationEffectTiming.iterationStart</title>
>  <link rel="help" href="https://w3c.github.io/web-animations/#dom-animationeffecttiming-iterationstart">
>  <link rel="author" title="Daisuke Akatsuka" href="mailto:daisuke@mozilla-japan.org">

Nit: drop this.

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/getComputedTiming.html:3
(Diff revision 1)
>  <!DOCTYPE html>
>  <meta charset=utf-8>
> -<title>KeyframeEffectReadOnly getComputedTiming() tests</title>
> +<title>KeyframeEffectReadOnly.getComputedTiming</title>

This should be AnimationEffectReadOnly.getComputedTiming?  I am not sure though.

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:4
(Diff revision 1)
>  <!doctype html>
>  <meta charset=utf-8>
> -<title>KeyframeEffect.iterationComposite tests</title>
> +<title>KeyframeEffect.iterationComposite</title>
>  <link rel="help" href="https://w3c.github.io/web-animations/#effect-accumulation-section">

https://w3c.github.io/web-animations/#dom-keyframeeffectreadonly-iterationcomposite ?

Oh no, this file tests accumulate only, so the title should be ' Effect accumulation'?
Attachment #8927145 - Flags: review?(hikezoe) → review+
Comment on attachment 8927146 [details]
Bug 1415448 - Rename setTarget.html to target.html;

https://reviewboard.mozilla.org/r/198400/#review203606
Attachment #8927146 - Flags: review?(hikezoe) → review+
Comment on attachment 8927147 [details]
Bug 1415448 - Move the tests for the type of the 'visibility' property to the animation-types section;

https://reviewboard.mozilla.org/r/198402/#review203608

Should we drop -type in the file name?  Though I don't have strong opnition, it's up to you.
Attachment #8927147 - Flags: review?(hikezoe) → review+
Comment on attachment 8927148 [details]
Bug 1415448 - Rename discrete-animation.html test file to discrete-type.html;

https://reviewboard.mozilla.org/r/198404/#review203612
Attachment #8927148 - Flags: review?(hikezoe) → review+
Comment on attachment 8927149 [details]
Bug 1415448 - Tidy up test descriptions for AnimationEffectTiming tests;

https://reviewboard.mozilla.org/r/198406/#review203616

It would be nice to modify descriptions in README.md as well?

::: testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/duration.html:59
(Diff revision 1)
>  test(function(t) {
>    var div = createDiv(t);
>    assert_throws({ name: 'TypeError' }, function() {
>      div.animate({ opacity: [ 0, 1 ] }, -1);
>    });
> -}, 'set negative duration in animate using a duration parameter');
> +}, 'animate() throws when passed a negative number');

We might want to move these tests that throw on animate() method into other files in future.
Attachment #8927149 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> :::
> testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/
> duration.html:59
> (Diff revision 1)
> >  test(function(t) {
> >    var div = createDiv(t);
> >    assert_throws({ name: 'TypeError' }, function() {
> >      div.animate({ opacity: [ 0, 1 ] }, -1);
> >    });
> > -}, 'set negative duration in animate using a duration parameter');
> > +}, 'animate() throws when passed a negative number');
> 
> We might want to move these tests that throw on animate() method into other
> files in future.

Yes, definitely. There are so many things that need fixing in these files. This bug is definitely not going to fix everything but hopefully it fixes a lot of common things. We can fix more specific problems when we next touch these files.
Comment on attachment 8927150 [details]
Bug 1415448 - Drop getComputedStyle.html test in interfaces/AnimationEffectTiming;

https://reviewboard.mozilla.org/r/198408/#review203620

MozReview is really useless for this kind of changes.  I haven't checked tests were moved correctly, but I believe it did.
Attachment #8927150 - Flags: review?(hikezoe) → review+
Comment on attachment 8927151 [details]
Bug 1415448 - Update test descriptions in interfaces/Animatable/getAnimations.html;

https://reviewboard.mozilla.org/r/198410/#review203622

::: commit-message-0ee73:3
(Diff revision 1)
> +Bug 1415448 - Update test descriptions in interfaces/Animatable/getAnimations.html; r?hiro
> +
> +We plan to merge in tests here from AnimationEffectTiming/getAnimations.html so

Nice!
Attachment #8927151 - Flags: review?(hikezoe) → review+
Comment on attachment 8927152 [details]
Bug 1415448 - Move tests from AnimationEffectTiming/getAnimations.html to Animatable/getAnimations.html;

https://reviewboard.mozilla.org/r/198412/#review203624

r=me with dropping AnimationEffectTiming/getAnimations.html.

::: testing/web-platform/tests/web-animations/interfaces/Animatable/getAnimations.html:85
(Diff revision 1)
> +                      'Animation should be returned after extending the'
> +                      + ' duration');
> +
> +  animation.effect.timing.duration = 0;
> +  assert_array_equals(div.getAnimations(), [],
> +                      'Animations should not be returned after setting the'

Nit: Animaion instead of Animations?
Attachment #8927152 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Comment on attachment 8927145 [details]
> Bug 1415448 - Tidy up titles of Web Animations web-platform-tests;
> 
> https://reviewboard.mozilla.org/r/198398/#review203598
> 
> :::
> testing/web-platform/tests/web-animations/animation-model/keyframe-effects/
> effect-value-overlapping-keyframes.html:3
> (Diff revision 1)
> >  <!DOCTYPE html>
> >  <meta charset=utf-8>
> > -<title>Effect value computation tests when keyframes overlap</title>
> > +<title>The effect value of a keyframe animation effect: Overlapping keyframes</title>
> 
> I am curious what the difference between 'keyframe animation effect' and
> 'keyframe effect' is.  The spec link is 'keyframe-animatioin-effect' but the
> spec itself says 'keyframe effect'.

Yeah, you're right. Keyframe effect is sufficient here.

> :::
> testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/
> getAnimations.html:4
> (Diff revision 1)
> >  <!DOCTYPE html>
> >  <meta charset=utf-8>
> > -<title>Element.getAnimations tests</title>
> > +<title>Element.getAnimations</title>
> >  <link rel="help" href="http://w3c.github.io/web-animations/#animationeffecttiming">
> 
> Nit: https.
> 
> I thought the spec link is incorrect but we already have test files for
> Animatable.getAnimations(), so the title is incorrect? 
> AnimationEffectTiming?
> 
> After I looked subsequent changes, the title seems to be correct, at least
> it's not incorrect.  I don't think it's really good but it's OK as it is.

Yes, later I will delete this file altogether so for now I will not change the spec link.

> :::
> testing/web-platform/tests/web-animations/interfaces/AnimationEffectTiming/
> getComputedStyle.html:4
> (Diff revision 1)
> >  <!DOCTYPE html>
> >  <meta charset=utf-8>
> > -<title>getComputedStyle tests</title>
> > +<title>getComputedStyle</title>
> >  <link rel="help" href="http://w3c.github.io/web-animations/#animationeffecttiming">
> 
> Nit: https.

Again, I am going to delete this file so I will leave the spec link as-is (so it doesn't bitrot the patch where I delete the file).

> :::
> testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/
> getComputedTiming.html:3
> (Diff revision 1)
> >  <!DOCTYPE html>
> >  <meta charset=utf-8>
> > -<title>KeyframeEffectReadOnly getComputedTiming() tests</title>
> > +<title>KeyframeEffectReadOnly.getComputedTiming</title>
> 
> This should be AnimationEffectReadOnly.getComputedTiming?  I am not sure
> though.

Yes, probably. I've made a note to do that in a follow-up.

> :::
> testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/
> iterationComposite.html:4
> (Diff revision 1)
> >  <!doctype html>
> >  <meta charset=utf-8>
> > -<title>KeyframeEffect.iterationComposite tests</title>
> > +<title>KeyframeEffect.iterationComposite</title>
> >  <link rel="help" href="https://w3c.github.io/web-animations/#effect-accumulation-section">
> 
> https://w3c.github.io/web-animations/#dom-keyframeeffectreadonly-
> iterationcomposite ?
> 
> Oh no, this file tests accumulate only, so the title should be ' Effect
> accumulation'?

Yeah, this test should be moved to the animation-model. I've made a note to do that too.
Keywords: leave-open
Attachment #8927144 - Attachment is obsolete: true
Attachment #8927145 - Attachment is obsolete: true
Attachment #8927146 - Attachment is obsolete: true
Attachment #8927147 - Attachment is obsolete: true
Attachment #8927148 - Attachment is obsolete: true
Attachment #8927149 - Attachment is obsolete: true
Attachment #8927150 - Attachment is obsolete: true
Attachment #8927151 - Attachment is obsolete: true
Attachment #8927152 - Attachment is obsolete: true
Attachment #8927226 - Flags: review?(hikezoe)
Attachment #8927227 - Flags: review?(hikezoe)
Attachment #8927228 - Flags: review?(hikezoe)
Attachment #8927229 - Flags: review?(hikezoe)
Attachment #8927230 - Flags: review?(hikezoe)
Attachment #8927231 - Flags: review?(hikezoe)
Attachment #8927232 - Flags: review?(hikezoe)
Attachment #8927233 - Flags: review?(hikezoe)
Attachment #8927234 - Flags: review?(hikezoe)
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab43c3e25edd
Update web-platform-tests manifest; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b0fcc3f732
Tidy up titles of Web Animations web-platform-tests; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/316d85f7d5dc
Rename setTarget.html to target.html; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9a5a6c1b06
Move the tests for the type of the 'visibility' property to the animation-types section; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3dee378cbad
Rename discrete-animation.html test file to discrete.html; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/a42ad6fd6afa
Tidy up test descriptions for AnimationEffectTiming tests; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0cbe23c132e
Drop getComputedStyle.html test in interfaces/AnimationEffectTiming; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/edba74152c40
Update test descriptions in interfaces/Animatable/getAnimations.html; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/b652edba2199
Move tests from AnimationEffectTiming/getAnimations.html to Animatable/getAnimations.html; r=hiro
Attachment #8927226 - Attachment is obsolete: true
Attachment #8927227 - Attachment is obsolete: true
Attachment #8927228 - Attachment is obsolete: true
Attachment #8927229 - Attachment is obsolete: true
Attachment #8927230 - Attachment is obsolete: true
Attachment #8927231 - Attachment is obsolete: true
Attachment #8927232 - Attachment is obsolete: true
Attachment #8927233 - Attachment is obsolete: true
Attachment #8927234 - Attachment is obsolete: true
Comment on attachment 8928809 [details]
Bug 1415448 - Use arrow functions in web-platform-tests/web-animations;

https://reviewboard.mozilla.org/r/200090/#review205224

It would be nice to update README.md and testcommon.js as well.  IIUC, below two files also needs to be updated.

animation-model/keyframe-effects/effect-value-transformed-distance.html
animation-model/animation-types/property-types.js
Attachment #8928809 - Flags: review?(hikezoe) → review+
Comment on attachment 8928810 [details]
Bug 1415448 - Replace var with const/let in web-platform-tests/web-animations;

https://reviewboard.mozilla.org/r/200092/#review205230

Looks good. I did check only 'let's though.

::: testing/web-platform/tests/web-animations/interfaces/Animation/ready.html:37
(Diff revision 1)
> -  var div = createDiv(t);
> -  var animation = div.animate(null, 100 * MS_PER_SEC);
> +  let div = createDiv(t);
> +  let animation = div.animate(null, 100 * MS_PER_SEC);

Nit: const.
Attachment #8928810 - Flags: review?(hikezoe) → review+
Comment on attachment 8928811 [details]
Bug 1415448 - Consistently use single quotes for string literals in web-platform-tests/web-animations;

https://reviewboard.mozilla.org/r/200094/#review205232

I guess IDL part in idlharness.html intentionally uses double quote there.
Attachment #8928811 - Flags: review?(hikezoe) → review+
Comment on attachment 8928812 [details]
Bug 1415448 - Use template literals in a few places in web-platform-tests/web-animations;

https://reviewboard.mozilla.org/r/200096/#review205238
Attachment #8928812 - Flags: review?(hikezoe) → review+
Comment on attachment 8928813 [details]
Bug 1415448 - Consistently use strict mode in web-platform-tests/web-animations;

https://reviewboard.mozilla.org/r/200098/#review205242

Below also need to be updated.

animation-model/keyframe-effects/effect-value-context.html
animation-model/animation-types/property-types.js
resources/easing-tests.js
resources/effect-tests.js
Attachment #8928813 - Flags: review?(hikezoe) → review+
Comment on attachment 8928815 [details]
Bug 1415448 - Update web-platform-tests MANIFEST.json;

https://reviewboard.mozilla.org/r/200102/#review205248
Attachment #8928815 - Flags: review?(hikezoe) → review+
Comment on attachment 8928814 [details]
Bug 1415448 - Use for...of instead of forEach in web-platform-tests/web-animations;

https://reviewboard.mozilla.org/r/200100/#review205250
Attachment #8928814 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #45)
> Comment on attachment 8928813 [details]
> Bug 1415448 - Consistently use strict mode in
> web-platform-tests/web-animations;
> 
> https://reviewboard.mozilla.org/r/200098/#review205242
> 
> Below also need to be updated.
> 
> animation-model/keyframe-effects/effect-value-context.html
> animation-model/animation-types/property-types.js
> resources/easing-tests.js
> resources/effect-tests.js

Do'h!  I did grep without applying the patch.  Never mind about this comment.
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff031af585d8
Use arrow functions in web-platform-tests/web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/31f60780e3c0
Replace var with const/let in web-platform-tests/web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/b0811e58e932
Consistently use single quotes for string literals in web-platform-tests/web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/066264c8cc1f
Use template literals in a few places in web-platform-tests/web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/355ad237707a
Consistently use strict mode in web-platform-tests/web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/5d398f92e237
Use for...of instead of forEach in web-platform-tests/web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/1735548b5e14
Update web-platform-tests MANIFEST.json; r=hiro
All that remains here now is:

* Move interfaces/KeyframeEffect/getComputedTiming.html to AnimationEffectTiming
* Move KeyframeEffect/iterationComposite.html to animation-model
Depends on: 1417808
(In reply to Brian Birtles (:birtles) from comment #57)
> All that remains here now is:
> 
> * Move interfaces/KeyframeEffect/getComputedTiming.html to
> AnimationEffectTiming
> * Move KeyframeEffect/iterationComposite.html to animation-model

Splitting off bug 1417808 for this because MozReview.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.