Closed
Bug 1283754
Opened 6 years ago
Closed 6 years ago
Intermittent test_animation_observers.html | Test timed out.
Categories
(Core :: DOM: Animation, defect, P3)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: cbook, Assigned: hiro)
References
()
Details
(Keywords: intermittent-failure)
Attachments
(5 files)
58 bytes,
text/x-review-board-request
|
boris
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
boris
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
boris
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
boris
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
boris
:
review+
|
Details |
https://treeherder.mozilla.org/logviewer.html#?job_id=31038041&repo=mozilla-inbound#L3774 22:35:26 INFO - 104 INFO TEST-UNEXPECTED-FAIL | dom/animation/test/chrome/test_animation_observers.html | Test timed out.
Updated•6 years ago
|
Priority: -- → P3
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 3•6 years ago
|
||
I think it's time to split test_animation_observers.html into smaller chunks.
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 6•6 years ago
|
||
That'd be great! Any chance you'd be willing to take that on soon? :)
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox52:
--- → affected
Flags: needinfo?(hiikezoe)
Assignee | ||
Comment 7•6 years ago
|
||
OK, taking. I am not sure why test cases using script animation are not run with parent node (i.e. not inside the iteration[1]), but I am going to split script animations into a new file, The name will be test_animation_observers_for_script_animation.html or test_observers_for_script_animation.html [1] http://hg.mozilla.org/mozilla-central/file/e8fa13708c07/dom/animation/test/chrome/test_animation_observers.html#l183
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hiikezoe)
Assignee | ||
Comment 8•6 years ago
|
||
Brian, before splitting a new test file out, I have started to rewrite test_animation_observer.html with testharness.js and also to clean it up. One thing I am wondering is that we don't use MutationObserver.takeRecords(), if we do use it, we don't need to wait for a frame every time. I have not checked all of test cases, but at first glance, it should work. Is(Was) there any concerns about using takeRecords()? This timeout might be solved by just eliminating await_frame(). Boris, pseudo element part is bit tricky, it uses declarative CSS animation keyframe and also uses Element.animate(), so I am going to split them into another file (named test_observers_for_pseudo_element?). Do you have any advises for it?
Flags: needinfo?(boris.chiou)
Flags: needinfo?(bbirtles)
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > Brian, before splitting a new test file out, I have started to rewrite > test_animation_observer.html with testharness.js and also to clean it up. > One thing I am wondering is that we don't use > MutationObserver.takeRecords(), if we do use it, we don't need to wait for > a frame every time. I have not checked all of test cases, but at first > glance, it should work. Is(Was) there any concerns about using > takeRecords()? This timeout might be solved by just eliminating > await_frame(). Oops, forget about it. Some of await_frame() can be removed, but some of it can't be removed.
Flags: needinfo?(bbirtles)
Comment hidden (Intermittent Failures Robot) |
Comment 11•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > Brian, before splitting a new test file out, I have started to rewrite > test_animation_observer.html with testharness.js and also to clean it up. > One thing I am wondering is that we don't use > MutationObserver.takeRecords(), if we do use it, we don't need to wait for > a frame every time. I have not checked all of test cases, but at first > glance, it should work. Is(Was) there any concerns about using > takeRecords()? This timeout might be solved by just eliminating > await_frame(). > > Boris, pseudo element part is bit tricky, it uses declarative CSS animation > keyframe and also uses Element.animate(), so I am going to split them into > another file (named test_observers_for_pseudo_element?). Do you have any > advises for it? It's Ok to me. This framework was written by Cameron and I just added what we need into this file. However, this test file is too large now, I think, so it's better to split them! Yes, pseudo element part is tricky, there are some other test files for pseudo element with test_harness.js: [1] http://searchfox.org/mozilla-central/rev/8071f94c4d4b66833ad08db39565669faad94dfd/dom/animation/test/css-animations/file_pseudoElement-get-animations.html [2] http://searchfox.org/mozilla-central/rev/8071f94c4d4b66833ad08db39565669faad94dfd/testing/web-platform/tests/web-animations/interfaces/Animatable/animate.html#164-178 [3] http://searchfox.org/mozilla-central/rev/8071f94c4d4b66833ad08db39565669faad94dfd/dom/animation/test/css-animations/file_effect-target.html#19-50 [4] http://searchfox.org/mozilla-central/rev/8071f94c4d4b66833ad08db39565669faad94dfd/dom/animation/test/css-animations/file_document-get-animations.html#243-272 Hope these tests can help you. Thank you.
Flags: needinfo?(boris.chiou)
Assignee | ||
Comment 12•6 years ago
|
||
Thank you Boris! It will help me a lot. What I was surprised is that we can specify multiple lines on searchfox. I did not know that!
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 14•6 years ago
|
||
I am still partway because I don't have a cool idea to handle MutationObserver in asynchronous tests with testharness.js. But splitting some tests seems to make improvements. https://treeherder.mozilla.org/#/jobs?repo=try&revision=278a443da943 So I've decided to land those partway patches for now, and will finish up this work later.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•6 years ago
|
||
Boris, could you please take time to review these patch? I am sorry that the difference of part 4 is hard to see.
Comment 21•6 years ago
|
||
mozreview-review |
Comment on attachment 8800522 [details] Bug 1283754 - Part 1: Drop await_timeout() and await_timeout(). https://reviewboard.mozilla.org/r/85436/#review84006
Attachment #8800522 -
Flags: review?(boris.chiou) → review+
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8800523 [details] Bug 1283754 - Part 2: Replace await_frame() with waitForFrame(). https://reviewboard.mozilla.org/r/85438/#review84012
Attachment #8800523 -
Flags: review?(boris.chiou) → review+
Comment 23•6 years ago
|
||
mozreview-review |
Comment on attachment 8800525 [details] Bug 1283754 - Part 4: Split test cases for script animations which can be re-written to synchronous test in test_animation_observers.html. https://reviewboard.mozilla.org/r/85442/#review84026 r=me with these comments addressed. Thanks for making these tests more concise by testharness.js. ::: dom/animation/test/chrome/test_animation_observers.html:1525 (Diff revision 1) > [ div, pseudoTarget ].forEach(function(target) { > addAsyncAnimTest("change_duration_and_currenttime", > { observe: div, subtree: true }, function*() { Looks like we still leave the sync tests for pseudo elements in test_animation_observers.html, and try to move all the tests for script animations to the new file. Is it possible to move this test for |div| into the new test file and leave this one for |pseudoTarget| here? (We may also need to rename it, I think. e.g. xxxx_on_animatons_targeting_pseudo_elements) So all the script animations, except those for pseudo elements, are in test_observers_for_script_animation.html. Would you rewrite the tests for pseudo elements by testharness.js later? If yes, we could merge them again in that patch. Thanks. ::: dom/animation/test/chrome/test_observers_for_script_animation.html:174 (Diff revision 1) > + var observer = setupSynchronousObserver(t, div, false); > + > + var effect = new KeyframeEffectReadOnly(null, > + { opacity: [ 0, 1 ] }, > + { duration: 10000 }); > + var anim = new Animation(effect, document.timeline); > + anim.play(); > + assert_equals_records(observer.takeRecords(), > + [], "no records after animation is added"); > +}, "create_animation_without_target"); How about "setupSynchronousObserver(t, document, true);" ? We don't do anything to div, so there is no mutation events obviously. However, we add an animation in this document, so I think it's better to observe docuement and its subtree. Hoever, using div here is also ok because this animation doesn't be attached to any element.
Attachment #8800525 -
Flags: review?(boris.chiou) → review+
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8800524 [details] Bug 1283754 - Part 3: Add setupSynchronousObserver to write synchronouse tests for MutaionObserver. https://reviewboard.mozilla.org/r/85440/#review84034
Attachment #8800524 -
Flags: review?(boris.chiou) → review+
Comment 25•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800525 [details] Bug 1283754 - Part 4: Split test cases for script animations which can be re-written to synchronous test in test_animation_observers.html. https://reviewboard.mozilla.org/r/85442/#review84026 > How about "setupSynchronousObserver(t, document, true);" ? > > We don't do anything to div, so there is no mutation events obviously. However, we add an animation in this document, so I think it's better to observe docuement and its subtree. Hoever, using div here is also ok because this animation doesn't be attached to any element. Oh. Part 5 will include both cases (i.e. div without subtree, and document with subtree), so this is OK. Thanks.
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8800526 [details] Bug 1283754 - Part 5: Run tests with subtree:true option. https://reviewboard.mozilla.org/r/85444/#review84064 r=me with above comments addressed. Thanks. ::: dom/animation/test/chrome/test_observers_for_script_animation.html:129 (Diff revision 1) > - var observer = setupSynchronousObserver(t, div, false); > + var observer = > + setupSynchronousObserver(t, > + aOptions.subtree ? div.parentNode : div, > + aOptions.subtree); > > - var anim = div.animate({ opacity: [ 0, 1 ] }, 100000); > + var anim = div.animate({ opacity: [ 0, 1 ] }, 100000); Nit: Using MS_PER_SEC may be better. ::: dom/animation/test/chrome/test_observers_for_script_animation.html:144 (Diff revision 1) > > - anim.effect.timing.delay = 100; > + anim.effect.timing.delay = 100; > - assert_equals_records(observer.takeRecords(), > + assert_equals_records(observer.takeRecords(), > - [], "records after assigning same value"); > + [], "records after assigning same value"); > > - anim.effect.timing.delay = -100000; > + anim.effect.timing.delay = -100000; Nit: Using MS_PER_SEC may be better. ::: dom/animation/test/chrome/test_observers_for_script_animation.html:202 (Diff revision 1) > + aOptions.subtree ? div.parentNode : div, > + aOptions.subtree); > > - var effect = new KeyframeEffectReadOnly(null, > + var effect = new KeyframeEffectReadOnly(null, > - { opacity: [ 0, 1 ] }, > + { opacity: [ 0, 1 ] }, > - { duration: 10000 }); > + { duration: 10000 }); Nit: Using MS_PER_SEC may be better. ::: dom/animation/test/chrome/test_observers_for_script_animation.html:394 (Diff revision 1) > + div.appendChild(child); > + > + var anim1 = div.animate({ marginLeft: [ "0px", "50px" ] }, > + 100 * MS_PER_SEC); > + var anim2 = child.animate({ marginLeft: [ "0px", "100px" ] }, > + 50 * MS_PER_SEC); Nit: indention: two redundent spaces before "50 * MS_PER_SEC"
Attachment #8800526 -
Flags: review?(boris.chiou) → review+
Assignee | ||
Comment 27•6 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #23) > ::: dom/animation/test/chrome/test_animation_observers.html:1525 > (Diff revision 1) > > [ div, pseudoTarget ].forEach(function(target) { > > addAsyncAnimTest("change_duration_and_currenttime", > > { observe: div, subtree: true }, function*() { > > Looks like we still leave the sync tests for pseudo elements in > test_animation_observers.html, and try to move all the tests for script > animations to the new file. > Is it possible to move this test for |div| into the new test file and leave > this one for |pseudoTarget| here? (We may also need to rename it, I think. > e.g. xxxx_on_animatons_targeting_pseudo_elements) So all the script > animations, except those for pseudo elements, are in > test_observers_for_script_animation.html. Ah, indeed. Moving the part of the test for non-pseudo target sounds reasonable. I've never thought it. Great, thanks! > Would you rewrite the tests for pseudo elements by testharness.js later? If > yes, we could merge them again in that patch. Thanks. Yes, right. A pseudo test needs asynchronous handling.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 33•6 years ago
|
||
Pushed by hiikezoe@mozilla-japan.org: https://hg.mozilla.org/integration/autoland/rev/0bfbcb61bcb2 Part 1: Drop await_timeout() and await_timeout(). r=boris https://hg.mozilla.org/integration/autoland/rev/ab6ca79361fd Part 2: Replace await_frame() with waitForFrame(). r=boris https://hg.mozilla.org/integration/autoland/rev/2664c275c9d5 Part 3: Add setupSynchronousObserver to write synchronouse tests for MutaionObserver. r=boris https://hg.mozilla.org/integration/autoland/rev/8a34e43924f5 Part 4: Split test cases for script animations which can be re-written to synchronous test in test_animation_observers.html. r=boris https://hg.mozilla.org/integration/autoland/rev/617c4ce41eb2 Part 5: Run tests with subtree:true option. r=boris
Reporter | ||
Comment 34•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0bfbcb61bcb2 https://hg.mozilla.org/mozilla-central/rev/ab6ca79361fd https://hg.mozilla.org/mozilla-central/rev/2664c275c9d5 https://hg.mozilla.org/mozilla-central/rev/8a34e43924f5 https://hg.mozilla.org/mozilla-central/rev/617c4ce41eb2
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 36•6 years ago
|
||
The orange frequency was actually reduced, but still there. https://treeherder.mozilla.org/logviewer.html#?repo=autoland&job_id=5129099#L3545 I will split test cases for CSS animations/transition that can be rewritten to synchronous test.
Assignee | ||
Comment 37•6 years ago
|
||
It turns out that many of test cases can not be rewritten to synchronous test. E.g. * Waiting for events, e.g. transitionend * Manipulated by style, e.g. style.animationDuraton = "0.5s". * This kind of test needs a tick, i.e. advancing time. * Falling into a pending state, e.g. play() or pause(). So I've decided that those of tests are left in the original file (test_animation_observers.html) and rename test_observers_for_script_animation.html to test_observers_for_synchronous_api.html (and tests can be rewritten to synchronous are moved into this file).
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 39•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=94597102b4d9aa35a15a8fb52553a6c0b1409b8f
Assignee | ||
Comment 40•6 years ago
|
||
Closing because there is no timeout failure on mozilla-central, and pushed bunch of fixes (in this bug and 1310605) to aurora branch as well. The elapsed time of the test file reduced to nearly half of timeout limit now. Let's open a new bug once this timeout happens again.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Keywords: leave-open
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•