680076 - getSuggestions should return null if no suggestions exist

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

Testing suggestions would fail a test if no suggestions are provided via getSuggestions.

I am creating a patch that will return null if the suggestions popup does not appear (meaning there are no suggestions).

We can use this because now we have a way of checking that a search engine has suggestions.

Comment 1

[Remus Pop (:RemusPop)]

Attachment: initial patch

Proposing a solution if a suggestions popup does not appear, instead of asserting.

I think assertions should be on the test side.

Comment 2

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

Comment on attachment 554084 [details] [diff] [review]
initial patch

>-    // After entering the search term the suggestions have to be visible
>-    this._controller.assert(function () {
>-      return popup.getNode().state === 'open';
>-    }, "Search suggestions are visible");
>+    // If we have no suggestions return null
>+    if (popup.getNode().state === 'closed')
>+		  return null;
>+

With the implementation of bug 680057 we should check first if the engine supports search suggestions at all. That way we could return immediately. So we even don't have to enter the search term first.

Also we should be conform with the return value. Right now it's an array, but you return null here. I think in case of no support for suggestions we should return null, otherwise if the panel hasn't been opened an empty array. That way we could differentiate between those two states.

Also please check if current tests would have to be updated.

Comment 3

[Remus Pop (:RemusPop)]

Attachment: proposed solution v1

getSuggestions() now returns null if the current selected engine does not support suggestions, an empty array if no suggestions are provided and an array with suggestions if they are visible.

It should be noted that using getSuggestions without verifying first if the engine supports them should be treated in a test file with 2 if structures. First we should check if we don't have a null from getSuggestions and after that if the array is not empty (length property). If we don't go through this steps and receive a null and we check the length property, the test will fail.

Comment 4

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

Comment on attachment 554828 [details] [diff] [review]
proposed solution v1

>   getSuggestions : function(searchTerm) {
>     var suggestions = [ ];
>     var popup = this.getElement({type: "searchBar_autoCompletePopup"});
>     var treeElem = this.getElement({type: "searchBar_suggestions"});
> 
>+    // Check if the current search engine supports suggestions
>+    if (this.hasSuggestions(this.selectedEngine) == false)
>+      return null;

Please move this block up so it is getting executed first and we do not have to find the elements. Also don't compare to false. Just add a '!' before this.

>+    // Return an empty array if no suggestions are provided
>+    if (popup.getNode().state === 'closed')
>+      return suggestions;

I would prefer to check for 'open' and execute the following code only under this condition. Then we will have a single return statement in this function.

Comment 5

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

Remus, do you have any update for this patch? Are there any blockers?

Comment 6

[Remus Pop (:RemusPop)]

Attachment: patch v2

Modified patch as requested.

Comment 7

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

Comment on attachment 555967 [details] [diff] [review]
patch v2

>+    // Get suggestions in an array if the popup with suggestions is opened
>+    if (popup.getNode().state === 'open') {
>+    
>+      this._controller.waitForElement(treeElem, TIMEOUT);

nit: remove this empty line.

>+      // Get all suggestions
>+      var tree = treeElem.getNode();
>+      this._controller.waitForEval("subject.tree.view != null", TIMEOUT, 100,
[..]
>+      this._controller.waitForEval("subject.popup.state == 'closed'", TIMEOUT, 100,
>+                                   {popup: popup.getNode()});

While we are at this code. Can you also please change both waitForEval calls to waitFor?

Otherwise it looks fine.

Comment 8

[Remus Pop (:RemusPop)]

Attachment: patch v3

Updated and polished.

Comment 9

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

Comment on attachment 556005 [details] [diff] [review]
patch v3

>+      this._controller.waitFor(function() {

If you don't use a space in-between function and () it gives the impression we call a function with the name 'function'. But it's a function definition. 

>+        return tree.view;

Please keep the comparison as it was before. We can't return an object here. It really has to be a boolean.

>+      }, TIMEOUT, 100, {tree: tree});

The second argument of waitFor is a message and there is no need to push in the other parameters. If the calling convention is unclear to you, just search our test code base for further examples.

Comment 10

[Remus Pop (:RemusPop)]

Attachment: patch v4

Updated the waitFor calls.

Comment 11

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

Comment on attachment 556012 [details] [diff] [review]
patch v4

>+      this._controller.waitFor(function () {
>+        return tree.view != null;
>+      }, TIMEOUT, 100);

Please consolidate the documentation. It is still wrong:
https://developer.mozilla.org/en/Mozmill_Tests/Mozmill_Controller_Object#waitFor%28%29

>+      for (var i = 0; i < tree.view.rowCount; i ++) {

Add an empty line before this for loop please.

>+      this._controller.waitFor(function () {
>+        return popup.state == 'closed'
>+      }, TIMEOUT, 100, {popup: popup.getNode()});

Same as above applies here too.

Comment 12

[Remus Pop (:RemusPop)]

Attachment: patch v5

Updated the waitFor calls.

Comment 13

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

Comment on attachment 556551 [details] [diff] [review]
patch v5

>+      this._controller.waitFor(function () {
>+        return tree.view != null;
>+      }, "Auto-complete results must be visible");

Because the callback in the waitFor is called multiple times we normally use the "has been" notation. I will make this small change before checking in the patch on all branches.

Comment 14

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/00a3af07a41f (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/62ca8678c79c (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/f21f08715b5e (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/b3e7f03dfd29 (release)

Thanks Remus! Now lets get the failure fixed.