840487 - Test failure in /restartTests/testDefaultBookmarks/test1.js because the location of the 'Getting Started' page has been changed

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Andreea Matei [:AndreeaMatei]]

This happened today on all OS X platforms, with Nightly fr locale.

Comment 1

[Andreea Matei [:AndreeaMatei]]

Happens now on all platforms, but only on fr locale.
I can reproduce it locally on latest fr build for nightly.

Comment 2

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

This appears to be a genuine regression, which I can replicate manually by downloading a recent French localised nightly and checking the URL of the getting started page. It is now http://www.mozilla.org/en-US/firefox/central/ instead of http://www.mozilla.com/fr/firefox/central/

Pushlog for regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5835bc763be7&tochange=36525224b14e

Comment 3

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

German is also affected, so this is not limited to the French locale.

Comment 4

[Daniela Petrovici]

This stopped reproducing after bug 840598 was fixed.

Comment 5

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

Great, thanks!

Comment 6

[u279076]

This happened again today with Firefox 21.0b3#1 ja-JP-mac on Mac OSX 10.7:
http://mozmill-ondemand.blargon7.com/#/functional/report/8ec48e7ab0431a61b624e36d31ddae0d

Comment 7

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

Oh the ja-JP issue might be a different issue given how this locale gets modified. But we should investigate and figure out if our test is correct.

Comment 8

[mario garbi]

We are expecting "http://www.mozilla.com/ja-jp-mac/firefox/central" as the locale for Mac is "ja-JP-mac", but in reality the bookmark url is "http://www.mozilla.com/ja/firefox/central" as it can be seen when running the test and selecting properties for the bookmark (I've used a sleep here to be able to check it).
We could change the way we gather the constant GETTING_STARTED_URL that currently uses utils.appInfo.locale and strings or simply use a conditional to check if we use Japanese locale with a Mac OS, since this is the only instance I know of where the locale is different for Mac.

Comment 9

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

(In reply to mario garbi from comment #8)
> currently uses utils.appInfo.locale and strings or simply use a conditional
> to check if we use Japanese locale with a Mac OS, since this is the only
> instance I know of where the locale is different for Mac.

Right. No other locale (as far as I know) behaves the same. So lets add a map at the top of the test which specifies the mapping. If we find another locale later we would only have to update the map.

Comment 10

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

P2 given that we see those failures by ondemand tests for releases and betas.

Comment 11

[mario garbi]

Attachment: Patch v1.0

Reports:
Linux- 
http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c2e145ed

WinXP-
http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c2e10e22

Mac-
http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c2e146ac

Comment 12

[Andreea Matei [:AndreeaMatei]]

We want this on default first, like all the other locale specific bugs. Even if we don't see failures there because we don't ran ja-JP on default, the error is present and contributors may use this locale. 

The current patch is not applying cleanly.

Comment 13

[mario garbi]

Attachment: Patch v1.1

Updated the patch so it applies on default cleanly again.

Comment 14

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 753197 [details] [diff] [review]
Patch v1.1

Review of attachment 753197 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/restartTests/testDefaultBookmarks/test1.js
@@ +13,5 @@
>  var utils = require("../../../../lib/utils");
>  
> +var l10nMap = {
> +  "ja-JP-mac" : "ja"
> +}

This should be an array and have the {} brackets wrapping the elements (now is just one).

@@ +16,5 @@
> +  "ja-JP-mac" : "ja"
> +}
> +var locale = utils.appInfo.locale;
> +if (locale in l10nMap)
> +  locale = l10nMap[locale];

Please add brackets.

Comment 15

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

Comment on attachment 753197 [details] [diff] [review]
Patch v1.1

Review of attachment 753197 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/restartTests/testDefaultBookmarks/test1.js
@@ +13,5 @@
>  var utils = require("../../../../lib/utils");
>  
> +var l10nMap = {
> +  "ja-JP-mac" : "ja"
> +}

This is a map and not an array. New entries will be separated by comma, just like in JSON. So from my perspective this is fine. But it should be a constant.

@@ +16,5 @@
> +  "ja-JP-mac" : "ja"
> +}
> +var locale = utils.appInfo.locale;
> +if (locale in l10nMap)
> +  locale = l10nMap[locale];

We don't want to execute any code in the global scope, except it is really necessary. Why is this not part of setupModule?

@@ +18,5 @@
> +var locale = utils.appInfo.locale;
> +if (locale in l10nMap)
> +  locale = l10nMap[locale];
> +
> +const GETTING_STARTED_URL = "http://www.mozilla.com/" + locale + "/firefox/central/";

