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)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: vladmaniac, Assigned: vladmaniac)

References

Details

(Whiteboard: [mozmill-test-failure])

Attachments

(2 files, 4 obsolete files)

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: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
Whiteboard: [mozmill-test-failure][mozmill-functional]
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!
Attached patch fix patch v1.0 (obsolete) — Splinter Review
easy fix
Attachment #594666 - Flags: review?(anthony.s.hughes)
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.
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'
Blocks: 455553
OS: Linux → All
Hardware: x86 → All
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 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+
Attached patch fix patch v1.1 (obsolete) — Splinter Review
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 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)
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.
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.
Attached patch fix patch v1.2 (obsolete) — Splinter Review
fixed according to Henrik and Tim's suggestions
Attachment #594670 - Attachment is obsolete: true
Attachment #594674 - Flags: review?(hskupin)
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-
(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
Attached patch fix patch v1.3 (obsolete) — Splinter Review
fixed to use pref
Attachment #594674 - Attachment is obsolete: true
Attachment #594678 - Flags: review?(hskupin)
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-
fixed
Attachment #594678 - Attachment is obsolete: true
Attachment #594688 - Flags: review?(hskupin)
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/64453ca78a0a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #594688 - Flags: review?(hskupin) → review+
This is still failing for Nightly and Aurora builds:

http://mozmill-ci.blargon7.com/#/functional/report/55d601cc2aabfac28f59060a84474cbd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
(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.
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
(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.
(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?
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.
(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
(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
The new tab page's tab title is contained in browser/locales/*/chrome/browser/newTab.dtd.
As Tim said, we want it from: /chrome/browser/newTab.dtd
I think this is the fix you guys need
Attachment #601241 - Flags: review?(hskupin)
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+
(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
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
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]
Attachment #594688 - Attachment description: fix patch v1.4 → fix patch v1.4 [checked-in]
Well, lets wait until tomorrow when we can proof that it doesn't fail.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
(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.
The new tab page is planned to be enabled by default for Fx 13. Fx 12 will ship the old, disabled version.
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?
No, that's fine in this case.
No recent failures -- verified fixed.
Status: RESOLVED → VERIFIED
Whiteboard: [mozmill-test-failure][mozmill-functional] → [mozmill-test-failure]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: