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

RESOLVED FIXED in Firefox 47

Status

DevTools
Animation Inspector
RESOLVED FIXED
2 years ago
10 days ago

People

(Reporter: philor, Assigned: pbro)

Tracking

({intermittent-failure})

unspecified
Firefox 47
intermittent-failure

Firefox Tracking Flags

(firefox47 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

status-firefox47: --- → affected
Component: Developer Tools → Developer Tools: Animation Inspector
Flags: needinfo?(pbrosset)

Comment 1

2 years ago
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
(Assignee)

Comment 2

2 years ago
This test fails on linux (often on debug builds), most probably because the animations are already ended when the test tries to access them.
Flags: needinfo?(pbrosset)
(Assignee)

Comment 3

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 5

2 years ago
Created attachment 8716905 [details]
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/
Attachment #8716905 - Flags: review?(poirot.alex)
(Assignee)

Updated

2 years ago
See Also: → bug 1246281
Attachment #8716905 - Flags: review?(poirot.alex) → review+
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/
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 10

2 years ago
(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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/da848893e83e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Blocks: 1246281
See Also: bug 1246281

Updated

10 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.