testDefaultBookmarks failed | waitForPageLoad(): Timeout waiting for page loaded

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ashughes, Assigned: vladmaniac)

Tracking

unspecified

Firefox Tracking Flags

(firefox14 fixed, firefox15 fixed, firefox16 fixed, firefox17 fixed, firefox-esr10 fixed)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(1 attachment, 4 obsolete attachments)

Noticed this failure today when running the 14.0b9 automation. Failure happens in test1.js::testVerifyDefaultBookmarks and only on Mac.
status-firefox14: --- → affected
Also fails on Ubuntu.
OS: Mac OS X → All
The test opens http://www.mozilla.com/en-US/firefox/central/ . If we could change this to use a page of our own we should be safe, but we should also assert that http://www.mozilla.com/en-US/firefox/central/ is the original bookmark.
I think we should stop loading the page and simply check if the bookmarks have been set correctly by comparing the values with the ones in the default profile under the application folder. Keep in mind that this contains different URLs for localized builds!
What file are you talking about? The only reference to bookmarks I've found are in sqlite databases (places, to be exactly).
You should check the omni.ja archive in the application folder. Also check another locale to ensure how they replace those URLs.
The only bookmark to check if it is correctly set is "Getting Started" and is placed inside bookmarks.html inside omni.ja archive. Do we want to check manually by hardcoding the link + locale + rest_of_link?

