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

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Noticed this failure today when running the 14.0b9 automation. Failure happens in test1.js::testVerifyDefaultBookmarks and only on Mac.

Comment 1

[Remus Pop (:RemusPop)]

Also fails on Ubuntu.

Comment 2

[Remus Pop (:RemusPop)]

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.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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!

Comment 4

[Remus Pop (:RemusPop)]

What file are you talking about? The only reference to bookmarks I've found are in sqlite databases (places, to be exactly).

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

You should check the omni.ja archive in the application folder. Also check another locale to ensure how they replace those URLs.

Comment 6

[Remus Pop (:RemusPop)]

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?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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.

Comment 8

[Remus Pop (:RemusPop)]

(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?

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 10

[Remus Pop (:RemusPop)]

Attachment: 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.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 12

[Remus Pop (:RemusPop)]

Attachment: patch v2 (default, aurora, beta)

Got rid of the prefs and now we get the locale from utils.appInfo.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 14

[Remus Pop (:RemusPop)]

Attachment: patch v3 (default, aurora, beta)

Removed the locale variable.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Alex, can you please take care of the remaining update? Thanks!

Comment 17

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.1 (default, aurora, beta)

fixed

Comment 18

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 20

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.2 (default, aurora, beta)

fixed

Comment 21

[Maniac Vlad Florin (:vladmaniac)]

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

adding Henrik

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/338103c6aacd

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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)