Closed
Bug 1260084
Opened 8 years ago
Closed 8 years ago
Move animation api mochitest to web-platform tests.
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
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
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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
Assignee | ||
Comment 4•8 years ago
|
||
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
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd8702fd8b80
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43449/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43449/
Attachment #8736988 -
Flags: review?(bbirtles)
Assignee | ||
Updated•8 years ago
|
Attachment #8736203 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8736988 -
Flags: review?(bbirtles)
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
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/
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43709/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43709/
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43711/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43711/
Assignee | ||
Comment 11•8 years ago
|
||
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/
Comment 12•8 years ago
|
||
(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.
Assignee | ||
Comment 13•8 years ago
|
||
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
Assignee | ||
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 15•8 years ago
|
||
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/
Assignee | ||
Comment 16•8 years ago
|
||
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/
Assignee | ||
Comment 17•8 years ago
|
||
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
Assignee | ||
Comment 18•8 years ago
|
||
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/
Assignee | ||
Comment 19•8 years ago
|
||
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 20•8 years ago
|
||
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+
Comment 21•8 years ago
|
||
Sorry, I didn't quite get to part 2 and part 3 today. I'll look at them on Thursday.
Comment 22•8 years ago
|
||
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 23•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8737071 -
Flags: review?(bbirtles) → review+
Comment 24•8 years ago
|
||
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.
Assignee | ||
Comment 25•8 years ago
|
||
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/
Assignee | ||
Comment 26•8 years ago
|
||
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/
Assignee | ||
Comment 27•8 years ago
|
||
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/
Assignee | ||
Comment 28•8 years ago
|
||
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
Assignee | ||
Comment 29•8 years ago
|
||
https://reviewboard.mozilla.org/r/43711/#review42859 > This doesn't seem to have been included in part 2. I added to part2 patch.
Comment 30•8 years ago
|
||
(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?
Assignee | ||
Comment 31•8 years ago
|
||
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/
Assignee | ||
Comment 32•8 years ago
|
||
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/
Assignee | ||
Comment 33•8 years ago
|
||
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/
Assignee | ||
Comment 34•8 years ago
|
||
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.
Assignee | ||
Comment 35•8 years ago
|
||
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/
Assignee | ||
Comment 36•8 years ago
|
||
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/
Assignee | ||
Comment 37•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 38•8 years ago
|
||
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.
Comment 39•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b72de5c5ac33 https://hg.mozilla.org/integration/mozilla-inbound/rev/0e54229af7f4 https://hg.mozilla.org/integration/mozilla-inbound/rev/545b4ff9a8e0
Keywords: checkin-needed
Comment 40•8 years ago
|
||
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 41•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/0968412b387d https://hg.mozilla.org/integration/mozilla-inbound/rev/56148c26e7a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/b6d92387fbcf
Assignee | ||
Comment 42•8 years ago
|
||
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
Assignee | ||
Comment 43•8 years ago
|
||
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/
Assignee | ||
Comment 44•8 years ago
|
||
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/
Assignee | ||
Comment 45•8 years ago
|
||
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
Assignee | ||
Comment 46•8 years ago
|
||
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/
Assignee | ||
Comment 47•8 years ago
|
||
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)
Assignee | ||
Comment 48•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 49•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9787fb3e8b https://hg.mozilla.org/integration/mozilla-inbound/rev/973ea9157b05 https://hg.mozilla.org/integration/mozilla-inbound/rev/642925954497
Keywords: checkin-needed
Comment 50•8 years ago
|
||
bugherder |
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
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 51•8 years ago
|
||
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
Assignee | ||
Comment 52•8 years ago
|
||
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.
Description
•