Closed Bug 1260084 Opened 8 years ago Closed 8 years ago

Move animation api mochitest to web-platform tests.

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

Details

Attachments

(3 files, 2 obsolete files)

We moved the animation api to web-platform tests as follow.[1][2]
 * finish()
 * playbackRate
 * play()
 * playState
 * cancel()

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1259344
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1258972

We can move other tests using same method.
 * finished()
 * AnimationEffect.computedTiming
 * oncancel
 * currenttime
 * onfinish
 * pausing
 * ready
 * reverse
 * starttime
 * AnimationEffect.target
 * AnimationEffect.getFrame
What I am concerned is that the original file is removed in bug 1258972.  I think most of test cases in that test file should be run against CSS animations/transitions as until now.
Thanks hiro.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> What I am concerned is that the original file is removed in bug 1258972.  I
> think most of test cases in that test file should be run against CSS
> animations/transitions as until now.
As you mentioned in skype, CSS animations object was different from script animations(Element.animate() object). Perhaps we had better test each object.
I moved several css-animations tests while investigating.

* file_animation-cancel.html          : leaving some css animation tests.(see bug 1259344)
* file_animation-finish.html          : leaving some css animation tests.(see bug 1258972)
* file_animation-finished.html        : leaving some css animation tests
* file_animation-computed-timing.html : leaving all css animation tests.
* file_animation-oncancel.html        : Move to web-platform tests
* file_animation-onfinish.html        : Ditto.
* file_animation-id.html              : add to web-platform tests.(still leave css-animation tests)
* file_animation-pausing.html         : leaving the tests which using css animation style(paused).
* file_animation-play.html            : Moving to web-platform tests.(see bug 1259344)
* file_animation-playbackrate.html    : Moving to web-platform tests.(see bug 1258972)
* file_animation-playstate.html       : leaving some css animation tests.(see bug 1259344)
* file_animation-ready.html           : leaving some css animation tests.
* file_animation-reverse.html         : Ditto.
* file_animations-dynamic-changes.html : leave all tests.
* file_cssanimation-animationname.html : Ditto.
* file_document-get-animations.html    : Ditto.
* file_element-get-animations.html     : Ditto.


I should still move rest tests as follow.
* file_animation-currenttime.html
* file_animation-starttime.html
* file_effect-target.html
* file_keyframeeffect-getframes.html
* file_pseudoElement-get-animations.html

https://treeherder.mozilla.org/#/jobs?repo=try&revision=174e1eb2727e
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
I decided that separate the patch as follow.
 1. modify using promise_test
 2. move to web-platform tests.

I modified using promise_test.
Attachment #8735755 - Attachment is obsolete: true
Attachment #8736203 - Attachment is obsolete: true
Comment on attachment 8736988 [details]
MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles

https://reviewboard.mozilla.org/r/43449/#review40231

Thanks for doing this.

r=me with the additional Animation.id test dropped and other minor comments below addressed.

