585760 - Make Mozmill-test testFindInPage local

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Module: testFindInPage/testFindInPage.js
Test-page: Use any 1 page from test-files/layout/mozilla*

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

Comment 2

[u279076]

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

>+const LOCAL_TEST_PAGES = [
>+  {url: LOCAL_TEST_FOLDER + 'layout/mozilla.html'}
>+];

>+  // Open a local page
>+  controller.open(LOCAL_TEST_PAGES[0].url);
>   controller.waitForPageLoad();
> 

It's unnecessary for this to be an array.  Simply use the following:

const LOCAL_TEST_PAGES = LOCAL_TEST_FOLDER + 'layout/mozilla.html';

Comment 3

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - (default)

NOTE: Lookup change on default that is different than that on 1.9.1 and 1.9.2

Comment 4

[Aaron Train [:aaronmt]]

This test has sleeps(0) that should be removed too. Patch forthcoming.

Comment 5

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (default)

Comment 6

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

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

This patch is bit-rotted based on bug 586997. Please update.

Comment 7

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (default)

Comment 8

[u279076]

> Created attachment 465858 [details] [diff] [review]
> Patch v3 - (default)

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/80a98ff34d36

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

Comment 9

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (1.9.2)

Comment 10

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (1.9.1)

1.9.1 had messy lookups all over the place extending more than 80 character limit lines. I rearranged everything and properly referenced the different lookup for the findbar-textbox that's different than it's counterpart in 1.9.2

Comment 11

[u279076]

Comment on attachment 465923 [details] [diff] [review]
Patch v3 - (1.9.1)

Code-wise it looks good, I just have some minor formatting nits:

>+ containerString = '/id("main-window")' +
>+                    '/id("browser-bottombox")/id("FindToolbar")' +
>+                    '/anon({"anonid":"findbar-container"})';

Please fix the indentation (make the front / aligned)

>+ findBarTextField = new elementslib.Lookup(controller.window.document,
>+                                           containerString + '/anon({"anonid":"find-field-container"})' + 
>+                                           '/anon({"anonid":"findbar-textbox"})');

Please move the appended string in the 2nd line to it's own line.

>+ var selectedText = tabContent.getSelection();
>   controller.assertJS("subject.selectedText == subject.searchTerm",
>                       {selectedText: selectedText.toString().toLowerCase(), searchTerm: searchTerm});

Please put each subject member on its own line.

Comment 12

[Aaron Train [:aaronmt]]

Attachment: Patch v3 [indentation] - (1.9.1)

fixes from comment #11

Comment 13

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

Comment on attachment 469156 [details] [diff] [review]
Patch v3 [indentation] - (1.9.1)

Please don't shuffle all those entries around. Backports shouldn't do that. Please file a new bug which will add a FindBar class to the ToolbarAPI.

Comment 14

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (1.9.1) [re-adjustment from comment #13]

Comment 15

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (1.9.1) [re-adjustment from comment #13]

re-adjustment + comment fix

Comment 16

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

Comment on attachment 476821 [details] [diff] [review]
Patch v3 - (1.9.1) [re-adjustment from comment #13]

Ok sorry for the last review comment. It was already the correct approach but I missed to check the existing tests on default and mozilla1.9.2. With the work on bug 530602 we missed to check-in the lookup string handling on mozilla1.9.1.

Please update the patch so that we are in sync with the code on all branches.

Comment 17

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (1.9.1) [reverting to cleanup]

Alright ... reverting to the cleanup patch again

Comment 18

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

Comment on attachment 476966 [details] [diff] [review]
Patch v3 - (1.9.1) [reverting to cleanup]

Please file a follow-up to get all the element declarations and some of the access functions moved to its own class in the ToolbarAPI.

>+  // Open a local web page
>+  controller.open(LOCAL_TEST_PAGE);

nit: please don't emphasize local in the comment. Just call it a web page. 

>-  var selectedText = tabContent.getSelection();
>+ var selectedText = tabContent.getSelection();

nit: please fix indentation.

r=me with those nit fixes.

Comment 19

[u279076]

Attachment: Patch v3.1

nits addressed

Comment 20

[u279076]

(In reply to comment #19)
> Created attachment 478035 [details] [diff] [review]
> Patch v3.1
> 
> nits addressed

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

Comment 21

[u279076]

Seems like mozilla1.9.2 was missed here.  Aaron, please provide a patch.

Comment 22

[Aaron Train [:aaronmt]]

(In reply to comment #21)
> Seems like mozilla1.9.2 was missed here.  Aaron, please provide a patch.

Look in the attachments

Comment 23

[u279076]

(In reply to comment #9)
> Created attachment 465922 [details] [diff] [review]
> Patch v3 - (1.9.2)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/c3e66e59c4cf [mozilla1.9.2]

Comment 24

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