Shouldn't we check also the functionality, that the bookmark really open the page, instead of just checking if it's correctly set?
(In reply to Remus Pop (:RemusPop) from comment #6)
> The only bookmark to check if it is correctly set is "Getting Started" and
> is placed inside bookmarks.html inside omni.ja archive. Do we want to check
> manually by hardcoding the link + locale + rest_of_link?

Do those URLs match across locales with just the locale identifier exchanged? If that's the case I don't think we have to parse the bookmarks.html but simply verify the correct format of the URL especially the locale.

> Shouldn't we check also the functionality, that the bookmark really open the
> page, instead of just checking if it's correctly set?

We (will) have other tests which click on items in the bookmarks toolbar and open those pages. I don't think it's worth checking that in this test.
(In reply to Henrik Skupin (:whimboo) from comment #7)
> Do those URLs match across locales with just the locale identifier
> exchanged? 

No, the URLs do not match, there are some redirected to EN-US locale and some with mozilla.org changed in mozilla.jp for example.

Can't we compare the title of the bookmark with the label from the browser.dtd?
I do not talk about redirects. I talk about retrieving the URL from the bookmark via the places API and directly comparing parts of it to the locale of the running Firefox. Keep in mind that all the tests have to be local and shoulnd't access the web.
Created attachment 641020 [details] [diff] [review]
patch v1 (default, aurora, beta)

We now compare the bookmark url in the toolbar folder to "http://www.mozilla.com/" + locale + "/firefox/central/" .
The locale is taken from the prefs. We don't click the bookmark any more.
Assignee: nobody → remus.pop
Status: NEW → ASSIGNED
Attachment #641020 - Flags: review?(hskupin)
Attachment #641020 - Flags: review?(dave.hunt)
Comment on attachment 641020 [details] [diff] [review]
patch v1 (default, aurora, beta)

>+  // Check for the correct link of the bookmark which also includes the locale
>+  var locale = prefs.preferences.getPref("general.useragent.locale", "en-US");

To get the locale please use the appInfo object from the utils module. 

>+  var url = "http://www.mozilla.com/" + locale + "/firefox/central/";
>+  var uri = utils.createURI(url);
>+  expect.ok(places.isBookmarkInFolder(uri, bs.toolbarFolder),
>+            url + " is in the Toolbar Folder");

I would remove the url declaration and use uri.spec in the message. Or will the line be too long here?
Attachment #641020 - Flags: review?(hskupin)
Attachment #641020 - Flags: review?(dave.hunt)
Attachment #641020 - Flags: review-
Created attachment 641404 [details] [diff] [review]
patch v2 (default, aurora, beta)

Got rid of the prefs and now we get the locale from utils.appInfo.
Attachment #641020 - Attachment is obsolete: true
Attachment #641404 - Flags: review?(hskupin)
Comment on attachment 641404 [details] [diff] [review]
patch v2 (default, aurora, beta)

>+  var locale = utils.appInfo.locale;
>+  var url = "http://www.mozilla.com/" + locale + "/firefox/central/";

There is no need to declare 'locale' unless the following line would be too long. I don't think that's the case here.

Otherwise it looks good and I think we can land it once the patch has been updated.
Attachment #641404 - Flags: review?(hskupin) → review-
Created attachment 641769 [details] [diff] [review]
patch v3 (default, aurora, beta)

Removed the locale variable.
Attachment #641404 - Attachment is obsolete: true
Attachment #641769 - Flags: review?(hskupin)
Comment on attachment 641769 [details] [diff] [review]
patch v3 (default, aurora, beta)

As mentioned in the last review all looks fine, but let us do two more updates:

>+  var url = "http://www.mozilla.com/" + utils.appInfo.locale + "/firefox/central/";

Lets move this target URL to a global constant so we don't have to check the code if it changes.

>+  var uri = utils.createURI(url);
>+  expect.ok(places.isBookmarkInFolder(uri, bs.toolbarFolder),
>+            url + " is in the Toolbar Folder");

Please remove the uri variable and directly create it inside the isBookmarkInFolder() method.
Attachment #641769 - Flags: review?(hskupin) → review-
Alex, can you please take care of the remaining update? Thanks!
Assignee: remus.pop → alex.lakatos
(Assignee)

Comment 17

5 years ago
Created attachment 643371 [details] [diff] [review]
patch v3.1 (default, aurora, beta)

fixed
Assignee: alex.lakatos → vlad.mozbugs
Attachment #641769 - Attachment is obsolete: true
Attachment #643371 - Flags: review?(hskupin)
(Assignee)

Comment 18

5 years ago
Comment on attachment 643371 [details] [diff] [review]
patch v3.1 (default, aurora, beta)

Adding Dave to r. 
Took the liberty of quick fixing those changes
Attachment #643371 - Flags: review?(dave.hunt)
Comment on attachment 643371 [details] [diff] [review]
patch v3.1 (default, aurora, beta)

+const URL_WITH_LOCALE = "http://www.mozilla.com/" + utils.appInfo.locale + 
+                        "/firefox/central/";
+

Can you name this simply GETTING_STARTED_URL

+  // Check for the correct link of the bookmark which also includes the locale
+  var url = URL_WITH_LOCALE;
+  expect.ok(places.isBookmarkInFolder(utils.createURI(url), bs.toolbarFolder),
+            url + " is in the Toolbar Folder");

There's no need to define url variable, simply use the constant in the expect call.
Attachment #643371 - Flags: review?(hskupin)
Attachment #643371 - Flags: review?(dave.hunt)
Attachment #643371 - Flags: review-
(Assignee)

Comment 20

5 years ago
Created attachment 643380 [details] [diff] [review]
patch v3.2 (default, aurora, beta)

fixed
Attachment #643371 - Attachment is obsolete: true
Attachment #643380 - Flags: review?(dave.hunt)
(Assignee)

Comment 21

5 years ago
Comment on attachment 643380 [details] [diff] [review]
patch v3.2 (default, aurora, beta)

adding Henrik
Attachment #643380 - Flags: review?(hskupin)
Comment on attachment 643380 [details] [diff] [review]
patch v3.2 (default, aurora, beta)

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/338103c6aacd
Attachment #643380 - Flags: review?(hskupin)
Attachment #643380 - Flags: review?(dave.hunt)
Attachment #643380 - Flags: review+
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox-esr10: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → fixed
Resolution: --- → FIXED
Please keep up your patch status. This patch was marked as needs to be backported to older branches but was lost. As an assignee you should track that on your own and ping us, Vlad. Please take care about in the future. I just found it by accident.

http://hg.mozilla.org/qa/mozmill-tests/rev/e8c4a6b6672a (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/b2c18d27d7c5 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/cb1583768137 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/d0373e54dee1 (esr10)
status-firefox-esr10: affected → fixed
status-firefox14: affected → fixed
status-firefox15: affected → fixed
status-firefox16: affected → fixed
You need to log in before you can comment on or make changes to this bug.