636080 - Mozmill Endurance test for launching new tabs in main window

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Create a Mozmill Endurance test for memory footprint of opening new tabs in the main window vs panorama view.

Steps:
1. Start a new session
2. Iteratively open 100 new tabs via + button in tab bar
3. Close Firefox
4. Start a new session
5. Open Panorama view
6. Click the + button in the tab group
7. Repeat from step 5 until 100 tabs are open

This test should be able to chart memory footprint before, during, and after for main window vs panorama.

Comment 1

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

This sounds like two endurance tests, which could be run separately and then the results can be compared.

Comment 2

[u279076]

Forked "panorama" aspect of this test to bug 636191.

Comment 3

[u279076]

Attachment: Patch v1

First attempt at an endurance test to open 100 new tabs and load a page in each.

Comment 4

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

Comment on attachment 514634 [details] [diff] [review]
Patch v1

+/**
+ * Test opening 100 new tabs from the main window
+ **/

We should allow the iteration parameter specified on the command line determine the number of iterations rather than specify this in the test. 

+function testCreateNewTabs() {
+  enduranceManager.addCheckpoint("Start opening 100 new tabs");

A checkpoint will be added at the start and end of each iteration so this checkpoint is not necessary.

+  // Open 100 new tabs loading a page in each
+  enduranceManager._iterations = 100;

As mentioned, we should let the parameter from the command line control the number of iterations.

+  enduranceManager.run(function () {
+    controller.open(LOCAL_TEST_PAGE);
+    controller.waitForPageLoad();
+    tabBrowser.openTab();
+  });

Consider adding checkpoints for page loaded and new tab opened.

+  enduranceManager.addCheckpoint("Stop opening 100 new tabs");

As with the initial checkpoint, this is not necessary.

Comment 5

[u279076]

Attachment: Patch v2

Comment 6

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

Comment on attachment 514830 [details] [diff] [review]
Patch v2

Looks good. Henrik, please could you give this a review?

Results for iterating 100 times can be seen here:
http://mozmill.blargon7.com/#/endurance/report/fbec1ff25afea97c50cc0889e2002569

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Comment on attachment 514830 [details] [diff] [review]
Patch v2

>+// Include the required modules
>+var Endurance = require("../../../shared-modules/endurance");
>+var Tabs = require("../../../shared-modules/tabs");

As discussed lately and AFAIR we agreed on to use no capital characters for the imported modules.

>+/**
>+ * Test opening new tabs from the main window
>+ **/
>+function testCreateNewTabs() {  

Can we update the function name to testOpenNewTabs, as what we are doing here?

>+    enduranceManager.addCheckpoint("Loading a web page in a new tab");
>+    controller.open(LOCAL_TEST_PAGE);

This page gets loaded in the current tab and not in a new tab.

r- only because of the misleading description of the checkpoint message.

Comment 8

[u279076]

(In reply to comment #7)
> Comment on attachment 514830 [details] [diff] [review]
> Patch v2
> 
> >+// Include the required modules
> >+var Endurance = require("../../../shared-modules/endurance");
> >+var Tabs = require("../../../shared-modules/tabs");
> 
> As discussed lately and AFAIR we agreed on to use no capital characters for the
> imported modules.

Yup, I missed that. I copied this from the tests Dave had already contributed. I assumed this was the way he wanted to do things. I'll revert this and all future tests to use lower-case module names.

> 
> >+/**
> >+ * Test opening new tabs from the main window
> >+ **/
> >+function testCreateNewTabs() {  
> 
> Can we update the function name to testOpenNewTabs, as what we are doing here?

I'm not sure what the concern is here, since we are indeed "creating" a new tab.  If I rename the function, I'll have to rename the file as well.  Is it really that important?  If you are confused by the use of "opening" in the comment vs "create" in the test, I'd be happy to change the wording in the comment.  It's a much more trivial change than what you propose.  Really, create and open mean the same thing in this instance.

> 
> >+    enduranceManager.addCheckpoint("Loading a web page in a new tab");
> >+    controller.open(LOCAL_TEST_PAGE);
> 
> This page gets loaded in the current tab and not in a new tab.

I disagree. The page gets loaded in the new tab created at the end of the previous iteration.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

(In reply to comment #8)
> comment vs "create" in the test, I'd be happy to change the wording in the
> comment.  It's a much more trivial change than what you propose.  Really,
> create and open mean the same thing in this instance.

Keep it as it is. See my explanation on the other bug. It's fine to have create here.

> > >+    enduranceManager.addCheckpoint("Loading a web page in a new tab");
> > >+    controller.open(LOCAL_TEST_PAGE);
> > 
> > This page gets loaded in the current tab and not in a new tab.
> 
> I disagree. The page gets loaded in the new tab created at the end of the
> previous iteration.

Then use 'blank' or remove the second part completely and just call it 'loading a web page'. It's different from performing an action like 'open as new tab'.

Comment 10

[u279076]

Attachment: Patch v2.1

Suggested revisions have been made.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Comment on attachment 516299 [details] [diff] [review]
Patch v2.1

>+++ b/firefox/enduranceTests/testTabbedBrowsing/testOpenNewTab.js

As said on the other bug (sorry that I haven't repeated it here) I have agreed on testCreateNewTab. So whatever you want to use, feel free to revert it.

>+var Endurance = require("../../../shared-modules/endurance");
>+var Tabs = require("../../../shared-modules/tabs");

nit: you missed to update those two lines and make the first letter lowercase.

With the nit updated r=me.

Comment 12

[u279076]

> As said on the other bug (sorry that I haven't repeated it here) I have agreed
> on testCreateNewTab. So whatever you want to use, feel free to revert it.

I decided to go with "Open" instead.  I've made this change across the board.

> >+var Endurance = require("../../../shared-modules/endurance");
> >+var Tabs = require("../../../shared-modules/tabs");
> 
> nit: you missed to update those two lines and make the first letter lowercase.

Oops.  I forgot, thanks for catching it.

Comment 13

[u279076]

Attachment: Patch v2.2 [checked-in]

Comment 14

[u279076]

Comment on attachment 516323 [details] [diff] [review]
Patch v2.2 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/eeb8014a7a46 [default]