594891 - Make sure autocomplete is closed before leaving testFaviconInAutocomplete

Status: RESOLVED WORKSFORME

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Module: testAwesomeBar/testFaviconInAutocomplete.js
Branch: ALL

In testFaviconInAutocomplete.js, the Awesomebar autocomplete is displayed to verify the existence of a favicon.  However, when the test finishes, this autocomplete is left open.  We don't even do anything to close the autocomplete and verify it is closed.

We should add this to the test, not only to verify the functionality, but to ensure it ends cleanly for following tests.

Comment 1

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

Firing an Escape event closes the auto-complete popup. We should add a new helper function to the autoCompleteResults class. Aaron could you make those quick changes?

Comment 2

[Aaron Train [:aaronmt]]

Attachment: v1 - API change + default test change

+ closeResults() in testToolbarAPI.js
+ teardownModule() in testFaviconInAutocomplete.js

Comment 3

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

Comment on attachment 476921 [details] [diff] [review]
v1 - API change + default test change

>+++ b/firefox/testAwesomeBar/testFaviconInAutocomplete.js	Mon Sep 20 17:44:16 2010 -0400
>
>+var teardownModule = function() { 
>+  // Close the autocomplete results
>+  locationBar.autoCompleteResults.closeResults();
>+}

Looks correct but please fix all instances and not only for this test.

>+  closeResults : function autoCompleteResults_closeResults() {

Simply call it close. There is no need to add 'Results'. The class name speaks for itself.

>+    var results = this.getElement({type: "popup"});

We have a popup here. Please name the variable appropriately.

>+    var stateOpen = results.isOpened;

While you are already on it, please also rename the isOpened attribute to isOpen.

>+    this._controller.keypress(null, "VK_ESCAPE", {});

Don't fire the event against the window but the popup itself. Otherwise unexpected behavior can happen. Doing so probably makes the check for isOpen unnecessary.

>+    this._controller.waitForEval("subject.isOpened == " + stateOpen,
>+                                 TIMEOUT, 100, results);

Please pass in an object with a better description.

Comment 4

[Aaron Train [:aaronmt]]

Attachment: V1.1 - API change + test changes

(In reply to comment #3)
> Comment on attachment 476921 [details] [diff] [review]
> Looks correct but please fix all instances and not only for this test.
Fixed for the awesomebar tests [3 of them] which keep the popup open on test finish. Any other changes in those tests are being taken care in the local-data conversion.
> >+  closeResults : function autoCompleteResults_closeResults() {
> 
> Simply call it close. There is no need to add 'Results'. The class name speaks
> for itself.
Done 
> >+    var results = this.getElement({type: "popup"});
> 
> We have a popup here. Please name the variable appropriately.
s/results/popup
> >+    var stateOpen = results.isOpened;
> 
> While you are already on it, please also rename the isOpened attribute to
> isOpen.
Renamed all instances in API and awesomebar tests
> >+    this._controller.keypress(null, "VK_ESCAPE", {});
> 
> Don't fire the event against the window but the popup itself. Otherwise
> unexpected behavior can happen. Doing so probably makes the check for isOpen
> unnecessary.
s/null/popup
> >+    this._controller.waitForEval("subject.isOpened == " + stateOpen,
> >+                                 TIMEOUT, 100, results);
> 
> Please pass in an object with a better description.
s/results/popup

Comment 5

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

Comment on attachment 476956 [details] [diff] [review]
V1.1 - API change + test changes

>-var setupModule = function(module)
>-{
>-  module.controller = mozmill.getBrowserController();
>-  module.locationBar =  new ToolbarAPI.locationBar(controller);
>+var setupModule = function() {
>+  controller = mozmill.getBrowserController();
>+  locationBar =  new ToolbarAPI.locationBar(controller);

In the future please only modify code which is related to the appropriate bug. We should do the same for all the other local test data work and have things like this changed in its own bug across the whole repository.

>+   * Closes and checks the state of the auto-complete results
>+   */
>+  close : function autoCompleteResults_closeResults() {
>+    var popup = this.getElement({type: "popup"});
>+    var stateOpen = popup.isOpen;
>+    this._controller.keypress(popup, "VK_ESCAPE", {});
>+    this._controller.waitForEval("subject.isOpen == " + stateOpen,
>+                                 TIMEOUT, 100, popup);

Have you tested this function? The waitForEval call still doesn't make sense because it finishes immediately. Always run negative tests. Also as said in the last comment please pass in an object as last parameter with a better description.

Comment 6

[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.

Comment 7

[Samvedana (:Samvedana)]

Hi Andreea,

Can I start working on this bug?

I saw the code of testFaviconInAutocomplete.js. Can I use following code of it to check the autocomplete list is closed?

 // Ensure the autocomplete list is open
  assert.waitFor(function () {
    return locationBar.autoCompleteResults.isOpened;
  }, "Autocomplete list has been opened");

Comment 8

[Andreea Matei [:AndreeaMatei]]

Hi Samvedana, sure you can!

From what I see in this bug we also need the action to close the autocomplete list, besides the check. If you've already done that and the above code is used for checking opening of autocomplete, then denying it will return you the correct state.

You can test it by using dump statements like:
dump("Autocomplete state isOpen " + locationBar.autoCompleteResults.isOpened + "\n");

But don't forget to remove them before you add the final patch.
You can find us on the #automation channel if you have more questions. I will assign you to this bug now.

Comment 9

[Samvedana (:Samvedana)]

Attachment: testFaviconInAutocompleteCleanClose.js

Hi Andreea,
The code already have close function as below:

 locationBar.autoCompleteResults.close();

So, I just added verification for it. Please let me know if the attached code require any addition or changes.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Thanks Samvedana, but looking more into this I think this bug is invalid now. I haven't checked the library at first, but now I see the close method which already ensures it's closed with the assert.waitFor:

http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/toolbars.js#l213

More than that, the test itself ensures of that in the teardownModule() method in case we fail in the test.

Henrik/Dave, is there something I'm missing here? We want the assert in the tests as well?

Comment 11

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

I don't anything else is necessary here.

Comment 12

[Andreea Matei [:AndreeaMatei]]

Marked as invalid as I don't seem to find the bug where this was introduced. 
Samvedana, you can look at this list of bugs:
* bug 849962
* bug 816084
* bug 882137

And please check this regarding the creation of patches. We upload patches for review/feedback, not the .js files.
https://developer.mozilla.org/en-US/docs/Mercurial_Queues

Thanks!

Comment 13

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

This bug is not invalid! It was filed before the current code has been landed. So this is WFM as usual.