874393 - Failures in '/testSearch' functional tests due to the fact that the search bar cannot be found

Status: RESOLVED FIXED

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

Description

[Daniela Petrovici]

All the tests in testSearch folder fail on Australis build on all OS versions due to this line: http://hg.mozilla.org/qa/mozmill-tests/file/05730582186d/lib/search.js#l26

SEARCH_BAR needs to contain: '/id("nav-bar-customizationtarget")' since this is the element that contains the search-container. The line should be changed to in order for the test to pass:
const SEARCH_BAR          = NAV_BAR + '/id("nav-bar-customizationtarget")/id("search-container")/id("searchbar")';

Before changing the above line, we need to see if nodeCollector cannot be used instead of Lookup.

Comment 1

[Daniela Petrovici]

This change is intentional due per bug 864355 where almost all nav-bar needs to be included under the customization target.

Comment 2

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

This bug is about something different and not fixed yet. But yes we have a new container around navbar elements.

Comment 3

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> This bug is about something different and not fixed yet.
A patch for that bug already landed: https://hg.mozilla.org/projects/ux/rev/7105d3282462. So, a part of the feature is already in the current Australis build and breaks our tests.

> But yes we have a new container around navbar elements.
Due to the introduced container, the search bar cannot be found and makes the search tests fail.

NOTE: Based on bug 874341 comment #6, we needed to figure out if these changes are intentional or not, but we still need to figure out if nodeCollector can be used instead of Lookup expression here.

Comment 4

[mario garbi]

Attachment: Patch v1.0

Initially I tried to use nodeCollector to get the chrome element but we had some issues with that as I said in Ask an Expert meeting. This patch uses a fix for the lookup that works for Bug 874394 too.

Basically what we do here is check if we use an Australis build and update the lookup string with either a correct Australis specific element or a blank string piece.

In the report bellow we can see that testSearch/ tests are failing anymore (except for testSearchSelection that fails with Tab Index 1 - which is a known bug already).
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947553feb

Is this approach acceptable or do we want to handle this in a different way?

Comment 5

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

Comment on attachment 775595 [details] [diff] [review]
Patch v1.0

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

I call this an interim fix which let us run our tests in both kinds of builds. But at the end we should get rid of the lookup strings completely. Please file the necessary work for that as a new bug and mark it blocking our australis meta bug.

Once you fixed the nits, please re-ask review from Dave, because I also want to hear his thoughts. Also provide reports for both kind of builds.

::: lib/utils.js
@@ +194,5 @@
>    return Services.io.newURI(spec, originCharset, baseURI);
>  }
>  
> +/**
> + * Check if we use Australis or not by checking for the presence of a 

nit: please kill those two trailing white-spaces in this file.

@@ +200,5 @@
> + *
> + * @returns {Boolean} Returns true if we use Australis or false if we don't
> + */
> +function isAustralis() {
> +  return !prefs.preferences.getPref("browser.uiCustomization.debug", true);

I wouldn't rely on a debug pref. Is there no browser.uiCustomization preference available?

Comment 6

[mario garbi]

We already have bugs for lookup refactoring that were filed by Jonathan French and have been mentioned in comment 8 of Bug 874341:
Bug 879884
Bug 879938
Bug 879946
Bug 879948
Bug 879950
Bug 879953
Bug 879962
There are all 'Good first bugs' and each handles a single library file.

Comment 7

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

Ok, so lets get this patch finalized then.

Comment 8

[mario garbi]

Attachment: Patch v1.1

Fix patch until we land the re-factoring patches
Reports:
Mac 
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947b30873
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947b39b5f

Linux
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947b2552a
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947b32e3c

Windows
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947b267b8
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947b2d6dc

Comment 9

[Andreea Matei [:AndreeaMatei]]

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

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

Please ask review from Dave as well as Henrik suggested earlier and reports to both builds (australis and nightly).

::: lib/utils.js
@@ +197,5 @@
> +/**
> + * Checks if we use Australis by calling 'isAustralis' method and returns
> + * a String containing either the Australist specific element or a blank
> + *
> + * @returns {String} String value for the Australis specific element 

Whitespace

@@ +402,5 @@
>  /**
> + * Check if we use Australis or not by checking for the presence of a 
> + * Australis specific element
> + * 
> + * @returns {Boolean} Returns true if we use Australis or false if we don't

Whitespace

@@ +406,5 @@
> + * @returns {Boolean} Returns true if we use Australis or false if we don't
> + */
> +function isAustralis() {
> +  var controller = new mozmill.getBrowserController();
> +  var browserWindow = controller.window.gCustomizeMode;

I don't see the point in having this extra variable.

Comment 10

[mario garbi]

Attachment: australisFix_1907.patch

After investigating the other Australis bugs I made a refactoring of the patch so we can use the library function for more cases.

Reports for both Central and Australis:

Windows:
-Central-
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61069a21
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d610704da
-Australis-
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d610733be
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d6107576e

Mac:
-Central-
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61072727
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d610772b9
-Australis-
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d610783c8
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61079ca8

Linux:
-Central-
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d610776d7
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61079267
-Australis-
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d610701e0
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d6107484e

Comment 11

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

Please give more information why changes are necessary and not only upload a patch. We all have to know about those internals to be able to understand the reason why you have used this code. This should always happen for all the patches you upload, independently of their complexity.

Comment 12

[mario garbi]

Of course, sorry for not providing more informations. First of all this is a temporary solution as you said yourself until we land the nodeCollector refactoring patch. Basically we check for an element that only exists in Australis (".gCustomizeMode") and if that returns true we select an appropriate node name string to add to the Lookup. The aLocation parameter tells us where we call this function (which library) so we can return the correct string.

I have grouped them in an object after talking with the team and deciding this is the best approach and upgraded the function with a switch so we can handle more than one library.

If there is something I failed to explain let me know and I will reply as soon as possible.

Comment 13

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 778413 [details] [diff] [review]
australisFix_1907.patch

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

::: lib/utils.js
@@ +146,5 @@
> + *
> + */
> +var australis = {
> +
> +  /**

This is off one space to the right, same for the other method's js doc.

@@ +156,5 @@
> +  *        correct string
> +  *
> +  * @returns {String} String value for the Australis specific element
> +  */
> +  getNewNodeName : function australis_getNewNodeName(aLocation) {

I'm thinking getAustralisNode is more suggestive, but I'll leave this to Dave.

Comment 14

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

Comment on attachment 778413 [details] [diff] [review]
australisFix_1907.patch

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

::: lib/utils.js
@@ +148,5 @@
> +var australis = {
> +
> +  /**
> +  * Checks if we use Australis by calling the 'is' method and returns
> +  * a String containing either the Australist specific element or a blank

Typo: Australist

"or a blank"..?

@@ +156,5 @@
> +  *        correct string
> +  *
> +  * @returns {String} String value for the Australis specific element
> +  */
> +  getNewNodeName : function australis_getNewNodeName(aLocation) {

What is the 'new' referring to in the function name?

@@ +175,5 @@
> +  * Australis specific element
> +  *
> +  * @returns {Boolean} Returns true if we use Australis or false if we don't
> +  */
> +  is : function australis_is() {

I don't like this function name as it's far too vague. I would prefer something like isAustralis

Comment 15

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

Comment on attachment 778413 [details] [diff] [review]
australisFix_1907.patch

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

Oops, took too long to review, and ended up reverting Andreea's review flag. Corrected now.

Comment 16

[mario garbi]

Attachment: australisFix_2307.patch

Updated the patch with the needed changes.

Comment 17

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 779712 [details] [diff] [review]
australisFix_2307.patch

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

Looks good now. Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/963408b08af3 (default)

Comment 18

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

Comment on attachment 779712 [details] [diff] [review]
australisFix_2307.patch

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

::: lib/utils.js
@@ +156,5 @@
> +   *        correct string
> +   *
> +   * @returns {String} String value for the Australis specific element
> +   */
> +  getAustralisNode : function australis_getAustralisNode(aLocation) {

It's just temporary but for the future please align to our getElement() method we make use of in different places. Also the 'Australis' part is unessential here given that all methods are bundled inside the australis object.