This has to be named TEST_DATA as of our latest coding styles.

Comment 16

[mario garbi]

Attachment: patch v1.2

Added the changes required.

Comment 17

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

Comment on attachment 754454 [details] [diff] [review]
patch v1.2

Review of attachment 754454 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/restartTests/testDefaultBookmarks/test1.js
@@ +16,5 @@
> +  {"ja-JP-mac" : "ja"}
> +  ];
> +
> +var locale = utils.appInfo.locale;
> +if (locale in l10nMap) {

How should this now work? You will never find the locale. Has this been tested at all?

Comment 18

[mario garbi]

Sorry about that, I will come with the right patch as soon as possible.

Comment 19

[mario garbi]

Attachment: patch v1.3

In order to move the if block in the setupModule we would have to move the constant TEST_DATA as well, and by doing that we don't see it in the test below.

Comment 20

[mario garbi]

Attachment: patch v1.4

Corrected a small indentation I overlooked.

Comment 21

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

Comment on attachment 754483 [details] [diff] [review]
patch v1.4

Review of attachment 754483 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/restartTests/testDefaultBookmarks/test1.js
@@ +18,5 @@
> +
> +var locale = utils.appInfo.locale;
> +if (locale in L10N_MAP) {
> +    locale = L10N_MAP[locale];
> +}

Lets completely move this out to the l10n.js module. Other tests most likely also want to have this code. Also there are two indentation failures you should fix.

Comment 22

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 754483 [details] [diff] [review]
patch v1.4

Review of attachment 754483 [details] [diff] [review]:
-----------------------------------------------------------------

Based on Henrik's comments above.

Comment 23

[Andreea Matei [:AndreeaMatei]]

This has failed a lot today:
http://mozmill-ci.blargon7.com/#/functional/failure?branch=All&platform=All&from=2013-05-29&to=&test=%2FrestartTests%2FtestDefaultBookmarks%2Ftest1.js&func=test1.js%3A%3AtestVerifyDefaultBookmarks

Mario, can you provide the updated patch so we can stop it or if it takes too much time, a skip patch? Thanks.

Comment 24

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

Has any Australis feature been landed yesterday? That might explain why we are failing. The URL might have been removed or changed.

Comment 25

[mario garbi]

Attachment: Skip patch

We get failures on all platforms not just Mac OS. I've attached a skip patch until I can come with a fix patch.

Comment 26

[mario garbi]

It would seem the page url has changed to "http://www.mozilla.org/LOCALE_HERE/firefox/central/" for most of the builds I've checked and to "http://www.mozilla.jp/firefox/central/" for Japanese build.
I will come with a fix patch to handle this change as soon as possible.

Comment 27

[mario garbi]

Attachment: patch v1.5 - 03.06.2013

Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a147ea
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a15263

WinXP:
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a137ef
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a13683

Mac 10.6.8:
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a127e1
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a117fe

We not check the correct bookmark value for l10n builds as well as en-US. The change that caused the failures was the bookmark url changing from .com to .org.

I've placed the code inside localization.js library and created a map where we can add elements as we find more exceptions.

Comment 28

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

Comment on attachment 757353 [details] [diff] [review]
patch v1.5 - 03.06.2013

Review of attachment 757353 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/localization.js
@@ +7,5 @@
>  var domUtils = require("dom-utils");
>  var screenshot = require("screenshot");
>  var utils = require("utils");
>  
> +// Add elements if we found any that are different on a certain OS

Please describe what this map is for and not what you will have to do.

@@ +241,5 @@
> +    locale = L10N_MAP[locale];
> +  }
> +  var localeURL = "https://www.mozilla.org/" + locale + "/firefox/central/";
> +
> +  return localeURL;

I don't see why we need this method in the l10n library. It is used only once in the default bookmarks test.

Comment 29

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #28)
> I don't see why we need this method in the l10n library. It is used only
> once in the default bookmarks test.

Probably because of comment 21 ...

Comment 30

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 757257 [details] [diff] [review]
Skip patch

Review of attachment 757257 [details] [diff] [review]:
-----------------------------------------------------------------

We need to edit the manifest file as well. Please change the commit message to the form "Skip test due to failure..".

::: tests/functional/restartTests/testDefaultBookmarks/test1.js
@@ +87,5 @@
>  
>    return root.QueryInterface(Ci.nsINavHistoryContainerResultNode);
>  }
>  
> +setupModule.__force_skip__ = "Bug 840487 - URL issue";

The message doesn't say much.

Comment 31

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

