Mozmill Endurance test for pinning App Tabs

VERIFIED FIXED

Status

Mozilla QA
Mozmill Tests
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: ashughes, Assigned: davehunt)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-endurance])

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

6 years ago
This is a tracking bug to develop an endurance test based on the functional test for pinning App Tabs (bug 648380).
Created attachment 545266 [details] [diff] [review]
TabPinning Endurance test
Attachment #545266 - Flags: review?
(Reporter)

Comment 2

6 years ago
Comment on attachment 545266 [details] [diff] [review]
TabPinning Endurance test

Don't forget to add the name of the reviewer when asking for review.
Attachment #545266 - Flags: review? → review?(anthony.s.hughes)
(Reporter)

Comment 3

6 years ago
Comment on attachment 545266 [details] [diff] [review]
TabPinning Endurance test

>+// Include required modules
>+var tabs = require("../../../lib/tabs");
>+var Endurance = require("../../../lib/endurance");

var endurance not Endurance

>+/**
>+ * Tests pinning and unpinning a tab successfully in a single window
>+ */

"Tests pinning and unpinning a single tab"

>+    var contextMenu = tabBrowser.controller.getMenu("#tabContextMenu");
>+    var currentTab = tabBrowser.getTab(tabBrowser.length - 1);

Unnecessary since opening a new tab makes that tab active by default.

>+    // Tab has been pinned

"Check performance of pinning a tab"

>+    enduranceManager.addCheckpoint("Pin a tab");
>+    contextMenu.select("#context_pinTab", currentTab);
>+    enduranceManager.addCheckpoint("Tab has been pinned");
>+
>+    // Tab has been unpinned