::: dom/animation/test/css-animations/file_animation-computed-timing.html:207
(Diff revision 1)
>  // = iteration duration * iteration count
>  // --------------------
>  test(function(t) {
>    var div = addDiv(t, {style: 'animation: moveAnimation 100s 5'});
>    var effect = div.getAnimations()[0].effect;
> -  var answer = 100000 * 5; // ms
> +  var answer = 100 * MS_PER_SEC * 5; // ms

We can drop the 'ms' comment here

::: dom/animation/test/css-animations/file_animation-computed-timing.html:521
(Diff revision 1)
>  
>    assert_equals(anim.effect.getComputedTiming().currentIteration, 0,
>                  'Initial value of currentIteration');
>    // The 3rd iteration
>    // Note: currentIteration = floor(scaled active time / iteration duration)
> -  anim.currentTime = 250000; // ms
> +  anim.currentTime = 250 * MS_PER_SEC; // ms

We can drop the 'ms' comment here

::: dom/animation/test/css-animations/file_animation-currenttime.html:42
(Diff revision 1)
> -const ANIM_DELAY_MS = 1000000; // 1000s
> -const ANIM_DUR_MS = 1000000; // 1000s
> +const ANIM_DELAY_MS = 1000 * MS_PER_SEC; // 1000s
> +const ANIM_DUR_MS = 1000 * MS_PER_SEC; // 1000s

We can drop the comments here.

::: dom/animation/test/css-animations/file_animation-currenttime.html:252
(Diff revision 1)
> -  })).catch(t.step_func(function(reason) {
> +  }).catch(function(reason) {
>      assert_unreached(reason);

I wonder what sort of exceptional cases we were expecting here and if this catch still works. Do you know if this pattern still works with promise_test or will the test complete before the catch() function runs?

::: dom/animation/test/css-animations/file_animation-currenttime.html:319
(Diff revision 1)
>    // This must come after we've set up the Promise chain, since requesting
>    // computed style will force events to be dispatched.
>    // XXX For some reason this fails occasionally (either the animation.playState
>    // check or the marginLeft check).
>    //checkStateAtActiveIntervalEndTime(animation);

This is unrelated to this bug, but would you be able to check if this still fails and, if it does, file a bug for it?

::: dom/animation/test/css-animations/file_animation-finished.html
(Diff revision 1)
> -
>    var previousFinishedPromise = animation.finished;
> -
>    animation.currentTime = ANIM_DURATION;
>    animation.currentTime = ANIM_DURATION / 2;
> -
>    assert_equals(animation.finished, previousFinishedPromise,
>                  'No new finished promise generated when finished state ' +
>                  'is checked asynchronously');
> -  t.done();

I'm not sure we need to get rid of *all* the blank lines, but ok.

::: dom/animation/test/css-animations/file_animation-id.html:21
(Diff revision 1)
> +test(function(t) {
> +  var div = addDiv(t);
> +  var animation = div.animate({}, 100 * MS_PER_SEC);
> +  assert_equals(animation.id, '', 'id for CSS Animation is initially empty');
> +  animation.id = 'anim'
> +
> +  assert_equals(animation.id, 'anim', 'animation.id reflects the value set');
> +}, 'Animation.id for CSS Animations');

Why are we adding this here, and to css-animations/file_animation-id.html? Also the test descriptions refer to CSS Animations but this is not a CSS Animation.

::: dom/animation/test/css-animations/file_animation-starttime.html:366
(Diff revision 1)
>    // This must come after we've set up the Promise chain, since requesting
>    // computed style will force events to be dispatched.
>    // XXX For some reason this fails occasionally (either the animation.playState
>    // check or the marginLeft check).
>    //checkStateAtActiveIntervalEndTime(animation);

As before, we should see if this still fails and file a bug if it does.
Comment on attachment 8736988 [details]
MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43449/diff/1-2/
Comment on attachment 8736988 [details]
MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43449/diff/2-3/
(In reply to Brian Birtles (:birtles, away 2-10 April) from comment #7)
> Comment on attachment 8736988 [details]
> MozReview Request: Bug 1260084 - Part1.Use promise_test instead of
> async_test. r?birtles
> ::: dom/animation/test/css-animations/file_animation-currenttime.html:252
> (Diff revision 1)
> > -  })).catch(t.step_func(function(reason) {
> > +  }).catch(function(reason) {
> >      assert_unreached(reason);
> 
> I wonder what sort of exceptional cases we were expecting here and if this
> catch still works. Do you know if this pattern still works with promise_test
> or will the test complete before the catch() function runs?

What did you discover here?

If this is ok, please land this patch. It has r+ per comment 7, will bitrot quickly, and blocks/fixes bug 1255682.
Thanks birtles.

(In reply to Brian Birtles (:birtles, away 2-10 April) from comment #12)
> (In reply to Brian Birtles (:birtles, away 2-10 April) from comment #7)
> > Comment on attachment 8736988 [details]
> > MozReview Request: Bug 1260084 - Part1.Use promise_test instead of
> > async_test. r?birtles
> > ::: dom/animation/test/css-animations/file_animation-currenttime.html:252
> > (Diff revision 1)
> > > -  })).catch(t.step_func(function(reason) {
> > > +  }).catch(function(reason) {
> > >      assert_unreached(reason);
> > 
> > I wonder what sort of exceptional cases we were expecting here and if this
> > catch still works. Do you know if this pattern still works with promise_test
> > or will the test complete before the catch() function runs?
> 
> What did you discover here?
If exception had occurred  in promise_test, testharness will execute assert[1]. So I think that we don't need to handling exception in this case.

[1] https://github.com/w3c/testharness.js/blob/master/testharness.js#L537
Blocks: 1255682
No longer blocks: 1255682
Comment on attachment 8736988 [details]
MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43449/diff/3-4/
Attachment #8736988 - Attachment description: MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles → MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r=birtles
Attachment #8737070 - Attachment description: MozReview Request: Bug 1260084 - Copy css animation mochitest to web-platform tests. r? → MozReview Request: Bug 1260084 - Copy css animation mochitest to web-platform tests. r?birtles
Attachment #8737071 - Attachment description: MozReview Request: Bug 1260084 - Remove unnecessary css animation mochitest. r? → MozReview Request: Bug 1260084 - Remove unnecessary css animation mochitest. r?birtles
Attachment #8736988 - Flags: review?(bbirtles)
Attachment #8737070 - Flags: review?(bbirtles)
Attachment #8737071 - Flags: review?(bbirtles)
Comment on attachment 8737070 [details]
MozReview Request: Bug 1260084 - Part2.Copy css animation mochitest to web-platform tests. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43709/diff/1-2/
Comment on attachment 8737071 [details]
MozReview Request: Bug 1260084 - Part3.Remove unnecessary css animation mochitest. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43711/diff/1-2/
Comment on attachment 8736988 [details]
MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43449/diff/4-5/
Attachment #8736988 - Attachment description: MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r=birtles → MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles
Attachment #8737070 - Attachment description: MozReview Request: Bug 1260084 - Copy css animation mochitest to web-platform tests. r?birtles → MozReview Request: Bug 1260084 - Part2.Copy css animation mochitest to web-platform tests. r?birtles
Attachment #8737071 - Attachment description: MozReview Request: Bug 1260084 - Remove unnecessary css animation mochitest. r?birtles → MozReview Request: Bug 1260084 - Part3.Remove unnecessary css animation mochitest. r?birtles
Comment on attachment 8737070 [details]
MozReview Request: Bug 1260084 - Part2.Copy css animation mochitest to web-platform tests. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43709/diff/2-3/
Comment on attachment 8737071 [details]
MozReview Request: Bug 1260084 - Part3.Remove unnecessary css animation mochitest. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43711/diff/2-3/
Comment on attachment 8736988 [details]
MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles

https://reviewboard.mozilla.org/r/43449/#review42193
Attachment #8736988 - Flags: review?(bbirtles) → review+
Sorry, I didn't quite get to part 2 and part 3 today. I'll look at them on Thursday.
https://reviewboard.mozilla.org/r/43709/#review42267

::: testing/web-platform/tests/web-animations/animation/finished.html:271
(Diff revision 3)
> +  return animation.ready.then(function() {
> +    animation.playbackRate = -1;
> +    return animation.finished;
> +  });

This does not test that the finished promise is resolved by setting playbackRate to -1.  I mean the finished promise is eventually resolved after animation duration.
Comment on attachment 8737070 [details]
MozReview Request: Bug 1260084 - Part2.Copy css animation mochitest to web-platform tests. r?birtles

https://reviewboard.mozilla.org/r/43709/#review42857

r=birtles with comments addressed

::: testing/web-platform/tests/web-animations/animation/finished.html:16
(Diff revision 3)
> +<script>
> +"use strict";
> +
> +promise_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({transform: ['none', 'translate(10px)']},

Nit: Here and elsewhere in this file, we probably don't need the 'transform' property values. I think {} would still work?

::: testing/web-platform/tests/web-animations/animation/finished.html:147
(Diff revision 3)
> +
> +promise_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({transform: ['none', 'translate(10px)']},
> +                              100 * MS_PER_SEC);
> +  animation.currentTime = 100 * MS_PER_SEC;

I think we could just use animation.finish() here.

::: testing/web-platform/tests/web-animations/animation/finished.html:183
(Diff revision 3)
> +promise_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({transform: ['none', 'translate(10px)']},
> +                              100 * MS_PER_SEC);
> +  var previousFinishedPromise = animation.finished;
> +  animation.currentTime = 100 * MS_PER_SEC;

I think we could just call finish() here too.

::: testing/web-platform/tests/web-animations/animation/finished.html:225
(Diff revision 3)
> +  var gotNextFrame = false;
> +  var currentTimeBeforeShortening;
> +  animation.currentTime = HALF_DUR;
> +  return animation.ready.then(function() {
> +    currentTimeBeforeShortening = animation.currentTime;
> +    animation.effect.timing.duration = QUARTER_DUR;    

Trailing whitespace here.

::: testing/web-platform/tests/web-animations/animation/oncancel.html:16
(Diff revision 3)
> +<script>
> +"use strict";
> +
> +async_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({transform: ['none','translate(10px)']},

Nit: I don't think we need the transform properties here? {} would do?

::: testing/web-platform/tests/web-animations/animation/oncancel.html:32
(Diff revision 3)
> +      'event.timelineTime should equal to the animation timeline ' +
> +      'when finished promise is rejected');
> +  });
> +
> +  animation.cancel();
> +}, 'oncancel event is fired when animation.cancel()');

Nit: We should fix this comment to say '... when animation.cancel() is called'

::: testing/web-platform/tests/web-animations/animation/onfinish.html:16
(Diff revision 3)
> +<script>
> +"use strict";
> +
> +async_test(function(t) {
> +  var div = createDiv(t);
> +  var animation = div.animate({transform: ['none', 'translate(10px)']},

Again, not sure we need 'transform' values here and elsewhere in this file.

::: testing/web-platform/tests/web-animations/animation/onfinish.html:109
(Diff revision 3)
> +  return animation.ready.then(function() {
> +    animation.playbackRate = 0;
> +    animation.currentTime = 100 * MS_PER_SEC;
> +    return waitForAnimationFrames(2);
> +  });
> +

Nit: drop this blank line

::: testing/web-platform/tests/web-animations/animation/pause.html:21
(Diff revision 3)
> +}
> +
> +promise_test(function(t) {
> +  var div = createDiv(t);
> +  var cs = window.getComputedStyle(div);
> +  var animation = div.animate({marginLeft: ['0px', '10000px']},

I know this is a little bit of work, but as discussed this morning, I think we don't need to use getComputedStyle here.

Instead, we can just compare animation.currentTime.

That means we can drop the 'marginLeft' property values and replace 'previousAnimVal' with 'previousCurrentTime' (and check 'currentTime is initially increasing' etc.).

That applies to all tests in this file and would also mean we can drop getMarginLeft.

::: testing/web-platform/tests/web-animations/animation/ready.html:79
(Diff revision 3)
> +}, 'ready promise is rejected when a play-pending animation is cancelled by'
> +   + ' calling cancel()');

It looks like we are missing the test for a *pause-pending* animation.

::: testing/web-platform/tests/web-animations/animation/reverse.html:16
(Diff revision 3)
> +  var animation = div.animate({transform: ['none', 'translate(100px)']},
> +                              {duration: 100 * MS_PER_SEC,
> +                               iterations: Infinity});

I don't think we need the transform values here or elsewhere in this file.

::: testing/web-platform/tests/web-animations/animation/reverse.html:28
(Diff revision 3)
> +    assert_greater_than(animation.currentTime, 0,
> +      'currentTime expected to be greater than 0, one frame after starting');
> +    var previousPlaybackRate = animation.playbackRate;
> +    animation.reverse();
> +    assert_equals(animation.playbackRate, -previousPlaybackRate,
> +      'playbackRate should be invetrted');

Nit: Let's this typo at the same time: s/invetrted/inverted/

::: testing/web-platform/tests/web-animations/animation/reverse.html:125
(Diff revision 3)
> +    function () { animation.reverse(); },
> +    'reverse() should throw InvalidStateError ' +
> +    'if the playbackRate > 0 and the currentTime < 0 ' +
> +    'and the target effect is positive infinity');
> +}, 'reverse() when playbackRate > 0 and currentTime < 0 ' +
> +   'and the target effect is positive infinity');

Nit: Let's fit this typo at the same time:
s/target effect/target effect end/

::: testing/web-platform/tests/web-animations/animation/reverse.html:141
(Diff revision 3)
> +  assert_equals(animation.currentTime, 0,
> +    'reverse() should start playing from the start of animation time ' +
> +    'if the playbackRate < 0 and the currentTime < 0 ' +
> +    'and the target effect is positive infinity');
> +}, 'reverse() when playbackRate < 0 and currentTime < 0 ' +
> +   'and the target effect is positive infinity');

s/target effect/target effect end/
Attachment #8737070 - Flags: review?(bbirtles) → review+
Attachment #8737071 - Flags: review?(bbirtles) → review+
Comment on attachment 8737071 [details]
MozReview Request: Bug 1260084 - Part3.Remove unnecessary css animation mochitest. r?birtles

https://reviewboard.mozilla.org/r/43711/#review42859

r=birtles with comments addressed

::: dom/animation/test/css-animations/file_animation-ready.html
(Diff revision 3)
> -promise_test(function(t) {
> -  var div = addDiv(t);
> -  div.style.animation = 'abc 100s';
> -  var animation = div.getAnimations()[0];
> -
> -  return animation.ready.then(function() {
> -    animation.pause();
> -
> -    // Set up listeners on pause-pending ready promise
> -    var retPromise = animation.ready.then(function() {
> -      assert_unreached('ready promise was fulfilled');
> -    }).catch(function(err) {
> -      assert_equals(err.name, 'AbortError',
> -                    'ready promise is rejected with AbortError');
> -    });
> -
> -    animation.cancel();
> -
> -    return retPromise;
> -  });
> -}, 'ready promise is rejected when a pause-pending animation is cancelled by'
> -   + ' calling cancel()');

This doesn't seem to have been included in part 2.
Comment on attachment 8736988 [details]
MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43449/diff/5-6/
Comment on attachment 8737070 [details]
MozReview Request: Bug 1260084 - Part2.Copy css animation mochitest to web-platform tests. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43709/diff/3-4/
Comment on attachment 8737071 [details]
MozReview Request: Bug 1260084 - Part3.Remove unnecessary css animation mochitest. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43711/diff/3-4/
https://reviewboard.mozilla.org/r/43709/#review42267

> This does not test that the finished promise is resolved by setting playbackRate to -1.  I mean the finished promise is eventually resolved after animation duration.

In Web animation specification, finished state defined as follow[1].
'The animation has reached the natural boundary of its playback range and the current time is no longer updating.'

I think that this test wanted to verify that animation has reached boundary of playback range with negative playbackRate. So I think we should leave this test.

[1] http://w3c.github.io/web-animations/#play-states
https://reviewboard.mozilla.org/r/43711/#review42859

> This doesn't seem to have been included in part 2.

I added to part2 patch.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #28)
> https://reviewboard.mozilla.org/r/43709/#review42267
> 
> > This does not test that the finished promise is resolved by setting playbackRate to -1.  I mean the finished promise is eventually resolved after animation duration.
> 
> In Web animation specification, finished state defined as follow[1].
> 'The animation has reached the natural boundary of its playback range and
> the current time is no longer updating.'
> 
> I think that this test wanted to verify that animation has reached boundary
> of playback range with negative playbackRate. So I think we should leave
> this test.
> 
> [1] http://w3c.github.io/web-animations/#play-states

Hmm, but you might not want to change the description of that test?
Comment on attachment 8736988 [details]
MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43449/diff/6-7/
Comment on attachment 8737070 [details]
MozReview Request: Bug 1260084 - Part2.Copy css animation mochitest to web-platform tests. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43709/diff/4-5/
Comment on attachment 8737071 [details]
MozReview Request: Bug 1260084 - Part3.Remove unnecessary css animation mochitest. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43711/diff/4-5/
https://reviewboard.mozilla.org/r/43709/#review42267

> In Web animation specification, finished state defined as follow[1].
> 'The animation has reached the natural boundary of its playback range and the current time is no longer updating.'
> 
> I think that this test wanted to verify that animation has reached boundary of playback range with negative playbackRate. So I think we should leave this test.
> 
> [1] http://w3c.github.io/web-animations/#play-states

Thanks hiro.

> Hmm, but you might not want to change the description of that test?

I agree with your suggestion.
This test comment was not correct. So I modified to the test comment.
Comment on attachment 8736988 [details]
MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43449/diff/7-8/
Comment on attachment 8737070 [details]
MozReview Request: Bug 1260084 - Part2.Copy css animation mochitest to web-platform tests. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43709/diff/5-6/
Comment on attachment 8737071 [details]
MozReview Request: Bug 1260084 - Part3.Remove unnecessary css animation mochitest. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43711/diff/5-6/
Keywords: checkin-needed
https://reviewboard.mozilla.org/r/43709/#review44057

::: testing/web-platform/meta/MANIFEST.json:28825
(Diff revision 6)
> +        "path": "web-animations/animation/reverse.html",
> +        "url": "/web-animations/animation/reverse.html"
> +      },
> +      {
>          "path": "web-animations/animation/play.html",

I wonder this should not be alphabetical order.
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=26101041&repo=mozilla-inbound
Flags: needinfo?(mantaroh)
Comment on attachment 8736988 [details]
MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43449/diff/8-9/
Attachment #8737070 - Attachment description: MozReview Request: Bug 1260084 - Part2.Copy css animation mochitest to web-platform tests. r?birtles → MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles
Comment on attachment 8737070 [details]
MozReview Request: Bug 1260084 - Part2.Copy css animation mochitest to web-platform tests. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43709/diff/6-7/
Comment on attachment 8737071 [details]
MozReview Request: Bug 1260084 - Part3.Remove unnecessary css animation mochitest. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43711/diff/6-7/
Comment on attachment 8737070 [details]
MozReview Request: Bug 1260084 - Part2.Copy css animation mochitest to web-platform tests. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43709/diff/7-8/
Attachment #8737070 - Attachment description: MozReview Request: Bug 1260084 - Part1.Use promise_test instead of async_test. r?birtles → MozReview Request: Bug 1260084 - Part2.Copy css animation mochitest to web-platform tests. r?birtles
Comment on attachment 8737071 [details]
MozReview Request: Bug 1260084 - Part3.Remove unnecessary css animation mochitest. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43711/diff/7-8/
I discussed to Brian this morning. We should remove test of currentTime from reverse.html. (This test is unnecessary at reverse tests.). So I removed this test ,and modified hiro's comment(Comment #38).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f06acc37b3b
Flags: needinfo?(mantaroh)
Thanks Tomcat.

(In reply to Carsten Book [:Tomcat] from comment #40)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=26101041&repo=mozilla-
> inbound
Sorry, I modified the tests.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1b9787fb3e8b
https://hg.mozilla.org/mozilla-central/rev/973ea9157b05
https://hg.mozilla.org/mozilla-central/rev/642925954497
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Hi, Mantaroh,

Just a reminder. If you add some tests in MANIFEST.json, it's better to use "./mach web-platform-tests --manifest-update" to update this manifest. Updating it manually is OK, but running this command assures the manifest to be in lexicographic order. If you accidentally add a test info into a disordering place, other people will have to create an extra patch to fix it. You can check this mail [1]. Thanks.

[1] https://groups.google.com/forum/#!topic/mozilla.dev.platform/ujRbgxUxKzY
Hi Boris.

(In reply to Boris Chiou [:boris]  from comment #51)
> Hi, Mantaroh,
> 
> Just a reminder. If you add some tests in MANIFEST.json, it's better to use
> "./mach web-platform-tests --manifest-update" to update this manifest.
> Updating it manually is OK, but running this command assures the manifest to
> be in lexicographic order. If you accidentally add a test info into a
> disordering place, other people will have to create an extra patch to fix
> it. You can check this mail [1]. Thanks.
> 
> [1] https://groups.google.com/forum/#!topic/mozilla.dev.platform/ujRbgxUxKzY
Thank you for your advice!!
I just read those email yesterday, and I've checked my modification after read.
(Unfortunately I found the bug 1266251..)

Many thanks.
You need to log in before you can comment on or make changes to this bug.