Mario, for those massive failures like this one you should find someone to review who is in the same timezone as you. So throughout this week please ask Dave then. We will try to get this landed so that the next testruns are not totally busted again. Andreea, currently checks it.

Comment 32

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

(In reply to Andrei Eftimie from comment #29)
> > I don't see why we need this method in the l10n library. It is used only
> > once in the default bookmarks test.
> 
> Probably because of comment 21 ...

That only mentioned the map and nothing more. I don't see why we would do tests for default bookmarks somewhere else again.

Comment 33

[Andreea Matei [:AndreeaMatei]]

Attachment: skip patch

This is still reproducing with the latest build so I've modified the patch to get it in as soon as possible.

Comment 34

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

Comment on attachment 757579 [details] [diff] [review]
skip patch

Review of attachment 757579 [details] [diff] [review]:
-----------------------------------------------------------------

One nit, but with it fixed we can get it landed.

::: tests/functional/restartTests/manifest.ini
@@ +10,5 @@
>  disabled = Bug 784305 - Current URL should match expected URL
>  [include:testAddons_uninstallExtension/manifest.ini]
>  [include:testAddons_uninstallTheme/manifest.ini]
>  [include:testDefaultBookmarks/manifest.ini]
> +disabled = Bug 840487 - Used url 'http://www.mozilla.com/locale/firefox/central' has been changed

Please say that the location of the 'Getting Started' web page has been changed. There is no need to reference the actual URL here.

Comment 35

[Andreea Matei [:AndreeaMatei]]

Landed the skip patch:
http://hg.mozilla.org/qa/mozmill-tests/rev/3d1ac11b1d34 (default)

Comment 36

[mario garbi]

Attachment: patch v1.6

Mac 10.6.8:
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a256d5
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a26c30

Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a24066
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a25f2c

Windows:
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a24020
http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916a2499c

Made requested changed, moved back the function from library to the test and only left the Map in the library. We have reports for the JA build but I've tested it with "de" and "en-US" as well.

Comment 37

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

Comment on attachment 757851 [details] [diff] [review]
patch v1.6

Review of attachment 757851 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/localization.js
@@ +7,5 @@
>  var domUtils = require("dom-utils");
>  var screenshot = require("screenshot");
>  var utils = require("utils");
>  
> +// An element Map of the locale exceptions and their correct values

I still don't think this comment makes it clear what this is for.

::: tests/functional/restartTests/testDefaultBookmarks/test1.js
@@ -87,5 @@
>  
>    return root.QueryInterface(Ci.nsINavHistoryContainerResultNode);
>  }
>  
> -setupModule.__force_skip__ = "Bug 840487 - Test failure due to url " +

You have missed to remove the disabled line in the manifest file.

Comment 38

[mario garbi]

Attachment: Patch v1.7

Fixed as requested, if the message is still not clear enough please let me know what I am missing and I will try to be more explicit.

Comment 39

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

Comment on attachment 759085 [details] [diff] [review]
Patch v1.7

Review of attachment 759085 [details] [diff] [review]:
-----------------------------------------------------------------

Just one minor nit.

::: tests/functional/restartTests/testDefaultBookmarks/test1.js
@@ +97,5 @@
> +  // Check the Map elements for a match and set the correct value
> +  if (locale in localization.L10N_MAP) {
> +    locale = localization.L10N_MAP[locale];
> +  }
> +  var localeURL = "https://www.mozilla.org/" + locale + "/firefox/central/";

Nit: We don't need this variable, we can just return the string directly.

Comment 40

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

Comment on attachment 759085 [details] [diff] [review]
Patch v1.7

Review of attachment 759085 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/localization.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Include required modules
> +var { expect } = require("assertions");

Why do we need expect here?

::: tests/functional/restartTests/testDefaultBookmarks/test1.js
@@ +12,5 @@
>  var places = require("../../../../lib/places");
>  var toolbars = require("../../../../lib/toolbars");
>  var utils = require("../../../../lib/utils");
>  
> +const TEST_DATA = this.getLocaleBookmarkURL();

Why the use of 'this'?

@@ +96,5 @@
> +
> +  // Check the Map elements for a match and set the correct value
> +  if (locale in localization.L10N_MAP) {
> +    locale = localization.L10N_MAP[locale];
> +  }

I expected to see all of this code in the library module. I don't think that each and every test has to check this on its own. Just calling something like localization.fooBar(locale) should return the real locale name.

Comment 41

[mario garbi]

Attachment: Patch v1.8

Thanks for the feedback, I have made the requested changes.

Comment 42

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

