879946 - Remove Lookup methods and Xpath elements from lib/search.js

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Jonathan French (:jfrench)]

Split off from bug 503192, this is a supplementary bug to remove and replace any elementslib.Lookup methods and Xpath elements from lib/search.js to make the code base less fragile. We would like to replace Lookup with ID, Selector, or if neither work, Class via NodeCollector.

An example of this approach can be found here in the getElements() method in lib/addons.js
http://hg.mozilla.org/qa/mozmill-tests/file/b8a22c8c9a3b/lib/addons.js#l1026

Comment 1

[Amod [:AMoz]]

I would like to work on this bug. I would upload the patch within a while.

Comment 2

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

Amod, I would suggest that you are getting started with a single bug like bug 879884 first. Once done or nearly finished we could assign additionally bugs to you. Would that work?

Comment 3

[Amod [:AMoz]]

sure..no problem !

Comment 4

[mario garbi]

Attachment: nodeCollectorRefactor.patch

Replaced lookup methods with nodeCollector in the library file.

Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/0fc916b47806e5d11cba8af7dc2a8642

Windows:
http://mozmill-crowd.blargon7.com/#/functional/report/0fc916b47806e5d11cba8af7dc2a80f0

Comment 5

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

Comment on attachment 8358301 [details] [diff] [review]
nodeCollectorRefactor.patch

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

::: firefox/lib/search.js
@@ +181,5 @@
> +    nodeCollector.queryNodes("#commonDialog");
> +    nodeCollector.root = nodeCollector.nodes[0];
> +    nodeCollector.queryAnonymousNode("dlgtype", "accept");
> +
> +    aController.click(nodeCollector.elements[0]);

All element accessors have to be moved into getElements().

