Last Comment Bug 767819 - testDefaultBookmarks failed | waitForPageLoad(): Timeout waiting for page loaded
: testDefaultBookmarks failed | waitForPageLoad(): Timeout waiting for page loaded
Status: RESOLVED FIXED
[mozmill-test-failure]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Maniac Vlad Florin (:vladmaniac)
:
:
Mentors:
http://mozmill-ci.blargon7.com/#/func...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-24 12:03 PDT by Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Modified: 2012-08-20 13:51 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
patch v1 (default, aurora, beta) (2.64 KB, patch)
2012-07-11 05:41 PDT, Remus Pop (:RemusPop)
hskupin: review-
Details | Diff | Splinter Review
patch v2 (default, aurora, beta) (2.44 KB, patch)
2012-07-12 02:47 PDT, Remus Pop (:RemusPop)
hskupin: review-
Details | Diff | Splinter Review
patch v3 (default, aurora, beta) (2.39 KB, patch)
2012-07-13 01:03 PDT, Remus Pop (:RemusPop)
hskupin: review-
Details | Diff | Splinter Review
patch v3.1 (default, aurora, beta) (2.63 KB, patch)
2012-07-18 07:45 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review-
Details | Diff | Splinter Review
patch v3.2 (default, aurora, beta) (2.64 KB, patch)
2012-07-18 08:08 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review+
Details | Diff | Splinter Review

Description Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-24 12:03:01 PDT
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) 2012-06-25 00:34:44 PDT
Also fails on Ubuntu.
Comment 2 Remus Pop (:RemusPop) 2012-06-25 01:07:50 PDT
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) [away 09/30 - 10/06] 2012-06-25 02:23:45 PDT
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) 2012-06-27 05:49:38 PDT
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) [away 09/30 - 10/06] 2012-06-27 05:55:13 PDT
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) 2012-06-27 06:20:02 PDT
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) [away 09/30 - 10/06] 2012-06-28 23:53:53 PDT
(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) 2012-07-05 05:57:13 PDT
(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) [away 09/30 - 10/06] 2012-07-05 06:39:39 PDT
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) 2012-07-11 05:41:51 PDT
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.
Comment 11 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-07-11 06:16:23 PDT
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) 2012-07-12 02:47:35 PDT
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.
Comment 13 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-07-12 08:35:03 PDT
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) 2012-07-13 01:03:05 PDT
Created attachment 641769 [details] [diff] [review]
patch v3 (default, aurora, beta)

Removed the locale variable.
Comment 15 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-07-18 07:05:21 PDT
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) [away 09/30 - 10/06] 2012-07-18 07:06:06 PDT
Alex, can you please take care of the remaining update? Thanks!
Comment 17 Maniac Vlad Florin (:vladmaniac) 2012-07-18 07:45:44 PDT
Created attachment 643371 [details] [diff] [review]
patch v3.1 (default, aurora, beta)

fixed
Comment 18 Maniac Vlad Florin (:vladmaniac) 2012-07-18 07:46:17 PDT
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) 2012-07-18 08:02:35 PDT
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) 2012-07-18 08:08:26 PDT
Created attachment 643380 [details] [diff] [review]
patch v3.2 (default, aurora, beta)

fixed
Comment 21 Maniac Vlad Florin (:vladmaniac) 2012-07-18 08:08:46 PDT
Comment on attachment 643380 [details] [diff] [review]
patch v3.2 (default, aurora, beta)

adding Henrik
Comment 22 Dave Hunt (:davehunt) 2012-07-18 08:34:00 PDT
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) [away 09/30 - 10/06] 2012-08-20 13:51:09 PDT
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)

Note You need to log in before you can comment on or make changes to this bug.