Comment on attachment 759122 [details] [diff] [review]
Patch v1.8

Review of attachment 759122 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/localization.js
@@ +7,5 @@
>  var domUtils = require("dom-utils");
>  var screenshot = require("screenshot");
>  var utils = require("utils");
>  
> +// A map of the element locales needed in order to create a valid bookmark URL

Not sure why you are referencing the bookmark URL creation here. This is NOT covered by this map. This map is really to translate possible platform dependent locales to their parent locale.

@@ +280,5 @@
>    }
>  }
>  
> +/**
> + * Check the Map elements for a match and set the correct value

This comment tells me nothing. Please explain what this method is for and not what the code is doing.

@@ +282,5 @@
>  
> +/**
> + * Check the Map elements for a match and set the correct value
> + *
> + * @param {String} The locale to check

Make this parameter optional so we default to appInfo.locale.

@@ +283,5 @@
> +/**
> + * Check the Map elements for a match and set the correct value
> + *
> + * @param {String} The locale to check
> + * @return {String} Locale value after checking against the L10N Map

Same here. No-one will be able to understand what is getting returned here. Can you please describe all that in words before we get one more patch we have to r-? Thanks.

@@ +285,5 @@
> + *
> + * @param {String} The locale to check
> + * @return {String} Locale value after checking against the L10N Map
> + */
> +function setLocale(locale) {

We don't set any locale here. See my comment from above what this method is really doing.

@@ +294,5 @@
> +  return locale;
> +}
> +
> +// Export of variables
> +exports.L10N_MAP = L10N_MAP;

I don't see a reason why to import this map.