"Check performance of unpinning a tab"
Attachment #545266 - Flags: review?(anthony.s.hughes) → review-
(In reply to comment #2)
> Comment on attachment 545266 [details] [diff] [review] [review]
> TabPinning Endurance test
> 
> Don't forget to add the name of the reviewer when asking for review.

Ah, due to a small interruption, I was a tad late in adding your name but clicked submit by then. will keep that in mind.
Created attachment 545276 [details] [diff] [review]
TabPinning Endurance test

Addressed the changes.
Attachment #545266 - Attachment is obsolete: true
Attachment #545276 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 6

6 years ago
Comment on attachment 545276 [details] [diff] [review]
TabPinning Endurance test

>diff --git a/tests/endurance/testTabbedBrowsing_TabPinning/test1.js b/tests/endurance/testTabbedBrowsing_TabPinning/test1.js
>new file mode 100644
>--- /dev/null
>+++ b/tests/endurance/testTabbedBrowsing_TabPinning/test1.js
>@@ -0,0 +1,74 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is MozMill Test code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *  Shriram Kunchanapalli <kshriram18@gmail.com> (original author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+ 
>+// Include required modules
>+var tabs = require("../../../lib/tabs");
>+var endurance = require("../../../lib/endurance");
>+
>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');
>+const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'layout/mozilla.html';
>+
>+function setupModule(module) {
>+  controller = mozmill.getBrowserController();
>+  tabBrowser = new tabs.tabBrowser(controller);
>+  enduranceManager = new endurance.EnduranceManager(controller);
>+}
>+
>+function teardownModule(module) {
>+  tabBrowser.closeAllTabs();
>+}
>+
>+/**
>+ * Tests pinning and unpinning a single tab
>+ */
>+function testTabPinning() {
>+  enduranceManager.run(function () {
>+    //open a new Tab, load a Test Page and wait for it to load
>+    tabBrowser.openTab();
>+    tabBrowser.controller.open(LOCAL_TEST_PAGE);
>+    tabBrowser.controller.waitForPageLoad();
>+
>+    // Check performance of pinning a tab
>+    enduranceManager.addCheckpoint("Pin a tab");
>+    contextMenu.select("#context_pinTab", currentTab);
>+    enduranceManager.addCheckpoint("Tab has been pinned");
>+
>+    // Check performance of unpinning a tab
>+    enduranceManager.addCheckpoint("Unpin a tab");
>+    contextMenu.select("#context_unpinTab", currentTab);
>+    enduranceManager.addCheckpoint("Tab has been unpinned");	
>+  });
>+}
Attachment #545276 - Flags: review?(dave.hunt)
Attachment #545276 - Flags: review?(anthony.s.hughes)
Attachment #545276 - Flags: review+
(Reporter)

Comment 7

6 years ago
Code-wise this looks good to me -- I haven't tested it yet though. Sending to Dave Hunt for follow up. Dave, do we want to have a micro-iteration here to test multiple tab pinning or should that be handled in a follow-up test?
(In reply to comment #6)
> >+    contextMenu.select("#context_pinTab", currentTab);
> >+    enduranceManager.addCheckpoint("Tab has been pinned");
> >+
> >+    // Check performance of unpinning a tab
> >+    enduranceManager.addCheckpoint("Unpin a tab");
> >+    contextMenu.select("#context_unpinTab", currentTab);

We could safe 1 checkpoint here. It's the same time and doesn't give us additional information.
(Assignee)

Comment 9

6 years ago
Comment on attachment 545276 [details] [diff] [review]
TabPinning Endurance test

+++ b/tests/endurance/testTabbedBrowsing_TabPinning/test1.js

Can we rename this test's directory to testTabbedBrowsing_PinAndUnpinAppTab

+function setupModule(module) {
+  controller = mozmill.getBrowserController();
+  tabBrowser = new tabs.tabBrowser(controller);
+  enduranceManager = new endurance.EnduranceManager(controller);
+}

Please add tabBrowser.closeAllTabs(); to setupModule.

+/**
+ * Tests pinning and unpinning a single tab
+ */
+function testTabPinning() {

Rename the test to testPinAndUnpinAppTab

+  enduranceManager.run(function () {
+    //open a new Tab, load a Test Page and wait for it to load
+    tabBrowser.openTab();
+    tabBrowser.controller.open(LOCAL_TEST_PAGE);
+    tabBrowser.controller.waitForPageLoad();

We don't need to open a new tab -- we can just work with the existing tab. Also, please move the open and waitForPageLoad outside the iterations (before enduranceManager.run)

+    // Check performance of pinning a tab
+    enduranceManager.addCheckpoint("Pin a tab");
+    contextMenu.select("#context_pinTab", currentTab);

This fails because contextMenu and currentTab are not defined. Looks like you had it in the previous version of your patch.

> > >+    enduranceManager.addCheckpoint("Tab has been pinned");
> > >+
> > >+    // Check performance of unpinning a tab
> > >+    enduranceManager.addCheckpoint("Unpin a tab");
> > >+    contextMenu.select("#context_unpinTab", currentTab);
> 
> We could safe 1 checkpoint here. It's the same time and doesn't give us
> additional information.

Henrik is right, perhaps we should change the in-between checkpoint message to "Tab has been pinned. Unpinning tab." to indicate what has occurred and what will follow.
Attachment #545276 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 10

6 years ago
(In reply to comment #7)
> Code-wise this looks good to me -- I haven't tested it yet though. Sending
> to Dave Hunt for follow up. Dave, do we want to have a micro-iteration here
> to test multiple tab pinning or should that be handled in a follow-up test?

If we had micro-iterations for this test we would need to use them twice (once for opening and pinning additional tabs, and once for unpinning all those tabs). I think a follow-up test that simply pins a tab would be more suitable, that way the test can ping multiple tabs and not worry about unpinning them.
Created attachment 545611 [details] [diff] [review]
TabPinning Endurance test

Renamed file, tests on existing tab, and made other appropriate changes
Attachment #545276 - Attachment is obsolete: true
Attachment #545611 - Flags: review?(dave.hunt)
(In reply to comment #11)
> Created attachment 545611 [details] [diff] [review] [review]
> TabPinning Endurance test
> 
> Renamed file, tests on existing tab, and made other appropriate changes
*Renamed folder name
(Assignee)

Comment 13

6 years ago
Comment on attachment 545611 [details] [diff] [review]
TabPinning Endurance test

+  enduranceManager.run(function () {
+    var contextMenu = tabBrowser.controller.getMenu("#tabContextMenu");
+    var currentTab = tabBrowser.getTab(0);

We can define contextMenu and currentTab outside the iteration.

Otherwise, looks good to me.
Attachment #545611 - Flags: review?(hskupin)
Attachment #545611 - Flags: review?(dave.hunt)
Attachment #545611 - Flags: review+
Attachment #545611 - Flags: review?(hskupin)
Created attachment 545664 [details] [diff] [review]
TabPinning Endurance test

Addressed that change
Attachment #545611 - Attachment is obsolete: true
Attachment #545664 - Flags: review?(dave.hunt)
(Assignee)

Comment 15

6 years ago
Comment on attachment 545664 [details] [diff] [review]
TabPinning Endurance test

Please add a commit message as explained here: https://developer.mozilla.org/en/Mozmill_Tests#Commit_Message

Also, let's add micro-iterations to this test. It will involve two loops within the iteration:

1. Open new tab, Open local test page, Pin tab
2. Unpin tab

Also, we need to make sure we don't open the new tab in the first micro-iteration as we already have a tab open.

I've updated the documentation with details for micro-iterations:
https://wiki.mozilla.org/QA/Mozmill_Test_Automation/Endurance_Tests/Documentation
Attachment #545664 - Flags: review?(dave.hunt) → review-
Created attachment 545986 [details] [diff] [review]
TabPinning Endurance test
Attachment #545664 - Attachment is obsolete: true
Attachment #545986 - Flags: review?(dave.hunt)
(Assignee)

Comment 17

6 years ago
Comment on attachment 545986 [details] [diff] [review]
TabPinning Endurance test

> # HG changeset patch
> # Parent b7a57d531abe9722d169172ac12793521d50f732
> # User Shriram Kunchanapalli <kshriram18@gmail.com>
> Bug 670721 - Introduced MicroIterations in test1.js. r=dave.hunt

The message should be "Bug 670721 - Mozmill Endurance test for pinning App Tabs. r=ahughes r=dhunt"

> +/**
> + * Tests pinning and unpinning a single tab
> + */

As we're now potentially pinning multiple tabs we should change the description to simply "Tests pinning and unpinning app tabs"

> +function testPinAndUnpinAppTab() {
> +  enduranceManager.run(function () {
> +    for (var i = 0; i < enduranceManager.microIterations; i++) {
> +      // Check performance of pinning and unpinning a tab	
> +      enduranceManager.addCheckpoint("Open a new Tab. Load local test page. Pin a tab");	
> +      //open a new tab
> +	  if (i != 0)
> +	    tabBrowser.openTab();
> +      //load a Test Page and wait for it to load
> +      tabBrowser.controller.open(LOCAL_TEST_PAGE);
> +      tabBrowser.controller.waitForPageLoad();
> +      var contextMenu = tabBrowser.controller.getMenu("#tabContextMenu");
> +      var currentTab = tabBrowser.getTab(tabBrowser.length-1);
> +      contextMenu.select("#context_pinTab", currentTab);
> +      enduranceManager.addCheckpoint("Tab has been pinned. Unpinning tab.");
> +      contextMenu.select("#context_unpinTab", currentTab);
> +      enduranceManager.addCheckpoint("Tab has been unpinned");
> +	}
> +  });

This isn't quite what I had in mind. With this code the micro-iterations are no different from the iterations. What we want is a loop to pin all the tabs, and then a loop to unpin them all. I've also been thinking about this some more and think we should start with a loop that opens all the tabs.

I managed to get a working version based on your patch, which I'll attach so you can see what I mean about the various loops.
Attachment #545986 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 18

6 years ago
Created attachment 546012 [details]
Working example with three loops for micro-iterations

Note that this is a working example, but fails once the number of micro-iterations means that tabs scroll out of view - we may need some advice on working around this issue.
(In reply to comment #17)
> Comment on attachment 545986 [details] [diff] [review] [review]
> TabPinning Endurance test
> 
> > # HG changeset patch
> > # Parent b7a57d531abe9722d169172ac12793521d50f732
> > # User Shriram Kunchanapalli <kshriram18@gmail.com>
> > Bug 670721 - Introduced MicroIterations in test1.js. r=dave.hunt
> 
> The message should be "Bug 670721 - Mozmill Endurance test for pinning App
> Tabs. r=ahughes r=dhunt"

https://developer.mozilla.org/en/Mozmill_Tests mentioned that the description should include the changes made. So, I thought it should be more specific, and didn't summarize it(anyway it could include unpinning as well). I understand now.

> This isn't quite what I had in mind. With this code the micro-iterations are
> no different from the iterations. 
The moment I read the "this isn't...mind" line, I realized that I had to loop it twice #1(pinning) and #2(unpinning) separately. I clearly misunderstood the purpose of microiterations. Later, when I looked at your working example, I couldn't get why first loop's needed for opening the tabs but now I do.
Where does it exactly fail?  
Did you mean that it's not possible to see(when tabs scroll out of view) the selection of the context menu entry - "pin as app tab" feature?
(Assignee)

Comment 20

6 years ago
(In reply to comment #19)
> > The message should be "Bug 670721 - Mozmill Endurance test for pinning App
> > Tabs. r=ahughes r=dhunt"
> 
> https://developer.mozilla.org/en/Mozmill_Tests mentioned that the
> description should include the changes made. So, I thought it should be more
> specific, and didn't summarize it(anyway it could include unpinning as
> well). I understand now.

It should be for the entire patch, not just the the latest change to the patch. I often base it on the summary from bugzilla, but you can mention unpinning too.

> > This isn't quite what I had in mind. With this code the micro-iterations are
> > no different from the iterations. 
> The moment I read the "this isn't...mind" line, I realized that I had to
> loop it twice #1(pinning) and #2(unpinning) separately. I clearly
> misunderstood the purpose of microiterations. Later, when I looked at your
> working example, I couldn't get why first loop's needed for opening the tabs
> but now I do.

It's because after we unpin we have X open tabs, so in order for the iteration to be well balanced we should start with X open tabs.

> Where does it exactly fail?  
> Did you mean that it's not possible to see(when tabs scroll out of view) the
> selection of the context menu entry - "pin as app tab" feature?

That's correct. It looks to me that the context menu is for the toolbar. I suspect we need to use controller.tabs.selectTabIndex(lastTab), and possibly wait for the tab to be in view.
Created attachment 554061 [details] [diff] [review]
TabPinning Endurance test

Updated the attachment based on prev. working/example with microiterations
Attachment #545986 - Attachment is obsolete: true
Attachment #554061 - Flags: review?(dave.hunt)
(Assignee)

Comment 22

6 years ago
Comment on attachment 554061 [details] [diff] [review]
TabPinning Endurance test

It looks like you need to update mozmill-tests with the recent changes. Micro-iterations have been renamed to entities. Also, use enduranceManager.loop to loop over entities. See https://wiki.mozilla.org/QA/Mozmill_Test_Automation/Endurance_Tests/Documentation

  // Open tabs
  enduranceManager.loop(function () {
    if (enduranceManager.currentEntity > 0) {
      tabBrowser.openTab();
    }
    controller.open(LOCAL_TEST_PAGE);
    controller.waitForPageLoad();
  }
Attachment #554061 - Flags: review?(dave.hunt) → review-
Created attachment 554081 [details] [diff] [review]
TabPinning Endurance test

Updated the attachment based on prev. working/example after replacing microiterations with entities
Attachment #554061 - Attachment is obsolete: true
Attachment #554081 - Flags: review?(dave.hunt)
(Assignee)

Comment 24

6 years ago
Comment on attachment 554081 [details] [diff] [review]
TabPinning Endurance test

+    // Open tabs
+    enduranceManager.loop(function () {
+        if (enduranceManager.currentEntity > 0) {

Sorry, my mistake -- this should be checking for > 1.

Also, we still have the issue of this failing when there are enough tabs to cause tab scrolling. Our daily testruns are using 10 entities so they shouldn't be affected, but I'm not keen on r+ this until it's reliable with a large number of entities.

Henrik: What are your thoughts?
Attachment #554081 - Flags: review?(dave.hunt) → review-
Have we gotten any reply from Dao yet?
(Assignee)

Comment 26

6 years ago
Yes, the response was: "If I understand you correctly, there's no event for what you're waiting for."

I sent a further e-mail in an attempt to make what we're trying to do very clear. There was no response to this. CCing Dao to this bug and pasting from my email below:

So the issue here is that we're opening a lot of tabs quickly, and then want to right-click the last opened tab to access the context menu. When more than a certain number of tabs are open, tab scrolling occurs and the new tab slides into view. The problem is that we're right-clicking in the middle of the tab whilst it's not fully in view. The side-effect of this is that we're right-clicking the toolbar to the right of the tab.

What we'd like to do is wait for the tab to be completely in view. At the moment the only workaround we've found is to wait for the tab scroll down button to be disabled, but this isn't the ideal solution.

I just wanted to clarify the problem in the hope that there is a better event to respond to.
I know of no other event for this.
Dao, is there a documented list of existent events for any action related to tab scrolling? If not do you have a link to the code? If none of those events help us we will have to fallback to the disabled scroll button.
This is handled by the tabbrowser-arrowscrollbox binding: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml
which extends the arrowscrollbox-clicktoscroll binding: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scrollbox.xml
Thanks Dao for the pointers. So lets use the disabled state of the scroll button then as Dave has already pointed out. It's a visual indicator we can rely on in our functional tests.
(Assignee)

Comment 31

6 years ago
Created attachment 559975 [details] [diff] [review]
Mozmill Endurance test for pinning App Tabs. v2.0

This patch builds on Shriram's patch, and includes support for when there are enough entities to cause tab scrolling. There is still an issue when there are so many app tabs that it becomes impossible to context-click a non-app tab. This seems to be related to bug 583299 and may be solved by app tabs being in their own scrolled tab list as mentioned in bug 654604.

We currently run daily testruns with 10 entities, so I doubt this will be an issue in the short term. If it becomes an issue then another option is to specify a maximum number of entities on a per-test basis, however reporting this accurately could prove challenging.
Assignee: kshriram18 → dave.hunt
Attachment #554081 - Attachment is obsolete: true
Attachment #559975 - Flags: review?(hskupin)
Attachment #559975 - Flags: review?(hskupin) → review?(anthony.s.hughes)
(Reporter)

Comment 32

6 years ago
Comment on attachment 559975 [details] [diff] [review]
Mozmill Endurance test for pinning App Tabs. v2.0

Overall this looks fine. Can you separate the code blocks with newlines so it is more readible?
Attachment #559975 - Flags: review?(anthony.s.hughes) → review-
(Reporter)

Comment 33

6 years ago
Comment on attachment 559975 [details] [diff] [review]
Mozmill Endurance test for pinning App Tabs. v2.0

Changing that to an r+. Please make the previous suggested changes before check-in.
Attachment #559975 - Flags: review- → review+
(Assignee)

Comment 34

6 years ago
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/c2b14fe5446e (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/99aa2354e014 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/e59f846742fe (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/fea40c57d4ac (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/cb14599164b0 (mozilla-2.0)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 687240
(Reporter)

Updated

6 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.