We're missing them for at least AnimationEffectTiming, KeyframeEffect, and AnimationPlaybackEvent. However, I think before adding them the DocumentTimeline tests need a lot of re-organizing.

Comment on attachment 8922639 [details] Bug 1411806 - Move document timeline tests to files corresponding to the sections in the spec they test; https://reviewboard.mozilla.org/r/193752/#review198870

Comment on attachment 8922640 [details] Bug 1411806 - Update test descriptions for document timeline tests; https://reviewboard.mozilla.org/r/193754/#review198872 ::: testing/web-platform/tests/web-animations/timing-model/timelines/timelines.html:19 (Diff revision 1) > const timeAtStart = window.performance.now(); > while (window.performance.now() - timeAtStart < 50) { > // Wait 50ms > } > assert_equals(document.timeline.currentTime, valueAtStart, > - 'document.timeline.currentTime does not change within a script block'); > + 'Timeline time does not change within an animation frame'); To be accurate, here is not inside an animation frame (an rAF), it's in a micro task for a Promise, but I guess this 'animation frame' does not mean just for rAF? ::: testing/web-platform/tests/web-animations/timing-model/timelines/timelines.html:71 (Diff revision 1) > assert_equals(document.timeline.currentTime, firstRafTime, > - 'currentTime should be the same'); > + 'Timeline time should be the same'); > }); > t.done(); > }); > -}, 'document.timeline.currentTime time should be the same for all RAF' > +}, 'Timeline time should be the same for all RAF callbacks in a frame'); Ok, a mismatch happens here. 'frame' is used here, but in other parts 'animation frame' were used, it should be consistent?

Comment on attachment 8922641 [details] Bug 1411806 - Move idlharness test for DocumentTimeline to DocumentTimeline directory; https://reviewboard.mozilla.org/r/193756/#review198876

Comment on attachment 8922642 [details] Bug 1411806 - Tidy up DocumentTimeline/idlharness.html test; https://reviewboard.mozilla.org/r/193758/#review198878

Comment on attachment 8922643 [details] Bug 1411806 - Tidy up Animation/idlharness.html test; https://reviewboard.mozilla.org/r/193760/#review198880

Comment on attachment 8922644 [details] Bug 1411806 - Add idlharness test for KeyframeEffect; https://reviewboard.mozilla.org/r/193762/#review198882 Note that AnimationEffectTimingReadOnly is tested in a subsequent patch. ::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/idlharness.html:19 (Diff revision 1) > + double delay = 0; > + double endDelay = 0; Interesting. These '0' value match what the spec describes, but why '0' instead of '0.0'? iterationStart below is '0.0' instead. Also the webidl file in our tree uses '0.0'.

Comment on attachment 8922645 [details] Bug 1411806 - Drop unnecessary test for KeyframeEffect.timing now that idlharness.html covers the same content; https://reviewboard.mozilla.org/r/193764/#review198886

Comment on attachment 8922646 [details] Bug 1411806 - Combine copy constructor tests for KeyframeEffect and KeyframeEffectReadOnly into one file; https://reviewboard.mozilla.org/r/193766/#review198888

Comment on attachment 8922647 [details] Bug 1411806 - Tidy up KeyframeEffect(ReadOnly) constructor tests; https://reviewboard.mozilla.org/r/193768/#review198890 ::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/constructor.html:108 (Diff revision 1) > > gKeyframesTests.forEach(function(subtest) { > test(function(t) { > - var effect = new KeyframeEffectReadOnly(target, subtest.input); > + const effect = new KeyframeEffectReadOnly(target, subtest.input); > assert_frame_lists_equal(effect.getKeyframes(), subtest.output); > - }, "a KeyframeEffectReadOnly can be constructed with " + subtest.desc); > + }, 'A KeyframeEffectReadOnly can be constructed with ' + subtest.desc); Use template string? ::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/constructor.html:116 (Diff revision 1) > - var effect = new KeyframeEffectReadOnly(target, subtest.input); > - var secondEffect = > + const effect = new KeyframeEffectReadOnly(target, subtest.input); > + const secondEffect = > new KeyframeEffectReadOnly(target, effect.getKeyframes()); > assert_frame_lists_equal(secondEffect.getKeyframes(), > effect.getKeyframes()); > - }, "a KeyframeEffectReadOnly constructed with " + subtest.desc + > + }, 'A KeyframeEffectReadOnly constructed with ' + subtest.desc + Likewise. ::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/constructor.html:156 (Diff revision 1) > - { left: ["10px", "20px"] }, > + { left: ['10px', '20px'] }, > - stest.input); > + stest.input); > > // Helper function to provide default expected values when the test does > // not supply them. > - var expected = function(field, defaultValue) { > + const expected = (field, defaultValue) => { I am wondering why only this function was reveised with arrow function. ::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/constructor.html:172 (Diff revision 1) > - assert_equals(timing.duration, expected("duration", "auto"), > - "timing duration"); > - assert_equals(timing.direction, expected("direction", "normal"), > - "timing direction"); > + assert_equals(timing.duration, expected('duration', 'auto'), > + 'timing duration'); > + assert_equals(timing.direction, expected('direction', 'normal'), > + 'timing direction'); > > - }, "a KeyframeEffectReadOnly constructed by " + stest.desc); > + }, 'A KeyframeEffectReadOnly constructed by ' + stest.desc); Use template string? ::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/constructor.html:182 (Diff revision 1) > assert_throws(new TypeError, function() { > new KeyframeEffectReadOnly(target, > - { left: ["10px", "20px"] }, > + { left: ['10px', '20px'] }, > stest.input); > }); > - }, "Invalid KeyframeEffectReadOnly option by " + stest.desc); > + }, 'Invalid KeyframeEffectReadOnly option by ' + stest.desc); Likewise.

