Closed Bug 1258972 Opened 4 years ago Closed 4 years ago

Move test of finish and playbackRate to web platform test.

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

Details

Attachments

(1 file, 8 obsolete files)

We can move test of several API using Element.animate().
For first step, we can move those tests to web platform test.

* dom/animation/test/css-animations/file_animation-finish.html
* dom/animation/test/css-animations/file_animation-playback.html
Comment on attachment 8733774 [details] [diff] [review]
[WIP] move-animation-test-to-webplatformtest

Review of attachment 8733774 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/test/mochitest.ini
@@ -13,5 @@
>  support-files = css-animations/file_animation-computed-timing.html
>  [css-animations/test_animation-currenttime.html]
>  support-files = css-animations/file_animation-currenttime.html
> -[css-animations/test_animation-finish.html]
> -support-files = css-animations/file_animation-finish.html

This patch does not appear to delete these files.

::: testing/web-platform/tests/web-animations/animation/finish.html
@@ +29,5 @@
> +
> +  var threw = false;
> +  try {
> +    animation.finish();
> +  } catch(e) { 

This line has a trailing space. Please check your editor settings.

@@ +35,5 @@
> +    assert_equals(e.name, 'InvalidStateError',
> +                  'Exception should be an InvalidStateError exception when ' +
> +                  'trying to finish an animation with playbackRate == 0');
> +  }
> +  assert_true(threw,

We should use assert_throws instead.
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Sorry, I forgot to add the new file in patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b2d5ffa4b2e
Attachment #8734208 - Attachment is obsolete: true
Comment on attachment 8734218 [details] [diff] [review]
Move several animation mochitest to web platform tests.

Hi brian.
Could you confirm this patch?
Attachment #8734218 - Flags: review?(bbirtles)
Comment on attachment 8734218 [details] [diff] [review]
Move several animation mochitest to web platform tests.

Review of attachment 8734218 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this. I've written quite a fair bit of feedback. I know this bug is mostly about moving code, but I think we should tidy it up at the same time to make it more suitable for a wider audience.

Also, I realize testharness.js is new to you, and it continues to change, so I've pointed out a few areas where I think we can use the test framework better.

::: testing/web-platform/tests/web-animations/animation/finish.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>finish tests</title>

Animation.finish()

@@ +1,5 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>finish tests</title>
> +<link rel="help" href="https://w3c.github.io/web-animations/#dom-animation-finish">
> +<link rel="author" title="Mantaroh" href="mailto:mantaroh@gmail.com">

I think we should just drop the author line. Most new tests don't seem to use it (and it's not quite fair since many people contributed to this file!)

@@ +7,5 @@
> +<script src="/resources/testharnessreport.js"></script>
> +<script src="../testcommon.js"></script>
> +<link rel="stylesheet" href="/resources/testharness.css">
> +<style>
> +

Nit: We don't need this blank line.

@@ +10,5 @@
> +<style>
> +
> +.animated-div {
> +  margin-left: 10px;
> +}

I think it would be more clear if we just dropped this, and below replaced:

  div.setAttribute('class', 'animated-div');

with:

  div.style.marginLeft = '10px';

@@ +18,5 @@
> +<script>
> +'use strict';
> +
> +var keyFrames = { 'margin-left': ['100px', '200px'] };
> +var ANIMATION_DURATION = 100000; // ms(100s)

This is largely a personal preference, but unless there's an obvious benefit, I'd prefer to just write these values where they are used.

The reason is that when you read:

  div.setAttribute('class', 'animated-div');
  var animation = div.animate(keyFrames, ANIMATION_DURATION);
 
  animation.ready.then(t.step_func(function() {
    animation.finish();
    var marginLeft = parseFloat(getComputedStyle(div).marginLeft);
    assert_equals(marginLeft, 10, ...);

it's not clear why marginLeft should be 10 or what is even being tested. On the other hand if you read:

  div.style.marginLeft = '10px';
  var animation = div.animate({ 'margin-left': ['100px', '200px'] }, 100000);

it's much more obvious what the expected effect is.

The ANIMATION_DURATION is probably ok since the duration doesn't have any effect on these tests (although I'd prefer to rename it to gAnimationDuration if you decide to keep it) but I'd at least like to see the margin-left values written out each time.

@@ +30,5 @@
> +    animation.finish();
> +  });
> +}, 'Test exceptions when finishing non-running animation');
> +
> +