::: tests/functional/restartTests/testDefaultBookmarks/test1.js
@@ +93,5 @@
> + */
> +function getLocaleBookmarkURL() {
> +  var locale = localization.setLocale(utils.appInfo.locale);
> +
> +  return "https://www.mozilla.org/" + locale + "/firefox/central/";

I don't see a reason for this extra method. Do this directly at the top of the test.

Comment 43

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #42)

I am taking the work on this bug until Mario is back from PTO. The comments and jsdoc comment are below. Please tell me if these are ok to use.

> Not sure why you are referencing the bookmark URL creation here. This is NOT
> covered by this map. This map is really to translate possible platform
> dependent locales to their parent locale.

Changed comment to:
// A map translating platform dependent locales to their parent locale
 
> Same here. No-one will be able to understand what is getting returned here.
> Can you please describe all that in words before we get one more patch we
> have to r-? Thanks.
What the method does: 
"Translate the platform dependent locales to parent locales"

Description for parameter and return value:
@param {String} locale (optional) 
       The locale value that is converted if it is platform dependent
@returns {String} Parent locale in case the original value is platform dependent

Comment 44

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

(In reply to Daniela Petrovici from comment #43)
> What the method does: 
> "Translate the platform dependent locales to parent locales"

'Translate a possible platform specific locale to its parent locale'

> Description for parameter and return value:
> @param {String} locale (optional) 
>        The locale value that is converted if it is platform dependent

This is not how optional parameters are setup. You have to use [] and also mention the default value.

> @returns {String} Parent locale in case the original value is platform
> dependent

'Parent locale' would be totally enough.

Comment 45

[Daniela Petrovici]

Attachment: patch v1.9

Modified patch based on review.

Reports are below:
Linux:
en-US: http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe90c759
fr: http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe8dc215
ja: http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe9075fd

MAC:
en-US: http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe90baee
fr: http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe8d7479
ja: http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe906c46

Windows:
en-US: http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe90a971
fr: http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe90629b
ja: http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbe8d8bb5

Comment 46

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

Comment on attachment 760343 [details] [diff] [review]
patch v1.9

Review of attachment 760343 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/localization.js
@@ +7,5 @@
>  var domUtils = require("dom-utils");
>  var screenshot = require("screenshot");
>  var utils = require("utils");
>  
> +// A map translating platform dependent locales to their parent locale

Please update the comment so it's in alignment with the function name below.

@@ +8,5 @@
>  var screenshot = require("screenshot");
>  var utils = require("utils");
>  
> +// A map translating platform dependent locales to their parent locale
> +const L10N_MAP = {

I would call it NORMALIZATION_MAP

@@ +227,5 @@
>    }
>  }
>  
>  /**
> + * Translate a possible platform specific locale to its parent locale

Please update the comment so it's in alignment with the function name.

@@ +229,5 @@
>  
>  /**
> + * Translate a possible platform specific locale to its parent locale
> + *
> + * @param {String} locale [optional - default: current locale]

This is not the way how optional parameters are specified. Please always have a look at other code in our repository. It has to look like: [aLocale=appInfo.locale]

@@ +231,5 @@
> + * Translate a possible platform specific locale to its parent locale
> + *
> + * @param {String} locale [optional - default: current locale]
> +          The locale value that is converted if it is platform dependent
> + * @returns {String} Parent locale

Please update so it also aligns with the function name.

::: tests/functional/restartTests/testDefaultBookmarks/test1.js
@@ +13,5 @@
>  var toolbars = require("../../../../lib/toolbars");
>  var utils = require("../../../../lib/utils");
>  
> +const TEST_DATA = "https://www.mozilla.org/" +
> +                  localization.normalizeLocale(utils.appInfo.locale) +

The argument is optional and we always take the current locale here.

Comment 47

[mario garbi]

Attachment: Patch 2.0

Made the required changes and it works ok with Default branch.

Comment 48

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

Comment on attachment 760832 [details] [diff] [review]
Patch 2.0

Review of attachment 760832 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/localization.js
@@ +230,5 @@
>  /**
> + * Translate a possible platform specific locale to its normalized locale
> + *
> + * @param {String} [aLocale = utils.appInfo.locale]
> + * The locale value that is converted if it is platform dependent

I would call it: 'The locale to be mapped'. Also please fix the indentation of the description.

@@ +234,5 @@
> + * The locale value that is converted if it is platform dependent
> + * @returns {String} Parent locale
> + */
> +function normalizeLocale(aLocale) {
> +  locale = aLocale || utils.appInfo.locale;

locale is not declared.

Comment 49

[mario garbi]

Attachment: Patch v2.1

Made the required changes.

Comment 50

[Andreea Matei [:AndreeaMatei]]

Mario, I would like to see reports here as it passed through 2 reviews since the last ones. Thanks

Comment 51

[mario garbi]

Reports:
Linux
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbecacb0c
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbecadd31

Mac
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbeca81f3
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbecab36d

WinXP
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbeca7fea
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbeca8ee5

Comment 52

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 760838 [details] [diff] [review]
Patch v2.1

Review of attachment 760838 [details] [diff] [review]:
-----------------------------------------------------------------

Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/be82a7599276 (default)

Comment 53

[mario garbi]

We had two set of failures here, one in 01.06.2013 for Nightly 24.0a1 and one in 12-13.02.2013 for Nightly 21.0a1. We don't seem to hit this problem with Aurora or prior as it was fixed back then. The issue now was different than what we had back in February since the bookmark url was changed to .org for all locales.
As it is we don't need a backport as the default mozmill-tests repository works fine with the locales I've tested.Reports are bellow:

Aurora:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef1f502
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef1ede7
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef208f3
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef218cb
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef2309d
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef235cd

Beta:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef23dc5
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef24681
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef24d04
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef25f6d
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef26d5a
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef275f6

Release:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef27bfe
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef28950
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef29297
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef2a272
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef2a752
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef2ae03

Esr17:
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef2aea6
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef2bd8e
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef2ccf4
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef2da73
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef2e89e
http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbef2ecde

Comment 54

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

I would see a benefit in getting this backported. Reason is the method to normalize the platform dependent locale. But we could do this once needed. Andreea, what do you think?

Comment 55

[Andreea Matei [:AndreeaMatei]]

Yes, definitely. 
And what I see from the reports is that only Linux was tested on the other branches. The issue we had was on mac mostly (ja-jp-mac) so I would like to see that tested as well.
More than that, in comment 6 we tested beta and failed, so we should backport this.

Comment 56

[mario garbi]

Attachment: Patch v2.1 - Backport

Attached is the patch for Aurora-Esr17 where we only handle the locale normalization since the url is the same as before.

Comment 57

[mario garbi]

Attachment: Report document

Here is the report document that contains the link for the report runs on the major operating systems using Aurora to ESR17 branches.

Comment 58

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 764164 [details] [diff] [review]
Patch v2.1 - Backport

Review of attachment 764164 [details] [diff] [review]:
-----------------------------------------------------------------

Please be more careful with the commit message in the future. "imported patch localeDefaultBookmark_Backport" is not in our template.
Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/fb9f09703958 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/82e68fb21850 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/030e5c72d0a4 (esr17)

Aurora already has these changes due to the merge.

Comment 59

[mario garbi]

Sorry about the commit message, it looks like that due to the fact that I forgot to add a commit message for the Backport patch, I should have checked before posting, I'll make sure it doesn't happen again.