519475 - [mozmill] Access the Location bar with drop down list

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Tracy Walker [:tracy]]

write mozmill test case for Litmus "Access the Location bar with drop down list"
test case 8024

Comment 1

[Tracy Walker [:tracy]]

Attachment: test case for Access the Location Bar with drop down list

Comment 2

[Aakash Desai [:aakashd]]

Comment on attachment 403911 [details] [diff] [review]
test case for Access the Location Bar with drop down list


>+  // Call the clear recent history dialog to clear recent history (only clears last hour by default) This is necessary to create a known history list.
>+  var clearHistoryHandler = function (controller) {
>+     var clearButton = new elementslib.Lookup(controller.window.document, '/id("SanitizeDialog")/anon({"anonid":"dlg-buttons"})/{"dlgtype":"accept"}');
>+     controller.waitThenClick(clearButton);

What we've been doing instead of clearing form history in the testcase, try to use the setupModule like the one used in testFormManager/testClearFormHistory.js ....it's like in the following:

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

  // Clear complete form history so we don't interfere with already added entries
  try {
    var formHistory = Cc["@mozilla.org/satchel/form-history;1"].
                      getService(Ci.nsIFormHistory2);
    formHistory.removeAllEntries();
  } catch (ex) {}
}



>+  // Open a few different sites to create a small history 
>+  controller.open("http://www.google.com");
>+  controller.waitForPageLoad();
>+  controller.open("http://www.mozilla.org");
>+  controller.waitForPageLoad();
>+  controller.open("http://www.getpersonas.com");
>+  controller.waitForPageLoad();

In order to scale this up in future versions of this testcase, I'd suggest using a for loop with an array of these url's instanced before the beginning of the testscript. If you want an example, check out testGeneral/testBackForwardButtons.js



>+  controller.assertJS(locationBar.getNode().value.indexOf("getpersonas") !== -1);

This actually failed for me on WinXP because the the mozilla start up pages for namoroka were in the awesome bar results, so it linked to another page than get personas. Remember to test your testscript against some of our nightly builds/beta releases because they do things differently per testscript  compared to Firefox releases.

Comment 3

[Tracy Walker [:tracy]]

Attachment: reworked test case for Access the Location Bar with drop down list

implemented historyService for the cleanup method and reworked the page loading sequence as requested. I also had to make a couple other changes to ensure proper behavior of the autocomplete list.  (that stuff is timing touchy, thus the brief waits)

Comment 4

[Aakash Desai [:aakashd]]

Attachment: patch_reviewed

Hey Tracy, I reviewed your testscript and its a r+ after the changes I made to put it in line with code conventions (i.e. changing gDelay=0 and all sleeps to be 500) as well removing some comments that were misspelled or were run-ons. Also, I moved the try and catch statement to the setupModule as its not really a part of a test more than it is setuping up the running of the test. Tell me what you think and if you want any changes, otherwise we're good to go (send to henrik for review).

Comment 5

[Tracy Walker [:tracy]]

hmmm, why hard code each delay time?  Consider that perhaps the delays, for whatever reason, need to be longer or shorter.  By hard coding them, the delays would have to be changed individually.  Using the constant value makes the change a one line fix.

Comment 6

[Tracy Walker [:tracy]]

I suppose the code for page loading to create known history isn't really part of the test case either.  It probably belongs in setup as well.

Comment 7

[Aakash Desai [:aakashd]]

We've been using 500 ms as a defacto time to deal with sleeps that we don't know how to deal with yet. By leaving them hard-coded, it'll be easier to change them later once we've figured out a better implementation for stuff like this. The precedence was set 1-2 months ago when we started using gTimeout = 1000 as a global variable. 

As for the page loads, keep those in the tests because that's a part of the steps to reproduce.

Comment 8

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

Comment on attachment 404161 [details] [diff] [review]
patch_reviewed

I just had a quick look at the patch. Here some comments...

>+  // Open a few different sites to create a small history
>+  for (var k = 0; k < websites.length; k++) {
>+    controller.open(websites[k]);
>+    controller.waitForPageLoad();
>+    controller.sleep(500);
>+  }

No sleep is needed here. Please drop it completely.

>+  // First - Focus the locationbar then delete any contents there
>+  controller.keypress(null, "l", {accelKey: true});
>+  controller.keypress(locationBar, "VK_DELETE", {});
>+  controller.sleep(500);

There is no need for the first keypress if you are operating on the location bar directly in the second call. It gets automatically focused. Why we need 500ms for sleep here? It should be fine by using gDelay with 0ms to fire the event. We really have to get in the fix for Mozmill itself so we don't have to use the sleep call in each of our tests. :/

>+  // Second - Arrow down to open the history list (displays most recent visit first)
>+  controller.keypress(null, "VK_DOWN", {});
>+  controller.sleep(500);

Sleep calls are always bad as what I have noticed lately when your system is slow. Tracy, as what I can see is that you just want to wait until the next entry in the popup is selected? you should better use a waitForEval call with the target index as subject like:

var richlistbox = new elementslib.ID(controller.window.document, "PopupAutoCompleteRichResult");

controller.waitForEval("subject.selectedIndex == 0", gTimeout, 100, richlistbox.getNode());

See also https://developer.mozilla.org/en/XUL/richlistbox

>+  controller.keypress(null, "VK_DOWN", {});
>+  controller.sleep(500);

Same as above but with '== 1".

>+  controller.assertJS(locationBar.getNode().value.indexOf("getpersonas") !== -1);

You should compare the locationbar value with the url member of the selected richlistbox item directly.

>+  controller.keypress(locationBar, "VK_RETURN", {});

Can you please use null here too? We wanna hit return on the selected item and not on the location bar.

>+  // Check that getpersonas is in the url bar
>+  controller.assertJS(locationBar.getNode().value.indexOf("getpersonas") !== -1);

Same url comparison as above is more secure.

Comment 9

[Tracy Walker [:tracy]]

 > >+  // Open a few different sites to create a small history
> >+  for (var k = 0; k < websites.length; k++) {
> >+    controller.open(websites[k]);
> >+    controller.waitForPageLoad();
> >+    controller.sleep(500);
> >+  }
> 
> No sleep is needed here. Please drop it completely.

There is something about mozmill loading pages in succession that causes page loading to unexpectedly/randomly stall. I know it's counter intuitive, but having a sleep there actually makes the page loading run much faster. Try it both ways to see the difference.

> 
> >+  // First - Focus the locationbar then delete any contents there
> >+  controller.keypress(null, "l", {accelKey: true});
> >+  controller.keypress(locationBar, "VK_DELETE", {});
> >+  controller.sleep(500);
> 
> There is no need for the first keypress if you are operating on the location
> bar directly in the second call. It gets automatically focused. 
Using accel key "l" resets focus to the url bar and thus selects it's entire contents, which we then delete on the next keystroke.  

Why we need
> 500ms for sleep here? It should be fine by using gDelay with 0ms to fire the
> event. 

Firefox needs a bit of time to generate the dropdownlist.  Without the sleeps mozmill enters keystrokes too quickly. In that case, the test would fail. However, by changing over to the method you suggest below the sleeps should  not be needed.
 
> >+  // Second - Arrow down to open the history list (displays most recent visit first)
> >+  controller.keypress(null, "VK_DOWN", {});
> >+  controller.sleep(500);
> 
> Sleep calls are always bad as what I have noticed lately when your system is
> slow. Tracy, as what I can see is that you just want to wait until the next
> entry in the popup is selected? you should better use a waitForEval call with
> the target index as subject like:
> 
> var richlistbox = new elementslib.ID(controller.window.document,
> "PopupAutoCompleteRichResult");
> 
> controller.waitForEval("subject.selectedIndex == 0", gTimeout, 100,
> richlistbox.getNode());
> 

As given that doesn't work.  I'm researching to understand how to correctly implement that.

> See also https://developer.mozilla.org/en/XUL/richlistbox
> 
> >+  controller.keypress(null, "VK_DOWN", {});
> >+  controller.sleep(500);
> 
> Same as above but with '== 1".
> 
> >+  controller.assertJS(locationBar.getNode().value.indexOf("getpersonas") !== -1);
> 
> You should compare the locationbar value with the url member of the selected
> richlistbox item directly.
> 
> >+  controller.keypress(locationBar, "VK_RETURN", {});
> 
> Can you please use null here too? We wanna hit return on the selected item and
> not on the location bar.

