585764 - Make Mozmill-test testClearFormHistory local

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Module: testFormManager/testClearFormHistory.js
Test-page: test-files/form_manager/form.html

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

cleanup + converts test to make use of local-data

Comment 2

[u279076]

Comment on attachment 467425 [details] [diff] [review]
Patch v1 - (default)

I notice a common flow in this test:
type()
sleep()
assert()

Can you make the test work by using assertJS (or waitForEval) with a timeout?
type()
assertJS()

Comment 3

[Aaron Train [:aaronmt]]

Henrik, what do you suggest?

AFAIK, each time we type we sleep for the popup auto complete listing to open. This will change to assertDOMProperty

Comment 4

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

Well, sleep is necessary as far as I can remember. So we would have to use waitFor or waitForEval.

Comment 5

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - (default)

Replaced existing sleep() calls with waitFor calls that check the DOM property popupOpen; a boolean that signifies the visibility.

I've never used waitFor before so it seems like a good opportunity here.

One other misc question for this test:

Do we want create a remove form history function in the Places API? Presently, the Places API has a function removeAllHistory(), which removes *everything* -- do we desire to eventually break this down?

Comment 6

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

Comment on attachment 471849 [details] [diff] [review]
Patch v2 - (default)

>+  controller.waitFor(function() { return popDownAutoCompList.getNode().popupOpen == true; }, TIMEOUT, 100,
>+                     "Popup Autocomplete should be visible");

Take the opportunity and use multiple lines for the calls like:

>+  controller.waitFor(function() { 
>+    return popDownAutoCompList.getNode().popupOpen == true;
>+  }, TIMEOUT, 100, "Locationbar auto-complete popup should be visible");

Also note that the message is optional. If none is given the content of the callback function is used. Depends on the situation which is the best solution.

Comment 7

[Aaron Train [:aaronmt]]

Attachment: Patch v2.1 - (default)

Fixed indent and removed optional message because the state of the property can be determined through the content of the callback function.

Comment 8

[u279076]

Comment on attachment 471868 [details] [diff] [review]
Patch v2.1 - (default)

>+  var checkBox = new elementslib.XPath(controller.window.document, 
>+                                       "/*[name()='prefwindow']/*[name()='prefpane'][1]" + 
>+                                       "/*[name()='listbox'][1]/*[name()='listitem'][2]");

Better break this out into more lines on the /.

Comment 9

[Aaron Train [:aaronmt]]

Attachment: Patch v2.2 - [nit fix] (default)

Comment 10

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

Comment on attachment 473508 [details] [diff] [review]
Patch v2.2 - [nit fix] (default)

>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../test-files/');
>+const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'form_manager/form.html';
>+const FNAME = "John";
>+const LNAME = "Smith";

Please separate the page declarations from the strings.

>+var setupModule = function()
> {

Brackets cleanups please.

>+  var submitButton = new elementslib.ID(controller.tabs.activeTab, "SubmitButton");

Please move the declaration up to the name elements.

>+  controller.type(firstName, FNAME.substring(0,2));
>+  controller.waitFor(function() {
>+    return popDownAutoCompList.getNode().popupOpen == true;
>+  }, TIMEOUT, 100);
>   controller.assertProperty(popDownAutoCompList, "popupOpen", true);

No need for assertProperty here. Also please always use a message for the waitFor function. It will be much clearer in the report as seeing the code which will even be available via the stack. 

>+  controller.waitFor(function() {
>+    return popDownAutoCompList.getNode().popupOpen == true;
>+  }, TIMEOUT, 100);
>+  controller.assertProperty(popDownAutoCompList, "popupOpen", true);  

Same here.

> var testClearFormHistory = function()

This function is one case why I don't like to have multiple test functions inside a module. It is absolutely tied to the first function in this test. If it fails, testClearFormHistory will fail too because no form data has been saved. We should absolutely prepare that part for this test. Both test functions can use a helper function for that.

>-  controller.sleep(500);
>+  controller.waitFor(function() {
>+    return popDownAutoCompList.getNode().popupOpen == false;
>+  }, TIMEOUT, 100);

This will always pass. Please remember to run negative tests. Waiting for popupOpen will return immediately. We will need better code here.

>-  controller.sleep(500);
>+  controller.waitFor(function() {
>+    return popDownAutoCompList.getNode().popupOpen == false;
>+  }, TIMEOUT, 100);
>   controller.assertProperty(popDownAutoCompList, "popupOpen", false);

Same here.

Comment 11

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (default)

>Please separate the page declarations from the strings.
Done

>Brackets cleanups please.
Done

>Please move the declaration up to the name elements.
Done

>No need for assertProperty here. Also please always use a message for the
>waitFor function. It will be much clearer in the report as seeing the code
>which will even be available via the stack. 
Done

>This function is one case why I don't like to have multiple test functions
>inside a module. It is absolutely tied to the first function in this test. If
>it fails, testClearFormHistory will fail too because no form data has been
>saved. We should absolutely prepare that part for this test. Both test
>functions can use a helper function for that.
For now let's just take this offline and file a followup

>This will always pass. Please remember to run negative tests. Waiting for
>popupOpen will return immediately. We will need better code here.

True, reverted to simply checking the popupOpen property after typing and re-inserted the sleep after the type to give it a chance to determine if the popup opens

Comment 12

[u279076]

Comment on attachment 477921 [details] [diff] [review]
Patch v3 - (default)

>+  var checkBox = new elementslib.XPath(controller.window.document, 
>+                                       "/*[name()='prefwindow']" +
>+                                       "/*[name()='prefpane'][1]" +
>+                                       "/*[name()='listbox'][1]" +
>+                                       "/*[name()='listitem'][2]");

>+  var clearButton = new elementslib.Lookup(controller.window.document,
>+                                           '/id("SanitizeDialog")' +
>+                                           '/anon({"anonid":"dlg-buttons"})' +
>+                                           '/{"dlgtype":"accept"}');

Parameters should begin on a newline and indented 2 spaces for 'n' in 'new' in both of these.  I'll fix prior to checkin.

Over to Henrik for final review.

Comment 13

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

Comment on attachment 477921 [details] [diff] [review]
Patch v3 - (default)

>+  controller.waitFor(function() {
>+    return popDownAutoCompList.getNode().popupOpen == true;
>+  }, TIMEOUT, 100, "Autocomplete popup is not open");

The message should contain the expected state not the failing state.

>+  controller.waitFor(function() {
>+    return popDownAutoCompList.getNode().popupOpen == true;
>+  }, TIMEOUT, 100, "Autocomplete popup is not open");

Same here.

>-var testClearFormHistory = function()

We should move this function out into its own module. Please file a bug for it.

r=me with both messages updated.

Comment 14

[u279076]

I'll take care of this during check-in.

Comment 15

[u279076]

Attachment: Patch v3.1 (default)

Nits addressed

Comment 16

[u279076]

(In reply to comment #15)
> Created attachment 478070 [details] [diff] [review]
> Patch v3.1 (default)
> 
> Nits addressed

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/1c87042cb6db [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/b1ccbeff37f8 [mozilla1.9.2]

Please provide a patch for mozilla1.9.1

Comment 17

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (1.9.1)

Comment 18

[u279076]

(In reply to comment #13)
> -var testClearFormHistory = function()
> 
> We should move this function out into its own module. Please file a bug for it.

Filed -> bug 599142

Comment 19

[u279076]

(In reply to comment #17)
> Created attachment 478083 [details] [diff] [review]
> Patch v3 - (1.9.1)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/26ab46dd4f3b [mozilla1.9.1]

Comment 20

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

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.