Intermittent TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Test timed out

VERIFIED FIXED in Firefox 7

Status

defect
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: mbrubeck, Assigned: raymondlee)

Tracking

({intermittent-failure})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: , [inbound])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

8 years ago
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1307972575.1307974430.21626.gz

Rev3 Fedora 12 mozilla-central opt test mochitest-other on 2011/06/13 06:42:55 

INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_group.js | finished in 1314ms
TEST-INFO | checking window state
TEST-INFO | before wait for focus -- loaded: complete active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XrayWrapper [object Window]]) about:blank desired window: ([object ChromeWindow]) chrome://browser/content/browser.xul child window: ([object XrayWrapper [object Window]]) about:blank docshell visible: true
TEST-INFO | already focused
TEST-INFO | maybe run tests <load:true, focus:true> -- loaded: complete active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XrayWrapper [object Window]]) about:blank desired window: ([object ChromeWindow]) chrome://browser/content/browser.xul child window: ([object XrayWrapper [object Window]]) about:blank docshell visible: true
TEST-START | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View starts hidden
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | The tab view iframe exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Test timed out
XScreenSaver state: Disabled
User input has been idle for 5476 seconds
before 569344, after 548864, break 08719000
args: ['/home/cltbld/talos-slave/test/build/bin/screentopng']
[screenshot cut for length]
INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | finished in 30004ms
TEST-INFO | checking window state
TEST-INFO | before wait for focus -- loaded: complete active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XrayWrapper [object Window]]) about:blank desired window: ([object ChromeWindow]) chrome://browser/content/browser.xul child window: ([object XrayWrapper [object Window]]) about:blank docshell visible: true
TEST-INFO | already focused
TEST-INFO | maybe run tests <load:true, focus:true> -- loaded: complete active window: ([object ChromeWindow]) chrome://browser/content/browser.xul focused window: ([object XrayWrapper [object Window]]) about:blank desired window: ([object ChromeWindow]) chrome://browser/content/browser.xul child window: ([object XrayWrapper [object Window]]) about:blank docshell visible: true
TEST-START | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_multiwindow_search.js
(Reporter)

Updated

8 years ago
Summary: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Test timed out → Intermittent TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Test timed out
(Reporter)

Comment 1

8 years ago
Posted image screenshot
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
(Assignee)

Comment 4

8 years ago
Posted patch v1 (obsolete) — Splinter Review
I am not sure what is causing the strange overlay box in the screenshot in comment 1.  I've modified the code to open the test in a new window and refactored the code.

Tim: any suggestions?
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #540004 - Flags: feedback?(tim.taubert)
(In reply to comment #4)
> I am not sure what is causing the strange overlay box in the screenshot in
> comment 1.  I've modified the code to open the test in a new window and
> refactored the code.

I guess that's just some artifacts from Xvfb on the linux build machines.

> Tim: any suggestions?

1.) We should refactor the test to not use setTimeout() with an interval bigger than zero. Otherwise we will be marked as "flaky" in the near future (bug 649012).

2.) The test times out in the waitForSwitch() loop, so we should find out why it fails there. I have no idea, yet.

3.) Why do we even have such a complicated approach here? showTabView() seems reliable and would be much easier (I don't know the history of the test and why we do that...)
(Assignee)

Comment 6

8 years ago
(In reply to comment #5)
> (In reply to comment #4)
> > I am not sure what is causing the strange overlay box in the screenshot in
> > comment 1.  I've modified the code to open the test in a new window and
> > refactored the code.
> 
> I guess that's just some artifacts from Xvfb on the linux build machines.
> 
> > Tim: any suggestions?
> 
> 1.) We should refactor the test to not use setTimeout() with an interval
> bigger than zero. Otherwise we will be marked as "flaky" in the near future
> (bug 649012).
> 
> 2.) The test times out in the waitForSwitch() loop, so we should find out
> why it fails there. I have no idea, yet.
> 
> 3.) Why do we even have such a complicated approach here? showTabView()
> seems reliable and would be much easier (I don't know the history of the
> test and why we do that...)

See bug 594909 comment 56 was the bug which added this approach.
(In reply to comment #6)
> > 3.) Why do we even have such a complicated approach here? showTabView()
> > seems reliable and would be much easier (I don't know the history of the
> > test and why we do that...)
> 
> See bug 594909 comment 56 was the bug which added this approach.

So, let's just add an event listener like this:

>  let onSelect = function (event) {
>    if (deck != event.target)
>      return;
>
>    let iframe = document.getElementById("tab-view");
>    if (!iframe || deck.selectedPanel != iframe)
>      return;
>
>    deck.removeEventListener("select", onSelect, false);
>
>    // do stuff here...
>  };
>
>  let deck = document.getElementById("tab-view-deck");
>  deck.addEventListener("select", onSelect, false);

This should of course be added in function "test" before TabView.toggle() is called. So we can get rid of all those flaky timeouts :)
(Assignee)

Comment 8

8 years ago
Posted patch v2 (obsolete) — Splinter Review
Push to try and wait for results http://tbpl.mozilla.org/?tree=Try&rev=a53cd7620dbf
Attachment #540004 - Attachment is obsolete: true
Attachment #540004 - Flags: feedback?(tim.taubert)
Attachment #540435 - Flags: feedback?(tim.taubert)
Comment on attachment 540435 [details] [diff] [review]
v2

Review of attachment 540435 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: browser/base/content/test/tabview/browser_tabview_launch.js
@@ +16,5 @@
> +      if (deck != event.target)
> +        return;
> +
> +      let iframe = win.document.getElementById("tab-view");
> +      if (!iframe || deck.selectedPanel != iframe)

nit: I know that line is from me but we don't need the "!iframe" check.

@@ +22,5 @@
> +
> +      deck.removeEventListener("select", onSelect, true);
> +
> +      whenTabViewIsShown(function() {
> +        waitForFocus(function() {

Please just wrap the EventUtils.synthesizeKey() call in waitForFocus().

@@ +35,5 @@
> +                finish();
> +              }, win);
> +              EventUtils.synthesizeKey("E", { accelKey: true, shiftKey: true }, win);
> +            }, win);
> +            win.document.getElementById("menu_tabview").doCommand();

Can't we just use this to hide the tabview and show it again with ctrl+shift+e? This would save some lines and make that test a bit cleaner.

