Closed Bug 705281 Opened 11 years ago Closed 11 years ago

Mozmill endurance test for open all bookmarks in tabs

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: vladmaniac, Assigned: vladmaniac)

Details

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

Attachments

(1 file, 3 obsolete files)

Create new mozmill endurance test for opening all bookmarks in tabs 

The test will use the following logic map:
    Setup Module
      1.Open X pages
      2.Save X pages as bookmarks in folder
    Main Test Checkpoints
      1.Open all bookmarks in folder as tabs
      2.Close all tabs
    Teardown Module
      1.Force close tabs
Whiteboard: [mozmill-endurance]
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
Whiteboard: [mozmill-endurance] → [mozmill-endurance][mozmill-bookmarks]
Here are some details about our logic map: 

Setup Module
  1. Open X pages  -  DOABLE 
  2. Save X pages as bookmarks in folder - we can use awesome bar to bookmark and automatically work with 'Unsorted bookmarks' folder - if we don't want to create a custom folder then DOABLE 

Main Test Checkpoints
  1. Open all bookmarks in folder as tabs - needs access to the Library - no library API yet - NOT DOABLE. 

Solutions: 
- we have prepared a UI map for a library API which we can share with whomever will want to help here 
- develop the shared module myself but it will take time and lose focus of the P1 jobs 
- try to have a test without a shared module support 

Which of the above do we choose? Please, flood in with opinions.
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #1)
> Main Test Checkpoints
>   1. Open all bookmarks in folder as tabs - needs access to the Library - no
> library API yet - NOT DOABLE. 

Store the bookmarks in the toolbar and you will be able to use the context menu to open all the bookmarks under this folder.
+1 to Henrik's suggestion.
Attached patch patch v1.0 (obsolete) — Splinter Review
Initial patch
Attachment #579613 - Flags: review?(anthony.s.hughes)
Comment on attachment 579613 [details] [diff] [review]
patch v1.0

Can you extract the bulk of the code used in setupModule() to a helper function (ie. the code used to build the bookmarks)?
Attachment #579613 - Flags: review?(anthony.s.hughes) → review-
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #5)
> Comment on attachment 579613 [details] [diff] [review]
> patch v1.0
> 
> Can you extract the bulk of the code used in setupModule() to a helper
> function (ie. the code used to build the bookmarks)
Of course, this test was tricky and I needed to come up with something robust and to address the endurance test needs as in comment 1 

The idea of simplifying the code in the setupModule was expected, and it is imminent
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #5)
> Comment on attachment 579613 [details] [diff] [review]
> patch v1.0
> 
> Can you extract the bulk of the code used in setupModule() to a helper
> function (ie. the code used to build the bookmarks)?

Thought more and i don't see the value of having the 'bookmarking code' in an API because its done in one line in the test: 

    places.bookmarksService.insertBookmark(customFolder, URI, defaultIndex,  
                                           "Entity no: " + 
                                           enduranceManager.currentEntity);
The rest are just parameters we need to define anyway in the test. 

the setupModule needs to contain: 
* trigger the bookmarks toolbar 
* create bookmarks in a custom subfolder saved in the bookmarks toolbar folder 

None of the above really apply to a shared module..right? I mean its no sense to duplicate the code of inserting a bookmark twice, this is why we use the bookmarksService API for.
Sorry if I wasn't clear, but I'm not talking about adding a helper function to the API; I want to see the extra shared module code extracted to a helper function within the test file. There is precedent for this, we usually put them at the bottom of some tests which need them.
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #8)
> Sorry if I wasn't clear, but I'm not talking about adding a helper function
> to the API; I want to see the extra shared module code extracted to a helper
> function within the test file. There is precedent for this, we usually put
> them at the bottom of some tests which need them.

Are we still doing this? Henrik said at some point that we encourage the test to have the setupModule, teardownModule and test<something> function. 

It's no problem adding that, i just want to do the right thing
This is a special case which requires it. It's not appropriate for the API and it makes the test easier to follow if it's in a helper function in the test.

It's my call and I say do it.

Thanks.
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #10)
> This is a special case which requires it. It's not appropriate for the API
> and it makes the test easier to follow if it's in a helper function in the
> test.
> 
> It's my call and I say do it.
> 
> Thanks.

cool, will do :)
Attached patch patch 2.0 (obsolete) — Splinter Review
Changed to insert a bookmark in a specified folder using a inside-test helper function. 

Still, we need to keep stuff in the setup module like: 
* trigger the toolbar (needs to be done in setup, only once across all test iterations) 
* create the custom folder (we create the folder on startup only once) 
* callback insertBookmarkInFolder(aUri, aFolder, aIndex) in a loop with the index being the entity number
Attachment #579613 - Attachment is obsolete: true
Attachment #580369 - Flags: review?(anthony.s.hughes)
Comment on attachment 580369 [details] [diff] [review]
patch 2.0

