637328 - Timeout failure in testNewTab.js | controller.waitForEval: Timeout exceeded for 'subject.tab.opened == true'

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aaron Train [:aaronmt]]

Module: testNewTab.js
Test: testNewTab
Error: controller.waitForEval: Timeout exceeded for 'subject.tab.opened == true'
Branch: default (4.0)
Platform: Ubuntu 10.10

Recent failure: http://mozmill-release.brasstacks.mozilla.com/#/general/report/ad63c40d27ca3a079362528243087e5c

http://mozmill-release.brasstacks.mozilla.com/#/general/failure?branch=4.0&from=2011-02-01&to=2011-02-28&test=firefox%2FtestTabbedBrowsing%2FtestNewTab.js&func=testNewTab

This failure is occurring on Ubuntu 10.10 32-bit and 64-bit.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Do we know which action in detail is failing here? I would assume the double click onto the tab bar.

Comment 2

[Aaron Train [:aaronmt]]

Double click on the tab-strip, i.e., checkOpenTab({type: "tabStrip"});
http://hg.mozilla.org/qa/mozmill-tests/file/d029fca85b45/shared-modules/tabs.js#l455

Comment 3

[Aaron Train [:aaronmt]]

This is caused by bug 635397

Comment 4

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default) [linux skip action][checked-in]

Skip tab strip action on Linux

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 515720 [details] [diff] [review]
Patch v1 - (default) [linux skip action][checked-in]

I dislike the new behavior, also with the fact it never got ui-review. But that's another issue. This patch looks fine.

Aaron, can you please update the assert method inside checkOpenTab which checks for the new tab opened? It should include the type property, which will make it much easier to spot failures. Another review will be necessary.

Comment 6

[Aaron Train [:aaronmt]]

(In reply to comment #5)
> Aaron, can you please update the assert method inside checkOpenTab which checks
> for the new tab opened? 
For clarification, do you mean the waitForEval? 
http://hg.mozilla.org/qa/mozmill-tests/file/d029fca85b45/firefox/testTabbedBrowsing/testNewTab.js#l83

>It should include the type property, which will make it much easier to spot failures.

type property of what object?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

(In reply to comment #6)
> type property of what object?

The function parameter: checkOpenTab({type: "tabStrip"});

Comment 8

[Aaron Train [:aaronmt]]

Initial patch landed on default - http://hg.mozilla.org/qa/mozmill-tests/rev/a95059db04e3

Comment 9

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default) [assert/wait refactor]

Refactored the asserts/waits to provide cleaner output

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 515944 [details] [diff] [review]
Patch v1 - (default) [assert/wait refactor]

>-  controller.waitForEval("subject.length == 2", TIMEOUT, 100, controller.tabs);

As talked on IRC we can completely remove TIMEOUT.

>+  controller.assert(function() {

nit: missing space before the brackets.

>+    return controller.tabs.activeTab.location == "about:blank";
>+  }, "The new tab opened via " + event.type + " with 'about:blank'");

I don't think we need event.type here and for the remaining cases. Do you think it's necessary?

Comment 11

[Aaron Train [:aaronmt]]

The value of reusing event.type for related code is that I believe it would assist in detailing the error message by exposing any issues should they occur. What do you think?

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Ok, lets do it. Sounds like it can really be useful if there is different behavior for opening tabs via the different ways.

Comment 13

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - (default) [assert/wait refactor]

Fixed nit, and changed last waitFor, to message hardcoded shortcut

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 516053 [details] [diff] [review]
Patch v2 - (default) [assert/wait refactor]

Looks ok, but I would like to see the get/expected part, which I missed to mention in my last review.

Comment 15

[Aaron Train [:aaronmt]]

Attachment: Path v2.1 - (default) [assert/wait refactor]

Fair enough, if this is +'ed, I'll land it.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 516077 [details] [diff] [review]
Path v2.1 - (default) [assert/wait refactor]

>+  controller.waitFor(function () { 
>+    return controller.tabs.length === 2;
>+  }, "A new tab has opened via " + event.type + " - got " + 
>+    controller.tabs.length + ", expected " + 2);

The values we compare we want to put inside single quotes. That applies to all data types.

>+  controller.waitFor(function () {
>+    return controller.tabs.length === 1;
>+  }, "The new tab closed via shortcut - got " + controller.tabs.length + 
>+    ", expected " + 1);
> }

Same here.

With both issues addressed r=me.

Comment 17

[Aaron Train [:aaronmt]]

Attachment: Patch v2.2 - (default) [assert/wait refactor][checked-in]

Comment 18

[Aaron Train [:aaronmt]]

Landed on default 
http://hg.mozilla.org/qa/mozmill-tests/rev/ac2f555c9f08

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

If a backport to 1.9.2/1.9.1 would be mostly cleanly apply I would say lets get those improvements landed on older branches.

Otherwise it looks like it doesn't fail anymore.

Comment 21

[Aaron Train [:aaronmt]]

Attachment: Patch 2.2 (Backport 1.9.2/1.9.1)[checked-in]

Required a little shifting around with the organizational change

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 519459 [details] [diff] [review]
Patch 2.2 (Backport 1.9.2/1.9.1)[checked-in]

Looks good and is reasonable to land. Thanks!

Comment 23

[Aaron Train [:aaronmt]]

Comment on attachment 519459 [details] [diff] [review]
Patch 2.2 (Backport 1.9.2/1.9.1)[checked-in]

Backport patch landed on 

http://hg.mozilla.org/qa/mozmill-tests/rev/6cfc445617ff  - 1.9.2
http://hg.mozilla.org/qa/mozmill-tests/rev/fa33440f242a - 1.9.1