Closed Bug 636080 Opened 9 years ago Closed 9 years ago

Mozmill Endurance test for launching new tabs in main window

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ashughes, Assigned: ashughes)

References

Details

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

Attachments

(1 file, 3 obsolete files)

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.
This sounds like two endurance tests, which could be run separately and then the results can be compared.
Blocks: 629065
Forked "panorama" aspect of this test to bug 636191.
Summary: Mozmill Endurance test for launching new tabs → Mozmill Endurance test for launching new tabs in main window
Attached patch Patch v1 (obsolete) — Splinter Review
First attempt at an endurance test to open 100 new tabs and load a page in each.
Attachment #514634 - Flags: review?(dave.hunt)
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.
Attachment #514634 - Flags: review?(dave.hunt) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #514634 - Attachment is obsolete: true
Attachment #514830 - Flags: review?(dave.hunt)
Attachment #514830 - Attachment is patch: true
Attachment #514830 - Attachment mime type: application/octet-stream → text/plain
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
Attachment #514830 - Flags: review?(hskupin)
Attachment #514830 - Flags: review?(dave.hunt)
Attachment #514830 - Flags: review+
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.
Attachment #514830 - Flags: review?(hskupin) → review-
(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.
(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'.
Attached patch Patch v2.1 (obsolete) — Splinter Review
Suggested revisions have been made.
Attachment #514830 - Attachment is obsolete: true
Attachment #516299 - Flags: review?(hskupin)
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.
Attachment #516299 - Flags: review?(hskupin) → review+
> 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.
Attachment #516299 - Attachment is obsolete: true
Attachment #516323 - Flags: review+
Comment on attachment 516323 [details] [diff] [review]
Patch v2.2 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/eeb8014a7a46 [default]
Attachment #516323 - Attachment description: Patch v2.2 → Patch v2.2 [checked-in]
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Whiteboard: [mozmill-endurance][mozmill-tabbedbrowsing]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.