Closed Bug 1406191 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: aryx, Assigned: ewright)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1406191 - Cleanup browser_widget_animation.js, remove event listener and close panel correctly.
59 bytes, text/x-review-board-request
johannh
: review+
Details 
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
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).
Blocks: 1390313
Flags: needinfo?(ewright)
(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...
Assignee: nobody → ewright
Flags: needinfo?(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

::: 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 :)
Attachment #8917129 - Flags: review?(jhofmann)
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 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).
Attachment #8917129 - Flags: review?(jhofmann) → review+
> (Feel free to remove the mentioned if-condition).

Heh, I reviewed an outdated version just before you updated it, this looks good, thank you.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b581fa3d8e7a
Cleanup browser_widget_animation.js, remove event listener and close panel correctly. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b581fa3d8e7a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: