Closed Bug 1402170 Opened 4 years ago Closed 4 years ago

Add test for error handling when processing a keyframes argument

Categories

(Core :: DOM: Animation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(21 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
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
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
Now that the error handling behavior when processing a keyframes argument has been clarified[1] we should add a test that all properties are read before any exceptions are thrown.

At the same time I'd like to tidy up this test so that:

* The <title> is correct
* It uses Element.animate() instead of the KeyframeEffectReadOnly constructor where possible (rationale: most UAs will implement Element.animate() first and we'd like to be sure they test this before shipping)
* It uses KeyframeEffect instead of KeyframeEffectReadOnly when we can't use Element.animate (rational: KeyframeEffectReadOnly might disappear, or might ship later than KeyframeEffect)
* Tests for accessing order are moved here (see FIXME at bottom of file)
* Tests for invalid timing functions on keyframes are moved here?

There will likely be other tidy ups needed too.

[1] https://github.com/w3c/web-animations/commit/d696468777c12a13bc7c46aa1a72c358e8da15fc
Blocks: 1404774
Comment on attachment 8914173 [details]
Bug 1402170 - Fix title in processing-a-keyframes-argument.html test;

https://reviewboard.mozilla.org/r/182490/#review190418
Attachment #8914173 - Flags: review?(hikezoe) → review+
Comment on attachment 8914174 [details]
Bug 1402170 - Consistently use KeyframeEffect constructor for testing in processing-a-keyframes-argument.html;

https://reviewboard.mozilla.org/r/182492/#review190422
Attachment #8914174 - Flags: review?(hikezoe) → review+
Comment on attachment 8914175 [details]
Bug 1402170 - Use ES6 let/const in processing-a-keyframes-argument.html;

https://reviewboard.mozilla.org/r/182494/#review190424
Attachment #8914175 - Flags: review?(hikezoe) → review+
Comment on attachment 8914176 [details]
Bug 1402170 - Use for...of instead of forEach for several tests in processing-a-keyframes-argument.html;

https://reviewboard.mozilla.org/r/182496/#review190426

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument.html:168
(Diff revision 1)
>  
> -gEquivalentSyntaxTests.forEach(function({description, indexedKeyframes, sequencedKeyframes}) {
> +for (const {description, indexedKeyframes, sequencedKeyframes} of
> +     gEquivalentSyntaxTests) {
>    test(function(t) {
>      assertEquivalentKeyframeSyntax(indexedKeyframes, sequencedKeyframes);
> -  }, 'Equivalent property indexed and sequenced keyframes: ' + description);
> +  }, `Equivalent property indexed and sequenced keyframes ${description}`);

Did you intentionally drop ':' after 'keyframes'?
Though it's not a bit deal since a failure test case annotated in processing-a-keyframes-argument.html.ini will be dropped in a subsequent patch, I am just curious.
Attachment #8914176 - Flags: review?(hikezoe) → review+
Comment on attachment 8914177 [details]
Bug 1402170 - Use template strings in processing-a-keyframes-argument.html;

https://reviewboard.mozilla.org/r/182498/#review190428
Attachment #8914177 - Flags: review?(hikezoe) → review+
Comment on attachment 8914179 [details]
Bug 1402170 - Update some test descriptions;

https://reviewboard.mozilla.org/r/182502/#review190430
Attachment #8914179 - Flags: review?(hikezoe) → review+
Comment on attachment 8914178 [details]
Bug 1402170 - Drop invalid test for offsets;

https://reviewboard.mozilla.org/r/182500/#review190432
Attachment #8914178 - Flags: review?(hikezoe) → review+
Comment on attachment 8914180 [details]
Bug 1402170 - Move test for accessing keyframe property order to processing-a-keyframes-argument.html;

https://reviewboard.mozilla.org/r/182548/#review190434

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument.html:324
(Diff revision 1)
>      {offset: null, computedOffset: 0, easing: 'linear', top: '100px'},
>      {offset: null, computedOffset: 1, easing: 'linear', top: '200px'},
>    ]);
>  }, 'Only properties defined directly on property-indexed keyframes are read');
>  
> -// FIXME: Test that properties are accessed in ascending order by Unicode
> +test(function(t) {

I was going to leave a comment later, but I should do it here.
This file has both 'test(function(t) {..})' and 'test(() => {..})'. We should use arrow function all over the place?

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument.html:325
(Diff revision 1)
> -//        codepoint
> -//        (There is an existing test for this in
> -//        keyframe-effect/constructor.html that should be moved here.)
> +  var expectedOrder = ['composite', 'easing', 'offset', 'left', 'marginLeft'];
> +  var actualOrder = [];
> +  var kf1 = {};

Nit: Use const instead.
Attachment #8914180 - Flags: review?(hikezoe) → review+
Comment on attachment 8914181 [details]
Bug 1402170 - Tidy up test for property access order somewhat;

https://reviewboard.mozilla.org/r/182550/#review190436

Oh, this patch changed the 'var's.
Attachment #8914181 - Flags: review?(hikezoe) → review+
Comment on attachment 8914182 [details]
Bug 1402170 - Update various test descriptions to make them testable statements;

https://reviewboard.mozilla.org/r/182552/#review190438
Attachment #8914182 - Flags: review?(hikezoe) → review+
Comment on attachment 8914183 [details]
Bug 1402170 - Consistently use spaces in object notation;

https://reviewboard.mozilla.org/r/182554/#review190442

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument.html:194
(Diff revision 1)
>  test(() => {
>    const keyframes = createIterable([
> -    {done: false, value: {left: '100px'}},
> -    {done: false, value: {left: '300px'}},
> -    {done: false, value: {left: '200px'}},
> -    {done: true},
> +    { done: false, value: { left: '100px' } },
> +    { done: false, value: { left: '300px' } },
> +    { done: false, value: { left: '200px' } },
> +    { done: true},

Nit: needs a space before }.

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument.html:211
(Diff revision 1)
>  test(() => {
>    const effect = new KeyframeEffect(null, createIterable([
> -    {done: false, value: {left: '100px', top: '200px'}},
> -    {done: false, value: {left: '300px'}},
> -    {done: false, value: {left: '200px', top: '100px'}},
> -    {done: true},
> +    { done: false, value: { left: '100px', top: '200px' } },
> +    { done: false, value: { left: '300px' } },
> +    { done: false, value: { left: '200px', top: '100px' } },
> +    { done: true},

Nit: Likewise here.

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument.html:228
(Diff revision 1)
>  test(() => {
>    const effect = new KeyframeEffect(null, createIterable([
> -    {done: false, value: {left: '100px'}},
> -    {done: false, value: {left: '250px', offset: 0.75}},
> -    {done: false, value: {left: '200px'}},
> -    {done: true},
> +    { done: false, value: { left: '100px' } },
> +    { done: false, value: { left: '250px', offset: 0.75 } },
> +    { done: false, value: { left: '200px' } },
> +    { done: true},

Likewise here.
Attachment #8914183 - Flags: review?(hikezoe) → review+
Comment on attachment 8914184 [details]
Bug 1402170 - Merge gInvalidEasingInKeyframeSequenceTests with gInvalidEasings;

https://reviewboard.mozilla.org/r/182556/#review190444

I think an additional patch to change 'test(function(t)' to arrow one is neccesary in this patch series.
Attachment #8914184 - Flags: review?(hikezoe) → review+
Comment on attachment 8914185 [details]
Bug 1402170 - Rename processing-a-keyframes-argument.html to have a 001 extension;

https://reviewboard.mozilla.org/r/182558/#review190446

Windows..
Attachment #8914185 - Flags: review?(hikezoe) → review+
Comment on attachment 8914192 [details]
Bug 1402170 - Rename copy-contructor.html to copy-constructor.html in two places in wpt for web-animations;

https://reviewboard.mozilla.org/r/185512/#review190450
Attachment #8914192 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> Comment on attachment 8914176 [details]
> Bug 1402170 - Use for...of instead of forEach for several tests in
> processing-a-keyframes-argument.html;
...
> > -gEquivalentSyntaxTests.forEach(function({description, indexedKeyframes, sequencedKeyframes}) {
> > +for (const {description, indexedKeyframes, sequencedKeyframes} of
> > +     gEquivalentSyntaxTests) {
> >    test(function(t) {
> >      assertEquivalentKeyframeSyntax(indexedKeyframes, sequencedKeyframes);
> > -  }, 'Equivalent property indexed and sequenced keyframes: ' + description);
> > +  }, `Equivalent property indexed and sequenced keyframes ${description}`);
> 
> Did you intentionally drop ':' after 'keyframes'?
> Though it's not a bit deal since a failure test case annotated in
> processing-a-keyframes-argument.html.ini will be dropped in a subsequent
> patch, I am just curious.

No, that's probably a mistake. I'll fix it. (I wrote these patches a few weeks ago so I don't really remember).

(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> Comment on attachment 8914180 [details]
> Bug 1402170 - Move test for accessing keyframe property order to
> processing-a-keyframes-argument.html;
...
> I was going to leave a comment later, but I should do it here.
> This file has both 'test(function(t) {..})' and 'test(() => {..})'. We
> should use arrow function all over the place?

Yeah, I agree. I'll add a follow up for that.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
> Comment on attachment 8914185 [details]
> Bug 1402170 - Rename processing-a-keyframes-argument.html to have a 001
> extension;
> 
> https://reviewboard.mozilla.org/r/182558/#review190446
> 
> Windows..

What does that mean? The files all appear to have unix line endings locally (but I did move these patches between Windows and Unix machines numerous times).
Comment on attachment 8914186 [details]
Bug 1402170 - Move tests for easing values on keyframes to processing-a-keyframes-argument-002.html;

https://reviewboard.mozilla.org/r/182560/#review190452
Attachment #8914186 - Flags: review?(hikezoe) → review+
Comment on attachment 8914187 [details]
Bug 1402170 - Tidy up processing-a-keyframes-argument-002.html;

https://reviewboard.mozilla.org/r/182562/#review190454
Attachment #8914187 - Flags: review?(hikezoe) → review+
Comment on attachment 8914188 [details]
Bug 1402170 - Move ease parsing tests to easing-tests.js;

https://reviewboard.mozilla.org/r/182564/#review190456
Attachment #8914188 - Flags: review?(hikezoe) → review+
Comment on attachment 8914189 [details]
Bug 1402170 - Use KeyframeEffect constructor in processing-a-keyframes-argument-002.html;

https://reviewboard.mozilla.org/r/182566/#review190458
Attachment #8914189 - Flags: review?(hikezoe) → review+
Comment on attachment 8914190 [details]
Bug 1402170 - Update test descriptions in processing-a-keyframes-argument-002.html;

https://reviewboard.mozilla.org/r/182568/#review190460
Attachment #8914190 - Flags: review?(hikezoe) → review+
Comment on attachment 8914191 [details]
Bug 1402170 - Add tests for error handling when parsing the 'easing' property on keyframes;

https://reviewboard.mozilla.org/r/185510/#review190462
Attachment #8914191 - Flags: review?(hikezoe) → review+
(In reply to Brian Birtles (:birtles) from comment #35)

> (In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
> > Comment on attachment 8914185 [details]
> > Bug 1402170 - Rename processing-a-keyframes-argument.html to have a 001
> > extension;
> > 
> > https://reviewboard.mozilla.org/r/182558/#review190446
> > 
> > Windows..
> 
> What does that mean? The files all appear to have unix line endings locally
> (but I did move these patches between Windows and Unix machines numerous
> times).

I meant the patch length limitation on Windows is not good.
Thanks for the reviews!
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Comment on attachment 8914219 [details]
Bug 1402170 - Consistently use arrow syntax for functions in processing-a-keyframes-argument-*.html tests;

https://reviewboard.mozilla.org/r/185542/#review190478

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001.html:52
(Diff revision 1)
> -    get: function() { _propAccessCount++; },
> +    get: () => { _propAccessCount++; },
>      enumerable: true,
>    });
>  
>    Object.defineProperty(this, 'propAccessCount', {
> -    get: function() { return _propAccessCount; }
> +    get: () => { return _propAccessCount; }

Oops, this should be simplified...
Comment on attachment 8914219 [details]
Bug 1402170 - Consistently use arrow syntax for functions in processing-a-keyframes-argument-*.html tests;

https://reviewboard.mozilla.org/r/185542/#review190482

Nice!
Attachment #8914219 - Flags: review?(hikezoe) → review+
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fef0a3277d1
Fix title in processing-a-keyframes-argument.html test; r=hiro
https://hg.mozilla.org/integration/autoland/rev/bf76291acd21
Consistently use KeyframeEffect constructor for testing in processing-a-keyframes-argument.html; r=hiro
https://hg.mozilla.org/integration/autoland/rev/98fdad400746
Use ES6 let/const in processing-a-keyframes-argument.html; r=hiro
https://hg.mozilla.org/integration/autoland/rev/5951672afb9d
Use for...of instead of forEach for several tests in processing-a-keyframes-argument.html; r=hiro
https://hg.mozilla.org/integration/autoland/rev/94a49bab02b0
Use template strings in processing-a-keyframes-argument.html; r=hiro
https://hg.mozilla.org/integration/autoland/rev/204dc1bf9d01
Drop invalid test for offsets; r=hiro
https://hg.mozilla.org/integration/autoland/rev/034009ae72f7
Update some test descriptions; r=hiro
https://hg.mozilla.org/integration/autoland/rev/9d2ce508c129
Move test for accessing keyframe property order to processing-a-keyframes-argument.html; r=hiro
https://hg.mozilla.org/integration/autoland/rev/4cc5c419823b
Tidy up test for property access order somewhat; r=hiro
https://hg.mozilla.org/integration/autoland/rev/40a7c9e5dab4
Update various test descriptions to make them testable statements; r=hiro
https://hg.mozilla.org/integration/autoland/rev/cd9648f4419c
Consistently use spaces in object notation; r=hiro
https://hg.mozilla.org/integration/autoland/rev/0a44bfaed20e
Merge gInvalidEasingInKeyframeSequenceTests with gInvalidEasings; r=hiro
https://hg.mozilla.org/integration/autoland/rev/48b90026515c
Rename processing-a-keyframes-argument.html to have a 001 extension; r=hiro
https://hg.mozilla.org/integration/autoland/rev/a58cf0030969
Move tests for easing values on keyframes to processing-a-keyframes-argument-002.html; r=hiro
https://hg.mozilla.org/integration/autoland/rev/22061df37890
Tidy up processing-a-keyframes-argument-002.html; r=hiro
https://hg.mozilla.org/integration/autoland/rev/1d430e13ba06
Move ease parsing tests to easing-tests.js; r=hiro
https://hg.mozilla.org/integration/autoland/rev/5defc27899a9
Use KeyframeEffect constructor in processing-a-keyframes-argument-002.html; r=hiro
https://hg.mozilla.org/integration/autoland/rev/ecf5976e67ac
Update test descriptions in processing-a-keyframes-argument-002.html; r=hiro
https://hg.mozilla.org/integration/autoland/rev/5a881fba2333
Add tests for error handling when parsing the 'easing' property on keyframes; r=hiro
https://hg.mozilla.org/integration/autoland/rev/16ec8168e8af
Rename copy-contructor.html to copy-constructor.html in two places in wpt for web-animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/1dc7aa09ea90
Consistently use arrow syntax for functions in processing-a-keyframes-argument-*.html tests; r=hiro
You need to log in before you can comment on or make changes to this bug.