636191 - Mozmill Endurance test for launching new tabs from Panorama

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

+++ This bug was initially created as a clone of Bug #636080 +++

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. Open Panorama view
3. Click the + button in the tab group
4. Repeat from step 2 until 100 tabs are open

This test should be able to chart memory footprint before, during, and after.

Comment 1

[u279076]

Attachment: Patch v1

Comment 2

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

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

Looks good. Henrik could you please review?

Results for 100 iterations can be found here:
http://mozmill.blargon7.com/#/endurance/report/fbec1ff25afea97c50cc0889e2002e18

Comment 3

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

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

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

Can we stick with 'open' and not 'create'? Should probably also apply to the other patch I have reviewed a minute ago.

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

Imported module names should not use capital letters at the beginning of the name.

>+function setupModule() {
>+  controller = mozmill.getBrowserController();
>+  enduranceManager = new Endurance.EnduranceManager(controller);
>+  
>+  tabBrowser = new Tabs.tabBrowser(controller);
>+  tabBrowser.closeAllTabs();
>+  
>+  tabView = new TabView.tabView(controller);

nit: It would be cleaner if all instantiations of classes would happen right after each other, followed by the single closeAllTabs call. 

>+function testCreateNewTabs() {

Create vs. Open

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

It's not a new tab.

>+    // Open a new tab via Panorama
>+    enduranceManager.addCheckpoint("Open a new tab via Panorama");
>+    tabView.open();

Do we also want to measure how long it takes to open the TabView?

>+    // Click the new tab button for the active group
>+    var group = tabView.getElement({
>+      type: "groups",
>+      subtype: "active"
>+    });

That should be tabView.getGroups({filter: "active"});

>+    var newTabButton = tabView.getElement({
>+      type: "group_newTabButton",
>+      parent: group
>+    });
>+    tabView.controller.click(newTabButton);

That's tabView.open({group: activeGroup});

Comment 4

[u279076]

(In reply to comment #3)
> Comment on attachment 514990 [details] [diff] [review]
> Patch v1
> 
> >+++ b/firefox/enduranceTests/testTabbedBrowsing/testCreateNewTabInPanorama.js
> 
> Can we stick with 'open' and not 'create'? Should probably also apply to the
> other patch I have reviewed a minute ago.

What's the difference?  Please explain.

> >+    enduranceManager.addCheckpoint("Load a web page in a new tab");
> >+    controller.open(LOCAL_TEST_PAGE);
> 
> It's not a new tab.

It's the new tab which is created at the end of the previous iteration.

> >+    // Click the new tab button for the active group
> >+    var group = tabView.getElement({
> >+      type: "groups",
> >+      subtype: "active"
> >+    });
> 
> That should be tabView.getGroups({filter: "active"});

I tried using that method and it fails to get the active tab group.  This is the only method which works.

> >+    var newTabButton = tabView.getElement({
> >+      type: "group_newTabButton",
> >+      parent: group
> >+    });
> >+    tabView.controller.click(newTabButton);
> 
> That's tabView.open({group: activeGroup});

Should it not be the variable that I've assigned the group to? As you've already seen in the code above, I call it "group" not "activeGroup".

Comment 5

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

(In reply to comment #4)
> > Can we stick with 'open' and not 'create'? Should probably also apply to the
> > other patch I have reviewed a minute ago.
> 
> What's the difference?  Please explain.

Ok, lets keep it as it is. Just checked again our existing tab browser tests and open we should only use when a link gets opened in a new tab.

> > >+    enduranceManager.addCheckpoint("Load a web page in a new tab");
> > >+    controller.open(LOCAL_TEST_PAGE);
> > 
> > It's not a new tab.
> 
> It's the new tab which is created at the end of the previous iteration.

That's the previous iteration, so it's not new. But we can use "blank" instead.

> > That should be tabView.getGroups({filter: "active"});
> 
> I tried using that method and it fails to get the active tab group.  This is
> the only method which works.

What failed? If it is not working then soemthing in the code base has been changed and the API needs to be updated. But don't add workarounds for code we own ourselves. Run the test for the module under helperClasses to verify.

> > That's tabView.open({group: activeGroup});
> 
> Should it not be the variable that I've assigned the group to? As you've
> already seen in the code above, I call it "group" not "activeGroup".

That was just an example.

Comment 6

[u279076]

Attachment: Patch v1.1

Tested patch and it works beautifully with the recent tabViewAPI changes.

Comment 7

[u279076]

Comment on attachment 516630 [details] [diff] [review]
Patch v1.1

Sorry, I forgot something on this patch.  Followup coming.

Comment 8

[u279076]

Attachment: Patch v1.2

Comment 9

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

Comment on attachment 516631 [details] [diff] [review]
Patch v1.2

>+  activeTabView = new tabView.tabView(controller);

Just a note... that's a real downside of having a small first letter for module variables. 

>+    enduranceManager.addCheckpoint("Load a web page in a new tab");

Can you please do the changes as you have made for:
http://hg.mozilla.org/qa/mozmill-tests/rev/eeb8014a7a46

>+    enduranceManager.addCheckpoint("Open a new tab via Panorama");

Anthony, can you please standardize the name we want to use for Panorama/TabView/Tab Groups tests? We should use the same name across the tests to avoid confusion. Aaron has checked in a patch today which mentioned Tab Groups, as listed in the all tabs menu.

I would love to see this test checked in before our testday tomorrow. Would that be possible Anthony?

Comment 10

[u279076]

Attachment: Patch v1.3 [checked-in]

Comment 11

[u279076]

Comment on attachment 516721 [details] [diff] [review]
Patch v1.3 [checked-in]

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