@@ +246,5 @@
>    getElement : function engineManager_getElement(spec) {
>      var elem = null;
>  
> +    var root = this._controller.window.document;
> +    var nodeCollector = new domUtils.nodeCollector(root);

There is a mismatch. getElement is a wrapper around getElements and should return a single element.

@@ +628,5 @@
>     *        value: Value of the element or property
>     * @returns Element which has been created
>     * @type ElemBase
>     */
> +  getElement : function searchBar_getElement(aSpec) {

I would also correct this to getElements() and define a new getElement() wrapper method.

@@ +637,1 @@
>      switch(spec.type) {

Missing blank line before the switch statement.

@@ +640,5 @@
>         * value: value to match
>         */
>        case "engine":
> +        nodeCollector.queryNodes("#searchbar");
> +        nodeCollector.root = nodeCollector.nodes[0];

There is no need for that. Base on the already defined searchBar element in this switch block.

@@ +642,5 @@
>        case "engine":
> +        nodeCollector.queryNodes("#searchbar");
> +        nodeCollector.root = nodeCollector.nodes[0];
> +        nodeCollector.queryAnonymousNode("anonid", "searchbar-engine-button");
> +        nodeCollector.root = nodeCollector.nodes[0];

Is this extra queryAnonymousNode call necessary?

@@ +648,4 @@
>          break;
>        case "engine_manager":
> +        nodeCollector.queryNodes("#searchbar");
> +        nodeCollector.root = nodeCollector.nodes[0];

Same as above.

@@ +659,4 @@
>          break;
>        case "searchBar_contextMenu":
> +        nodeCollector.queryNodes("#searchbar");
> +        nodeCollector.root = nodeCollector.nodes[0];

Same as above.

@@ +663,5 @@
> +        nodeCollector.queryAnonymousNode("anonid", "searchbar-textbox");
> +        nodeCollector.root = nodeCollector.nodes[0];
> +        nodeCollector.queryAnonymousNode("anonid", "textbox-input-box");
> +        nodeCollector.root = nodeCollector.nodes[0];
> +        nodeCollector.queryAnonymousNode("anonid", "input-box-contextmenu");

Are all those extra queryAnonymousNode calls necessary?

@@ +668,4 @@
>          break;
>        case "searchBar_dropDown":
> +        nodeCollector.queryNodes("#searchbar");
> +        nodeCollector.root = nodeCollector.nodes[0];

Same as above and for all the following cases. Personally I would suggest we cache this element, so we don't have to retrieve it all the time.

@@ +684,5 @@
>        case "searchBar_input":
> +        nodeCollector.queryNodes("#searchbar");
> +        nodeCollector.root = nodeCollector.nodes[0];
> +        nodeCollector.queryAnonymousNode("anonid", "searchbar-textbox");
> +        nodeCollector.root = nodeCollector.nodes[0];

Do we really need this case? Can't we directly type into the textbox element?

Comment 6

[mario garbi]

Attachment: nodeCollectorRefactor_v2.patch

Comment 7

[mario garbi]

(In reply to Henrik Skupin (:whimboo) from comment #5)
> Comment on attachment 8358301 [details] [diff] [review]

> > +    var nodeCollector = new domUtils.nodeCollector(root);
> 
> There is a mismatch. getElement is a wrapper around getElements and should
> return a single element.

I have have updated in this patch the classes so we have getElement here too. I haven't initially modified it in order to keep the changes to a required minimum.

> > +        nodeCollector.root = nodeCollector.nodes[0];
> 
> Is this extra queryAnonymousNode call necessary?

I tried to keep the queries as minimal as possible, removing those queryAnonymousNode calls will cause the code to fail in finding the bound element.

> > +        nodeCollector.root = nodeCollector.nodes[0];
> 
> Do we really need this case? Can't we directly type into the textbox element?

Here is the only place we use the element and we check the placeholder attribute. If we want to remove the element from the library we will need to change this test as well. Perhaps this should be handled in a different bug as it's not directly related to this one.

http://hg.mozilla.org/qa/mozmill-tests/file/435cdb211085/firefox/tests/functional/testSearch/testOpenSearchAutodiscovery.js#l60

Comment 8

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

Comment on attachment 8359120 [details] [diff] [review]
nodeCollectorRefactor_v2.patch

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

Keep in mind that I want to do a follow-up review once I gave a r- somewhere. If a review? is not necessary at the moment that is fine but request a final review from me later.

Comment 9

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8359120 [details] [diff] [review]
nodeCollectorRefactor_v2.patch

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

It looks good to me, but it misses a commit message. Please also ask review from Henrik next time. It might be good to retest this since it waiting review a while, just to be sure everything is still fine.

Comment 10

[mario garbi]

Attachment: nodeCollectorRefactor_v3.patch

The patch applies correctly, adding Henrik for review as well.

Comment 11

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

Comment on attachment 8365835 [details] [diff] [review]
nodeCollectorRefactor_v3.patch

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

::: firefox/lib/search.js
@@ +181,5 @@
> +    nodeCollector.queryNodes("#commonDialog");
> +    nodeCollector.root = nodeCollector.nodes[0];
> +    nodeCollector.queryAnonymousNode("dlgtype", "accept");
> +
> +    aController.click(nodeCollector.elements[0]);

This code can be removed in favor of the getElements('button') type.

@@ +302,3 @@
>              break;
>            case "up":
> +            nodeCollector.queryNodes("#up");

I don't see a reason for the switch. Just do queryNodes('#' + spec.subtype).

@@ +687,5 @@
> +    var parent = spec.parent;
> +    var root = parent ? parent.getNode() : this._controller.window.document;
> +    var nodeCollector = new domUtils.nodeCollector(root);
> +
> +    var searchBarNode = nodeCollector.queryNodes("#searchbar").nodes[0];

If this is your root element specify it as such, and cache it as class property if its used that often?

@@ +714,5 @@
> +        nodeCollector.queryAnonymousNode("anonid", "searchbar-textbox");
> +        nodeCollector.root = nodeCollector.nodes[0];
> +        nodeCollector.queryAnonymousNode("anonid", "textbox-input-box");
> +        nodeCollector.root = nodeCollector.nodes[0];
> +        nodeCollector.queryAnonymousNode("anonid", "input-box-contextmenu");

I'm still not sure why we need three separate calls here.

@@ +732,5 @@
>        case "searchBar_input":
> +        nodeCollector.root = searchBarNode;
> +        nodeCollector.queryAnonymousNode("anonid", "searchbar-textbox");
> +        nodeCollector.root = nodeCollector.nodes[0];
> +        nodeCollector.queryAnonymousNode("anonid", "input");

Not sure if that chain is necessary too, but you want to refer to searchBar_textBox

@@ +737,3 @@
>          break;
>        case "searchBar_suggestions":
> +        nodeCollector.queryNodes("#PopupAutoComplete");

You want to refer to searchBar_autoCompletePopup

Comment 12

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

Gaurav is interested to work on this bug. So thank you already in advance. As first thing you should read through our documentation as pointed out in the URL field above. That will get you familiar with the Mozmill tests for Firefox.

Once that has been done, you will have a focus on the following file:
http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/search.js

Best would be if you could show up on IRC (#automation) channel for further discussions. If that is not possible we can continue here for sure.

Comment 13

[Gaurav Rai [:gaurav0x]]

I have cloned the repository. I have read the documentation and unable to find information on nodecollector. Is it a part of mozilla specific API? I may need some help to get started on this bug

Comment 14

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

The NodeCollector class is part of our library and you can find it in /lib/dom-utils.js inside the same repository. You may want to also look at other tests in this repository how they work with the node collector.

Comment 15

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

Since the last update on this bug the Mozmill tests have been deprecated in favor of the Firefox UI tests. So we no longer want to fix this bug.