Closed Bug 705284 Opened 13 years ago Closed 12 years ago

Mozmill endurance test for bookmarking all tabs and opening and closing them

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox14 fixed, firefox15 fixed, firefox16 fixed, firefox17 fixed, firefox-esr10 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox14 --- fixed
firefox15 --- fixed
firefox16 --- fixed
firefox17 --- fixed
firefox-esr10 --- fixed

People

(Reporter: remus.pop, Assigned: vladmaniac)

References

Details

(Whiteboard: [mozmill-endurance][bookmarks])

Attachments

(2 files, 10 obsolete files)

patch v4.5 [checked-in]
6.04 KB, patch
davehunt
: review+
Details | Diff | Splinter Review
[mozilla-esr10] patch v1.0 [checked-in]
5.87 KB, patch
whimboo
: review+
davehunt
: review+
Details | Diff | Splinter Review 
Tracking creation of a mozmill endurance test for bookmarking all the open tabs and then closing and opening them.

The test should be like this:
Setup Module
  Open X pages
  Save X pages as bookmarks
Main Test Checkpoints
  Open all bookmarks one by one
  Close all tabs
Teardown Module
  Force close tabs

Dave, please let me know if this is the behaviour we want.
Summary: Mozmill endurance test for bookmarking all tabs → Mozmill endurance test for bookmarking all tabs and opening and closing them
Whiteboard: [mozmill-endurance]
I'm not familiar with this test. I did suggest a similar one that added bookmarks to a single folder and then opened all bookmarks using 'Open All in Tabs'.
There is one test in the MozMill test coverage spreadsheet. Look into the Data sheet on line 456.
On line 455 is the test you mentioned.
Can you link to the spreadsheet?
I found the original reference to this in the etherpad [1]: "Save bookmarks from all open tabs and reopen those over and over again". This looks like one of Henrik's suggestions, however there's not a lot of detail there.

My guess is that the following is how the test would look:

Name: testBookmarks_OpenBookmark

Setup Module
  Open X pages
  Save X pages as bookmarks
  Close all tabs
Main Test Checkpoints
  Open X bookmarks one by one
  Close all tabs
Teardown Module
  Force close tabs

One problem I foresee with this test (and other bookmark endurance tests using entities) is that we would need unique pages for each bookmark.

[1] https://etherpad.mozilla.org/mLWlTg7DwK
I was thinking of using all pages in http://mozqa.com/data/firefox/layout/ . So they are unique.
The problem is that the number of entities is set on the command line. What if someone wants to run with 100 entities, or 1000?
(In reply to Dave Hunt (:davehunt) from comment #7)
> The problem is that the number of entities is set on the command line. What
> if someone wants to run with 100 entities, or 1000?

That seems like a fundamental limitation. The only way I see around it is if we could somehow dynamically and randomly generate X number of temporary test pages which are created on start and destroyed on exit.
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #8)
> (In reply to Dave Hunt (:davehunt) from comment #7)
> > The problem is that the number of entities is set on the command line. What
> > if someone wants to run with 100 entities, or 1000?
> 
> That seems like a fundamental limitation. The only way I see around it is if
> we could somehow dynamically and randomly generate X number of temporary
> test pages which are created on start and destroyed on exit.

Pretty science for just a mozmill test i would say - but I don't have a better idea. 

If we serve a fixed number of testpages and use only iterations in the endurance test, well, there is not much value in it in the end. 

If we consider having a testpage opened as part of an entity, then we won't have separate testpages. 

Initially I was thinking of having an array of urls and randomly take one page from there for a current entity to handle - bad idea because eventually they will repeat and we won't bookmark one page twice there is no sense in that.
Do they have to be completely unique though? As an experiment:

1) Bookmark this page
2) Bookmarks > Show all Bookmarks > Unsorted Bookmarks
3) Right click bookmark for this page and select Copy
4) Left click and select Paste 3 times
5) Create a new folder on the bookmarks toolbar called "temp"
6) Move the newly created bookmarks into this folder
7) Close the Library and select Bookmarks > temp > Open all in tabs

Result:
All of the same site opens in multiple tabs (one for each bookmark).

Any reason we can't preload a bookmark folder with X number of bookmarks for the same URL for an Endurance test?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #10)
> Do they have to be completely unique though? As an experiment:
> 
> 1) Bookmark this page
> 2) Bookmarks > Show all Bookmarks > Unsorted Bookmarks
> 3) Right click bookmark for this page and select Copy
> 4) Left click and select Paste 3 times
> 5) Create a new folder on the bookmarks toolbar called "temp"
> 6) Move the newly created bookmarks into this folder
> 7) Close the Library and select Bookmarks > temp > Open all in tabs
> 
> Result:
> All of the same site opens in multiple tabs (one for each bookmark).
> 
> Any reason we can't preload a bookmark folder with X number of bookmarks for
> the same URL for an Endurance test?
Yes - your idea solves duplicating bookmarks but..

Without a strong API this would take a long time and would be not robust. 
Imagine the amount of code there..

Anyway, its doable but with costs
Dave, any feedback on this?
As discussed on IRC, we should try adding a URL parameter with a unique value (use the current entity number). The parameter should be ignored but be a unique URL for a bookmark.
Assuming that works, it looks good to me.
Whiteboard: [mozmill-endurance] → [mozmill-endurance][mozmill-bookmarks]
Dave's idea is fine with using a parameter.

From this point on we have some options from which we need to choose:

1. How to bookmark
  option a: bookmark each page when visited
  option b: bookmark all pages at once () (Dave opted for this one)

2. Where to put the folder with the bookmarks (bookmarks will reside in a folder to keep them grouped)
  option a: Bookmarks Menu
  option b: Bookmarks Toolbar
  option c: Unsorted Bookmarks
  option d: Existing folder

Each option from 2 implies a different degree of difficulty and steps to perform in order to open the bookmarked page.
Here is what I propose:

1) Open each page in a new tab
2) Right click tab bar > Bookmark All Tabs
3) Give the folder a name (ie. "Mozmill Bookmarks"), save to the Bookmarks Toolbar
4) Close all tabs
> Start iteration
5) Bookmarks > "Mozmill Bookmarks" > Open all in tabs
6) Close all tabs
> End iteration

Dave, what do you think?
I'm in agreements with all but item 5. This test is to open bookmarks one-by-one. We have another test that uses 'Open all in tabs'.

We would have to open a specific bookmark by reference. If we save it with a predictable name (probably easiest to use the current entity number) then opening it should be relatively simple.
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #14)
> http://mozqa.com/data/firefox/layout/mozilla.html?entity=0

Instead of this page you probably want to use the following page or create a new one: http://mozqa.com/data/firefox/tabbedbrowsing/openinnewtab_target.html?id=1
We will need the last tab to be an empty one in the case the test is run with 1 entity because if 1 tab is opened, bookmark all tabs option is not available.
It sounds like we have a case for minimum entities. I would suggest setting checking if entities < 2 and if true set entities to 2. This would need a suitable comment.
Vlad, mind taking this test so we can finish it up?
Assignee: remus.pop → vlad.mozbugs
(In reply to Dave Hunt (:davehunt) from comment #21)
> It sounds like we have a case for minimum entities. I would suggest setting
> checking if entities < 2 and if true set entities to 2. This would need a
> suitable comment.

How would you set entities to 2? afaik we set the entities number from the command line, can we set them at runtime?
In setupModule() where the test is getting prepared, and we have to restore the value in teardownModule(). Otherwise we can let tests define a minimum number of entities and if set the endurance manager makes use of it. That's probably the better way and we do not mess around with the persisted entries.
(In reply to Remus Pop (:RemusPop) from comment #20)
> We will need the last tab to be an empty one in the case the test is run
> with 1 entity because if 1 tab is opened, bookmark all tabs option is not
> available.

There will always be at least 2 tabs because tabBrowser.closeAllTabs() closes the tabs and leaves and empty one. Then we filter our bookmarked pages by title and open and close them, so no need to treat the case for minimum entities
Attached patch [initial patch] patch v1.0 (obsolete) — — Splinter Review 
Proposed solution

You will probably wonder why there are three methods in this test (except setup and teardown): 

* the test method 
* setBookmarks method because we need to have lots of prerequisites for this test, meaning:
     1. open pages
     2. bookmark all pages
     and I would not want to place lots of code in setupModule(), IMHO
* handle bookmark prefs modal dialog method
     1. need to handle the modal dialog
        we cannot put this into the API because start() method from modal-dialog.js has a callback which has only the controller as parameter and I need to setup some test specific data such as the bookmarks folder name, which means that in the API this method will need more parameters. Other tests are developed in the same manner, like functional/restartTests/testMasterPassword

In terms of functionality, the test behaves normally 
http://mozmill-crowd.blargon7.com/#/endurance/report/27072cb61461b83e1447b1979b06cd3e
Attachment #645232 - Flags: review?(hskupin)
Attachment #645232 - Flags: review?(dave.hunt)
Comment on attachment 645232 [details] [diff] [review]
[initial patch] patch v1.0

>+      case "tabsStrip_bookmarkAllTabs":
>+        elem = new elementslib.ID(this._controller.window.document, "context_bookmarkAllTabs");
>+      break;

This is a context menu entry and has nothing to do with the tabbrowser module. Remove that please.

>+var domUtils = require("../../../lib/dom-utils"); 
>+var endurance = require("../../../lib/endurance");
>+var modalDialog = require("../../../lib/modal-dialog");
>+var places = require("../../../lib/places");
>+var prefs = require("../../../lib/prefs");
>+var tabs = require("../../../lib/tabs");
>+var toolbars = require("../../../lib/toolbars");
>+var utils = require("../../../lib/utils");



>+const TIMEOUT_DIALOG = 25000;

This doesn't tell me anything. Which dialog? Please be more specific with the name of the variable.

>+function testOpenAndCloseAllBookmarks() {


>+  enduranceManager.run(function () {
>+    tabBrowser.closeAllTabs();

I don't think we need this call at this position.

>+      controller.rightClick(page);
>+
>+      var openInNewTab = new elementslib.ID(controller.window.document, 
>+                                            "placesContext_open:newtab");
>+      controller.click(openInNewTab);

You want to use the Menu API here which also handles context menus.

>+      controller.waitForPageLoad();

You first have to wait for the new tab been opened and selected.

>+      // Dismiss the context menu
>+      controller.keypress(null , 'VK_ESCAPE', {});

This is not necessary when used via the Menu API.


>+function setupBookmarks() {
>+  // Enable bookmarks toolbar 
>+  var navbar = new elementslib.ID(controller.window.document, "nav-bar");
>+  var tabStrip = tabBrowser.getElement({type: "tabs_strip"});
>+
>+  controller.rightClick(navbar, navbar.getNode().boxObject.width / 2, 2);
>+
>+  var toggle = new elementslib.ID(controller.window.document,
>+                                  "toggle_PersonalToolbar");
>+  controller.mouseDown(toggle);
>+  controller.mouseUp(toggle);

Now we have this code twice. In defaultbookmarks and here. I would say lets move it to a library module.

>+  for (var i = 1; i <= enduranceManager.entities; i++) {
>+    controller.open(LOCAL_TEST_PAGE + i); 
>+    controller.waitForPageLoad();
>+
>+    tabBrowser.openTab(); 
>+  } 
>+  controller.rightClick(tabStrip);
>+  
>+  // Bookmark all tabs
>+  var bookmarkAllTabs = tabBrowser.getElement({type: "tabsStrip_bookmarkAllTabs"});

If we do not perform any endurance related tasks I would say we do not go through the ui here but always make use of back-end API calls to setup the list of new bookmarks.


>+function handleBookmarkPreferencesDialog(aController) {

I would like to kill that.
Attachment #645232 - Flags: review?(hskupin)
Attachment #645232 - Flags: review?(dave.hunt)
Attachment #645232 - Flags: review-
> 
> If we do not perform any endurance related tasks I would say we do not go
> through the ui here but always make use of back-end API calls to setup the
> list of new bookmarks.
> 
> 
> >+function handleBookmarkPreferencesDialog(aController) {
> 
> I would like to kill that.

Perfectly agree but please see comment 17 and ealier. It was an explicit decision to use bookmark all tabs by right click...
> 
> Now we have this code twice. In defaultbookmarks and here. I would say lets
> move it to a library module.
> 

Just to make sure, are you suggesting we should create a new shared module called library.js and include a helper method to enable the bookmarks toolbar?
This test covers opening and closing of bookmarks but not the addition of those. Really, I do not see why we have to go through the UI here. It's a chance of getting additional failures.
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #29)
> Just to make sure, are you suggesting we should create a new shared module
> called library.js and include a helper method to enable the bookmarks
> toolbar?

No, this was meant like: a module under lib. I haven't proposed a name.
(In reply to Henrik Skupin (:whimboo) from comment #30)
> This test covers opening and closing of bookmarks but not the addition of
> those. Really, I do not see why we have to go through the UI here. It's a
> chance of getting additional failures.

I could not agree more. So I will move on and make the changes
(In reply to Henrik Skupin (:whimboo) from comment #31)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #29)
> > Just to make sure, are you suggesting we should create a new shared module
> > called library.js and include a helper method to enable the bookmarks
> > toolbar?
> 
> No, this was meant like: a module under lib. I haven't proposed a name.

In this case will be toolbars.js since we are enabling the bookmarks toolbar
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #32)
> I could not agree more. So I will move on and make the changes

Please wait for Dave what he thinks about.
(In reply to Henrik Skupin (:whimboo) from comment #34)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #32)
> > I could not agree more. So I will move on and make the changes
> 
> Please wait for Dave what he thinks about.

Alright, Dave, can you please give your input re comment 32?
> 
> >+      controller.rightClick(page);
> >+
> >+      var openInNewTab = new elementslib.ID(controller.window.document, 
> >+                                            "placesContext_open:newtab");
> >+      controller.click(openInNewTab);
> 
> You want to use the Menu API here which also handles context menus.
> 
Is this applicable here? I was browsing into the menu API in Mozmill and seen that the API works for the firefox main menu

MozMillController.prototype.__defineGetter__("mainMenu", function () {
  return this.getMenu("menubar");
});

In this case we are talking about the bookmarks toolbar folder menu 
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc#451

My code is 

var page = new elementslib.Selector(controller.window.document, 
                                    "*[label='Test Bookmark " + 
                                    enduranceManager.currentEntity + "']");

controller.mainMenu.select("#placesContext_open:newtab", page);

Can you help?
You should check at least testTabbedBrowsing_PinAndUnpinAppTab which is already making use of the Menu API.
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #35)
> (In reply to Henrik Skupin (:whimboo) from comment #34)
> > (In reply to Maniac Vlad Florin (:vladmaniac) from comment #32)
> > > I could not agree more. So I will move on and make the changes
> > 
> > Please wait for Dave what he thinks about.
> 
> Alright, Dave, can you please give your input re comment 32?

I absolutely agree that the setup should use backed API calls and not the UI.
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #25)
> (In reply to Remus Pop (:RemusPop) from comment #20)
> > We will need the last tab to be an empty one in the case the test is run
> > with 1 entity because if 1 tab is opened, bookmark all tabs option is not
> > available.
> 
> There will always be at least 2 tabs because tabBrowser.closeAllTabs()
> closes the tabs and leaves and empty one. Then we filter our bookmarked
> pages by title and open and close them, so no need to treat the case for
> minimum entities

This is still relevant if we use the UI to bookmark all tabs as the option is not available for < 2 tabs. If we use a backend API to add the bookmarks then this is not relevant.
Attached patch patch v2.0 (obsolete) — — Splinter Review 
Fixed and all changes addressed 

Note: For using the menu API, needed to add escape char '\' because of the pseudo selector in the dropdown menu, please see this example for clarifications

http://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/places/places.css#197 

the problem was the ":" char in the id 

patch works as expected 
http://mozmill-crowd.blargon7.com/#/endurance/report/27072cb61461b83e1447b1979b16e91e
Attachment #645232 - Attachment is obsolete: true
Attachment #645738 - Flags: review?(hskupin)
Attachment #645738 - Flags: review?(dave.hunt)
Whiteboard: [mozmill-endurance][mozmill-bookmarks] → [mozmill-endurance][bookmarks]
Comment on attachment 645738 [details] [diff] [review]
patch v2.0

>+var domUtils = require("../../../lib/dom-utils"); 
>+var modalDialog = require("../../../lib/modal-dialog");

I think both modules can now be removed.

>+    var testFolder = new elementslib.Selector(controller.window.document, 
>+                                              ".bookmark-item[label='testFolder']");

Can we make 'testFolder' a global constant? It's getting used on multiple places across functions.

>+    enduranceManager.loop(function () {
>+      var contextMenu = controller.getMenu("#placesContext");
>+      var page = new elementslib.Selector(controller.window.document, 
>+                                          "*[label='Test Bookmark " + 
>+                                          enduranceManager.currentEntity + "']");  
>+     
>+      contextMenu.select("#placesContext_open\\\:newtab", page);

Hm, on OS X you actually don't have the change to open the context menu of a bookmark's toolbar sub folder. So that test would not that be accurate on this platform. Please x-check if I'm wrong here. 

>+      enduranceManager.addCheckpoint("Bookmarked page no: " +  
>+                                     enduranceManager.currentEntity + 
>+                                     " was opened");
>+    });
>+    // Close all tabs

nit: emptly line please to separate blocks.

>+function setupBookmarks(controller) {

nit: aController

Otherwise looks way better now! I haven't tested this patch. I would leave this up for Dave as owner of the testrun.
Attachment #645738 - Flags: review?(hskupin)
Attachment #645738 - Flags: review?(dave.hunt)
Attachment #645738 - Flags: review-
Comment on attachment 645738 [details] [diff] [review]
patch v2.0

>Bug 705284 - Mozmill endurance test for bookmarking all tabs and opening and closing them. r=hskupin, r=dhunt

We no longer bookmark all tabs, but seed a folder with bookmarks. A better commit message would be 'Mozmill endurance test opening and closing bookmarks'

>
>diff --git a/lib/toolbars.js b/lib/toolbars.js
>--- a/lib/toolbars.js
>+++ b/lib/toolbars.js
>@@ -564,13 +564,32 @@ locationBar.prototype = {
>    *        Text to enter into the location bar
>    */
>   type : function locationBar_type(text) {
>     this._controller.type(this.urlbar, text);
>     this.contains(text);
>   }
> }
> 
>+/**
>+ * Enable bookmarks toolbar
>+ *
>+ * @param {MozmillController} aController
>+ *        MozMillController of the window to operate on
>+ */
>+function enableBookmarksToolbar(aController) { 
>+  var navbar = new elementslib.ID(aController.window.document, "nav-bar");
>+
>+  aController.rightClick(navbar, navbar.getNode().boxObject.width / 2, 2);
>+
>+  var toggle = new elementslib.ID(aController.window.document,
>+                                  "toggle_PersonalToolbar");
>+  aController.mouseDown(toggle);
>+  aController.mouseUp(toggle);
>+}
>+
> // Export of classes
> exports.locationBar = locationBar;
> exports.editBookmarksPanel = editBookmarksPanel;
> exports.autoCompleteResults = autoCompleteResults;
> 
>+// Export of functions
>+exports.enableBookmarksToolbar = enableBookmarksToolbar;
>diff --git a/tests/endurance/manifest.ini b/tests/endurance/manifest.ini

>+[include:testBookmarks_OpenAndCloseAllTabs/manifest.ini]

Rename this to simply testBookmarks_OpenAndClose

>+var prefs = require("../../../lib/prefs");

>+const PREF_TAB_NUMBER_WARNING = "browser.tabs.maxOpenBeforeWarn";

>+  // Do not warn about max opened tab number
>+  prefs.preferences.setPref(PREF_TAB_NUMBER_WARNING, 
>+                            enduranceManager.entities + 1);

We shouldn't need these as we're opening the bookmarks one by one.

>+/* 
>+ * Test open all bookmarks one by one and close all  
>+ */
>+function testOpenAndCloseAllBookmarks() {
>+  enduranceManager.run(function () {
>+    var testFolder = new elementslib.Selector(controller.window.document, 
>+                                              ".bookmark-item[label='testFolder']");
>+
>+    controller.click(testFolder);      

Remove the trailing whitespace.

>+
>+    enduranceManager.loop(function () {
>+      var contextMenu = controller.getMenu("#placesContext");                 

Remove the trailing whitespace.

>+      var page = new elementslib.Selector(controller.window.document, 
>+                                          "*[label='Test Bookmark " + 
>+                                          enduranceManager.currentEntity + "']");  
>+     
>+      contextMenu.select("#placesContext_open\\\:newtab", page);
>+      controller.waitForPageLoad();
>+      enduranceManager.addCheckpoint("Bookmarked page no: " +  
>+                                     enduranceManager.currentEntity + 
>+                                     " was opened");

The checkpoint will already contain the iteration and entity, so simply 'Bookmark opened' would be enough here.

>+    });
>+    // Close all tabs
>+    tabBrowser.closeAllTabs();

Again, this comment is redundant.

>+    enduranceManager.addCheckpoint("All tabs are closed");
>+  }); 
>+}
>+
>+/*
>+ * Insert bookmarks in a custom folder under Bookmarks Toolbar  
>+ */
>+function setupBookmarks(controller) {
>+  // Enable bookmarks toolbar 
>+  toolbars.enableBookmarksToolbar(controller);

This comment seems unnecessary.

Also, this test is failing for me locally with just a single entity/iteration. The error message is "could not find element Selector: .bookmark-item[label='testFolder']"
Attachment #645738 - Flags: review-
Sorry for the noise in the above comment. I submitted before trimming.
(In reply to Dave Hunt (:davehunt) from comment #42)
> Also, this test is failing for me locally with just a single
> entity/iteration. The error message is "could not find element Selector:
> .bookmark-item[label='testFolder']"

That could be related to what I have mentioned in comment 41. I think we can't run this test on OS X.
Attached patch patch v2.1 (obsolete) — — Splinter Review 
* fixed nits and change requests
* skipping the test if OS = mac, with confirmation of Dave's error on Mac OS X systems - we can't make usage of this test on mac platform
Attachment #645738 - Attachment is obsolete: true
Attachment #646043 - Flags: review?(hskupin)
Attachment #646043 - Flags: review?(dave.hunt)
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #45)
> * skipping the test if OS = mac, with confirmation of Dave's error on Mac OS
> X systems - we can't make usage of this test on mac platform

Have you tried to reproduce this issue on your own on OS X?
(In reply to Henrik Skupin (:whimboo) from comment #46)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #45)
> > * skipping the test if OS = mac, with confirmation of Dave's error on Mac OS
> > X systems - we can't make usage of this test on mac platform
> 
> Have you tried to reproduce this issue on your own on OS X?

Yes, got the exact same error
Hm, that failure isn't caused by trying to access a context menu entry. In which iteration does it happen?
(In reply to Henrik Skupin (:whimboo) from comment #48)
> Hm, that failure isn't caused by trying to access a context menu entry. In
> which iteration does it happen?

in the first iteration
@resource://mozmill/modules/frame.js:687\n@resource://jsbridge/modules/server.js:184\n@resource://jsbridge/modules/server.js:188\n@resource://jsbridge/modules/server.js:288\n", "message": ": could not find element Selector: .bookmark-item[label='testFolder']"
In that case it's not the problem I was talking about and it needs further investigation. Just skipping the test for OS X is not the right solution at this point.
(In reply to Henrik Skupin (:whimboo) from comment #51)
> In that case it's not the problem I was talking about and it needs further
> investigation. Just skipping the test for OS X is not the right solution at
> this point.

    var testFolder = new elementslib.Selector(controller.window.document, 
                                              ".bookmark-item[label='" + 
                                              BOOKMARK_FOLDER_NAME + "']");

    controller.click(testFolder);

so replaced click with waitThenClick and does not fail anymore for me. 
http://mozmill-crowd.blargon7.com/#/endurance/report/27072cb61461b83e1447b1979b23c67b
(In reply to Henrik Skupin (:whimboo) from comment #51)
> In that case it's not the problem I was talking about and it needs further
> investigation. Just skipping the test for OS X is not the right solution at
> this point.

And yes, we do not need to skip the test on OS X. Yey!
In that case I think it's because the popup gets populated during onshow. And in some cases it can take a bit longer. So yeah, would make sense to me.

Would you mind testing on a heavily loaded system too?
Nightly: http://mozmill-crowd.blargon7.com/#/endurance/report/27072cb61461b83e1447b1979b2c46fc
Aurora: http://mozmill-crowd.blargon7.com/#/endurance/report/27072cb61461b83e1447b1979b23fa00 

Its a pass, I think we did it, so I'm going to just upload the patch and we'll not going to skip it on mac os x, which is great news
Attached patch patch v3.0 (obsolete) — — Splinter Review 
* fixed change requests
* not skipping for mac os x
* changed click with waitThenClick to fix intermittent failure
Attachment #646043 - Attachment is obsolete: true
Attachment #646043 - Flags: review?(hskupin)
Attachment #646043 - Flags: review?(dave.hunt)
Attachment #646494 - Flags: review?(dave.hunt)
Attachment #646494 - Flags: review?(hskupin)
Comment on attachment 646494 [details] [diff] [review]
patch v3.0

>+var prefs = require("../../../lib/prefs");

This is not used.

>+      contextMenu.select("#placesContext_open\\\:newtab", page);

You should open in the current tab, and if this is not the first entity then open a new tab before opening the bookmark. This means we use all the tabs, and you can see examples of this in other endurance tests.

>+    // Bookmark page and save in a custom folder 
>+    places.bookmarksService.insertBookmark(customFolder, URI, defaultIndex,  
>+                                           "Test Bookmark " + 
>+                                           enduranceManager.currentEntity);

You're not using enduranceManager.loop, so currentEntity will not be incremented. Use i instead.

Have you tried this with a large number of bookmarks? I would be interested to know what happens when there are enough to cause scrolling in the bookmarks menu. With the code as it currently is, you're always opening the first bookmark.
Attachment #646494 - Flags: review?(hskupin)
Attachment #646494 - Flags: review?(dave.hunt)
Attachment #646494 - Flags: review-
(In reply to Dave Hunt (:davehunt) from comment #57)
> Comment on attachment 646494 [details] [diff] [review]
> patch v3.0
> 
> >+var prefs = require("../../../lib/prefs");
> 
> This is not used.

Ops, forgot to kill that

> 
> >+      contextMenu.select("#placesContext_open\\\:newtab", page);
> 
> You should open in the current tab, and if this is not the first entity then
> open a new tab before opening the bookmark. This means we use all the tabs,
> and you can see examples of this in other endurance tests.
> 

Not sure if I get the picture here, but the test requires we open a new tab from the context menu. 

> >+    // Bookmark page and save in a custom folder 
> >+    places.bookmarksService.insertBookmark(customFolder, URI, defaultIndex,  
> >+                                           "Test Bookmark " + 
> >+                                           enduranceManager.currentEntity);
> 
> You're not using enduranceManager.loop, so currentEntity will not be
> incremented. Use i instead.
> 
> Have you tried this with a large number of bookmarks? I would be interested
> to know what happens when there are enough to cause scrolling in the
> bookmarks menu. With the code as it currently is, you're always opening the
> first bookmark.

Yes this is a bad one and was forgotten due to the lots of refactoring I did here
> Not sure if I get the picture here, but the test requires we open a new tab
> from the context menu. 

Where are you getting that requirement from?
(In reply to Dave Hunt (:davehunt) from comment #59)
> > Not sure if I get the picture here, but the test requires we open a new tab
> > from the context menu. 
> 
> Where are you getting that requirement from?

The test is about opening bookmarks one by one from the bookmarks toolbar, and from a folder created there, please see previous comments in this bug. this implies using the context menu, simulating a user-action.  If that changed, I am not aware of atm
The test is simply about opening bookmarks one by one in separate tabs. The bookmarks toolbar and folder is an implementation choice (I personally expected this to use the main menu, but that isn't likely to make much difference). I don't see how a context menu was ever implied, and it certainly shouldn't be necessary.
(In reply to Dave Hunt (:davehunt) from comment #61)
> The test is simply about opening bookmarks one by one in separate tabs. The
> bookmarks toolbar and folder is an implementation choice (I personally
> expected this to use the main menu, but that isn't likely to make much
> difference). I don't see how a context menu was ever implied, and it
> certainly shouldn't be necessary.

Glad we settled it on good old iRC aka "problem fixer" 
In the meantime, it seems like the scrollable bookmarks toolbar menu is not a problem for the test 
http://mozmill-crowd.blargon7.com/#/endurance/report/a6839eac670d27cd915461a1da0150dd

Ran with 2 iterations and 40 entities, meaning 40 bookmarks, just enough to trigger the scrollbars
Attached patch patch v4.0 (obsolete) — — Splinter Review 
Changes addressed 
See previous comment for test results
We do not use context menu anymore, but open a new tab via tabBrowser.openTab() helper method
Attachment #646494 - Attachment is obsolete: true
Attachment #646587 - Flags: review?(dave.hunt)
Comment on attachment 646587 [details] [diff] [review]
patch v4.0

This is looking good. Just a few minor things left.

>+    controller.waitThenClick(testFolder);

I feel this should be inside the loop (sorry for not picking this up before).

>+    enduranceManager.loop(function () {              

There's still trailing whitespace here.

>+      var page = new elementslib.Selector(controller.window.document, 
>+                                          "*[label='Test Bookmark " + 
>+                                          enduranceManager.currentEntity + "']");

I think we should call this bookmark rather than page.

>+    // Dismiss the context menu
>+    controller.keypress(null , 'VK_ESCAPE', {});

I'm surprised that the click doesn't dismiss the menu... Can we also move this into the loop so the menu is closed between entities? Also, this isn't a context menu, so simply drop that from the comment.

>+    tabBrowser.closeAllTabs();
>+    enduranceManager.addCheckpoint("All tabs are closed");

Let's drop this checkpoint, as it will be immediately followed by an iteration end checkpoint anyway.

>+function setupBookmarks(aController) { 
>+  toolbars.enableBookmarksToolbar(aController);
>+  

Remove this whitespace. Blank lines should be empty.
Attachment #646587 - Flags: review?(dave.hunt) → review-
Attached patch patch v4.1 (obsolete) — — Splinter Review 
There is no one available at this hour to review my patch internally but judging there were simple changes to address, moving on with r from Dave 

This was tested on the default branch since we are going to land it there first
http://mozmill-crowd.blargon7.com/#/endurance/report/a6839eac670d27cd915461a1da19ed24
Attachment #646587 - Attachment is obsolete: true
Attachment #647086 - Flags: review?(dave.hunt)
Comment on attachment 647086 [details] [diff] [review]
patch v4.1

>+      controller.click(bookmark);
>+      controller.waitForPageLoad();
>+
>+      tabBrowser.openTab();

Sorry for not spotting this before, but we only want to open a new tab ahead of using it. As it is, we will open a new tab that is not used. See other endurance tests that open a new tab only in this is not the first entity:

  if (enduranceManager.currentEntity > 1) {
    tabBrowser.openTab();
  }

Otherwise, this looks good to me. I would like Henrik to take a look over this patch though, as I would expect the click on the menu would be necessary to click the menu item (it seems it is not), and the click on the menu item should really dismiss the menu.

Please update the patch and request final review from Henrik. Also, please check your patch for unnecessary whitespace.
Attachment #647086 - Flags: review?(dave.hunt) → review-
Attached patch patch v4.2 (obsolete) — — Splinter Review 
* change addressed 
* no more trailing spaces (hope so...) 

local testrun report
http://mozmill-crowd.blargon7.com/#/endurance/report/1cdc75cbd62095d2665556bbf400ecb1
Attachment #647086 - Attachment is obsolete: true
Attachment #647142 - Flags: review?(hskupin)
Attached patch patch v4.3 (obsolete) — — Splinter Review 
Yes, the 'ESC' is not necessary any more. 
Killed that code in this new patch
Attachment #647142 - Attachment is obsolete: true
Attachment #647142 - Flags: review?(hskupin)
Attachment #647164 - Flags: review?(hskupin)
Comment on attachment 647164 [details] [diff] [review]
patch v4.3

I think you've misunderstood me. The escape key should not be necessary because the bookmark folder menu should be hidden after a click on an individual bookmark. Can you please confirm that this is not the case? We may have an issue with clicking menu items where it is not effectively simulating a user click. I was only expecting the open tab fix and whitespace fixes, and hoping for feedback from Henrik on the menu click concerns.
Attachment #647164 - Flags: review?(hskupin) → review-
(In reply to Dave Hunt (:davehunt) from comment #69)
> Comment on attachment 647164 [details] [diff] [review]
> patch v4.3
> 
> I think you've misunderstood me. The escape key should not be necessary
> because the bookmark folder menu should be hidden after a click on an
> individual bookmark. Can you please confirm that this is not the case? We
> may have an issue with clicking menu items where it is not effectively
> simulating a user click. I was only expecting the open tab fix and
> whitespace fixes, and hoping for feedback from Henrik on the menu click
> concerns.

This is happening in all mozmill tests which use menus, not only in this one, and afair, it has always worked this way. Henrik can provide further clarifications to you, I'm sure.
Attached patch patch v4.4 (obsolete) — — Splinter Review 
* using esc key to dismiss the menu is back
Attachment #647164 - Attachment is obsolete: true
Attachment #647465 - Flags: review?(hskupin)
Attachment #647465 - Flags: feedback?(dave.hunt)
Comment on attachment 647465 [details] [diff] [review]
patch v4.4

>+      controller.waitThenClick(testFolder);
>+
>+      var bookmark = new elementslib.Selector(controller.window.document,
>+                                              "*[label='Test Bookmark " +
>+                                              enduranceManager.currentEntity + "']");
>+
>+      controller.click(bookmark);
>+      controller.waitForPageLoad();
>+
>+      // Dismiss the menu
>+      controller.keypress(null , 'VK_ESCAPE', {});

Clint: Is it expected that a click on a menu item (this is from a bookmark folder from the bookmarks toolbar) would not dismiss the menu as it would when an actual user clicks the item? It also appears that the menu doesn't even need to be open in order for the child item to be clicked. If this is a Mozmill limitation then I'm happy with this new test.
Attachment #647465 - Flags: review?(hskupin)
Attachment #647465 - Flags: review-
Attachment #647465 - Flags: feedback?(dave.hunt)
Attachment #647465 - Flags: feedback?(ctalbert)
Comment on attachment 647465 [details] [diff] [review]
patch v4.4

(In reply to Dave Hunt (:davehunt) from comment #72)
> Comment on attachment 647465 [details] [diff] [review]
> patch v4.4
> 
> >+      controller.waitThenClick(testFolder);
> >+
> >+      var bookmark = new elementslib.Selector(controller.window.document,
> >+                                              "*[label='Test Bookmark " +
> >+                                              enduranceManager.currentEntity + "']");
> >+
> >+      controller.click(bookmark);
> >+      controller.waitForPageLoad();
> >+
> >+      // Dismiss the menu
> >+      controller.keypress(null , 'VK_ESCAPE', {});
> 
> Clint: Is it expected that a click on a menu item (this is from a bookmark
> folder from the bookmarks toolbar) would not dismiss the menu as it would
> when an actual user clicks the item? It also appears that the menu doesn't
> even need to be open in order for the child item to be clicked. If this is a
> Mozmill limitation then I'm happy with this new test.

I think this test is fine. I think that mozmill should close the menu after simulating the click on it. If that isn't happening then it's a bug and should be opened separately. But I don't see why we'd block this test on that mozmill bug when simply simulating the ESC keypress is a simple, logical work around.  I'd just file the bug and add the bug number in a comment before the ESC press and get this test checked in.
Attachment #647465 - Flags: feedback?(ctalbert) → feedback+
Sounds good to me. Vlad: can you raise the bug as Clint suggests (preferably with a minimal failing testcase), and add an appropriate comment to your patch. Thanks.
* Comment dismissing of menus with ESC key since Mozmill does not do that atm
Attachment #647465 - Attachment is obsolete: true
Attachment #648661 - Flags: review?(dave.hunt)
Comment on attachment 648661 [details] [diff] [review]
patch v4.5 [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/6f4ed9189c7e (default)
Attachment #648661 - Flags: review?(dave.hunt) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I see no failures on default branch so I think we can check this test in for all branches 

http://mozmill-ci.blargon7.com/#/endurance/report/29fc09ba0c8360d637617903a01aeb83
(In reply to Dave Hunt (:davehunt) from comment #64)
> >+    // Dismiss the context menu
> >+    controller.keypress(null , 'VK_ESCAPE', {});
> 
> I'm surprised that the click doesn't dismiss the menu... Can we also move
> this into the loop so the menu is closed between entities? Also, this isn't
> a context menu, so simply drop that from the comment.

Items on the bookmarks toolbar are implemented differently. Means those react on mousedown/mouseup and not click. We have had to make some workarounds already. But there was no need yet to make use of the Menu API together. So I'm not surprised that we do not close the popup. As Clint suggested lets get this fixed in bug
Depends on: 780107
(In reply to Dave Hunt (:davehunt) from comment #78)
 
> Please provide a patch for mozilla-esr10 as this patch does not apply
> cleanly.

This might sound stupid but at the moment the test is failing on mozilla-esr10 branch only, with the error

"message": "Timeout exceeded for waitForElement Selector: *[label='Test Bookmark 1']", "fileName": "resource://mozmill/modules/utils.js", "name": "TimeoutError", "lineNumber": 447}}]

This means, and can be easily seen, that the test does not click the bookmarks folder created under bookmarks toolbar. The weird thing is that the locator are the same for all branches so I cannot say what is wrong here. 
I've tested the WIP patch on many machines and all systems and its the same situation with Firefox 10.0.6
Attached patch [mozilla-esr10] WIP patch (obsolete) — — Splinter Review 
Attaching a WIP patch for mozilla-esr10
Attachment #649986 - Flags: feedback?(hskupin)
Comment on attachment 649986 [details] [diff] [review]
[mozilla-esr10] WIP patch

There is really no need to request feedback on a patch which hasn't been changed according to the other branches. If it doesn't work, you should investigate the problem, and figure out why we do not click the element.
Attachment #649986 - Flags: feedback?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #82)
> Comment on attachment 649986 [details] [diff] [review]
> [mozilla-esr10] WIP patch
> 
> There is really no need to request feedback on a patch which hasn't been
> changed according to the other branches. If it doesn't work, you should
> investigate the problem, and figure out why we do not click the element.

At the moment I have concluded to no real reason why esr behaves this way. 
What I did is: 

* Check the locators see if they somehow changed - not the case (used DOM Inspector and Inspect Context to check that) 
* Check with previous versions of Mozmill until 1.5.14 - same error
* Check if we can click other elements in the toolbar - we can 
* Minimize the test to the bare minimum in which we click a folder under the Bookmarks Toolbar - the test is a pass but the actual click does not happen 

It remains to check with previous versions of esr branch and find a possible regression range. This is my P1 after lunch.
I would propose then to use controller.click() and add the expectedEvent as parameter. See: https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/js/controller/expected_events.js
(In reply to Henrik Skupin (:whimboo) from comment #84)
> I would propose then to use controller.click() and add the expectedEvent as
> parameter. See:
> https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/js/controller/
> expected_events.js

Thanks for the tip. 
I've used controller.click(testFolder, {type: "click"});
but had no luck identifying other error than the one mentioned
By using expected events you should definitely get another error, directly from the click() method. One thing to say is that it doesn't have to be the click event. It could also be mousedown or whatever. Better you check the source of the bookmarks toolbar how folders behave, or ask in #places.
(In reply to Henrik Skupin (:whimboo) from comment #86)
> By using expected events you should definitely get another error, directly
> from the click() method. One thing to say is that it doesn't have to be the
> click event. It could also be mousedown or whatever. Better you check the
> source of the bookmarks toolbar how folders behave, or ask in #places.

So the event is click, no doubt in that anymore. 
But after click, the folder does not expand - this can be easily verified by checking the 
"open" attribute

testFolder.getNode().hasAttribute("open") and this equals false all the time. 
It is very strange..
This sounds similar to bug 780107 but for the initial folder click. I'm not sure how much effort we should spend trying to get this working on ESR. Have you tried using mouseDown and mouseUp rather than click? Could you try previous versions of Firefox in case this is a regression?
(In reply to Dave Hunt (:davehunt) from comment #88)
> This sounds similar to bug 780107 but for the initial folder click. I'm not
> sure how much effort we should spend trying to get this working on ESR. Have
> you tried using mouseDown and mouseUp rather than click? Could you try
> previous versions of Firefox in case this is a regression?

works Dave! 

http://mozmill-crowd.blargon7.com/#/endurance/report/d87d47fd1034f072b9bece6ee6452a7e
* fixed
Attachment #649986 - Attachment is obsolete: true
Attachment #652394 - Flags: review?(hskupin)
Attachment #652394 - Flags: review?(dave.hunt)
Still I have no clue why click() won't work so what I did was just 'magic' code - see Dave's suggestion in comment 88.
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #91)
> Still I have no clue why click() won't work so what I did was just 'magic'
> code - see Dave's suggestion in comment 88.

Which I have already mentioned in comment 79. So there could have been a change in how the bookmarks toolbar react on mouse clicks since Firefox 11.
Comment on attachment 652394 [details] [diff] [review]
[mozilla-esr10] patch v1.0 [checked-in]

I think it's fine but I will leave it up to Dave for the final decision.
Attachment #652394 - Flags: review?(hskupin) → review+
Comment on attachment 652394 [details] [diff] [review]
[mozilla-esr10] patch v1.0 [checked-in]

Looks fine. Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/4380ab2e050a
Attachment #652394 - Flags: review?(dave.hunt) → review+
Attachment #648661 - Attachment description: patch v4.5 → patch v4.5 [checked-in]
Attachment #652394 - Attachment description: [mozilla-esr10] patch v1.0 → [mozilla-esr10] patch v1.0 [checked-in]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: