585762 - Make Mozmill-test testBasicFormCompletion local

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

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

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

NOTE: On default, the lookup string used has changed over 1.9.2 and 1.9.1

Comment 2

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (1.9.2)

Comment 3

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (1.9.1)

Comment 4

[u279076]

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

>+  // Fill out the name field with the input text and click Submit
>+  controller.type(inputField, inputText);
>+  controller.sleep(0);
> 

A sleep of 0 is pointless, please remove it.

Comment 5

[u279076]

Comment on attachment 465722 [details] [diff] [review]
Patch v1 - (1.9.2)

>+  // Fill out the name field with the input text and click Submit
>+  controller.type(inputField, inputText);
>+  controller.sleep(0);
> 

A sleep of 0 is pointless, please remove it.

Comment 6

[u279076]

Comment on attachment 465723 [details] [diff] [review]
Patch v1 - (1.9.1)

>+  // Fill out the name field with the input text and click Submit
>+  controller.type(inputField, inputText);
>+  controller.sleep(0);
> 

A sleep of 0 is pointless, please remove it.

Comment 7

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - (default)

Comment 8

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - (1.9.2)

Comment 9

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - (1.9.1)

Comment 10

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

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

Please don't ask for review for backport patches until the default one has been gotten r+. Thanks.

> var setupModule = function(module) {
>   module.controller = mozmill.getBrowserController();

Please remove all instances of module from setupModule. Also the parameter.

>+  controller.type(inputField, inputText);
>+  controller.click(new elementslib.ID(controller.tabs.activeTab, "SubmitButton"));

Please declare the element separately.

>-  var popDownAutoCompList = new elementslib.Lookup(controller.window.document, '/id("main-window")/id("mainPopupSet")/id("PopupAutoComplete")/anon({"anonid":"tree"})/{"class":"autocomplete-treebody"}');
>+ var popDownAutoCompList = new elementslib.Lookup(controller.window.document, 
>+  '/id("main-window")/id("tab-view-deck")/{"flex":"1"}/id("mainPopupSet")' +
>+  '/id("PopupAutoComplete")/anon({"anonid":"tree"})/{"class":"autocomplete-treebody"}'

Given the problems we have right now with those lookup strings we have to get it to work with getAnonymousElementByAttribute. There are a lot of examples in our tree. As attribute you would have to use "class":"autocomplete-treebody" for the element with the id("PopupAutoComplete").

Comment 11

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

(In reply to comment #10)
elementslib.Lookup(controller.window.document, 
> >+  '/id("main-window")/id("tab-view-deck")/{"flex":"1"}/id("mainPopupSet")' +
> >+  '/id("PopupAutoComplete")/anon({"anonid":"tree"})/{"class":"autocomplete-treebody"}'
> 
> Given the problems we have right now with those lookup strings we have to get
> it to work with getAnonymousElementByAttribute. There are a lot of examples in
> our tree. As attribute you would have to use "class":"autocomplete-treebody"
> for the element with the id("PopupAutoComplete").

Leave it that way for now. We will have to discuss what way we want to use in the future.

Comment 12

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (default)

Fair enough. I made the element into a variable and removed the module keywords from the setupModule.

Comment 13

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

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

Due to bug 586997 this patch has been bit-rotted. Please update.

Comment 14

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (default)

Comment 15

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

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

># HG changeset patch
># User Aaron Train <[e-mail]>
># Date 1281739178 14400
># Node ID d69e965b58892721080a416b2bb86bcf2fa12ea7
># Parent  96cee65efb08874e3d6340897d9ab4c2bb92667a
>Bug 585762 - Make Mozmill-test testBasicFormCompletion local. r=ashughes
>
>diff -r 96cee65efb08 -r d69e965b5889 firefox/testFormManager/testBasicFormCompletion.js
>--- a/firefox/testFormManager/testBasicFormCompletion.js	Fri Aug 13 22:43:03 2010 +0200
>+++ b/firefox/testFormManager/testBasicFormCompletion.js	Fri Aug 13 18:39:38 2010 -0400
>@@ -20,6 +20,7 @@
>  * Contributor(s):
>  *   Aakash Desai <[e-mail]>
>  *   Henrik Skupin <[e-mail]>
>+ *   Aaron Train <[e-mail]>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>@@ -35,10 +36,11 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>-const gDelay = 0;
>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../test-files/');
>+const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'form_manager/form.html';
> 
>-var setupModule = function(module) {
>-  module.controller = mozmill.getBrowserController();
>+var setupModule = function() {
>+  controller = mozmill.getBrowserController();
> 
>   try {
>     // Clear complete form history so we don't interfer with already added entries
>@@ -50,33 +52,32 @@
> }
> 
> var testFormCompletion = function() {
>-  var url = 'http://www.mozilla.org/';
>-  var searchText = 'mozillazine';
>+  var inputText = 'John';
> 
>-  // Open the URL and verify it's the correct page
>-  controller.open(url);
>+  // Open the local site and verify it's the correct page
>+  controller.open(LOCAL_TEST_PAGE);
>   controller.waitForPageLoad();
> 
>-  var searchField = new elementslib.ID(controller.tabs.activeTab, "q");
>-  controller.assertNode(searchField);
>+  var inputField = new elementslib.ID(controller.tabs.activeTab, "ship_fname");
>+  controller.assertNode(inputField);
> 
>-  // Perform a search
>-  controller.type(searchField, searchText);
>-  controller.sleep(gDelay);
>-
>-  controller.click(new elementslib.ID(controller.tabs.activeTab, "quick-search-btn"));
>+  // Fill out the name field with the input text: 'John' and click the Submit button
>+  controller.type(inputField, inputText);
>+  
>+  var submitButton = new elementslib.ID(controller.tabs.activeTab, "SubmitButton");
>+  controller.click(submitButton);
>   controller.waitForPageLoad();
> 
>-  // Go to a filler site
>-  controller.open('http://www.yahoo.com/');
>+  // Go to a filler site: about:blank
>+  controller.open("about:blank");
>   controller.waitForPageLoad();
> 
>-  // Go back to the starting page
>-  controller.open(url);
>+  // Go back to the starting local page
>+  controller.open(LOCAL_TEST_PAGE);
>   controller.waitForPageLoad();
> 
>-  // Verify search field element and type in a portion of the field
>-  controller.type(searchField, "mozilla");
>+  // Verify name field element, and type in a portion of the field
>+  controller.type(inputField, inputText);
> 
>   // Select the first element of the drop down
>   var popDownAutoCompList = new elementslib.Lookup(controller.window.document,
>@@ -84,12 +85,12 @@
>                               '/id("mainPopupSet")/id("PopupAutoComplete")' +
>                               '/anon({"anonid":"tree"})/{"class":"autocomplete-treebody"}');
> 
>-  controller.keypress(searchField, "VK_DOWN", {});
>+  controller.keypress(inputField, "VK_DOWN", {});
>   controller.sleep(1000);
>   controller.click(popDownAutoCompList);
> 
>   // Verify the field element and the text in it
>-  controller.assertValue(searchField, searchText);
>+  controller.assertValue(inputField, inputText);
> }
> 
> /**

Comment 16

[u279076]

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/345f6f11c4ea (default)

Does not apply cleanly to mozilla1.9.2 or mozilla1.9.1, please provide a followup patch.

Comment 17

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (1.9.2)

1.9.2

Comment 18

[u279076]

Comment on attachment 466299 [details] [diff] [review]
Patch v3 - (1.9.2)

>   // Select the first element of the drop down
>-  var popDownAutoCompList = new elementslib.Lookup(controller.window.document, '/id("main-window")/id("mainPopupSet")/id("PopupAutoComplete")/anon({"anonid":"tree"})/{"class":"autocomplete-treebody"}');
>+ var popDownAutoCompList = new elementslib.Lookup(controller.window.document, 
>+  '/id("main-window")/id("mainPopupSet")' +
>+  '/id("PopupAutoComplete")/anon({"anonid":"tree"})/{"class":"autocomplete-treebody"}'
>+ );

Please break this string into more lines.

Comment 19

[Aaron Train [:aaronmt]]

Attachment: Patch v3 [nit fix] - (1.9.2)

Comment 20

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

Comment on attachment 468303 [details] [diff] [review]
Patch v3 [nit fix] - (1.9.2)

>+ var popDownAutoCompList = new elementslib.Lookup(controller.window.document, 
>+  '/id("main-window")/id("mainPopupSet")' +
>+  '/id("PopupAutoComplete")/anon({"anonid":"tree"})' + 
>+  '/{"class":"autocomplete-treebody"}'
>+ );

nit: Two spaces please. Also trim it to two lines.

r=me with that change.

Comment 21

[u279076]

(In reply to comment #20)
> Comment on attachment 468303 [details] [diff] [review]
> Patch v3 [nit fix] - (1.9.2)
> 
> >+ var popDownAutoCompList = new elementslib.Lookup(controller.window.document, 
> >+  '/id("main-window")/id("mainPopupSet")' +
> >+  '/id("PopupAutoComplete")/anon({"anonid":"tree"})' + 
> >+  '/{"class":"autocomplete-treebody"}'
> >+ );
> 
> nit: Two spaces please. Also trim it to two lines.
> 
> r=me with that change.

I sort of disagree with that.  Everything after the ( should be on a new line, like a function declaration:

var popDownAutoCompList = new elementslib.Lookup(
  controller.window.document, 
  '/id("main-window")/id("mainPopupSet")/id("PopupAutoComplete")' +
  '/anon({"anonid":"tree"})/{"class":"autocomplete-treebody"}'
);

Breaking down the string into multiple lines should respect the 80char limit as much as possible.  Priority needs to be given to that limitation rather than keeping it to "two lines".

Feedback?

Comment 22

[Aaron Train [:aaronmt]]

Attachment: Patch v3.1 [nit fix] - (1.9.2)

In lieu of the feedback and your comments, I don't think this should be sat for potentially till Monday, so I'm going ahead with a fix here. Adjusted the two spaces from Henrik's comment above, but at the same time I agree with Anthony in that we should abide by the 80 char limit as best we can.

Comment 23

[u279076]

My bad on this one.  The string will fit nicely on 2 lines; I didn't think it would.  I've made the revision prior to check-in.

Comment 24

[u279076]

(In reply to comment #22)
> Created attachment 473700 [details] [diff] [review]
> Patch v3.1 [nit fix] - (1.9.2)

Landed on mozilla1.9.2:
http://hg.mozilla.org/qa/mozmill-tests/rev/2140b48d7c42

Comment 25

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

> var popDownAutoCompList = new elementslib.Lookup(
>   controller.window.document, 
>   '/id("main-window")/id("mainPopupSet")/id("PopupAutoComplete")' +
>   '/anon({"anonid":"tree"})/{"class":"autocomplete-treebody"}'
> );
>
> Feedback?

Sorry for being late in the game. As I have seen in other tests on mozilla-central we shouldn't use 2 spaces at the beginning of the line. That can only be used when we have a function call. In that case we have an assignment and having 2 spaces makes it not discoverable. Instead the following should be used:

> var popDownAutoCompList = new elementslib.Lookup(
>                             controller.window.document, 
>                             '/id("main-window")' +
>                             '/id("mainPopupSet")' +
>                             '/id("PopupAutoComplete")' +
>                             '/anon({"anonid":"tree"})' +
>                             '/{"class":"autocomplete-treebody"}'
> );

Can we update it on default and 1.9.2 and having a patch for 1.9.1 with it already included?

Comment 26

[Aaron Train [:aaronmt]]

Attachment: Patch [indent fix] - (default)

Comment 27

[Aaron Train [:aaronmt]]

Attachment: Patch [indent fix] - (1.9.2)

Comment 28

[Aaron Train [:aaronmt]]

Attachment: Patch [indent fix] - (1.9.1)

Comment 29

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

Comment on attachment 477516 [details] [diff] [review]
Patch [indent fix] - (1.9.1)

Should the 1.9.1 patch be a combined one?

Comment 30

[Aaron Train [:aaronmt]]

Did you want to convert the 1.9.1 to local-data or leave it be? Wasn't sure

Comment 31

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

Is there a reason why we don't want to convert this test?

Comment 32

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

Comment on attachment 477514 [details] [diff] [review]
Patch [indent fix] - (1.9.2)

>+  var popDownAutoCompList = new elementslib.Lookup(
>+                              controller.window.document, 
>+                              '/id("main-window")' + 
>+                              '/id("mainPopupSet")' + 
>+                              '/id("PopupAutoComplete")' +
>+                              '/anon({"anonid":"tree"})' + 
>+                              '/{"class":"autocomplete-treebody"}'
>   );

The closing bracket should not be at the beginning of the line. We have an assignment here and that's totally confusing. Put it under 'n'new.

r=me with that change.

Comment 33

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

Same applies for the default patch. Sorry, missed that.

Comment 34

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (1.9.1) + [indent fix]

Comment 35

[u279076]

(In reply to comment #34)
> Created attachment 477932 [details] [diff] [review]
> Patch v3 - (1.9.1) + [indent fix]

Landed on mozilla1.9.1:
http://hg.mozilla.org/qa/mozmill-tests/rev/993b4f60177d

Comment 36

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