Closed Bug 840487 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect

Tracking

(firefox22 fixed, firefox23 fixed, firefox24 fixed, firefox25 fixed, firefox-esr17 fixed)

RESOLVED FIXED
Tracking Status
firefox22 --- fixed
firefox23 --- fixed
firefox24 --- fixed
firefox25 --- fixed
firefox-esr17 --- fixed

People

(Reporter: AndreeaMatei, Assigned: mario.garbi)

References

()

Details

(Whiteboard: [mozmill-test-failure][sprint2013-32])

Attachments

(4 files, 12 obsolete files)

2.36 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
6.47 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
4.61 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
2.32 KB, text/plain
Details
This happened today on all OS X platforms, with Nightly fr locale.
Whiteboard: [mozmill-test-failure]
Happens now on all platforms, but only on fr locale.
I can reproduce it locally on latest fr build for nightly.
OS: Mac OS X → All
Hardware: x86 → All
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
German is also affected, so this is not limited to the French locale.
This stopped reproducing after bug 840598 was fixed.
Great, thanks!
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Priority: -- → P5
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][sprint2013-32]
Assignee: nobody → mario.garbi
Status: REOPENED → ASSIGNED
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.
(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.
P2 given that we see those failures by ondemand tests for releases and betas.
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.
Attached patch Patch v1.1 (obsolete) — Splinter Review
Updated the patch so it applies on default cleanly again.
Attachment #751656 - Attachment is obsolete: true
Attachment #751656 - Flags: review?(andreea.matei)
Attachment #753197 - Flags: review?(andreea.matei)
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.
Attachment #753197 - Flags: review?(andreea.matei) → review-
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.
Attached patch patch v1.2 (obsolete) — Splinter Review
Added the changes required.
Attachment #753197 - Attachment is obsolete: true
Attachment #754454 - Flags: review?(andreea.matei)
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?
Attachment #754454 - Flags: review?(andreea.matei) → review-
Sorry about that, I will come with the right patch as soon as possible.
Attached patch patch v1.3 (obsolete) — Splinter Review
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.
Attachment #754454 - Attachment is obsolete: true
Attachment #754481 - Flags: review?(andreea.matei)
Attached patch patch v1.4 (obsolete) — Splinter Review
Corrected a small indentation I overlooked.
Attachment #754481 - Attachment is obsolete: true
Attachment #754481 - Flags: review?(andreea.matei)
Attachment #754483 - Flags: review?(andreea.matei)
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.
Attachment #754483 - Flags: feedback-
Comment on attachment 754483 [details] [diff] [review]
patch v1.4

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

Based on Henrik's comments above.
Attachment #754483 - Flags: review?(andreea.matei) → review-
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.
Has any Australis feature been landed yesterday? That might explain why we are failing. The URL might have been removed or changed.
Attached patch Skip patch (obsolete) — Splinter Review
We get failures on all platforms not just Mac OS. I've attached a skip patch until I can come with a fix patch.
Attachment #757257 - Flags: review?(andreea.matei)
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.
Attached patch patch v1.5 - 03.06.2013 (obsolete) — Splinter Review
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.
Attachment #754483 - Attachment is obsolete: true
Attachment #757353 - Flags: review?(andreea.matei)
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.
Attachment #757353 - Flags: review?(andreea.matei) → review-
(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 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.
Attachment #757257 - Flags: review?(andreea.matei) → review-
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.
(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.
Attached patch skip patchSplinter Review
This is still reproducing with the latest build so I've modified the patch to get it in as soon as possible.
Attachment #757257 - Attachment is obsolete: true
Attachment #757579 - Flags: review?(hskupin)
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.
Attachment #757579 - Flags: review?(hskupin) → review+
Summary: Test failure "http://www.mozilla.com/fr/firefox/central/ is in the Toolbar Folder - got 'false'" in /restartTests/testDefaultBookmarks/test1.js → Test failure in /restartTests/testDefaultBookmarks/test1.js because the location of the 'Getting Started' page has been changed
Whiteboard: [mozmill-test-failure][sprint2013-32] → [mozmill-test-failure][sprint2013-32][mozmill-test-skipped]
Attachment #757851 - Attachment description: patch v1.6 - 03.04.2013 → patch v1.6
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.
Attachment #757851 - Flags: review?(dave.hunt) → review-
Attached patch Patch v1.7 (obsolete) — Splinter Review
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.
Attachment #757851 - Attachment is obsolete: true
Attachment #759085 - Flags: review?(dave.hunt)
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.
Attachment #759085 - Flags: review?(dave.hunt) → review-
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.
Attachment #759085 - Flags: review-
Priority: P2 → P1
Attached patch Patch v1.8 (obsolete) — Splinter Review
Thanks for the feedback, I have made the requested changes.
Attachment #759085 - Attachment is obsolete: true
Attachment #759122 - Flags: review?(hskupin)
Attachment #759122 - Flags: review?(dave.hunt)
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.
Attachment #759122 - Flags: review?(hskupin)
Attachment #759122 - Flags: review?(dave.hunt)
Attachment #759122 - Flags: review-
(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
(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 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.
Attachment #760343 - Flags: review?(hskupin) → review-
Attached patch Patch 2.0 (obsolete) — Splinter Review
Made the required changes and it works ok with Default branch.
Attachment #760343 - Attachment is obsolete: true
Attachment #760832 - Flags: review?(hskupin)
Attachment #760832 - Flags: review?(andreea.matei)
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.
Attachment #760832 - Flags: review?(hskupin)
Attachment #760832 - Flags: review?(andreea.matei)
Attachment #760832 - Flags: review-
Attached patch Patch v2.1Splinter Review
Made the required changes.
Attachment #760832 - Attachment is obsolete: true
Attachment #760838 - Flags: review?(hskupin)
Attachment #760838 - Flags: review?(andreea.matei)
Mario, I would like to see reports here as it passed through 2 reviews since the last ones. Thanks
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)
Attachment #760838 - Flags: review?(hskupin)
Attachment #760838 - Flags: review?(andreea.matei)
Attachment #760838 - Flags: review+
Whiteboard: [mozmill-test-failure][sprint2013-32][mozmill-test-skipped] → [mozmill-test-failure][sprint2013-32]
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
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?
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.
Attached is the patch for Aurora-Esr17 where we only handle the locale normalization since the url is the same as before.
Attachment #764164 - Flags: review?(andreea.matei)
Attached file 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 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.
Attachment #764164 - Flags: review?(andreea.matei) → review+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
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.
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.