705281 - Mozmill endurance test for open all bookmarks in tabs

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

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.

Comment 2

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

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

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

+1 to Henrik's suggestion.

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0

Initial patch

Comment 5

[u279076]

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)?

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

(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

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 8

[u279076]

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.

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

(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

Comment 10

[u279076]

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.

Comment 11

[Maniac Vlad Florin (:vladmaniac)]

(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 :)

Comment 12

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch 2.0

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

Comment 13

[u279076]

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");   
>+}

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.0

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

Comment 15

[u279076]

Comment on attachment 581210 [details] [diff] [review]
patch v3.0

Overall this looks good. One suggestion, I would change "Entity no: " to "Test Bookmark #"

Comment 16

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v4.0: all [checked-in]

nit fixed

Comment 17

[u279076]

Comment on attachment 581557 [details] [diff] [review]
patch v4.0: all [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/173cb424819b (default)

Comment 18

[u279076]

Please verify with tomorrow's endurance testrun to verify for landing on other branches.

Comment 19

[Maniac Vlad Florin (:vladmaniac)]

(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

Comment 20

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Interesting -- this appears to have taken the top spot for memory consumption on Windows and Linux. Not necessarily concerning, just interesting. :)

Comment 22

[Maniac Vlad Florin (:vladmaniac)]

(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)

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

We have other endurance tests that open just as many tabs, but just not in this way.

Comment 24

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

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?

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 26

[u279076]

This should not be marked verified until landed and passing on all branches.

Comment 27

[u279076]

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.

Comment 28

[Maniac Vlad Florin (:vladmaniac)]

Does not fail - marking as verified