1406191 - Perma test-verify browser/components/customizableui/test/browser_widget_animation.js | Test timed out

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This seems to always fail when run more than once and so lets the test-verify suite fail if it has been modified:
browser/components/customizableui/test/browser_widget_animation.js | Test timed out 

See https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1053bf2fbaed195d37a07ae81d5086cac0b8f6b8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

[task 2017-10-05T19:33:48.009Z] 19:33:48     INFO - TEST-START | browser/components/customizableui/test/browser_widget_animation.js
[task 2017-10-05T19:33:50.609Z] 19:33:50     INFO - GECKO(1147) | 1507232030601	addons.xpi	WARN	Attempting to activate an already active default theme
[task 2017-10-05T19:33:50.632Z] 19:33:50     INFO - GECKO(1147) | MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
[task 2017-10-05T19:33:50.633Z] 19:33:50     INFO - GECKO(1147) | MEMORY STAT | vsize 2185MB | residentFast 270MB | heapAllocated 126MB
[task 2017-10-05T19:33:50.634Z] 19:33:50     INFO - TEST-OK | browser/components/customizableui/test/browser_widget_animation.js | took 2619ms
[task 2017-10-05T19:33:50.660Z] 19:33:50     INFO - checking window state
[task 2017-10-05T19:33:50.681Z] 19:33:50     INFO - TEST-START | browser/components/customizableui/test/browser_widget_animation.js
[task 2017-10-05T19:34:35.694Z] 19:34:35     INFO - TEST-INFO | started process screentopng
[task 2017-10-05T19:34:36.546Z] 19:34:36     INFO - TEST-INFO | screentopng: exit 0
[task 2017-10-05T19:34:36.546Z] 19:34:36     INFO - Buffered messages logged at 19:33:50
[task 2017-10-05T19:34:36.547Z] 19:34:36     INFO - Entering test bound 
[task 2017-10-05T19:34:36.549Z] 19:34:36     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 174}]
[task 2017-10-05T19:34:36.550Z] 19:34:36     INFO - Buffered messages finished
[task 2017-10-05T19:34:36.551Z] 19:34:36     INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_widget_animation.js | Test timed out - 
[task 2017-10-05T19:34:36.552Z] 19:34:36     INFO - GECKO(1147) | 1507232075693	addons.xpi	WARN	Attempting to activate an already active default theme
[task 2017-10-05T19:34:36.553Z] 19:34:36     INFO - Console message: 1507232075693	addons.xpi	WARN	Attempting to activate an already active default theme
[task 2017-10-05T19:34:36.555Z] 19:34:36     INFO - GECKO(1147) | MEMORY STAT | vsize 2294MB | residentFast 226MB | heapAllocated 90MB
[task 2017-10-05T19:34:36.556Z] 19:34:36     INFO - TEST-OK | browser/components/customizableui/test/browser_widget_animation.js | took 45047ms

Comment 1

[Intermittent Failures Robot]

12 failures in 824 pushes (0.015 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-inbound: 4
* mozilla-central: 4
* autoland: 4

Platform breakdown:
* linux64: 12

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1406191&startday=2017-10-02&endday=2017-10-08&tree=all

Comment 2

[:Gijs (he/him)]

Erica, seems the test we added in bug 1406191 intermittently fails, which breaks in the test-verify suite. Can you take a look to see if we can make it more reliable so it doesn't perma-fail the TV job? ( see https://groups.google.com/forum/#!topic/mozilla.dev.platform/sf3SjDAKL60 and https://groups.google.com/forum/#!topic/mozilla.dev.platform/e67mB7rXFLM for some more details).

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs (PTO EOB on 11th) from comment #2)
> Erica, seems the test we added in bug 1406191

Err, bug 1390313, of course...

Comment 4

[Erica Wright [:ewright]]

Attachment: Bug 1406191 - Cleanup browser_widget_animation.js, remove event listener and close panel correctly.

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

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8917129 [details]
Bug 1406191 - Cleanup browser_widget_animation.js, remove event listener and close panel correctly.

https://reviewboard.mozilla.org/r/188122/#review193506

::: browser/components/customizableui/test/browser_widget_animation.js:48
(Diff revision 1)
>    let shownPromise = popupShown(contextMenu);
>    EventUtils.synthesizeMouseAtCenter(homeButton, {type: "contextmenu", button: 2 });
>    await shownPromise;
>  
>    let moveToPanel = contextMenu.querySelector(".customize-context-moveToPanel");
>    if (moveToPanel) {

Why is there an if-condition here? Doing this can be a good pattern in production code but is definitely an anti-pattern when writing tests.

Is there any reason to believe that moveToPanel could not exist? Should we fail with an assertion if it doesn't exist, instead? If this click is really optional in this test, that should be marked with a comment.

::: browser/components/customizableui/test/browser_widget_animation.js:51
(Diff revision 1)
>  
>    let moveToPanel = contextMenu.querySelector(".customize-context-moveToPanel");
>    if (moveToPanel) {
>      moveToPanel.click();
>    }
> +  await contextMenu.hidePopup();

Since this is fixing an intermittent issue, I suppose the popup is sometimes hidden when clicking moveToPanel and sometimes not? Do we know why? Could this be because of the if (moveToPanel) condition skipping the click?

I don't require you to dive deep into XUL popup issues to find it out, I'd just like to rule out that there's a race condition in your code that this test is exposing.

If you don't know, I'd be okay with just adding a comment to this line :)

Comment 6

[Erica Wright [:ewright]]

Comment on attachment 8917129 [details]
Bug 1406191 - Cleanup browser_widget_animation.js, remove event listener and close panel correctly.

https://reviewboard.mozilla.org/r/188122/#review193506

> Since this is fixing an intermittent issue, I suppose the popup is sometimes hidden when clicking moveToPanel and sometimes not? Do we know why? Could this be because of the if (moveToPanel) condition skipping the click?
> 
> I don't require you to dive deep into XUL popup issues to find it out, I'd just like to rule out that there's a race condition in your code that this test is exposing.
> 
> If you don't know, I'd be okay with just adding a comment to this line :)

The issue happens from the test-verify command, when it tests 10 times on the same browser. The second time (and subsequent) always fails. This is because the popup was never closing, so the `await shownPromise;` line fails. The first time, the popup being left open was never a problem. I don't think it requires a comment, since it seems pretty good to me to close things after opening them.

Comment 7

[Erica Wright [:ewright]]

Comment on attachment 8917129 [details]
Bug 1406191 - Cleanup browser_widget_animation.js, remove event listener and close panel correctly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188122/diff/1-2/

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8917129 [details]
Bug 1406191 - Cleanup browser_widget_animation.js, remove event listener and close panel correctly.

https://reviewboard.mozilla.org/r/188122/#review193578

Ah, thank you for the context. Please add this explanation to the commit message then that's r=me.

(Feel free to remove the mentioned if-condition).

Comment 9

[Johann Hofmann [:johannh]]

> (Feel free to remove the mentioned if-condition).

Heh, I reviewed an outdated version just before you updated it, this looks good, thank you.

Comment 10

[Erica Wright [:ewright]]

Comment on attachment 8917129 [details]
Bug 1406191 - Cleanup browser_widget_animation.js, remove event listener and close panel correctly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188122/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b581fa3d8e7a
Cleanup browser_widget_animation.js, remove event listener and close panel correctly. r=johannh

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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