629084 - Mozmill test for Panorama (un)pinning App tabs

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Litmus ID:
https://litmus.mozilla.org/show_test.cgi?id=12842

Comment 1

[Aaron Train [:aaronmt]]

Attachment: v1 (default) - automation script for litmus test case #12842

Comment 2

[Aaron Train [:aaronmt]]

Self-Note: Should reuse existing groups/appTabs variables on re-entry without redeclaring.

Comment 3

[u279076]

Comment on attachment 517830 [details] [diff] [review]
v1 (default) - automation script for litmus test case #12842

>diff --git a/firefox/testTabView/testAppTabPinning.js b/firefox/testTabView/testAppTabPinning.js
>+function setupModule(module) {
>+  controller = mozmill.getBrowserController();
>+  tabView = new tabView.tabView(controller);

Please refer to tabView as activeTabView.

>+var teardownModule = function() {
>+  tabBrowser.closeAllTabs();
>+}

This should be "function teardownModule(module)" to match setupModule().

>+
>+/**
>+ *  Pinning and Unpinning Application Tabs 
>+ *  and verifying the changes in Tab View
>+ */
>+function testToggleTabView() {

I'd like to see a different test name. Maybe "testAppTabsInTabGroups" might be more appropriate?

>+  // Open local pages in separate tabs and wait for each to finish loading
>+  LOCAL_TEST_PAGES.forEach(function (url) {

Please make sure acronym variable names are ALLCAPS.

Overall, I think the test is great. However, I'd like to see testing single, multiple, and no App Tabs.  Can you add the case for "multiple" to this test?

Comment 4

[Aaron Train [:aaronmt]]

This is taking a bit longer (incorporating multiple) as I'm running into a {"exception": {"message": "TabView is not open."}} exception after calling open(), which isn't making sense. Investigating.

Comment 5

[Aaron Train [:aaronmt]]

Attachment: v2 (default) - automation script for litmus test case #12842

Refactoring and now includes case for multiple application tabs

Comment 6

[u279076]

Comment on attachment 518566 [details] [diff] [review]
v2 (default) - automation script for litmus test case #12842

>+ * Contributor(s):
>+ *   Aaron Train <[e-mail]>

Nit: you should add "(orignal author)" whenever you are the person creating the file.

>+var teardownModule = function(module) {
>+  tabBrowser.closeAllTabs();
>+}

Please use "function teardownModule(module) {"
 
>+  testTabs.firstTab = tabBrowser.getTab(0);

Sorry if I missed it, but where does testTabs come from?

>+  controller.assert(function () {
>+    return appTabs.length === 1;
>+  }, "Tab Group has one application tab - got " + "'" + appTabs.length + "'" +
>+    ", expected: " + "'" + "1" + "'");

You can simply use ", expected: '1'");

>+  controller.assert(function () {
>+    return appTabs.length === 2;
>+  }, "Tab Group has two application tabs - got " + "'" + appTabs.length + "'" +
>+    ", expected: " + "'" + "2" + "'");

Similarly here.

>+  controller.assert(function () {
>+    return !appTabs.length;
>+  }, "Tab Group has no application tabs - got " + "'" + appTabs.length + "'" +
>+    ", expected: " + "'" + "0" + "'");

And here.

Comment 7

[Aaron Train [:aaronmt]]

Attachment: v3 (default) - automation script for litmus test case #12842

(In reply to comment #6)
> Comment on attachment 518566 [details] [diff] [review]
> v2 (default) - automation script for litmus test case #12842
> 
> >+ * Contributor(s):
> >+ *   Aaron Train <[e-mail]>
> 
> Nit: you should add "(orignal author)" whenever you are the person creating the
> file.

Done. 
> >+var teardownModule = function(module) {
> >+  tabBrowser.closeAllTabs();
> >+}
> 
> Please use "function teardownModule(module) {"

Done.
> >+  testTabs.firstTab = tabBrowser.getTab(0);
> 
> Sorry if I missed it, but where does testTabs come from?

I am using a global object I created to help manage the tabs (as properties) I am using in the test.

> >+  controller.assert(function () {
> >+    return appTabs.length === 1;
> >+  }, "Tab Group has one application tab - got " + "'" + appTabs.length + "'" +
> >+    ", expected: " + "'" + "1" + "'");
> 
> You can simply use ", expected: '1'");

Done (w/ the two others too).

Comment 8

[u279076]

Comment on attachment 518714 [details] [diff] [review]
v3 (default) - automation script for litmus test case #12842

>+function teardownModule(module) {
>+  tabBrowser.closeAllTabs();
>+}

I'm not sure if we need tabview.reset() in this test. Waiting for feedback on bug 641574.

>+  testTabs.firstTab = tabBrowser.getTab(0);

I'm wondering if using an array would be simpler (at least the indexes would map).  In essence, an array is what you've created here.

>+  // Verify that there is a single application tab in the default group
>+  var groups = activeTabView.getGroups();
>+  var appTabs = activeTabView.getElements({type: "group_appTabs", parent: groups[0]});

You can act on activeTabView.activeGroup.

>+  controller.assert(function () {
>+    return appTabs.length === 1;
>+  }, "Tab Group has one application tab - got " + "'" + appTabs.length + "'" +
>+    ", expected: '1'");

>+  // Make the second tab an application tab by pinning it
>+  testTabs.secondTab = tabBrowser.getTab(1);
>+  controller.rightClick(testTabs.secondTab);
>+  controller.waitThenClick(pinTab);
>+  utils.closeContentAreaContextMenu(controller);
>+
>+  // Open Tab Groups View (default via keyboard shortcut)
>+  activeTabView.open();
>+
>+  // Verify that there are two application tabs in the default group
>+  appTabs = activeTabView.getElements({type: "group_appTabs", parent: groups[0]});
>+  controller.assert(function () {
>+    return appTabs.length === 2;
>+  }, "Tab Group has two application tabs - got " + "'" + appTabs.length + "'" +
>+    ", expected: '2'");
>+
>+  // Close Tab Groups View (default via keyboard shortcut)
>+  activeTabView.close();

Since you are basically doing the same thing twice here, it might be better to simply do this in a for loop.  Then you can check for appTabs.length === i.  Again, this is a case where using an array for testTabs would be beneficial.

>+  // Make both tabs regular tabs by unpinning them
>+  var unpinTab = new elementslib.ID(controller.window.document, "context_unpinTab");
>+  for(var appTab in testTabs) {
>+    controller.rightClick(testTabs[appTab]);
>+    controller.waitThenClick(unpinTab);
>+    utils.closeContentAreaContextMenu(controller);
>+  }
>+
>+  // Open Tab Groups View (default via keyboard shortcut)
>+  activeTabView.open();
>+
>+  // Verify that there are no application tabs in the default group
>+  appTabs = activeTabView.getElements({type: "group_appTabs", parent: groups[0]});
>+
>+  controller.assert(function () {
>+    return !appTabs.length;
>+  }, "Tab Group has no application tabs - got " + "'" + appTabs.length + "'" +
>+    ", expected: '0'");
>+
>+  // Close Tab Groups View (default via keyboard shortcut)
>+  activeTabView.close();
>+}

Speaking to my last point, you could even encapsulate this in a helper function which takes an int as a parameter (representing number of app tabs).

Comment 9

[Aaron Train [:aaronmt]]

Didn't forget about this. Working on a revision right now, will post tonight or tomorrow.

Comment 10

[Aaron Train [:aaronmt]]

Attachment: v4 - (default) - automation script for litmus test case #12842

+ Large refactoring

I simplified and condensed code using your suggestions to two primary loops, one for pinning application tabs and verifying via Tab Groups, and the other the act of unpinning.

Note: appTabs is not a zero based array , so it requires a little nuisance when comparing it with my tabs array (testTabs.length - 1 - i).

Comment 11

[u279076]

Comment on attachment 521097 [details] [diff] [review]
v4 - (default) - automation script for litmus test case #12842

>+function setupModule(module) {
>+  controller = mozmill.getBrowserController();
>+  activeTabView = new tabView.tabView(controller);
>+  tabBrowser = new tabs.tabBrowser(controller);
>+  tabBrowser.closeAllTabs();
>+}

Nit: can you separate the tabBrowser declaration and the closeAllTabs() call with a newline?

>+
>+function teardownModule(module) {
>+  tabBrowser.closeAllTabs();
>+  activeTabView.reset();
>+}

I'm not sure about the order here -- it might not matter.  In other tests we have been making closeAllTabs() the last call in teardownModule(). This is a nit, I won't block landing on it unless Henrik agrees.

>+
>+/**
>+ *  Pinning and Unpinning Application Tabs 
>+ *  and verifying the changes in Tab View
>+ */

Nit: Let's keep the "App Tab" wording across the board (not "Application Tab")

>+    testTabs.push(tabBrowser.getTab(tabBrowser.selectedIndex));

Is this necessary? Can you not simply use tabBrowser.getTabs() after the fact?

>+  var pinTab = new elementslib.ID(controller.window.document, "context_pinTab");
>+  var unpinTab = new elementslib.ID(controller.window.document, "context_unpinTab");

Can you name this inclusive of the element type (ie. pinTabMenuItem or even pinTabContextMenuItem)?

>+  var activeGroup = activeTabView.activeGroup;

I don't think you need this -- simply use activeTabView.activeGroup anywhere it is needed.

Thanks.

Comment 12

[Aaron Train [:aaronmt]]

Attachment: v5 - (default) - automation script for litmus test case #12842

I re-factored the test again eliminating portions from prior patches, and incorporating your comments from the previous to simplify it.

Things to note, each app tab is closed from index 0.

Comment 13

[u279076]

Comment on attachment 521588 [details] [diff] [review]
v5 - (default) - automation script for litmus test case #12842

>+  LOCAL_TEST_PAGES.forEach(function (URL) {
>+    controller.open(URL);
>+    controller.waitForPageLoad();
>+    tabBrowser.openTab();
>+  });

Something Al recently introduced was adding tabBrowser.closeTab() after this loop. The problem with this for loop is that we always  end up with a lingering blank tab.  Can you add this?

>+  // Pin each tab
>+  for(var i = 0, l = tabBrowser.length - 1; i < l; i++) {

Nit: can you just use (tabBrowser.length - 1) in your test instead of l? l and 1 look too similar.

Otherwise, this looks good. Thanks.

Comment 14

[Aaron Train [:aaronmt]]

Attachment: v5.1 - (default) automation script for litmus test case #12842

+ closes extra blank tab
+ s/l/tabCount/

Comment 15

[u279076]

Comment on attachment 521604 [details] [diff] [review]
v5.1 - (default) automation script for litmus test case #12842

That looks a lot better.  Over to Henrik for follow up.

Comment 16

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

Comment on attachment 521604 [details] [diff] [review]
v5.1 - (default) automation script for litmus test case #12842

>+  var pinTabMenuItem = new elementslib.ID(controller.window.document, "context_pinTab");
>+  var unpinTabMenuItem = new elementslib.ID(controller.window.document, "context_unpinTab");
>+
>+  // Pin each tab
>+  for(var i = 0, tabCount = tabBrowser.length - 1; i < tabCount; i++) {
>+    controller.rightClick(tabBrowser.getTab(i));
>+    controller.waitThenClick(pinTabMenuItem);
>+    utils.closeContentAreaContextMenu(controller);

You should use the controller.Menu API here. Also there is no need to declare tabCount. It only makes it harder to understand the loop header.

>+    // Verify the addition of the app tab in the active tab group
>+    var appTabs = activeTabView.getElements({type: "group_appTabs", 
>+                                             parent: activeTabView.activeGroup});
>+    controller.assert(function () {
>+      return appTabs.length === (i + 1);
>+    }, "Active Group has an app tab - got " + appTabs.length + "'" +
>+      ", expected: '" + (i + 1) + "'");

Does closeAllTabs() also closes AppTabs? If not, we will run into troubles here.

>+    controller.rightClick(tabBrowser.getTab(0));
>+    controller.waitThenClick(unpinTabMenuItem);
>+    utils.closeContentAreaContextMenu(controller);

Same as above.

>+    var appTabs = activeTabView.getElements({type: "group_appTabs",
>+                                             parent: activeTabView.activeGroup});

nit: you re-declare the variable.

>+    controller.assert(function () {
>+      return appTabs.length === (tabCount - (i + 1))
>+    }, "Active Group has an unpinned app tab - got " + "'" + appTabs.length + "'" +
>+      ", expected: '" + (tabCount - (i + 1)) + "'");

Shouldn't it be simply compared to '0'?

Comment 17

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

Bug 836758 will remove Panorama from Firefox soon and make it available as add-on. That means no new tests are necessary. Closing as WONTFIX.