559836 - Make tests locale independent

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Adrian Kalla [:adriank]]

Attachment: patch v1

When bug 504635 will be fixed, we can make the tests which were indicated as not locale independent in their comments locale independent.

A patch for that is attached.

Comment 1

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

Comment on attachment 439537 [details] [diff] [review]
patch v1

>+++ b/firefox/testDownloading/testCloseDownloadManager.js
>
> var testCloseDownloadManager = function()
> {
>+  var dtds = ["chrome://mozapps/locale/downloads/downloads.dtd"];

I would be in favor of adding an array of DTD files in the appropriate API instead and make it accessible via a helper function. That applies also to all successive usages.

>+  var msg = UtilsAPI.getEntity(dtds, "privatebrowsingpage.issueDesc.normal");

Please name it statusEntity.

>+++ b/firefox/testPrivateBrowsing/testStartStopPBMode.js
>-  var longDescElem = new elementslib.ID(controller.tabs.activeTab, "errorLongDescText")
>+  var longDesc = UtilsAPI.getEntity(dtds, "privatebrowsingpage.description");

longDescEntity please.

>+  var moreInfo = UtilsAPI.getEntity(dtds, "privatebrowsingpage.learnMore");
>+  var longDescElem = new elementslib.ID(controller.tabs.activeTab, "errorLongDescText");
>   var moreInfoElem = new elementslib.ID(controller.tabs.activeTab, "moreInfoLink");

Same for moreInfo and move that line below the moreInfoElem declaration.

>+++ b/shared-modules/testDownloadsAPI.js
>
>+// Include necessary modules
>+var MODULE_REQUIRES = ['UtilsAPI'];

You forgot the RELATIVE_ROOT declaration.

Comment 2

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

Also please make sure to replace hard-coded strings for restart tests. At least I'm working on bug 536730 which still has one. Probably others are affected too.

Comment 3

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

Now that the base patch has been landed we can continue work on those tests. Please also check the dependency list on bug 504635 if we have catched all instances. Thanks!

Comment 4

[Adrian Kalla [:adriank]]

Attachment: patch (WIP) v2

updated patch. It fixes basically only the entities which were marked by comments to be fixed, so a second patch is still necessary.

Comment 5

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

Comment on attachment 445741 [details] [diff] [review]
patch (WIP) v2

>+  this.dtds = dtds;

Can you please move the DTD definition directly to the get function? It can happen that a shared module contains more than one window and would need different DTD's. That applies to all shared modules. While you are updating the modules, can you please add the getDTDs() function to all shared modules which have ui classes, even they return null or not implemented?

>+   * @type {Array of strings}

Just use @type [string]

Otherwise I think that's the way we wanna go. Looks good.

Comment 6

[Adrian Kalla [:adriank]]

Attachment: patch for shared-modules

Comment 7

[Adrian Kalla [:adriank]]

Attachment: patch for firefox tests

Comment 8

[Adrian Kalla [:adriank]]

Attachment: patch for shared-modules v2

Comment 9

[Adrian Kalla [:adriank]]

Attachment: patch for firefox tests v2

Comment 10

[Adrian Kalla [:adriank]]

Attachment: patch for shared-modules v3

fixed a few 'UtilsAPI is not defined'...

Comment 11

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

Comment on attachment 457193 [details] [diff] [review]
patch for shared-modules v3

>+  getDtds : function downloadManager_getDtds() {
>+    var dtds = ["chrome://mozapps/locale/extensions/extensions.dtd",
>+               "chrome://browser/locale/browser.dtd",];

I would vote for moving the array to a global const and add a property to the class to return the array.

>+++ b/shared-modules/testPrefsAPI.js
>+   * @returns Array of external DTD urls
>+   * @type [string]
>+   */
>+  getDtds : function preferencesDialog_getDtds() {
>+    return null;
>+  },

Can you please add the dtd file for the preferences dialog, or would we have to use several ones depending on the open pane.

>+++ b/shared-modules/testPrivateBrowsingAPI.js

>   /**
>+   * Instance of the Utils API
>+   * @private
>+   */
>+  this._utilsApi = collector.getModule('UtilsAPI');

Please remove all those comments from the constructor. Those aren't really helpful.

>+  getDtds : function engineManager_getDtds() {
>+    return null;

We should have DTD's here. Please add those, even we aren't using them yet.

>+  this._utilsAPI = collector.getModule('UtilsAPI');
>   this._ModalDialogAPI = collector.getModule('ModalDialogAPI');

nit: Just reverse the order.

>-    this._controller.keypress(searchInput, 'a', {accelKey: true});
>+    var selectAllEntity = this._utilsAPI.getEntity(this.getDtds(), "selectAllCmd.key");

Can we use the same variable for all those instances? I like to see cmdKey. That would also apply to all other instances.

>+  getDtds : function aboutSessionRestore_getDtds() {
>+    return null;
>+  },

I think we should also have a dtd here.

>+++ b/shared-modules/testSoftwareUpdateAPI.js
>+  getDtds : function softwareUpdate_getDtds() {
>+    return null;

Here we definitely should have a DTD we will use in the future.


>+++ b/shared-modules/testTabbedBrowsingAPI.js
>       case "tabStrip":
>         var tabStrip = this.getElement({type: "tabs_strip"});
> 
>         // XXX: Workaround until bug 537968 has been fixed
>-        this._controller.click(tabStrip, tabStrip.getNode().clientWidth - 100, 3);
>+        // RTL-locales need at least "300"
>+        this._controller.click(tabStrip, tabStrip.getNode().clientWidth - 300, 3);

Well, for RTL locales we shouldn't use clientWidth but simply 100.

>-        this._controller.doubleClick(tabStrip, tabStrip.getNode().clientWidth - 100, 3);
>+        // RTL-locales need at least "300"
>+        this._controller.doubleClick(tabStrip, tabStrip.getNode().clientWidth - 300, 3);

Same here.

>+++ b/shared-modules/testToolbarAPI.js
>+  getDtds : function locationBar_getDtds() {
>+    return null;
>+  },

Shouldn't we at least include the brand.dtd and browser.dtd?

Comment 12

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

Comment on attachment 457191 [details] [diff] [review]
patch for firefox tests v2

>+const browserLocale = PrefsAPI.preferences.getPref("general.useragent.locale", "en-US");

Please move that into setupModule.

>+  for (i = 0; i < language.length; i++) {
>+    controller.keypress(langDropDown, language.charAt(i), {});
>+    controller.sleep(100);

You can make it a for each or at least access language[i].

>+  // There can be more than two languages already set (e.g.: Firefox-PL),
>+  // so setting to a safe value of 6 for now
>   var upButton = new elementslib.ID(controller.window.document, "up");
>-  controller.click(upButton);
>-  controller.sleep(gDelay);
>-  controller.click(upButton);
>-  controller.sleep(gDelay);
>+  
>+  for (i = 0; i < 6; i++) {
>+    controller.click(upButton);
>+    controller.sleep(gDelay);
>+  };

Can we do that until the up button is greyed out? Once we check if an element is disabled when we click on it, it will start failing.

>+++ b/firefox/testPrivateBrowsing/testAboutPrivateBrowsing.js
>+  var statusEntity = UtilsAPI.getEntity(pb.getDtds(), "privatebrowsingpage.issueDesc.normal");
>   var statusText = new elementslib.ID(controller.tabs.activeTab, "errorShortDescTextNormal");

Could you use the last but one part of the entity name here? In that case issueDesc? 

>+  var longDescEntity = UtilsAPI.getEntity(pb.getDtds(), "privatebrowsingpage.description");
>+  var moreInfoEntity = UtilsAPI.getEntity(pb.getDtds(), "privatebrowsingpage.learnMore");

Same here.

>+++ b/firefox/testSecurity/testGreenLarry.js
>+  // not every locale uses a ",", so:
>+  var location_alt = city + '\n' + state + '\u060c ' + country;

What is \u060c for a value? A better description would be great.

>   var ownerLocation = new elementslib.ID(controller.window.document,
>                                          "identity-popup-content-supplemental");
>-  controller.assertProperty(ownerLocation, "textContent", location);
>+  try {
>+    controller.assertProperty(ownerLocation, "textContent", location);
>+  } catch (e) {
>+    controller.assertProperty(ownerLocation, "textContent", location_alt);

Not that clear right now. I would need the input from above to see if that construct is really necessary.


>+++ b/firefox/testSecurity/testSSLDisabledErrorPage.js
>+  
>+  // TODO: move the dtds to a SecurityAPI, if one will be created
>+  var dtds = ["chrome://browser/locale/netError.dtd"];
>+  var property = "chrome://pipnss/locale/pipnss.properties";

Please move those up to global constants.

>+  controller.assertJS("subject.errorTitle == '" + UtilsAPI.
>+                      getEntity(dtds, "nssFailure2.title") + "'",
>                       {errorTitle: title.getNode().textContent});

Please move the getEntity call to its own line and feed it in as a second parameter for the object param.

>-  controller.assertJS("subject.errorMessage.indexOf('SSL protocol has been disabled') != -1",
>+  controller.assertJS('subject.errorMessage.indexOf("' + UtilsAPI.
>+                      getProperty(property, 'PSMERR_SSL_Disabled') + '") != -1',
>                       {errorMessage: text.getNode().textContent});

Same here.

>+++ b/firefox/testTabbedBrowsing/testOpenInBackground.js
>-
>+    
>+    // Sleep to prevent random failures
>+    controller.sleep(1000);

this is worked out on another bug. Please remove it from this patch.

Comment 13

[Adrian Kalla [:adriank]]

(In reply to comment #11)
> Comment on attachment 457193 [details] [diff] [review]
> patch for shared-modules v3
> 
> >+  getDtds : function downloadManager_getDtds() {
> >+    var dtds = ["chrome://mozapps/locale/extensions/extensions.dtd",
> >+               "chrome://browser/locale/browser.dtd",];
> 
> I would vote for moving the array to a global const and add a property to the
> class to return the array.

Leaving it the current way for now (see also Comment 5)


> >+++ b/shared-modules/testPrefsAPI.js
> >+   * @returns Array of external DTD urls
> >+   * @type [string]
> >+   */
> >+  getDtds : function preferencesDialog_getDtds() {
> >+    return null;
> >+  },
> 
> Can you please add the dtd file for the preferences dialog, or would we have to
> use several ones depending on the open pane.

There are at least 19 DTD files for the preference dialog, so I'd leave it as it is and add files depending on what we really need.


> >+++ b/shared-modules/testPrivateBrowsingAPI.js
> 
> >   /**
> >+   * Instance of the Utils API
> >+   * @private
> >+   */
> >+  this._utilsApi = collector.getModule('UtilsAPI');
> 
> Please remove all those comments from the constructor. Those aren't really
> helpful.

Done.

> >+  getDtds : function engineManager_getDtds() {
> >+    return null;
> 
> We should have DTD's here. Please add those, even we aren't using them yet.

done

> 
> >+  this._utilsAPI = collector.getModule('UtilsAPI');
> >   this._ModalDialogAPI = collector.getModule('ModalDialogAPI');
> 
> nit: Just reverse the order.

done


> 
> >-    this._controller.keypress(searchInput, 'a', {accelKey: true});
> >+    var selectAllEntity = this._utilsAPI.getEntity(this.getDtds(), "selectAllCmd.key");
> 
> Can we use the same variable for all those instances? I like to see cmdKey.
> That would also apply to all other instances.

Do you mean a global variable?  I think that could get chaotic...
If you mean just the name of the variable: that could work in cases where we have just one entity.


> >+  getDtds : function aboutSessionRestore_getDtds() {
> >+    return null;
> >+  },
> 
> I think we should also have a dtd here.

done

 
> >+++ b/shared-modules/testSoftwareUpdateAPI.js
> >+  getDtds : function softwareUpdate_getDtds() {
> >+    return null;
> 
> Here we definitely should have a DTD we will use in the future.

done

 
> >+++ b/shared-modules/testTabbedBrowsingAPI.js
> >       case "tabStrip":
> >         var tabStrip = this.getElement({type: "tabs_strip"});
> > 
> >         // XXX: Workaround until bug 537968 has been fixed
> >-        this._controller.click(tabStrip, tabStrip.getNode().clientWidth - 100, 3);
> >+        // RTL-locales need at least "300"
> >+        this._controller.click(tabStrip, tabStrip.getNode().clientWidth - 300, 3);
> 
> Well, for RTL locales we shouldn't use clientWidth but simply 100.
> 
> >-        this._controller.doubleClick(tabStrip, tabStrip.getNode().clientWidth - 100, 3);
> >+        // RTL-locales need at least "300"
> >+        this._controller.doubleClick(tabStrip, tabStrip.getNode().clientWidth - 300, 3);
> 
> Same here.

done


> >+++ b/shared-modules/testToolbarAPI.js
> >+  getDtds : function locationBar_getDtds() {
> >+    return null;
> >+  },
> 
> Shouldn't we at least include the brand.dtd and browser.dtd?

added

Comment 14

[Adrian Kalla [:adriank]]

Attachment: patch for shared-modules v4 [checked-in]

fixed everything mentioned above

Comment 15

[Adrian Kalla [:adriank]]

Attachment: patch for firefox tests v3

(In reply to comment #12)
> Comment on attachment 457191 [details] [diff] [review]
> patch for firefox tests v2
> 
> >+const browserLocale = PrefsAPI.preferences.getPref("general.useragent.locale", "en-US");
> 
> Please move that into setupModule.

done


> >+  for (i = 0; i < language.length; i++) {
> >+    controller.keypress(langDropDown, language.charAt(i), {});
> >+    controller.sleep(100);
> 
> You can make it a for each or at least access language[i].

done



> >+  // There can be more than two languages already set (e.g.: Firefox-PL),
> >+  // so setting to a safe value of 6 for now
> >   var upButton = new elementslib.ID(controller.window.document, "up");
> >-  controller.click(upButton);
> >-  controller.sleep(gDelay);
> >-  controller.click(upButton);
> >-  controller.sleep(gDelay);
> >+  
> >+  for (i = 0; i < 6; i++) {
> >+    controller.click(upButton);
> >+    controller.sleep(gDelay);
> >+  };
> 
> Can we do that until the up button is greyed out? Once we check if an element
> is disabled when we click on it, it will start failing.

done

 
> >+++ b/firefox/testPrivateBrowsing/testAboutPrivateBrowsing.js
> >+  var statusEntity = UtilsAPI.getEntity(pb.getDtds(), "privatebrowsingpage.issueDesc.normal");
> >   var statusText = new elementslib.ID(controller.tabs.activeTab, "errorShortDescTextNormal");
> 
> Could you use the last but one part of the entity name here? In that case
> issueDesc? 
> 
> >+  var longDescEntity = UtilsAPI.getEntity(pb.getDtds(), "privatebrowsingpage.description");
> >+  var moreInfoEntity = UtilsAPI.getEntity(pb.getDtds(), "privatebrowsingpage.learnMore");
> 
> Same here.

done


> >+++ b/firefox/testSecurity/testGreenLarry.js
> >+  // not every locale uses a ",", so:
> >+  var location_alt = city + '\n' + state + '\u060c ' + country;
> 
> What is \u060c for a value? A better description would be great.
> 
> >   var ownerLocation = new elementslib.ID(controller.window.document,
> >                                          "identity-popup-content-supplemental");
> >-  controller.assertProperty(ownerLocation, "textContent", location);
> >+  try {
> >+    controller.assertProperty(ownerLocation, "textContent", location);
> >+  } catch (e) {
> >+    controller.assertProperty(ownerLocation, "textContent", location_alt);
> 
> Not that clear right now. I would need the input from above to see if that
> construct is really necessary.

\u060c is the "arabic comma": http://www.w3.org/International/Spread/raw.txt
IIRC, I have seen it in more than just one locale.


> >+++ b/firefox/testSecurity/testSSLDisabledErrorPage.js
> >+  
> >+  // TODO: move the dtds to a SecurityAPI, if one will be created
> >+  var dtds = ["chrome://browser/locale/netError.dtd"];
> >+  var property = "chrome://pipnss/locale/pipnss.properties";
> 
> Please move those up to global constants.

done


> >+  controller.assertJS("subject.errorTitle == '" + UtilsAPI.
> >+                      getEntity(dtds, "nssFailure2.title") + "'",
> >                       {errorTitle: title.getNode().textContent});
> 
> Please move the getEntity call to its own line and feed it in as a second
> parameter for the object param.
> 
> >-  controller.assertJS("subject.errorMessage.indexOf('SSL protocol has been disabled') != -1",
> >+  controller.assertJS('subject.errorMessage.indexOf("' + UtilsAPI.
> >+                      getProperty(property, 'PSMERR_SSL_Disabled') + '") != -1',
> >                       {errorMessage: text.getNode().textContent});
> 
> Same here.

done


> >+++ b/firefox/testTabbedBrowsing/testOpenInBackground.js
> >-
> >+    
> >+    // Sleep to prevent random failures
> >+    controller.sleep(1000);
> 
> this is worked out on another bug. Please remove it from this patch.

looks like I forgot to exclude this from the patch...

Comment 16

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

Comment on attachment 457909 [details] [diff] [review]
patch for shared-modules v4 [checked-in]

>+  getDtds : function downloadManager_getDtds() {
>+    var dtds = ["chrome://mozapps/locale/extensions/extensions.dtd",
>+               "chrome://browser/locale/browser.dtd",];

I have fixed the indentation.

>+  getDtds : function downloadManager_getDtds() {
>+    var dtds = ["chrome://browser/locale/browser.dtd",
>+               "chrome://mozapps/locale/downloads/downloads.dtd"];

Same here.

>   this._prefs = collector.getModule('PrefsAPI').preferences;
> 
>-  /**
>-   * The MozMillController to operate on
>-   * @private
>-   */
>+  this._utilsApi = collector.getModule('UtilsAPI');
>+
>   this._controller = controller;

I have removed the empty rows.

>+    var dtds = ["chrome://branding/locale/brand.dtd",
>+               "chrome://browser/locale/browser.dtd",
>+               "chrome://browser/locale/aboutPrivateBrowsing.dtd"];

snip

>-      this._controller.keypress(null, 'p', {accelKey: true, shiftKey: true});
>+      var privateBrowsingCmdKey = this._utilsApi.getEntity(this.getDtds(), "privateBrowsingCmd.commandkey");

That should be cmdKey. I have updated that.

>   this._ModalDialogAPI = collector.getModule('ModalDialogAPI');
>+  this._utilsAPI = collector.getModule('UtilsAPI');

I have renamed it to this._UtilsAPI

>+    var dtds = ["chrome://mozapps/locale/update/history.dtd",
>+               "chrome://mozapps/locale/update/updates.dtd"]

snip

>+  this._utilsApi = collector.getModule('UtilsAPI');

snip

>+    var dtds = ["chrome://branding/locale/brand.dtd",
>+               "chrome://browser/locale/browser.dtd"];

snip.

Code-wise it looks fine and also applies correctly on top of the shared modules. I will have to test it thought, but nothing looks like to block from check-in. r=me.

Comment 17

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

Comment on attachment 457963 [details] [diff] [review]
patch for firefox tests v3

You miss the restart tests. There is one case for the master password test. Please supply a follow-up for restart tests.

Comment 18

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

Comment on attachment 457963 [details] [diff] [review]
patch for firefox tests v3

>+  // If we test a italian build, we have to use a non-Italian version of Google

an Italian build...

>+  if (browserLocale == "it") {
>+    // Verify the site is Polish oriented
>+    controller.assertNode(new elementslib.Link(controller.tabs.activeTab, "Zaloguj"));
>+    controller.assertNode(new elementslib.Link(controller.tabs.activeTab, "Grupy"));
>+    controller.assertNode(new elementslib.Link(controller.tabs.activeTab, "Szukanie zaawansowane"));
>+  } else {
>+    // Verify the site is Italian oriented
>+    controller.assertNode(new elementslib.Link(controller.tabs.activeTab, "Accedi"));
>+    controller.assertNode(new elementslib.Link(controller.tabs.activeTab, "Gruppi"));
>+    controller.assertNode(new elementslib.Link(controller.tabs.activeTab, "Ricerca avanzata"));
>+  }

When moving the remote webpage to our local storage we can hopefully eliminate this separation.

>+    var language = UtilsAPI.getProperty("chrome://global/locale/languageNames.properties",
>+                                       "pl");
>+  } else {
>+    var language = UtilsAPI.getProperty("chrome://global/locale/languageNames.properties",
>+                                       "it");

Wrong indentation.

>+  // Arabian has it's own comma: http://www.w3.org/International/Spread/raw.txt:
>+  var location_ar = city + '\n' + state + '\u060c ' + country;
>   var ownerLocation = new elementslib.ID(controller.window.document,
>                                          "identity-popup-content-supplemental");
>-  controller.assertProperty(ownerLocation, "textContent", location);
>+  try {
>+    controller.assertProperty(ownerLocation, "textContent", location);
>+  } catch (e) {
>+    controller.assertProperty(ownerLocation, "textContent", location_ar);
>+  };

Can we make that depending on the browser locale and define an hash of locales with a different comma?

>   // The tabs title should be 'Untitled'
>-  var title = UtilsAPI.getEntity(["chrome://browser/locale/browser.dtd"],
>+  var newTablabel = UtilsAPI.getEntity(["chrome://browser/locale/browser.dtd"],
>                                   "newTab.label");

Why we have the dtd definition here? It should be part of the tabbrowser class we could get with getDtds().

Well, please don't create a follow-up as proposed before but fix all of that in a new revision. r=me with that change.

Comment 19

[Adrian Kalla [:adriank]]

(In reply to comment #18)
> Comment on attachment 457963 [details] [diff] [review]
> >+  // Arabian has it's own comma: http://www.w3.org/International/Spread/raw.txt:
> >+  var location_ar = city + '\n' + state + '\u060c ' + country;
> >   var ownerLocation = new elementslib.ID(controller.window.document,
> >                                          "identity-popup-content-supplemental");
> >-  controller.assertProperty(ownerLocation, "textContent", location);
> >+  try {
> >+    controller.assertProperty(ownerLocation, "textContent", location);
> >+  } catch (e) {
> >+    controller.assertProperty(ownerLocation, "textContent", location_ar);
> >+  };
> 
> Can we make that depending on the browser locale and define an hash of locales
> with a different comma?

We could, but we would have to update the hash every time a new Firefox locale will be accepted.
Another problem I see with that are localizers who would like to test their yet unofficial locales with Mozmill - they would see unneeded test failures...

Comment 20

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

(In reply to comment #19)
> We could, but we would have to update the hash every time a new Firefox locale
> will be accepted.
> Another problem I see with that are localizers who would like to test their yet
> unofficial locales with Mozmill - they would see unneeded test failures...

Well, I don't meant to build a hash for all locales but only for those which behave differently like the ar one. All others should use the default code. Also doable with a switch for building the 'location' location string.

Comment 21

[Adrian Kalla [:adriank]]

Attachment: follow-up patch for shared-modules [checked-in]

tis is a "follow-up patch", to make it easier to review - as discussed on IRC

Comment 22

[Adrian Kalla [:adriank]]

Attachment: follow-up patch for firefox tests

tis is a "follow-up patch", to make it easier to review - as discussed on IRC

Comment 23

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

Comment on attachment 457909 [details] [diff] [review]
patch for shared-modules v4 [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/96cb0b78c586

Comment 24

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

Comment on attachment 460340 [details] [diff] [review]
follow-up patch for shared-modules [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/933b0510bb63

Comment 25

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

Comment on attachment 457963 [details] [diff] [review]
patch for firefox tests v3

>diff --git a/firefox/testTabbedBrowsing/testNewTab.js b/firefox/testTabbedBrowsing/testNewTab.js
>
>   // The tabs title should be 'Untitled'
>-  var title = UtilsAPI.getEntity(["chrome://browser/locale/browser.dtd"],
>+  var newTablabel = UtilsAPI.getEntity(["chrome://browser/locale/browser.dtd"],
>                                   "newTab.label");
>   var tab = tabBrowser.getTab();
>-  controller.assertJS("subject.label == '" + title + "'", tab.getNode());
>+  controller.assertJS("subject.label == '" + newTablabel + "'", tab.getNode());

This patch isn't necessary anymore because we are using a property now. I will update the patch for default.

Comment 26

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

Comment on attachment 460341 [details] [diff] [review]
follow-up patch for firefox tests

>diff --git a/firefox/helperClasses/testSessionStoreAPI.js b/firefox/helperClasses/testSessionStoreAPI.js
>-  controller.keypress(null, "t", {accelKey: true});
>+  tabBrowser = new TabbedBrowsingAPI.tabBrowser(controller);
>+  tabBrowser.openTab({type: "shortcut"});

If you need the tabBrowser it has to be declared inside setupModule.

>+  // Arabic locales have it's own comma: http://www.w3.org/International/Spread/raw.txt:
>+  var arCommaLocales = ['ar', 'fa'];
>+  if (arCommaLocales.indexOf(UtilsAPI.appInfo.locale) != -1)
>+    var comma = '\u060c';
>+  else
>+    var comma = ',';

Please change that to use an hash. It should be easy to add more entries here which could probably require other characters:

var locales = ['ar': '\u060c', 'fa': '\u060c'];

>+++ b/firefox/testTabbedBrowsing/testNewTab.js
>   // The tabs title should be 'Untitled'
>-  var newTablabel = UtilsAPI.getEntity(["chrome://browser/locale/browser.dtd"],
>-                                  "newTab.label");
>+  var newTablabel = UtilsAPI.getEntity(tabBrowser.getDtds(), "newTab.label");

This has been duplicated. Please remove it from this patch.

Also running the test I get the following error:

TEST-START | /Volumes/data/testing/mozmill/default/firefox/testPrivateBrowsing/testCloseWindow.js | testCloseWindow
ERROR | Test Failure: {"exception": {"message": "TabbedBrowsingAPI.getDtds is not a function", "lineNumber": 94, 

getDtds() is part of tabBrowser and not of the API.


Please update those comments. Then we can check-in the remaining stuff. Thanks!

Comment 27

[Adrian Kalla [:adriank]]

Attachment: follow-up patch for firefox tests v2

Comment 28

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

Comment on attachment 461361 [details] [diff] [review]
follow-up patch for firefox tests v2

>+++ b/firefox/helperClasses/testSessionStoreAPI.js
>-  controller.keypress(null, "t", {accelKey: true});
>+  tabBrowser = new TabbedBrowsingAPI.tabBrowser(controller);
>+  tabBrowser.openTab({type: "shortcut"});

Once again, the tabbrowser instantiation has to be part of the constructor and not a member function.

>+  // Arabic locales have it's own comma: http://www.w3.org/International/Spread/raw.txt:
>+  var diffCommaLocales = {'ar': '\u060c', 'fa': '\u060c'};
>+  if (UtilsAPI.appInfo.locale in diffCommaLocales)
>+    var comma = diffCommaLocales[UtilsAPI.appInfo.locale];
>+  else
>+    var comma = ',';
>+  controller.window.alert(comma);

Please remove that alert and it would be good to give the variable a name like commaList.


As usual, please execute a test-run before asking for review.

Comment 29

[Adrian Kalla [:adriank]]

Attachment: follow-up patch for firefox tests v3 [checked-in]

looks like I was blind yesterday...

Hope this patch covers all comments above.

Comment 30

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

Comment on attachment 461571 [details] [diff] [review]
follow-up patch for firefox tests v3 [checked-in]

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

Comment 31

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

Yay! Thanks Adrian. Now lets wait one day how tomorrows test results look like. If everything is ok, we can start to backport this patch.

Comment 32

[Adrian Kalla [:adriank]]

Attachment: all-in-one patch backported to the 1.9.2 branch

Comment 33

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

Comment on attachment 464858 [details] [diff] [review]
all-in-one patch backported to the 1.9.2 branch

>+++ b/firefox/testPrivateBrowsing/testCloseWindow.js
> var setupModule = function(module) {
>   controller = mozmill.getBrowserController();
>   pb = new PrivateBrowsingAPI.privateBrowsing(controller);
>+  tabBrowser = new TabbedBrowsingAPI.tabBrowser(controller);
> 
>   TabbedBrowsingAPI.closeAllTabs(controller);
> }

Please use the tabBrowser instance to call closeAllTabs.

>+++ b/firefox/testSecurity/testSSLDisabledErrorPage.js
>-  controller.assertJS("subject.errorTitle == 'Secure Connection Failed'",
>+  var nssFailure2title = UtilsAPI.getEntity(dtds, "nssFailure2.title")
>+  controller.assertJS("subject.errorTitle == '" + nssFailure2title + "'",
>                       {errorTitle: title.getNode().textContent});

nssFailure2title also has to be passed in as part of the object.

>-  controller.assertJS("subject.errorMessage.indexOf('SSL protocol has been disabled') != -1",
>+  var PSMERR_SSL_Disabled = UtilsAPI.getProperty(property, 'PSMERR_SSL_Disabled')
>+  controller.assertJS('subject.errorMessage.indexOf("' + PSMERR_SSL_Disabled + '") != -1',
>                       {errorMessage: text.getNode().textContent});

Same here.

>+++ b/shared-modules/testPrivateBrowsingAPI.js
>+  this._utilsApi = collector.getModule('UtilsAPI');
>   this._controller = controller;

It should be this._UtilsAPI and please move it below the controller assignment separated by an empty line.

>+++ b/shared-modules/testSearchAPI.js
>   this._ModalDialogAPI = collector.getModule('ModalDialogAPI');
>+  this._utilsAPI = collector.getModule('UtilsAPI');

Same here.

>+++ b/shared-modules/testSessionStoreAPI.js
>+  this._utilsApi = collector.getModule('UtilsAPI');
>   this._WidgetsAPI = collector.getModule('WidgetsAPI');

And here.

>+++ b/shared-modules/testTabbedBrowsingAPI.js

The patch doesn't apply anymore cleanly on that API.

>+  this._utilsApi = collector.getModule('UtilsAPI');

See above.

>+++ b/shared-modules/testToolbarAPI.js
>+  this._utilsApi = collector.getModule('UtilsAPI');

See above.

Comment 34

[Adrian Kalla [:adriank]]

Attachment: all-in-one patch backported to the 1.9.2 branch v2

patch with addressed Henrik's review comments

Comment 35

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

Comment on attachment 465343 [details] [diff] [review]
all-in-one patch backported to the 1.9.2 branch v2

Review comments still not fully addressed. Feedback via IRC.

Comment 36

[Adrian Kalla [:adriank]]

Attachment: all-in-one patch backported to the 1.9.2 branch v3

"Api" vs "API"... how couldn't I see the difference...
Fixed that now

Comment 37

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

Comment on attachment 465647 [details] [diff] [review]
all-in-one patch backported to the 1.9.2 branch v3

Adrian, please looks closely at my review comments. As said the patch doesn't apply cleanly on top of the TabbedBrowsingAPI. Please fix that so that we can check the patch in asap. Thanks.

Comment 38

[Adrian Kalla [:adriank]]

Attachment: all-in-one export patch backported to the 1.9.2 branch v4 [checked in]

corrected to apply cleanly on current TabbedBrowsingAPI

Comment 39

[Adrian Kalla [:adriank]]

Attachment: all-in-one export patch backported to the 1.9.1 branch v1

backported patch to 1.9.1 - based on the 1.9.2 patch v4

Comment 40

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

Comment on attachment 465770 [details] [diff] [review]
all-in-one export patch backported to the 1.9.2 branch v4 [checked in]

Landed on 1.9.2 as:
http://hg.mozilla.org/qa/mozmill-tests/rev/b12c20436815

Comment 41

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

Comment on attachment 465779 [details] [diff] [review]
all-in-one export patch backported to the 1.9.1 branch v1

Landed on 1.9.1:
http://hg.mozilla.org/qa/mozmill-tests/rev/2e429e003571

Comment 42

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

Everything has been landed. Thanks Adrian for the work! Lets call it fixed.

This is a big step forward and will be announced once Mozmill 1.4.2 (1.5) has been released.

Comment 43

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

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.