Closed
Bug 724494
Opened 12 years ago
Closed 12 years ago
Mozmill test failure /testTabbedBrowsing/testNewTab.js: Correct tab title - 'Connecting…' should equal 'New Tab'
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: vladmaniac, Assigned: vladmaniac)
References
Details
(Whiteboard: [mozmill-test-failure])
Attachments
(2 files, 4 obsolete files)
3.44 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
2.78 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
Branch: Firefox 13.0a1 (13.0a1, en-US, 20120205031129) ------------------------------------------------------------------------------------ Platform: All http://mozmill-release.blargon7.com/#/functional/failure?branch=13.0&platform=All&from=2012-01-03&to=2012-02-06&test=%2FtestTabbedBrowsing%2FtestNewTab.js&func=testNewTab.js%3A%3AtestNewTab ------------------------------------------------------------------------------------ Error Information: Correct tab title - 'Connecting…' should equal 'New Tab' Correct tab title - 'Connecting…' should equal 'New Tab' Opened blank tab - 'about:newtab' should equal 'about:blank' Correct tab title - 'Connecting…' should equal 'New Tab' Opened blank tab - 'about:newtab' should equal 'about:blank' Correct tab title - 'Connecting…' should equal 'New Tab' Correct tab title - 'Connecting…' should equal 'New Tab' Correct tab title - 'Connecting…' should equal 'New Tab' Opened blank tab - 'about:newtab' should equal 'about:blank' Correct tab title - 'Connecting…' should equal 'New Tab' ------------------------------------------------------------------------------------ Mozmill Version: 1.5.2 Failed: More than 200 times
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
Whiteboard: [mozmill-test-failure][mozmill-functional]
Comment 1•12 years ago
|
||
Arg, not sure why but it looks like that Clint has pushed the results from a DTC testrun to mozmill-release instead of mozmill-addons? Clint, please check this immediately! We have to stop reporting test results to this database!
Comment 3•12 years ago
|
||
Oh wait. That's not the reason here. Sorry. My fault. I think it has been caused by the landing of the new tab feature on bug 455553.
Assignee | ||
Comment 4•12 years ago
|
||
We do not open 'about:blank' on open new tab event now. we open 'about:newtab' which contains the tab thumbnails which needed to waitFor loading. a simple waitForPageLoad fixes the test, and expecting for 'about:newtab' instead of 'about:blank'
Updated•12 years ago
|
Comment 5•12 years ago
|
||
Comment on attachment 594666 [details] [diff] [review] fix patch v1.0 Review of attachment 594666 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/testTabbedBrowsing/testNewTab.js @@ +101,5 @@ > */ > function checkOpenTab(aEventType) { > // Open a new tab and check that 'about:blank' has been opened > tabBrowser.openTab(aEventType); > + controller.waitForPageLoad(); That shouldn't be necessary with bug 716108 fixed. We don't want to see 'Connecting...' at all. @@ +106,3 @@ > > expect.equal(tabBrowser.length, 2, "Two tabs visible - opened via " + aEventType); > + expect.equal(controller.tabs.activeTab.location.href, "about:newtab", "Opened blank tab"); We do now have the 'constant' gWindow.BROWSER_NEW_TAB_URL that contains the urls used for new tabs. I think you should use that. Or if that isn't accessible you can always use the pref browser.newtab.url.
Comment 6•12 years ago
|
||
Comment on attachment 594666 [details] [diff] [review] fix patch v1.0 Sorry for stealing the review on it but I kinda would like to see it fixed for my next CI testrun today. Thanks Vlad for the quick fix. Here one nit I have to mention: >- expect.equal(controller.tabs.activeTab.location.href, "about:blank", "Opened blank tab"); >+ expect.equal(controller.tabs.activeTab.location.href, "about:newtab", "Opened blank tab"); Please change the message from 'blank' to 'new' because that's what we are really doing here. Feel free to ask review from me again. I can check it in asap.
Attachment #594666 -
Flags: review?(anthony.s.hughes) → review+
Assignee | ||
Comment 7•12 years ago
|
||
nit fixed. Thanks henrik, I should really be more attentive to messages, not just the functionality of the patch
Attachment #594666 -
Attachment is obsolete: true
Attachment #594670 -
Flags: review?(hskupin)
Comment 8•12 years ago
|
||
Comment on attachment 594670 [details] [diff] [review] fix patch v1.1 (In reply to Tim Taubert [:ttaubert] from comment #5) > > tabBrowser.openTab(aEventType); > > + controller.waitForPageLoad(); > > That shouldn't be necessary with bug 716108 fixed. We don't want to see > 'Connecting...' at all. Thanks Tim! Good to know. Vlad, please make a comment in the test so we can remove this line once bug 716108 has been landed. > > + expect.equal(controller.tabs.activeTab.location.href, "about:newtab", "Opened blank tab"); > > We do now have the 'constant' gWindow.BROWSER_NEW_TAB_URL that contains the > urls used for new tabs. I think you should use that. Or if that isn't > accessible you can always use the pref browser.newtab.url. I would prefer that we use the preference. We want to keep our dependency on other variables as low as possible. But yes, we should use this one here. Vlad, please also incorporate this value.
Attachment #594670 -
Flags: review?(hskupin)
Comment 9•12 years ago
|
||
Tim, could you please help us with the status for Aurora? As given by bug 455553 it has been landed for Firefox 12. But with a latest Aurora build we are not seeing this failure with a fresh profile.
Comment 10•12 years ago
|
||
The New Tab Page landed pref'ed off on Fx 12. Bug 716538 (that pref'ed it on) landed right after the Aurora merge on Fx 13. Sorry for the confusion.
Assignee | ||
Comment 11•12 years ago
|
||
fixed according to Henrik and Tim's suggestions
Attachment #594670 -
Attachment is obsolete: true
Attachment #594674 -
Flags: review?(hskupin)
Comment 12•12 years ago
|
||
Comment on attachment 594674 [details] [diff] [review] fix patch v1.2 >+ expect.equal(controller.tabs.activeTab.location.href, controller.window.BROWSER_NEW_TAB_URL, >+ "Opened new tab"); As I have said in my last review comment we want to use the preference and not the window property.
Attachment #594674 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #12) > Comment on attachment 594674 [details] [diff] [review] > fix patch v1.2 > > >+ expect.equal(controller.tabs.activeTab.location.href, controller.window.BROWSER_NEW_TAB_URL, > >+ "Opened new tab"); > > As I have said in my last review comment we want to use the preference and > not the window property. From the comment shows that you wanted to go with the window property this time " But yes, we should use this one here" - sorry, changing asap
Assignee | ||
Comment 14•12 years ago
|
||
fixed to use pref
Attachment #594674 -
Attachment is obsolete: true
Attachment #594678 -
Flags: review?(hskupin)
Comment 15•12 years ago
|
||
Comment on attachment 594678 [details] [diff] [review] fix patch v1.3 > const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/'); > const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'layout/mozilla.html'; >+const PREF_NEWTAB_URL = "browser.newtab.url"; >+const PREF_VALUE = "about:newtab"; Separate the pref constant from the above constants into its own logical block. Also we shouldn't define the new tab page as default value. It's a persistent pref and will always exist. So use an empty string as second parameter to getPref().
Attachment #594678 -
Flags: review?(hskupin) → review-
Assignee | ||
Comment 16•12 years ago
|
||
fixed
Attachment #594678 -
Attachment is obsolete: true
Attachment #594688 -
Flags: review?(hskupin)
Comment 17•12 years ago
|
||
Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/64453ca78a0a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Attachment #594688 -
Flags: review?(hskupin) → review+
Comment 18•12 years ago
|
||
This is still failing for Nightly and Aurora builds: http://mozmill-ci.blargon7.com/#/functional/report/55d601cc2aabfac28f59060a84474cbd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 19•12 years ago
|
||
That doesn't make sense. The changeset from above clearly references the waitForPageLoad() call but I don't see it in the test revision we are using: http://hg.mozilla.org/qa/mozmill-tests/file/72e7479c3684/tests/functional/testTabbedBrowsing/testNewTab.js
Status: REOPENED → ASSIGNED
Comment 20•12 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #19) > That doesn't make sense. The changeset from above clearly references the > waitForPageLoad() call but I don't see it in the test revision we are using: > > http://hg.mozilla.org/qa/mozmill-tests/file/72e7479c3684/tests/functional/ > testTabbedBrowsing/testNewTab.js If the blargon7 instance is pulling the wrong changeset, that's not the fault of this test and should be filed in a new bug.
Comment 21•12 years ago
|
||
Anthony, the dashboard doesn't have to do anything with this failure. It's caused by something else. So checked this now a bit deeper and that's what I can see: http://mozmill-ci.blargon7.com/#/functional/report/55d601cc2aabfac28f59060a84474152 As you can see by this link the test for other locales fail for 13.0 builds. The current code retrieves the value of the new tab title from the preference but missed to get the localized version. The other report I gave in comment 18 is for 12.0. Not sure why the new tab page is present now in Aurora but as you can see we also fail in tests executed by qa-horus. Tim, do you have a pointer for us? http://mozmill-release.blargon7.com/#/functional/failure?branch=All&platform=All&from=2012-02-01&to=2012-02-14&test=%2FtestTabbedBrowsing%2FtestNewTab.js&func=testNewTab.js%3A%3AtestNewTab
Comment 22•12 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [away 02/17 - 02/26] from comment #21) > The other report I gave in comment 18 is for 12.0. Not sure why the new tab > page is present now in Aurora but as you can see we also fail in tests > executed by qa-horus. Tim, do you have a pointer for us? We enabled the New Tab Page on Aurora for a week to gather some feedback. Will be turned off by today again.
Comment 23•12 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [away 02/17 - 02/26] from comment #21) > As you can see by this link the test for other locales fail for 13.0 builds. > The current code retrieves the value of the new tab title from the > preference but missed to get the localized version. The new tab page has its own DTD to pick the page title from. Not sure if that's been translated already?
Comment 24•12 years ago
|
||
Yes, it's already translated at least for those locales I have added for my Mozmill CI PoC. But in any case we never should use hardcoded strings for comparisons. Vlad, can you please fix the DTD issue on default? For Aurora we don't have to care about.
Assignee | ||
Comment 25•12 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [away 02/17 - 02/26] from comment #24) > Yes, it's already translated at least for those locales I have added for my > Mozmill CI PoC. But in any case we never should use hardcoded strings for > comparisons. Vlad, can you please fix the DTD issue on default? For Aurora > we don't have to care about. Hopefully I can get to it later today
Assignee | ||
Comment 26•12 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [away 02/17 - 02/26] from comment #24) > Yes, it's already translated at least for those locales I have added for my > Mozmill CI PoC. But in any case we never should use hardcoded strings for > comparisons. Vlad, can you please fix the DTD issue on default? For Aurora > we don't have to care about. Henrik, which is the specific path for the DTD we want? I've browsed in "chrome://browser/locale/browser.dtd", "chrome://browser/locale/tabbrowser.dtd" so far but no luck finding the tab title line
Comment 27•12 years ago
|
||
The new tab page's tab title is contained in browser/locales/*/chrome/browser/newTab.dtd.
Comment 28•12 years ago
|
||
As Tim said, we want it from: /chrome/browser/newTab.dtd
Assignee | ||
Comment 29•12 years ago
|
||
I think this is the fix you guys need
Attachment #601241 -
Flags: review?(hskupin)
Comment 30•12 years ago
|
||
Comment on attachment 601241 [details] [diff] [review] get tab title from dtd on default - patch v1.0 [checked-in] I assume you have tested this with at least a different locale. r=me.
Attachment #601241 -
Flags: review?(hskupin) → review+
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 31•12 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #30) > Comment on attachment 601241 [details] [diff] [review] > get tab title from dtd on default - patch v1.0 > > I assume you have tested this with at least a different locale. r=me. You bet! :) It's safe to land
Comment 32•12 years ago
|
||
Landed on default: http://hg.mozilla.org/qa/mozmill-tests/rev/af51ce5c0c9a Vlad, please update the patch so we can also fix Aurora. It doesn't apply cleanly.
Keywords: checkin-needed
Updated•12 years ago
|
Attachment #601241 -
Attachment description: get tab title from dtd on default - patch v1.0 → get tab title from dtd on default - patch v1.0 [checked-in]
Updated•12 years ago
|
Attachment #594688 -
Attachment description: fix patch v1.4 → fix patch v1.4 [checked-in]
Comment 33•12 years ago
|
||
Well, lets wait until tomorrow when we can proof that it doesn't fail.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 34•12 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #32) > Landed on default: > http://hg.mozilla.org/qa/mozmill-tests/rev/af51ce5c0c9a > > Vlad, please update the patch so we can also fix Aurora. It doesn't apply > cleanly. Tim, will the new tab page be turned on for Firefox 12 merge to Beta. If not I think we want to hold off landing this on Aurora. In other words, this *fix* should ride the train.
Comment 35•12 years ago
|
||
The new tab page is planned to be enabled by default for Fx 13. Fx 12 will ship the old, disabled version.
Comment 36•12 years ago
|
||
Given comment 35, I don't think we should port these patches to Aurora or any other channel. They should simply ride with the merges. Henrik, do you disagree?
Comment 37•12 years ago
|
||
No, that's fine in this case.
Comment 38•12 years ago
|
||
No recent failures -- verified fixed.
Status: RESOLVED → VERIFIED
Whiteboard: [mozmill-test-failure][mozmill-functional] → [mozmill-test-failure]
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
•