Closed Bug 1283754 Opened 5 years ago Closed 4 years ago

Intermittent test_animation_observers.html | Test timed out.

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: cbook, Assigned: hiro)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(5 files)

Bug 1283754 - Part 1: Drop await_timeout() and await_timeout().
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1283754 - Part 2: Replace await_frame() with waitForFrame().
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1283754 - Part 3: Add setupSynchronousObserver to write synchronouse tests for MutaionObserver.
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1283754 - Part 4: Split test cases for script animations which can be re-written to synchronous test in test_animation_observers.html.
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1283754 - Part 5: Run tests with subtree:true option.
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.
Priority: -- → P3
I think it's time to split test_animation_observers.html into smaller chunks.
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)
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)
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)
(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)
(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)
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!
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.
Boris, could you please take time to review these patch?  I am sorry that the difference of part 4 is hard to see.
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 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 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 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 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 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+
(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.
Keywords: leave-open
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
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.
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).
Depends on: 1310605
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: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.