Comment on attachment 8922648 [details] Bug 1411806 - Drop references to no-longer-existent 'spacing' property in KeyframeEffect(ReadOny) constructor tests; https://reviewboard.mozilla.org/r/193770/#review198892

Comment on attachment 8922649 [details] Bug 1411806 - Add idlharness test for AnimationEffectTimingReadOnly; https://reviewboard.mozilla.org/r/193772/#review198894

Comment on attachment 8922650 [details] Bug 1411806 - Add idlharness test for AnimationPlaybackEvent; https://reviewboard.mozilla.org/r/193774/#review198896

Comment on attachment 8922651 [details] Bug 1411806 - Add test for AnimationPlaybackEvent constructor; https://reviewboard.mozilla.org/r/193776/#review198898 Put a link to the spec?

As we discussed on IRC, it would be nice to have links to the spec in each idlharness.html. Thanks!

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22) > Comment on attachment 8922647 [details] > Bug 1411806 - Tidy up KeyframeEffect(ReadOnly) constructor tests; > > https://reviewboard.mozilla.org/r/193768/#review198890 > > ::: > testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/ > constructor.html:108 > (Diff revision 1) > > > > gKeyframesTests.forEach(function(subtest) { > > test(function(t) { > > - var effect = new KeyframeEffectReadOnly(target, subtest.input); > > + const effect = new KeyframeEffectReadOnly(target, subtest.input); > > assert_frame_lists_equal(effect.getKeyframes(), subtest.output); > > - }, "a KeyframeEffectReadOnly can be constructed with " + subtest.desc); > > + }, 'A KeyframeEffectReadOnly can be constructed with ' + subtest.desc); > > Use template string? I generally didn't change these when it was simply concatenating two parts since template strings don't make it any shorter or easier to read in that case but I'm happy to change this. > ::: > testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/ > constructor.html:156 > (Diff revision 1) > > - { left: ["10px", "20px"] }, > > + { left: ['10px', '20px'] }, > > - stest.input); > > + stest.input); > > > > // Helper function to provide default expected values when the test does > > // not supply them. > > - var expected = function(field, defaultValue) { > > + const expected = (field, defaultValue) => { > > I am wondering why only this function was reveised with arrow function. Arrow functions are generally preferable to regular functions (easier to read, this is normally more predictable etc.), particularly for local definitions. Some coding styles like airbnb's (which is probably the most popular style at the moment) even require this. I haven't changed all the outer functions (that can happen later), but for local helpers it seems worthwhile.

Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2951f637d061 Move document timeline tests to files corresponding to the sections in the spec they test; r=hiro https://hg.mozilla.org/integration/autoland/rev/7feda3cfcbf4 Update test descriptions for document timeline tests; r=hiro https://hg.mozilla.org/integration/autoland/rev/53610608036f Move idlharness test for DocumentTimeline to DocumentTimeline directory; r=hiro https://hg.mozilla.org/integration/autoland/rev/1abb342de639 Tidy up DocumentTimeline/idlharness.html test; r=hiro https://hg.mozilla.org/integration/autoland/rev/19617296fc5b Tidy up Animation/idlharness.html test; r=hiro https://hg.mozilla.org/integration/autoland/rev/3803aef029b3 Add idlharness test for KeyframeEffect; r=hiro https://hg.mozilla.org/integration/autoland/rev/af857a550e35 Drop unnecessary test for KeyframeEffect.timing now that idlharness.html covers the same content; r=hiro https://hg.mozilla.org/integration/autoland/rev/add7b5de8f30 Combine copy constructor tests for KeyframeEffect and KeyframeEffectReadOnly into one file; r=hiro https://hg.mozilla.org/integration/autoland/rev/12848ee901bf Tidy up KeyframeEffect(ReadOnly) constructor tests; r=hiro https://hg.mozilla.org/integration/autoland/rev/69c7fd9d5903 Drop references to no-longer-existent 'spacing' property in KeyframeEffect(ReadOny) constructor tests; r=hiro https://hg.mozilla.org/integration/autoland/rev/5f9558b14c38 Add idlharness test for AnimationEffectTimingReadOnly; r=hiro https://hg.mozilla.org/integration/autoland/rev/178f52e9716d Add idlharness test for AnimationPlaybackEvent; r=hiro https://hg.mozilla.org/integration/autoland/rev/07668a227d73 Add test for AnimationPlaybackEvent constructor; r=hiro

https://hg.mozilla.org/mozilla-central/rev/2951f637d061 https://hg.mozilla.org/mozilla-central/rev/7feda3cfcbf4 https://hg.mozilla.org/mozilla-central/rev/53610608036f https://hg.mozilla.org/mozilla-central/rev/1abb342de639 https://hg.mozilla.org/mozilla-central/rev/19617296fc5b https://hg.mozilla.org/mozilla-central/rev/3803aef029b3 https://hg.mozilla.org/mozilla-central/rev/af857a550e35 https://hg.mozilla.org/mozilla-central/rev/add7b5de8f30 https://hg.mozilla.org/mozilla-central/rev/12848ee901bf https://hg.mozilla.org/mozilla-central/rev/69c7fd9d5903 https://hg.mozilla.org/mozilla-central/rev/5f9558b14c38 https://hg.mozilla.org/mozilla-central/rev/178f52e9716d https://hg.mozilla.org/mozilla-central/rev/07668a227d73