Make Mozmill-test testBasicFormCompletion local

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: ashughes, Assigned: aaronmt)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [litmus-data])

Attachments

(6 attachments, 9 obsolete attachments)

3.57 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
3.73 KB, patch
ashughes
: review+
whimboo
: review+
Details | Diff | Splinter Review
3.74 KB, patch
Details | Diff | Splinter Review
1.36 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
1.33 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
4.14 KB, patch
ashughes
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
Module: testFormManager/testBasicFormCompletion.js
Test-page: test-files/form_manager/form.html
(Reporter)

Updated

8 years ago
Blocks: 579965
(Assignee)

Updated

8 years ago
Assignee: nobody → aaron.train
Status: NEW → ASSIGNED
(Assignee)

Comment 1

8 years ago
Created attachment 465720 [details] [diff] [review]
Patch v1 - (default)

NOTE: On default, the lookup string used has changed over 1.9.2 and 1.9.1
Attachment #465720 - Flags: review?(anthony.s.hughes)
(Assignee)

Comment 2

8 years ago
Created attachment 465722 [details] [diff] [review]
Patch v1 - (1.9.2)
Attachment #465722 - Flags: review?(anthony.s.hughes)
(Assignee)

Comment 3

8 years ago
Created attachment 465723 [details] [diff] [review]
Patch v1 - (1.9.1)
Attachment #465723 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 4

8 years ago
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.
Attachment #465720 - Flags: review?(anthony.s.hughes) → review-
(Reporter)

Comment 5

8 years ago
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.
Attachment #465722 - Flags: review?(anthony.s.hughes) → review-
(Reporter)

Comment 6

8 years ago
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.
Attachment #465723 - Flags: review?(anthony.s.hughes) → review-
(Assignee)

Comment 7

8 years ago
Created attachment 465730 [details] [diff] [review]
Patch v2 - (default)
Attachment #465720 - Attachment is obsolete: true
Attachment #465730 - Flags: review?(anthony.s.hughes)
(Assignee)

Updated

8 years ago
Attachment #465730 - Attachment description: Patch v2 - (1.9.1) → Patch v2 - (default)
(Assignee)

Comment 8

8 years ago
Created attachment 465732 [details] [diff] [review]
Patch v2 - (1.9.2)
Attachment #465722 - Attachment is obsolete: true
Attachment #465732 - Flags: review?(anthony.s.hughes)
(Assignee)

Comment 9

8 years ago
Created attachment 465733 [details] [diff] [review]
Patch v2 - (1.9.1)
Attachment #465723 - Attachment is obsolete: true
Attachment #465733 - Flags: review?(anthony.s.hughes)
(Reporter)

Updated

8 years ago
Attachment #465730 - Flags: review?(hskupin)
Attachment #465730 - Flags: review?(anthony.s.hughes)
Attachment #465730 - Flags: review+
(Reporter)

Updated

8 years ago
Attachment #465732 - Flags: review?(hskupin)
Attachment #465732 - Flags: review?(anthony.s.hughes)
Attachment #465732 - Flags: review+
(Reporter)

Updated