Actually, the key press "return" must go to the location bar. Doing so dismisses the drop down list and loads the selected history item.

> 
> >+  // Check that getpersonas is in the url bar
> >+  controller.assertJS(locationBar.getNode().value.indexOf("getpersonas") !== -1);
> 
> Same url comparison as above is more secure.

No, the test case generates a specific history list so that we know exactly what the contents of the history list should be.  If getpersonas is not the page loaded at the end of the test case, then we have one of two problems; either the getpersonas page never loaded correctly or the history list didn't work properly. Let's assume all the pages loaded properly. If we compare the final loaded page with whatever is the first entry in the dropdownlist (it could incorrectly be something other than getpersonas), the test case will give a false PASS.  No other frecency points have been awarded besides recency,  Since getpersonas was the most recent page loaded, it should be at the top of the list.  Thus getpersonas loaded must be the check for this test case to pass.

Comment 10

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

(In reply to comment #9)
> There is something about mozmill loading pages in succession that causes page
> loading to unexpectedly/randomly stall. I know it's counter intuitive, but
> having a sleep there actually makes the page loading run much faster. Try it
> both ways to see the difference.

You probably had some network issues if page loading stalled. It's nothing about Mozmill. I repeated those steps a couple of times and with 500ms wait the test takes always a bit more then 20s instead of 18s (run from command line).

> > There is no need for the first keypress if you are operating on the location
> > bar directly in the second call. It gets automatically focused. 
>
> Using accel key "l" resets focus to the url bar and thus selects it's entire
> contents, which we then delete on the next keystroke.  

It's not needed. When you are using the element itself as first parameter the focus will automatically be placed on that element. Existing content will be selected.

> As given that doesn't work.  I'm researching to understand how to correctly
> implement that.

I only checked the elements via DOMi but haven't tested it. I hope you will get it working that way. It's the safest solution we really should prefer in favor of sleep.

> Actually, the key press "return" must go to the location bar. Doing so
> dismisses the drop down list and loads the selected history item.

Sorry bad wording from my side. The focus will be in the location bar but the selection moves in the dropdown list. Using null as first parameter always operates on the currently focus element. That way we can make sure navigating through the entries in the drop down menu doesn't remove the focus from the location bar.

> a false PASS.  No other frecency points have been awarded besides recency, 
> Since getpersonas was the most recent page loaded, it should be at the top of
> the list.  Thus getpersonas loaded must be the check for this test case to
> pass.

The given Litmus test doesn't talk about frequency only about that the selected page has been loaded correctly. Isn't the frequency part covered in another test?

Comment 11

[Tracy Walker [:tracy]]

  // Second - Arrow down to open the history list (displays most recent visit first), then arrow down again to the first entry, in this case www.getpersonas.com;
  controller.keypress(null, "VK_DOWN", {});
  // Henrik's suggested method for list entry access
  var richlistbox = new elementslib.ID(controller.window.document, "PopupAutoCompleteRichResult");
  controller.waitForEval("subject.selectedIndex == 1", gTimeout, 100, richlistbox.getNode());
  controller.keypress(null, "VK_DOWN", {});
  // The last unsuccessful guess I made at list item access
  var richlistbox = new elementslib.ID(controller.window.document, "panel1245599688556");
  try {
    controller.waitForEval("subject.selectedIndex == 1", gTimeout, 100, richlistbox.getNode());
  }
  catch (e) {}

Comment 12

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

(In reply to comment #11)
>   // The last unsuccessful guess I made at list item access
>   var richlistbox = new elementslib.ID(controller.window.document,
> "panel1245599688556");
>   try {
>     controller.waitForEval("subject.selectedIndex == 1", gTimeout, 100,
> richlistbox.getNode());
>   }
>   catch (e) {}

That's definitely not the richlistbox. panel1245599688556 looks like to be the notification bar for me. What you really wanna have is the following:

>  var richlistbox = new elementslib.Lookup(controller.window.document, '/id("main-window")/id("mainPopupSet")/id("PopupAutoCompleteRichResult")/anon({"anonid":"richlistbox"})');
>  controller.window.alert(richlistbox.getNode().itemCount);

For the future you can always use getNode().localName to check the type of your element. I hope that helps.

Comment 13

[Tracy Walker [:tracy]]

Attachment: testAccessLocationBar

Comment 14

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

Comment on attachment 412871 [details]
testAccessLocationBar

Tracy, can you please attach the patch instead of the original file please?

Comment 15

[Tracy Walker [:tracy]]

Attachment: patchVersion_AccessLocationBar

sorry, forgot about that requirement.

Comment 16

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

Comment on attachment 413078 [details] [diff] [review]
patchVersion_AccessLocationBar

Tracy, you should upgrade Mozmill to 1.3. There were a couple of fixes and new features which make this test fail. I hope you are ok, that I have updated those parts. It's working on all platforms now. You can use it as a reference for all remaining tests.

>+  for (var k = 0; k < websites.length; k++) {
>+    controller.open(websites[k]);

Tests which directly involve the location bar shouldn't use the open method because no history will be build up now. Just use .type and pressing return afterward.

>+  try {
>+    controller.waitForEval("subject.selectedIndex == 0", gTimeout, 100, richlistbox.getNode().selectedIndex);
>+  }
>+  catch (e) {}

That shouldn't be inside the try/catch. We need the exception if the first entry is not selected.

>+  controller.assertJS(locationBar.getNode().value.indexOf("getpersonas") !== -1);

I have updated that part too. We can use the same style as for waitForEval now to make it more descriptive.

I will upload the fixed things right now.

Comment 17

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

Attachment: Patch (corrected)

Comment 18

[Tracy Walker [:tracy]]

(In reply to comment #16)
> I hope you are ok, that I have updated those parts.
> 
whatever you need to do is fine with me, with notes in-line:

> >+  for (var k = 0; k < websites.length; k++) {
> >+    controller.open(websites[k]);
> 
> Tests which directly involve the location bar shouldn't use the open method
> because no history will be build up now. Just use .type and pressing return
> afterward.

That's the method I initially preferred for any entry into the URL bar.  It more closely emulates a humans action.  But you denied review of previous test cases based on that method.  Now you've flip flopped preferred technique.

> 
> >+  try {
> >+    controller.waitForEval("subject.selectedIndex == 0", gTimeout, 100, richlistbox.getNode().selectedIndex);
> >+  }
> >+  catch (e) {}
> 
> That shouldn't be inside the try/catch. We need the exception if the first
> entry is not selected.
> 
> >+  controller.assertJS(locationBar.getNode().value.indexOf("getpersonas") !== -1);
> 
> I have updated that part too. We can use the same style as for waitForEval now
> to make it more descriptive.
> 
 
In what way was that updated?  Why weren't these things pointed out in my previous patches so I could fix them in my last patch?

Comment 19

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

(In reply to comment #18)
> > Tests which directly involve the location bar shouldn't use the open method
> > because no history will be build up now. Just use .type and pressing return
> > afterward.
> 
> That's the method I initially preferred for any entry into the URL bar.  It
> more closely emulates a humans action.  But you denied review of previous test
> cases based on that method.  Now you've flip flopped preferred technique.

The reason is that controller.open has been modified for Mozmill 1.3. Given that change we have to revert it here. I mentioned that in the other location bar test we have checked in two weeks ago.

> > >+  controller.assertJS(locationBar.getNode().value.indexOf("getpersonas") !== -1);
> > 
> > I have updated that part too. We can use the same style as for waitForEval now
> > to make it more descriptive.
> > 
> 
> In what way was that updated?  Why weren't these things pointed out in my
> previous patches so I could fix them in my last patch?

It was one of the last minute patches which went into Mozmill 1.3. So that happened after I gave the last review on your patch.

Comment 20

[Tracy Walker [:tracy]]

henrik, have you checked in the corrected patch?

Comment 21

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

I haven't checked in this patch yet because it waits for a review from you. Your last comment wasn't clear for me that you are ok with the changes.

Comment 22

[Tracy Walker [:tracy]]

Comment on attachment 413172 [details] [diff] [review]
Patch (corrected)

ah, sorry, missed that request.  +,  thanks

Comment 23

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/75b15b3af811
http://hg.mozilla.org/qa/mozmill-tests/rev/880258024637

Comment 24

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

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