[mozmill] use the location bar, suggest: Bookmarks and History

RESOLVED FIXED

Status

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

People

(Reporter: tracy, Assigned: tracy)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 428273 [details] [diff] [review]
test case

write mozmill test case for Litmus "Verify preference "When using the location bar, suggest: Bookmarks and History""
test case 9235
Attachment #428273 - Flags: review?(hskupin)
Status: NEW → ASSIGNED
Comment on attachment 428273 [details] [diff] [review]
test case

>--- a/firefox/testAwesomeBar/testEscapeAutocomplete.js
>+++ b/firefox/testAwesomeBar/testEscapeAutocomplete.js
>@@ -9,17 +9,21 @@
>  * Software distributed under the License is distributed on an "AS IS" basis,
>  * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>  * for the specific language governing rights and limitations under the
>  * License.
>  *
>  * The Original Code is MozMill Test code.
>  *
>  * The Initial Developer of the Original Code is Mozilla Foundation.
>+<<<<<<< local
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+=======

This diff is broken and contains left-overs from a previous unsuccessful patch. Please delete that file and pull again from the repository. 

When you update this please also take the other advices from the other review into account. Thanks.
Attachment #428273 - Flags: review?(hskupin) → review-
(Assignee)

Comment 2

9 years ago
Created attachment 428794 [details] [diff] [review]
test case v2

ick, told ya my repository got messed up.

I think got this test case all fixed up.  I changed the test page to yahoo.com. The david blaine site has a youtube video playing. It's a possible page load annoyance.
Attachment #428273 - Attachment is obsolete: true
Attachment #428794 - Flags: review?(hskupin)
Comment on attachment 428794 [details] [diff] [review]
test case v2

Please provide a patch instead of the test file itself. 

>const gDelay = 500;
>const tDelay = 100;

What is the difference? You can get rid of tDelay. See below.

>  var teardownModule = function(module) 
>  {
>    PlacesAPI.restoreDefaultBookmarks();
>  }

Please correct the indentation and always cross-check in the future before attaching a new patch. I think we can save us from those review comments in the future.

>  var testSite = 'http://www.yahoo.com/'; 

Please use a page which loads faster, e.g. Bing or Google.

>  locationBar.loadURL(testSite);
>  controller.waitForPageLoad();
>  controller.click(new elementslib.Elem(controller.menus.bookmarksMenu.menu_bookmarkThisPage));
>  controller.sleep(gDelay);
>  controller.click(doneButton);
>  controller.sleep(gDelay);
>  locationBar.loadURL('about:blank');
>  controller.waitForPageLoad();

Please add empty line between major steps. Further please check the test under testBookmarks how to correctly use the bookmark panel. You can remove the sleep calls then.

>  var testString = 'yahoo';

Please define the URL and the test string next to each other. I would prefer an object type here like {url: "", string: ""}.

>  // Use type and sleep on each letter of test string to allow the autocomplete to populate with results. 
>  //for (var i = 0; i < testString.length; i++) {
>  //locationBar.type(testString[i]);
>  for each (letter in testString) {
>  locationBar.type(letter);
>  controller.sleep(tDelay);
>  }

Please fix the indentation and remove commented out code.

>  // defines the path to the (ElemBase) result 
>  var richlistItem = locationBar.autoCompleteResults.getResult(0);

Just say in the comment that this is the first auto-complete result. It makes it easier for others who haven't seen the test before.  

Please also add a test that only one result is displayed.

>  // For icons, check that the bookmark star is present
>  var isBookmarked = richlistItem.getNode().getAttribute('type');
>  controller.assertJS(isBookmarked == 'bookmark');

assertJS has to be used with subject for better failure output.
Attachment #428794 - Flags: review?(hskupin) → review-
(Assignee)

Comment 4

9 years ago
Created attachment 429191 [details] [diff] [review]
test case v3
Attachment #428794 - Attachment is obsolete: true
Attachment #429191 - Flags: review?(hskupin)
(Assignee)

Comment 5

9 years ago
Created attachment 430191 [details] [diff] [review]
test case v4

Since Litmus test case 8539 has basically the same setup as this test case and exactly the same check of the bookmark star in the auto-complete list, I've split off the star icon check into a separate function within this test case with its own litmus meta line.
Attachment #429191 - Attachment is obsolete: true
Attachment #430191 - Flags: review?(hskupin)
Attachment #429191 - Flags: review?(hskupin)
Comment on attachment 430191 [details] [diff] [review]
test case v4

Good idea Tracy, but we will have to change some parts to make it work correctly.

>var teardownModule = function(module)
>{
>  PlacesAPI.restoreDefaultBookmarks();
>}

Please rename that to teardownTest.

>  // editBookmarksPanel is loaded lazily. Wait until overlay for StarUI has been loaded, then close the dialog
>  controller.waitForEval("subject._overlayLoaded == true", 2000, 100, controller.window.top.StarUI);
>  var doneButton = new elementslib.ID(controller.window.document, "editBookmarkPanelDoneButton");
>  controller.click(doneButton);

Please do all that stuff inside setupTest.

>  for each (letter in testSite.string) {
>    locationBar.type(letter);
>    controller.sleep(gDelay);
>  }

As talked for another page please don't use sleep here. Instead check for the open state of the popup.

>  // For the url, check matched text is underlined
>  locationBar.autoCompleteResults.assertTextUnderlined(richlistItem, 'url', testSite.string);

We should check against the url itself to make sure the bookmark entry is displayed.

>  // For icons, check that the bookmark star is present
>  var richlistItemNode = richlistItem.getNode();
>  controller.assertJS("subject.getAttribute('type') == 'bookmark'", richlistItemNode);

Please combine both lines.

>  var autoCompleteResultsListNode = autoCompleteResultsList.getNode();
>  controller.assertJS("subject.getNumberOfVisibleRows() == 1", autoCompleteResultsListNode);
>}

Combine both lines too.

>  // For icons, check that the bookmark star is present
>  var richlistItemNode = richlistItem.getNode();
>  controller.assertJS("subject.getAttribute('type') == 'bookmark'", richlistItemNode);

Please combine both lines.
Attachment #430191 - Flags: review?(hskupin) → review-
(Assignee)

Comment 7

8 years ago
Created attachment 434594 [details] [diff] [review]
3.6 test case
Attachment #430191 - Attachment is obsolete: true
Attachment #434594 - Flags: review?(hskupin)
(Assignee)

Comment 8

8 years ago
Created attachment 434595 [details] [diff] [review]
3.5 test case
Attachment #434595 - Flags: review?(hskupin)
Tracy, for the future please only ask for review of patches against default. Once we have it checked in continue with 3.5 if changes are necessary. Thanks.
Comment on attachment 434594 [details] [diff] [review]
3.6 test case

>  PrefsAPI.preferencesDialog.open(prefDialogSuggestsCallback);

Please move that line into the test itself.

>  // Get the visible results from the autocomplete list. Verify it is 1
>  controller.assertJS("subject.getNumberOfVisibleRows() == 1", locationBar.autoCompleteResults.getElement({type:"results"}).getNode());

This is "locationBar.autoCompleteResults.allResults". Please separate its declaration into its own line before the assertJS call.

>    // Clear history
>  try {

nit: indentation

>  // For icons, check that the bookmark star is present
>  controller.assertJS("subject.getAttribute('type') == 'bookmark'", richlistItem.getNode());

We can make it more descriptive when moving the function call out of the eval expression and have something like that in there: "subject.isResultBookmarked == true". Therefore the second parameter should be an object: "{isResultBookmarked: richtlistItem.getNode().getAttribute('type') == 'bookmark'.

>var prefDialogSuggestsCallback = function(controller)

Please update to the new preferences Dialog class.

>testSuggestHistoryAndBookmarks.meta = {litmusids : [9235]};
>testStarInAutocomplete.meta = {litmusids : [8539]};

And remove this.
Attachment #434594 - Flags: review?(hskupin) → review-
Comment on attachment 434595 [details] [diff] [review]
3.5 test case

Please don't request a 3.5 review before the patch for the default branch has been accepted. That will only cause more work for me. Thanks.
Attachment #434595 - Flags: review?(hskupin)
(Assignee)

Comment 12

8 years ago
Created attachment 436032 [details] [diff] [review]
patch_testSuggestHistoryBookmarks

This is a correctly working test case.  assertJS messages may not be brilliant, but this is what I am able to make work.
Attachment #434594 - Attachment is obsolete: true
Attachment #434595 - Attachment is obsolete: true
Attachment #436032 - Flags: review?(hskupin)
Comment on attachment 436032 [details] [diff] [review]
patch_testSuggestHistoryBookmarks

The test fails on all platforms for me at different positions:

OS X and Windows:
ERROR - Test Failure: {'exception': {'message': 'Controller.assertJS("subject.getNumberOfVisibleRows() == 1")', 'lineNumber': 714, 


Also please update the following:

>+  var autoCompleteResultsListNode = locationBar.autoCompleteResults.getElement({type:"results"}).getNode();
>+  controller.assertJS("subject.getNumberOfVisibleRows() == 1", autoCompleteResultsListNode);

Move the .getNode() part to the 2nd parameter of the assertJS call and rename it to autoCompleteResultsList. We should try to have always elementBase elements around and only use the internal data for functions like assertJS/waitForEval.

>+var testStarInAutocomplete = function()
>+  // Clear history
>+  try {
>+    PlacesAPI.historyService.removeAllPages();
>+  } catch (ex) {}

Now that you have to use it in several functions please add a setupTest() function and move that piece of code from here and setupModule to that location. That way it will automatically called before each test function inside a module.

>+  // For the page title check matched text is underlined
>+  var entries = locationBar.autoCompleteResults.getUnderlinedText(richlistItem, "title");

Which richtlistItem is that? It hasn't been defined in that test function.

>+  // For icons, check that the bookmark star is present
>+  controller.assertJS("subject.getAttribute('type') == 'bookmark'", richlistItem.getNode());

Same here.

>+    PlacesAPI.restoreDefaultBookmarks();

nit: indentation.
Attachment #436032 - Flags: review?(hskupin) → review-
(Assignee)

Comment 14

8 years ago
> The test fails on all platforms for me at different positions:
> 
> OS X and Windows:
> ERROR - Test Failure: {'exception': {'message':
> 'Controller.assertJS("subject.getNumberOfVisibleRows() == 1")', 'lineNumber':
> 714, 

I don't know how I can fix this.  The test case works for me.

> 
> Also please update the following:
> 
> >+  var autoCompleteResultsListNode = locationBar.autoCompleteResults.getElement({type:"results"}).getNode();
> >+  controller.assertJS("subject.getNumberOfVisibleRows() == 1", autoCompleteResultsListNode);
> 
> Move the .getNode() part to the 2nd parameter of the assertJS call and rename
> it to autoCompleteResultsList. We should try to have always elementBase
> elements around and only use the internal data for functions like
> assertJS/waitForEval.

This is the third different way you've asked me to do that bit of code.  I'm done with it, It works for me as is.

> 
> >+var testStarInAutocomplete = function()
> >+  // Clear history
> >+  try {
> >+    PlacesAPI.historyService.removeAllPages();
> >+  } catch (ex) {}
> 
> Now that you have to use it in several functions please add a setupTest()
> function and move that piece of code from here and setupModule to that
> location. That way it will automatically called before each test function
> inside a module.

Doing it automatically was unexpectedly disrupting the flow from the first test case to the second. 

> 
> >+  // For the page title check matched text is underlined
> >+  var entries = locationBar.autoCompleteResults.getUnderlinedText(richlistItem, "title");
> 
> Which richtlistItem is that? It hasn't been defined in that test function.
> 
it's defined after the 3rd comment in this block.
(Assignee)

Comment 15

8 years ago
Created attachment 436212 [details] [diff] [review]
patch_testSuggestHistoryBookmarksWithCheckitemBits

updated patch to fix intermittent failure.

Path also includes similar fix to testCheckItemHighlight
Attachment #436032 - Attachment is obsolete: true
Attachment #436212 - Flags: review?(hskupin)
Comment on attachment 436212 [details] [diff] [review]
patch_testSuggestHistoryBookmarksWithCheckitemBits

Please attach a patch with all your mentioned updates.
Attachment #436212 - Flags: review?(hskupin) → review-
(Assignee)

Comment 17

8 years ago
Created attachment 436276 [details] [diff] [review]
patch_testSuggestHistoryBookmarksWithCheckItemHighlightBits

sorry about that.
Attachment #436212 - Attachment is obsolete: true
Attachment #436276 - Flags: review?(hskupin)
Comment on attachment 436276 [details] [diff] [review]
patch_testSuggestHistoryBookmarksWithCheckItemHighlightBits

Review denied because of:

>+++ b/firefox/testAwesomeBar/testSuggestHistoryBookmarks.js
>+
>+var testSite = {url : 'http://www.google.com/', string: 'google'};

Google is the wrong page to test here because of the geolocation based redirect.

>+  var autoCompleteResultsListNode = locationBar.autoCompleteResults.getElement({type:"results"});
>+  controller.assertJS("subject.getNumberOfVisibleRows() == 1", autoCompleteResultsListNode.getNode());

And let this test fail because two entries are in the location bar.

I'll fix it and check it in.
Attachment #436276 - Flags: review?(hskupin) → review-
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/1b27aa7d8515
http://hg.mozilla.org/qa/mozmill-tests/rev/0edd2b53fcaa
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Tracy, please check the referenced litmus tests in the spreadsheet. There seem to be dupes! Especially for 'Verify preference "When using the location bar, suggest: Bookmarks and History"'.
(Assignee)

Comment 21

8 years ago
(In reply to comment #20)
> Tracy, please check the referenced litmus tests in the spreadsheet. There seem
> to be dupes! Especially for 'Verify preference "When using the location bar,
> suggest: Bookmarks and History"'.

I removed the second entry from the spreadsheet.  It was either a FFT or been marked as a dupe of the case we covered in Mozmill, depending on the branch.
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.
Component: Location Bar → Mozmill Tests
Product: Firefox → Mozilla QA
QA Contact: location.bar → mozmill-tests
Version: 3.6 Branch → unspecified
You need to log in before you can comment on or make changes to this bug.