8 years ago
Attachment #465733 - Flags: review?(hskupin)
Attachment #465733 - Flags: review?(anthony.s.hughes)
Attachment #465733 - Flags: review+
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").
Attachment #465730 - Flags: review?(hskupin) → review-
Attachment #465732 - Flags: review?(hskupin)
Attachment #465733 - Flags: review?(hskupin) → review-
Attachment #465732 - Flags: review-
(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.
(Assignee)

Comment 12

8 years ago
Created attachment 465749 [details] [diff] [review]
Patch v3 - (default)

Fair enough. I made the element into a variable and removed the module keywords from the setupModule.
Attachment #465730 - Attachment is obsolete: true
Attachment #465732 - Attachment is obsolete: true
Attachment #465733 - Attachment is obsolete: true
Attachment #465749 - Flags: review?(hskupin)
Comment on attachment 465749 [details] [diff] [review]
Patch v3 - (default)

Due to bug 586997 this patch has been bit-rotted. Please update.
Attachment #465749 - Flags: review?(hskupin)
(Assignee)

Comment 14

8 years ago
Created attachment 465869 [details] [diff] [review]
Patch v3 - (default)
Attachment #465749 - Attachment is obsolete: true
Attachment #465869 - Flags: review?(hskupin)
Comment on attachment 465869 [details] [diff] [review]
Patch v3 - (default)

># HG changeset patch
># User Aaron Train <atrain@mozilla.com>
># 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 <adesai@mozilla.com>
>  *   Henrik Skupin <hskupin@mozilla.com>
>+ *   Aaron Train <atrain@mozilla.com>
>  *
>  * 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);
> }
> 
> /**
Attachment #465869 - Flags: review?(hskupin) → review+
(Reporter)

Comment 16

8 years ago
> 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.
(Assignee)

Comment 17

8 years ago
Created attachment 466299 [details] [diff] [review]
Patch v3 - (1.9.2)

1.9.2
Attachment #466299 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 18

8 years ago
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.
Attachment #466299 - Flags: review?(anthony.s.hughes) → review-
(Assignee)

Comment 19

8 years ago
Created attachment 468303 [details] [diff] [review]
Patch v3 [nit fix] - (1.9.2)
Attachment #466299 - Attachment is obsolete: true
Attachment #468303 - Flags: review?(anthony.s.hughes)
(Reporter)

Updated

8 years ago
Attachment #468303 - Flags: review?(hskupin)
Attachment #468303 - Flags: review?(anthony.s.hughes)
Attachment #468303 - Flags: review+
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.
Attachment #468303 - Flags: review?(hskupin) → review+
Keywords: checkin-needed
Keywords: checkin-needed
(Reporter)

Comment 21

8 years ago
(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?
(Assignee)

Comment 22

8 years ago
Created attachment 473700 [details] [diff] [review]
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.
Attachment #473700 - Flags: review?(anthony.s.hughes)
(Assignee)

Updated

8 years ago
Attachment #473700 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 23

8 years ago
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.
(Reporter)

Comment 24

8 years ago
(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
> 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?
(Assignee)

Comment 26

8 years ago
Created attachment 477513 [details] [diff] [review]
Patch [indent fix] - (default)
Attachment #477513 - Flags: review?(hskupin)
(Assignee)

Comment 27

8 years ago
Created attachment 477514 [details] [diff] [review]
Patch [indent fix] - (1.9.2)
(Assignee)

Comment 28

8 years ago
Created attachment 477516 [details] [diff] [review]
Patch [indent fix] - (1.9.1)
(Reporter)

Updated

8 years ago
Keywords: checkin-needed
Attachment #477513 - Flags: review?(hskupin) → review+
Comment on attachment 477516 [details] [diff] [review]
Patch [indent fix] - (1.9.1)

Should the 1.9.1 patch be a combined one?
(Assignee)

Comment 30

8 years ago
Did you want to convert the 1.9.1 to local-data or leave it be? Wasn't sure
(Assignee)

Updated

8 years ago
Attachment #477514 - Flags: review?(hskupin)
Is there a reason why we don't want to convert this test?
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.
Attachment #477514 - Flags: review?(hskupin) → review+
Same applies for the default patch. Sorry, missed that.
Keywords: checkin-needed
(Assignee)

Comment 34

8 years ago
Created attachment 477932 [details] [diff] [review]
Patch v3 - (1.9.1) + [indent fix]
Attachment #477516 - Attachment is obsolete: true
Attachment #477932 - Flags: review?(anthony.s.hughes)
(Reporter)

Updated

8 years ago
Attachment #477932 - Flags: review?(anthony.s.hughes) → review+
(Reporter)

Updated

8 years ago
Keywords: checkin-needed
(Reporter)

Comment 35

8 years ago
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Component: Mozmill Tests → Mozmill Tests
Product: Testing → Mozilla QA
You need to log in before you can comment on or make changes to this bug.