@@ +48,3 @@
>  
>    registerCleanupFunction(function () {
> +    if (newWin)

nit: this check is unnecessary.
Attachment #540435 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Comment 10

8 years ago
Posted patch v3 (obsolete) — Splinter Review
(In reply to comment #9)
> Comment on attachment 540435 [details] [diff] [review] [review]
> v2
> 
> Review of attachment 540435 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> ::: browser/base/content/test/tabview/browser_tabview_launch.js
> @@ +16,5 @@
> > +      if (deck != event.target)
> > +        return;
> > +
> > +      let iframe = win.document.getElementById("tab-view");
> > +      if (!iframe || deck.selectedPanel != iframe)
> 
> nit: I know that line is from me but we don't need the "!iframe" check.

Removed

> 
> @@ +22,5 @@
> > +
> > +      deck.removeEventListener("select", onSelect, true);
> > +
> > +      whenTabViewIsShown(function() {
> > +        waitForFocus(function() {
> 
> Please just wrap the EventUtils.synthesizeKey() call in waitForFocus().
> 

I've removed it but it turned out that the test would time out.  I have replaced it with executeSoon() and it works again.

> @@ +35,5 @@
> > +                finish();
> > +              }, win);
> > +              EventUtils.synthesizeKey("E", { accelKey: true, shiftKey: true }, win);
> > +            }, win);
> > +            win.document.getElementById("menu_tabview").doCommand();
> 
> Can't we just use this to hide the tabview and show it again with
> ctrl+shift+e? This would save some lines and make that test a bit cleaner.
> 

Changed to ctrl+shift+e

> @@ +48,3 @@
> >  
> >    registerCleanupFunction(function () {
> > +    if (newWin)
> 
> nit: this check is unnecessary.

Removed
Attachment #540435 - Attachment is obsolete: true
Attachment #540444 - Flags: review?(ian)
Comment on attachment 540444 [details] [diff] [review]
v3

Review of attachment 540444 [details] [diff] [review]:
-----------------------------------------------------------------

* Why the onSelect thing, instead of just whenTabViewIsShown?
* The old patch tested both the key combo and the menu item, while the new patch tests only the key combo; should test the menu item as well
* For both the key combo and menu item, test both launching into and leaving from tabView (the original tested only launching into, but since we have to exit anyway, might as well exit in the fashion you entered... you're already doing this with the key combo; might as well do the same for menu item)
Attachment #540444 - Flags: review?(ian) → review-
(Assignee)

Comment 12

8 years ago
(In reply to comment #11)
> Comment on attachment 540444 [details] [diff] [review] [review]
> v3
> 
> Review of attachment 540444 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> * Why the onSelect thing, instead of just whenTabViewIsShown?

We used it to replace the waitForSwitch() which was added in bug 594909 comment 56.  There was another Intermittent.  I guess we can try without the onSelect thing and use the whenTabViewIsShown instead.
(In reply to comment #11)
> * Why the onSelect thing, instead of just whenTabViewIsShown?

I thought that was the whole crux of the patch. Testing whether the tabview is _really_ shown (the iframe is the selectedPanel), although we can be quite confident about that :)

> * The old patch tested both the key combo and the menu item, while the new
> patch tests only the key combo; should test the menu item as well

Yeah, I actually meant that both should be tested. Sorry, if I was a bit unclear about that.
Adding intermittent test failures to new meta bug.

(bugspam)
Blocks: 665844
(Assignee)

Comment 15

8 years ago
Posted patch v4 (obsolete) — Splinter Review
(In reply to comment #13)
> (In reply to comment #11)
> > * Why the onSelect thing, instead of just whenTabViewIsShown?
> 
> I thought that was the whole crux of the patch. Testing whether the tabview
> is _really_ shown (the iframe is the selectedPanel), although we can be
> quite confident about that :)

Yes, I am keeping this part.

> 
> > * The old patch tested both the key combo and the menu item, while the new
> > patch tests only the key combo; should test the menu item as well
> 
> Yeah, I actually meant that both should be tested. Sorry, if I was a bit
> unclear about that.

I put the menu item test back in and simplified the part.
Attachment #540444 - Attachment is obsolete: true
Attachment #540978 - Flags: review?(ian)
Comment on attachment 540978 [details] [diff] [review]
v4

Review of attachment 540978 [details] [diff] [review]:
-----------------------------------------------------------------

Now you're exiting with the command and entering with the key. My suggestion was to do two rounds: enter and exit with the command, and enter and exit with the key. That way you get even test coverage. Sorry if I was unclear about that.
Attachment #540978 - Flags: review?(ian) → review-
(Assignee)

Comment 17

8 years ago
Posted patch v5 (obsolete) — Splinter Review
(In reply to comment #16)
> Comment on attachment 540978 [details] [diff] [review] [review]
> v4
> 
> Review of attachment 540978 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Now you're exiting with the command and entering with the key. My suggestion
> was to do two rounds: enter and exit with the command, and enter and exit
> with the key. That way you get even test coverage. Sorry if I was unclear
> about that.

Done.

Pushed to try and waiting for results
http://tbpl.mozilla.org/?tree=Try&rev=d52569b535fd
Attachment #540978 - Attachment is obsolete: true
Attachment #541492 - Flags: review?(ian)
Comment on attachment 541492 [details] [diff] [review]
v5

Review of attachment 541492 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful, thank you!
Attachment #541492 - Flags: review?(ian) → review+
(Assignee)

Comment 19

8 years ago
Attachment #541492 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/33fa6743bde1
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
Comment hidden (Legacy TBPL/Treeherder Robot)
Verified fixed based on results in:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1314760632.1314761843.20879.gz
Status: RESOLVED → VERIFIED
Whiteboard: [orange], [inbound] → , [inbound]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.