>+  // Enable bookmarks toolbar 
>+  var navbar = new elementslib.ID(controller.window.document, "nav-bar");
>+
>+  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);
>+  
>+  // Create a custom folder in Bookmarks Toolbar
>+  var toolbarFolder = places.bookmarksService.toolbarFolder;
>+  var defaultIndex = places.bookmarksService.DEFAULT_INDEX;
>+  var customFolder = places.bookmarksService.createFolder(toolbarFolder, 
>+                                                          "Test Folder", 
>+                                                          defaultIndex);
>+
>+  for (var i = 0; i < enduranceManager.entities; i++) {
>+    var URI = utils.createURI(LOCAL_TEST_PAGE + i);
>+    
>+    // Insert bookmark under the specified folder
>+    insertBookmarkInFolder(URI, customFolder, defaultIndex);
>+  }

I still think all of this can be contained within an setupTestBookmarks() helper which returns true on success. Your following code can be contained within this function too.

>+/*
>+ * Insert a bookmark in a custom folder under Bookmarks Toolbar  
>+ */
>+function insertBookmarkInFolder(aURI, aFolder, aIndex) {
>+  // Bookmark page and save in a custom folder 
>+  places.bookmarksService.insertBookmark(aFolder, aURI, aIndex,  
>+                                         "Entity no: " + 
>+                                         enduranceManager.currentEntity);
>+
>+  // Polling the bookmarks service if such a bookmark has been added
>+  controller.waitFor(function () {
>+    return places.bookmarksService.isBookmarked(aURI);
>+  }, "The bookmark was created");   
>+}
Attachment #580369 - Flags: review?(anthony.s.hughes) → review-
Attached patch patch v3.0 (obsolete) — Splinter Review
Ported all code in the helper function 

Named setupBookmarks 

The function does not return boolean - makes no sense since the waitFor in it returns true is the bookmarking process is successful
Attachment #580369 - Attachment is obsolete: true
Attachment #581210 - Flags: review?(anthony.s.hughes)
Comment on attachment 581210 [details] [diff] [review]
patch v3.0

Overall this looks good. One suggestion, I would change "Entity no: " to "Test Bookmark #"
Attachment #581210 - Flags: review?(anthony.s.hughes) → review-
nit fixed
Attachment #581210 - Attachment is obsolete: true
Attachment #581557 - Flags: review?(anthony.s.hughes)
Comment on attachment 581557 [details] [diff] [review]
patch v4.0: all [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/173cb424819b (default)
Attachment #581557 - Attachment description: patch v4.0 → patch v4.0: all [checked-in:default]
Attachment #581557 - Flags: review?(anthony.s.hughes) → review+
Please verify with tomorrow's endurance testrun to verify for landing on other branches.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #18)
> Please verify with tomorrow's endurance testrun to verify for landing on
> other branches.

http://mozmill-release.brasstacks.mozilla.com/#/endurance/report/af57f09d864178a273023d90695839f3

It seems it does not fail so marking as verified
Status: RESOLVED → VERIFIED
We can land on other branches - 

not sure if i did right to put this bug on verified yet - please change if this is a mistake
Interesting -- this appears to have taken the top spot for memory consumption on Windows and Linux. Not necessarily concerning, just interesting. :)
(In reply to Dave Hunt (:davehunt) from comment #21)
> Interesting -- this appears to have taken the top spot for memory
> consumption on Windows and Linux. Not necessarily concerning, just
> interesting. :)

I would have expected this test to be memory consuming - we even have a pref setup in config to warn users that opening many tabs at once can consume memory - (see the test code, the case is addressed)
We have other endurance tests that open just as many tabs, but just not in this way.
We are only opening our test pages, right? That shouldn't be that memory intensive. Do we already have some results and graphs on brasstacks?
Like I said, this isn't necessarily concerning. I was simply observing that this test uses marginally more memory than the others when run on Windows. Reports are available on mozmill-release for 11.0.
This should not be marked verified until landed and passing on all branches.
Status: VERIFIED → RESOLVED
Closed: 11 years ago11 years ago
Comment on attachment 581557 [details] [diff] [review]
patch v4.0: all [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/173cb424819b (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/540d9ff7d352 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/f82721305d44 (mozilla-release)

Please verify this passes on all branches in tomorrow's testruns.
Attachment #581557 - Attachment description: patch v4.0: all [checked-in:default] → patch v4.0: all [checked-in]
Does not fail - marking as verified
Status: RESOLVED → VERIFIED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.