Nit: Extra blank line here.

@@ +34,5 @@
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate(keyFrames, ANIMATION_DURATION);
> +  animation.effect.timing.iterations = +Infinity;

Perhaps we could just write:

 var animation = div.animate({ ... },
                             { duration: 100000, iterations: Infinity });

In any case, we don't need the '+' before Infinity.

@@ +35,5 @@
> +test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate(keyFrames, ANIMATION_DURATION);
> +  animation.effect.timing.iterations = +Infinity;
> +  animation.play();

No need to call play(). animate() does that automatically.

@@ +45,5 @@
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate(keyFrames, ANIMATION_DURATION);
> +  animation.play();

No need for play().

@@ +46,5 @@
> +test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate(keyFrames, ANIMATION_DURATION);
> +  animation.play();
> +

Nit: Probably don't need this blank line.

@@ +56,5 @@
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate(keyFrames, ANIMATION_DURATION);
> +  animation.play();

play()

@@ +58,5 @@
> +  var div = createDiv(t);
> +  var animation = div.animate(keyFrames, ANIMATION_DURATION);
> +  animation.play();
> +
> +  animation.currentTime = ANIMATION_DURATION + 1000; // 1s past effect end

(If we decide to drop ANIMATION_DURATION, we can write `animation.currentTime = animation.effect.getComputedTiming().endTime + 1000;` here)

@@ +66,5 @@
> +                'After finishing, the currentTime should be set back to the ' +
> +                'end of the active duration');
> +}, 'Test finishing of animation with a current time past the effect end');
> +
> +async_test(function(t) {

I guess we should use promise_test for this.

See: https://github.com/w3c/testharness.js/blob/master/docs/api.md#promise-tests

One thing to take note of is that when we use promise_test, we don't need to use t.step_func() or t.done().

(I think promise_test didn't exist when we wrote this test--or we simply didn't know about it.)

@@ +69,5 @@
> +
> +async_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate(keyFrames, ANIMATION_DURATION);
> +

(Nit: I feel like this file has some fairly random whitespace. Perhaps drop this blank line?)

@@ +82,5 @@
> +    t.done();
> +  }));
> +}, 'Test finishing of reversed animation');
> +
> +async_test(function(t) {

Let's see if we can use promise_test for this too--and probably all the async_tests in this file.

@@ +90,5 @@
> +  animation.currentTime = ANIMATION_DURATION;
> +
> +  animation.finished.then(t.step_func(function() {
> +    animation.playbackRate = -1;
> +

Nit: Let's drop this blank line. Actually, we can probably drop a few other blank lines here too.

@@ +182,5 @@
> +
> +async_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate(keyFrames, ANIMATION_DURATION);
> +  animation.pause();

This is not in the original test and we shouldn't add it.

@@ +203,5 @@
> +}, 'Test finish() during aborted pause');
> +
> +async_test(function(t) {
> +  var div = createDiv(t);
> +  div.setAttribute('class', 'animated-div');

As mentioned earlier, we should just set the style here.

@@ +237,5 @@
> +
> +}, 'Test finish() resolves finished promise synchronously');
> +
> +
> +

Nit: Too many blank line here.

::: testing/web-platform/tests/web-animations/animation/playbackrate.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>Animation playback tests</title>

Animation.playbackRate

@@ +1,5 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>Animation playback tests</title>
> +<link rel="help" href="https://w3c.github.io/web-animations/#dom-animation-playbackrate">
> +<link rel="author" title="Mantaroh Yoshinaga" href="mailto:mantaroh@gmail.com">

Drop the "author" line.

@@ +9,5 @@
> +<link rel="stylesheet" href="/resources/testharness.css">
> +<style>
> +.animated-div {
> +  margin-left: 10px;
> +}

As with the finish test, I think we should drop this and write the style inline.

@@ +16,5 @@
> +<div id="log"></div>
> +<script>
> +"use strict";
> +
> +var keyFrames = { 'margin-left': ['100px', '200px'] };

As before, I think we should probably write this inline.

@@ +35,5 @@
> +                       accuracy,
> +                       description);
> +}
> +
> +async_test(function(t) {

promise_test here and elsewhere in this file.

@@ +37,5 @@
> +}
> +
> +async_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({keyFrames}, 10000);

Let's make this 100000

@@ +38,5 @@
> +
> +async_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({keyFrames}, 10000);
> +  animation.play();

No need for play() here.

@@ +59,5 @@
> +
> +async_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({keyFrames}, 100000);
> +  animation.play();

play()
Attachment #8734218 - Flags: review?(bbirtles)
Comment on attachment 8735327 [details] [diff] [review]
Move several mochitest in animation to web platform tests.

Review of attachment 8735327 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brian Birtles (:birtles) from comment #5)
> 
> @@ +37,5 @@
> > +}
> > +
> > +async_test(function(t) {
> > +  var div = createDiv(t);
> > +  var animation = div.animate({keyFrames}, 10000);
> 
> Let's make this 100000

I'd suggest we use 100 * MS_PER_SEC just like bug 1256503.

::: testing/web-platform/tests/web-animations/testcommon.js
@@ +156,5 @@
> +  return new Promise(function(resolve, reject) {
> +    window.requestAnimationFrame(resolve);
> +  });
> +}
> +

Can't we use waitForAnimationFrames(0) here?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Comment on attachment 8735327 [details] [diff] [review]
> Move several mochitest in animation to web platform tests.
> 
> Review of attachment 8735327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Brian Birtles (:birtles) from comment #5)
> > 
> > @@ +37,5 @@
> > > +}
> > > +
> > > +async_test(function(t) {
> > > +  var div = createDiv(t);
> > > +  var animation = div.animate({keyFrames}, 10000);
> > 
> > Let's make this 100000
> 
> I'd suggest we use 100 * MS_PER_SEC just like bug 1256503.
> 
> ::: testing/web-platform/tests/web-animations/testcommon.js
> @@ +156,5 @@
> > +  return new Promise(function(resolve, reject) {
> > +    window.requestAnimationFrame(resolve);
> > +  });
> > +}
> > +
> 
> Can't we use waitForAnimationFrames(0) here?

I meant waitForAnimationFrames(1).
Summary: Move test of finish and playback to web platform test. → Move test of finish and playbackRate to web platform test.
I addressed the comment #7 and comment #8.
Attachment #8735327 - Attachment is obsolete: true
Comment on attachment 8735379 [details] [diff] [review]
Move several mochitest in animation to web platform tests.

Review of attachment 8735379 [details] [diff] [review]:
-----------------------------------------------------------------

I don't check whole codes but I leave some comments what I noticed at first glance.

::: testing/web-platform/tests/web-animations/animation/finish.html
@@ +58,5 @@
> +  var div = createDiv(t);
> +  var animation = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> +  animation.currentTime = 100 * MS_PER_SEC;
> +  animation.finished.then(t.step_func(function() {
> +    animation.playbackRate = -1;

You need to return a promise object from promise_test.  step_func is not necessary at all in promise_test.

@@ +64,5 @@
> +
> +    assert_equals(animation.currentTime, 0,
> +                  'After finishing a reversed animation the currentTime ' +
> +                  'should be set to zero');
> +    t.done();

done() is not necessary too.
Hiro,thank you as always!

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> You need to return a promise object from promise_test.  step_func is not
> necessary at all in promise_test.
Oh, Sorry. I misunderstood the usage of promise_test.
I addressed your comment #10.

I will leave css-animations test which related CSS Animation spec. (For detail, see bug 1260084 comment #2) So I have left the some finish tests.

Thanks.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef0ff643068b
Attachment #8735379 - Attachment is obsolete: true
Comment on attachment 8735699 [details] [diff] [review]
Move several mochitest in animation to web platform tests.

Hi brian.

I moved playbackrate test and finish test to web-platform tests.
As mentioned at comment 11, I have left several finish tests for css-animations tests.

Could you confirm this patch?
Attachment #8735699 - Flags: review?(bbirtles)
Comment on attachment 8735699 [details] [diff] [review]
Move several mochitest in animation to web platform tests.

Review of attachment 8735699 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work.

r=me with comments addressed.

::: testing/web-platform/meta/MANIFEST.json
@@ +28457,5 @@
> +        "path": "web-animations/animation/finish.html",
> +        "url": "/web-animations/animation/finish.html"
> +      },
> +      {
> +        "path": "web-animations/animation/playbackrate.html",

Maybe we should name this playbackRate.html to match the spec?

::: testing/web-platform/tests/web-animations/animation/finish.html
@@ +25,5 @@
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate(gKeyFrames,
> +                              {duration : 100 * MS_PER_SEC, iterations : Infinity});

I think this is > 80 chars.

@@ +45,5 @@
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> +  animation.currentTime = animation.effect.getComputedTiming().endTime + 1000; // 1s past effect end

> 80 chars

@@ +71,5 @@
> +promise_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> +  animation.currentTime = 100 * MS_PER_SEC;
> +  animation.finished.then(function() {

I think this needs to be 'return animation.finished.then(...)'?

@@ +153,5 @@
> +}, 'Test finish() while play-pending');
> +
> +// FIXME: Add a test for when we are play-pending without an active timeline.
> +// - In that case even after calling finish() we should still be pending but
> +//   the current time should be updated

Nit: Add a blank line after this (to indicate that it is not describing the following test)

@@ +173,5 @@
> +}, 'Test finish() during aborted pause');
> +
> +promise_test(function(t) {
> +  var div = createDiv(t);
> +  div.setAttribute('class', 'animated-div');

I don't think we need this line?

::: testing/web-platform/tests/web-animations/animation/playbackrate.html
@@ +32,5 @@
> +promise_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({keyFrames}, 100 * MS_PER_SEC);
> +  return animation.ready.then(function() {
> +    animation.currentTime = 7000; // ms

7 * MS_PER_SEC?

@@ +59,5 @@
> +  }).then(function() {
> +    assert_playbackrate(animation,
> +      previousAnimationCurrentTime,
> +      previousTimelineCurrentTime,
> +      'animation.currentTime should be 10 times faster than timeline.');

10 times? 2 times?
I think this might be a bug in the original test.
Attachment #8735699 - Flags: review?(bbirtles) → review+
Thanks Brian.

I fixed the place that you commented.

> ::: testing/web-platform/tests/web-animations/animation/finish.html
> @@ +25,5 @@
> > +
> > +test(function(t) {
> > +  var div = createDiv(t);
> > +  var animation = div.animate(gKeyFrames,
> > +                              {duration : 100 * MS_PER_SEC, iterations : Infinity});
> 
> I think this is > 80 chars.
Sorry. I fixed same line in this patch.

Thanks.
Attachment #8735699 - Attachment is obsolete: true
Attachment #8736526 - Flags: review?(bbirtles)
Comment on attachment 8736526 [details] [diff] [review]
Move several mochitest in animation to web platform tests.

Carrying r+ from birtles.(comment #14)
Attachment #8736526 - Flags: review?(bbirtles) → review+
Rebased.
Carrying r+ from birtles.(comment #14)
Attachment #8736526 - Attachment is obsolete: true
Attachment #8737020 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8ab93deb75d7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b3a45aebcaf
Baldr: Implement wasm start section; r=luke
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bcaeba7dd14
Backed out changeset 1b3a45aebcaf for landing with wrong bugnumber
You need to log in before you can comment on or make changes to this bug.