500095 - Mozmill test for Find Bar functionality

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

This is a placeholder bug for creating a Mozmill test for the findbar Smoketest in Litmus (https://litmus.mozilla.org/show_test.cgi?id=5927)

Comment 1

[u279076]

Attachment: Test Case: Iteration 1

Here is iteration 1 of the find in page test case.

Comment 2

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

Comment on attachment 385149 [details]
Test Case: Iteration 1

>var teardownModule = function(module) {
>  // Make sure the find bar is closed
>  var findBar = new elementslib.Lookup(controller.window.document, '/id("main-window")/id("browser-bottombox")/id("FindToolbar")/anon({"anonid":"findbar-container"})');
>  if (findBar != null) {
>    var findBarTextField = new elementslib.Lookup(controller.window.document, '/id("main-window")/id("browser-bottombox")/id("FindToolbar")/anon({"anonid":"findbar-container"})/anon({"anonid":"find-field-container"})/anon({"anonid":"findbar-textbox"})');
>    controller.keypress(findBarTextField, 'VK_ESCAPE', {});    

We could use the close button and perform a click on it. You can encapsulate it into a try/catch so no error is thrown if the find bar is not open.

>  // Press CTRL/CMD+F to open the find bar
>  if (mozmill.isMac) {  
>    controller.keypress(null,"f", {ctrlKey:false, altKey:false,shiftKey:false, metaKey:true});
>  } else {
>    controller.keypress(null,"f", {ctrlKey:true, altKey:false,shiftKey:false, metaKey:false});
>  }  

You can use {accelKey: true} which automatically checks the platform. So you can remove the check.

>  // Check that the find bar is visible
>  var findBar = new elementslib.Lookup(controller.window.document, '/id("main-window")/id("browser-bottombox")/id("FindToolbar")/anon({"anonid":"findbar-container"})');
>  UtilsAPI.delayedAssertNode(controller, findBar);

Do we have to check for the existence of the findbar or can we run the delayedAssertNode against the text field?

>  // Click the next button and check the strings again
>  var findBarNextButton = new elementslib.Lookup(controller.window.document, '/id("main-window")/id("browser-bottombox")/id("FindToolbar")/anon({"anonid":"findbar-container"})/anon({"anonid":"find-next"})');
>  controller.click(findBarNextButton);
>  controller.sleep(gDelay);
>  selectedText = controller.tabs.activeTabWindow.getSelection();
>  if (selectedText.toString().toLowerCase() != searchedText) {
>    throw "Searched string does not match Selected string!"
>  }

Eventually we should check that pressing a button moves to the next node. We could use getRangeAt. But I think it's too complex for now. So for now it would be nice to have tests with Return, Shift+Return, F3, and Shift+F3. Remaining things we can check next quarter.

Comment 3

[u279076]

> We could use the close button and perform a click on it. You can encapsulate it
> into a try/catch so no error is thrown if the find bar is not open.
That's more work for the same result.  I don't see the benefit to this change.

> You can use {accelKey: true} which automatically checks the platform. So you
> can remove the check.
Please point me to an example.  I have no idea how to implement that.  

> Do we have to check for the existence of the findbar or can we run the
> delayedAssertNode against the text field?
Checking that the findbar is being displayed is a critical part of this test.  In theory we could check for the findbar text box and assume the findbar is visible, but I see nothing wrong with checking for the findbar itself.  In reality, it is no less code and still one check.

> Eventually we should check that pressing a button moves to the next node. We
> could use getRangeAt. But I think it's too complex for now. So for now it would
> be nice to have tests with Return, Shift+Return, F3, and Shift+F3. Remaining
> things we can check next quarter.

I think what you are asking for here is beyond the scope of the smoketest.  Since the smoketest mentions using ENTER/SHIFT+ENTER to navigate NEXT/PREV I'll add it.  Anything beyond this would be a candidate for BFTs or FFTs.

Comment 4

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

(In reply to comment #3)
> > We could use the close button and perform a click on it. You can encapsulate it
> > into a try/catch so no error is thrown if the find bar is not open.
> That's more work for the same result.  I don't see the benefit to this change.

Why it is more work? It only needs one lookup call. Your used if statement will always be true because elementslib.Lookup returns an object at any time. It will never be null. 

try {
  controller.click(new elementslib.Lookup(controller.window.document, '/id("main-window")/id("browser-bottombox")/id("FindToolbar")/anon({"anonid":"findbar-container"})/anon({"anonid":"find-closebutton"})'));
} catch(e) {
}

> > You can use {accelKey: true} which automatically checks the platform. So you
> > can remove the check.
> Please point me to an example.  I have no idea how to implement that.  

controller.keypress(null, "f", {accelKey:true});

> > Do we have to check for the existence of the findbar or can we run the
> > delayedAssertNode against the text field?
> Checking that the findbar is being displayed is a critical part of this test. 
> In theory we could check for the findbar text box and assume the findbar is
> visible, but I see nothing wrong with checking for the findbar itself.  In
> reality, it is no less code and still one check.

So leave it in.
 
> I think what you are asking for here is beyond the scope of the smoketest. 
> Since the smoketest mentions using ENTER/SHIFT+ENTER to navigate NEXT/PREV I'll
> add it.  Anything beyond this would be a candidate for BFTs or FFTs.

We only check for clicks nothing more. We don't know if the buttons and keypresses really move forward/backward through the selection. Lets punt this to Q3 but it should be solved in this bug. So we can leave it open for a follow-up patch.

Comment 5

[u279076]

Attachment: Patch r0

Here is the first patch with the revisions made.

Comment 6

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

Comment on attachment 385529 [details] [diff] [review]
Patch r0

Looks good. Just some small remaining issues:

>+var MODULE_REQUIRES = ['PrefsAPI', 'UtilsAPI'];

PrefsAPI isn't necessary for this test.

>+const gDelay = 500;

This should be set to 0 for tests we check into the repository.

>+var teardownModule = function(module) {
>+  // Make sure the find bar is closed
>+  var findBar = new elementslib.Lookup(controller.window.document, '/id("main-window")/id("browser-bottombox")/id("FindToolbar")/anon({"anonid":"findbar-container"})');

This line is not needed anymore since we click the close button directly. Further I have noticed that we should clear the find bar text first before closing the bar. Can you insert a VK_DELETE or VK_BACK_SPACE keypress event before clicking the close button?

Comment 7

[u279076]

> Further I have noticed that we should clear the find bar text first before
> closing the bar. Can you insert a VK_DELETE or VK_BACK_SPACE keypress event
> before clicking the close button?

Why?  I do not think this reflects typical user behaviour.  Please explain your reasoning behind this suggestion.

Comment 8

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

Because each test has to leave the browser in a clean state. Other tests could be impacted by an already entered search term in the find bar.

Comment 9

[u279076]

Attachment: Patch r1

Suggested revisions made.  Please review.

Comment 10

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

Comment on attachment 385824 [details] [diff] [review]
Patch r1

Thanks Anthony! That looks good. There are only small two nits which I have already updated. So no new cycle is needed.

>+++ b/firefox/testFindBar/testFindInPage.js

It should be placed under firefox/testFindInPage so it matches our Litmus subgroup.

>+  // Press CTRL/CMD+F to open the find bar
>+  // This also highlights all text in the text field
>+  controller.keypress(null, "f", {accelKey:true});

The find bar is already open so we only use the keypress to select the whole search string. If it is not open we will fail too.

>+  try {
>+    controller.keypress(findBarTextField, 'VK_DELETE', {});
>+  } catch(e) {}
>+  
>+  // Make sure the find bar is closed by click the X button
>+  try {
>+    controller.click(new elementslib.Lookup(controller.window.document, '/id("main-window")/id("browser-bottombox")/id("FindToolbar")/anon({"anonid":"findbar-container"})/anon({"anonid":"find-closebutton"})'));
>+  } catch(e) {}

We can move everything in one try/catch clause. 

>+  // Type "mozilla" into the find bar text field
>+  var findBarTextField = new elementslib.Lookup(controller.window.document, '/id("main-window")/id("browser-bottombox")/id("FindToolbar")/anon({"anonid":"findbar-container"})/anon({"anonid":"find-field-container"})/anon({"anonid":"findbar-textbox"})');
>+  controller.type(findBarTextField, "mozilla");

You have to send a "VK_RETURN" key event afterward to start searching. Otherwise we miss the first search result. I have added this line.

Will be checked in immediately. r=me.

Comment 11

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

Checked in as http://hg.mozilla.org/qa/mozmill-tests/rev/9a3c778c274f

I'll leave this bug open for the remaining work in Q2. We really have to check that pressing next/previous gets the next/previous selection. In between we can mark the test as finished in the Mozmill spreadsheet.

Comment 12

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

Graeme, do you know if there is a simple way we can use to discover if the selection is moved to the next element when clicking the next/previous buttons for this Mozmill test?

Comment 13

[Graeme McCutcheon [:graememcc]]

I think you would need to go with what is suggested in comment 2.

When you've verified that selectedText.toString() matches the desired text, store the results of selectedText.getRangeAt(0) in a variable. After doing the same test after find next, get the range again, and call compareBoundaryPoints on one of the ranges and test either START_TO_START or END_TO_END (you would only need to test one). If the result of this call != 0 then the selection has changed.

Comment 14

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

Attachment: Followup patch

Thanks Graeme. That works fine and fully beat our needs. Anthony, I hope you are fine with it that I have taken a look at this. Can you try it on your local box? If yes, we can finalize this test.

Comment 15

[u279076]

(In reply to comment #14)
> Created an attachment (id=387442) [details]
> Followup patch
> 
> Thanks Graeme. That works fine and fully beat our needs. Anthony, I hope you
> are fine with it that I have taken a look at this. Can you try it on your local
> box? If yes, we can finalize this test.

I'm perfectly fine with you jumping in.  Collaboration FTW.  If you have a chance, please test it yourself as I am AFK all day today.

Comment 16

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

For me it works perfectly on OS X and Windows. So if you will not have a chance to test today just do it tomorrow. But I think it should work.

Comment 17

[u279076]

I applied your patch to my repo on Linux and the test fails.  controller.open("http://www.mozilla.org") does not load the web page but it appears in the Output tab as a PASS.  As a result, the rest of the test fails.

If I run controller.open("http://www.mozilla.org") from the Shell tab, it returns null and no page is loaded.  I'm using todays revision of Mozmill and Tests.

Comment 18

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

This is an issue we have on Linux and which is covered in bug 503480.

Pushed this changes as http://hg.mozilla.org/qa/mozmill-tests/rev/d0a067615201

Comment 19

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

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.

Comment 20

[u279076]

I believe this can finally be marked verified.