1245849 - Intermittent browser_animation_name.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/server/tests/browser/browser_animation_name.js:48 - TypeError: player is undefined

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Phil Ringnalda (:philor)]

https://treeherder.mozilla.org/logviewer.html#?job_id=21052423&repo=mozilla-inbound

Comment 1

[Intermittent Failures Robot]

12 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* fx-team: 6
* try: 5
* mozilla-inbound: 1

Platform breakdown:
* linux64: 10
* linux32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1245849&startday=2016-02-01&endday=2016-02-07&tree=all

Comment 2

[Patrick Brosset <:pbro>]

This test fails on linux (often on debug builds), most probably because the animations are already ended when the test tries to access them.

Comment 3

[Patrick Brosset <:pbro>]

My strategy now is to investigate a little bit how I could turn this (and other) tests into xpcshell tests instead, so that I can safely test the underlying code logic rather than have to instantiate a debugger server and rely try to get animation objects.
If that doesn't work, I'll just have to make animations last longer.

Comment 4

[Patrick Brosset <:pbro>]

I managed to make an xpcshell test instead. My logic was that this test was only exercising a simple helper method anyway. The fact that we started a debugger server and went through the debugger protocol to get this information was just extra things on top of what we really wanted to test in the first place.
Now the test doesn't rely on a real animation, the whole DOM is mocked, and we only test the function's logic.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1245849 - Remove mochitest browser_animation_name.js and add a xpcshell test instead; r=ochameau

Review commit: https://reviewboard.mozilla.org/r/33975/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33975/

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8716905 [details]
MozReview Request: Bug 1245849 - Remove mochitest browser_animation_name.js and add a xpcshell test instead; r=ochameau

https://reviewboard.mozilla.org/r/33975/#review30581

Looks good even if I tend to prefer integration test in favor of unit test, especially when involving Mocks.
Here, we reduce the coverage of this test. We no longer cover the DOM to AnimationPlayerActor translation.
But we may be covering that with some other tests.

::: devtools/server/tests/unit/test_animation_name.js:27
(Diff revision 1)
> +  window.CSSTransition.prototype = Object.create(window.Animation.prototype);

A comment about why we need to hack the prototypes would help, here and in the other test.

::: devtools/server/tests/unit/test_animation_name.js:44
(Diff revision 1)
> +  // - expectedName {String} The expected name out of getName.

s/getName/AnimationPlayerActor.getName/

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Alexandre Poirot [:ochameau] from comment #6)
> Comment on attachment 8716905 [details]
> MozReview Request: Bug 1245849 - Remove mochitest browser_animation_name.js
> and add a xpcshell test instead; r=ochameau
> 
> https://reviewboard.mozilla.org/r/33975/#review30581
> 
> Looks good even if I tend to prefer integration test in favor of unit test,
> especially when involving Mocks.
> Here, we reduce the coverage of this test. We no longer cover the DOM to
> AnimationPlayerActor translation.
> But we may be covering that with some other tests.
We have a lot of integration tests for both the server methods and the UI, so yes, we are covering this in other tests. So, for this test, it seemed silly to me to have this much extra overhead for really just testing the logic in getName and getType.

> ::: devtools/server/tests/unit/test_animation_name.js:27
> (Diff revision 1)
> > +  window.CSSTransition.prototype = Object.create(window.Animation.prototype);
> 
> A comment about why we need to hack the prototypes would help, here and in
> the other test.
We really don't need it in fact. I thought we did, but that's not going to be true until bug 1232681 lands. So I can remove this part for now.

Comment 8

[Patrick Brosset <:pbro>]

https://bugzilla.mozilla.org/show_bug.cgi?id=1245849

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/da848893e83e

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #8)
> https://bugzilla.mozilla.org/show_bug.cgi?id=1245849
Ignore this link, I meant to paste the link to my try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf0d67aa00c2

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da848893e83e