Closed
Bug 624027
Opened 14 years ago
Closed 14 years ago
Timeout exceeded for 'subject.scrollButton.hasAttribute('notifybgtab') == true' (testBackgroundTabScrolling.js)
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: whimboo, Assigned: whimboo)
References
()
Details
(Whiteboard: [mozmill-test-failure])
Attachments
(1 file, 1 obsolete file)
11.95 KB,
patch
|
gmealer
:
review+
|
Details | Diff | Splinter Review |
MODULE: firefox/testTabbedBrowsing/testBackgroundTabScrolling.js TEST: testScrollBackgroundTabIntoView FAIL: controller.waitForEval: Timeout exceeded for 'subject.scrollButton.hasAttribute('notifybgtab') == true' BRANCH: default OS: Windows We have to disable the test on Windows until bug 578162 has been fixed.
Comment 1•14 years ago
|
||
The notifybgtab should be there regardless of bug 578162.
Assignee | ||
Comment 2•14 years ago
|
||
Looks like you are right and this is a timing issue in the test. I was finally able to reproduce it in my XP VM. I will investigate.
Assignee | ||
Comment 3•14 years ago
|
||
This patch updates the test to not use the old waitForEval and assertJS members, which makes it much cleaner. For the real issue on this bug I'm not totally sure, what's going on. After the check for the scroll arrows the test has to wait for at least 100ms before I can open the last tab, which will cause a highlight on the scroll arrow. Dao, is there an event we have to wait for? I really would like to get rid of this sleep call. For opening tabs we already register for the TabOpen event. So not sure what's really missing here.
Assignee: nobody → hskupin
Attachment #502161 -
Flags: review?(gmealer)
Attachment #502161 -
Flags: feedback?(dao)
Comment 4•14 years ago
|
||
Comment on attachment 502161 [details] [diff] [review] Patch v1 > Dao, is there an event we have to wait for? There's an overflow event dispatched to the tab strip, which might be useful here.
Attachment #502161 -
Flags: feedback?(dao)
Assignee | ||
Comment 5•14 years ago
|
||
Comment on attachment 502161 [details] [diff] [review] Patch v1 Thanks. Lets see if that helps.
Attachment #502161 -
Flags: review?(gmealer)
Assignee | ||
Comment 6•14 years ago
|
||
(In reply to comment #4) > There's an overflow event dispatched to the tab strip, which might be useful > here. Dao, are we talking about tabbrowser-tabs? If yes, the overflow event never gets dispatched for me: function overflow(event) { tabstrip.removeEventListener("overflow", this); window.alert("overflow"); } var tabstrip = window.document.getElementById("tabbrowser-tabs"); tabstrip.addEventListener("overflow", overflow, false); Am I doing something wrong?
Assignee | ||
Comment 7•14 years ago
|
||
Ok, got it. The failure was the missing 3rd parameter of the removeEventListener call.
Assignee | ||
Comment 8•14 years ago
|
||
(In reply to comment #4) > There's an overflow event dispatched to the tab strip, which might be useful > here. Ok, so using the event doesn't make any difference for this code path. Even with attaching a listener for the overflow event, opening a new background tab right after the scroll arrows have been shown, doesn't add the 'notifybgtab' attribute to the right arrow button. Only a sleep of about 100ms before the tab open action solved this problem. For now I will add back the dependency for bug 578162 because the sleep is only necessary on Windows but no other platform. And bug 578162 is Windows only.
Depends on: 578162
Assignee | ||
Comment 9•14 years ago
|
||
Slightly updated patch
Attachment #502161 -
Attachment is obsolete: true
Attachment #502664 -
Flags: review?(gmealer)
Comment on attachment 502664 [details] [diff] [review] Patch v1.1 [checked-in] >- // Wait until the new tab has been opened >- controller.waitForEval("subject.tabs.length == subject.count", gTimeout, 100, >- {tabs: tabBrowser, count: ++count}); >- } while ((scrollButtonDown.getNode().getAttribute("collapsed") == 'true') || count > 50) >+ // Wait until the pages have been loaded, so they can be loaded from the cache >+ var tab = controller.tabs.getTab(controller.tabs.length - 1); >+ controller.waitForPageLoad(tab); Not necessarily wrong, but I think you may have weakened the code a little bit here. Prior to your edit, it kept a running count of how many tabs should be there, and ensured that new tabs were opened by the middle-click. Post-edit, it just makes sure the right-most tab is fully loaded. If the click didn't fire for some reason or was too slow to pop the tab before you enter waitForPageLoad(), you'd simply move on because you'll be checking the wrong tab. If you wanted to belt-and-suspenders, best bet might be to waitFor {controller.tabs.length === ++count} as before, then waitForPageLoad() once you've determined you have the right number of tabs. Other than that, looks fine. r+, but please consider the comment above. r=me if you decide to make that change.
Attachment #502664 -
Flags: review?(gmealer) → review+
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #10) > Prior to your edit, it kept a running count of how many tabs should be there, > and ensured that new tabs were opened by the middle-click. Post-edit, it just > makes sure the right-most tab is fully loaded. If the click didn't fire for > some reason or was too slow to pop the tab before you enter waitForPageLoad(), > you'd simply move on because you'll be checking the wrong tab. We don't have to check the tab count anymore. It's part of the openInNewTab method from the tabs module. We are waiting for the 'TabOpen' event, which should be enough in most cases. I don't think it is worth adding the tab count check to all of our tests. We have testNewTab which should take care of this check. I can update the mentioned test right after, if you agree on.
Assignee | ||
Comment 12•14 years ago
|
||
Geo, please see my last comment.
Henrik, ah, OK. I hadn't realized openInNewTab did an end-check. Thanks for letting me know, and r+!
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 14•14 years ago
|
||
Comment on attachment 502664 [details] [diff] [review] Patch v1.1 [checked-in] Landed on default as: http://hg.mozilla.org/qa/mozmill-tests/rev/4be98fab891c Lets keep this bug open for todays results. Also we should think about backporting it to the older branches, which would make the output more verbose.
Attachment #502664 -
Attachment description: Patch v1.1 → Patch v1.1 [checked-in]
Assignee | ||
Comment 15•14 years ago
|
||
Test failure has been gone in todays test-run. I will close this bug by leaving the sleep value in the test. We will have to revisit once bug 578162 has been fixed.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•