620589 - Mozmill test for Panorama ESC on exposed stack remains in tab view

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Clay Earl Uyenghua]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.224 Safari/534.10
Build Identifier: 

https://litmus.mozilla.org/show_test.cgi?id=12848

Reproducible: Always

Steps to Reproduce:
1. Open a couple of tabs and open Tab View
2. In Tab View, reduce a tab group size until the tabs go into stacked mode
3. Open the fan view of the stack by clicking on the stack expander icon
4. Press the escape key


Expected Results:  
The fan view closes and we remain in Tab View

Comment 1

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12848

Comment 2

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

Comment on attachment 498940 [details] [diff] [review]
Automation script for testcase 12848

>+ * Contributor(s):
>+ * Clay Earl Uyenghua <[e-mail]> (original author)

Should be indented by 2 spaces.

>+// Include the required modules
>+var domUtils = require("../../shared-modules/dom-utils");
>+var tabs = require("../../shared-modules/tabs");
>+var tabViews = require("../../shared-modules/tabview");
>+var utils = require("../../shared-modules/utils");

The first letter of the variable should be a capital letter.

>+const TIMEOUT = 5000;

As in the other tests, this constant can be removed.

>+var setupModule = function(module) {
>+var testFanView = function() {

Same here for named functions.

>+  //nodeCollector = new domUtils.nodeCollector(controller.window.document);

That looks like a left-over and can be removed.

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

There has to be a space between function and the opening bracket.

>+  // Get the active group element
>+  var activeGroup = tabView.getElement({
>+      type: "groups"
>+    });

Please correct the indentation.

>+  // Get the resizer and hold left click on it
>+  // Move mouse to title box position and release click to resize group
>+  var resizer = tabView.getElement({type: "group_resizer", 
>+                                    parent: activeGroup});
>+  controller.mouseDown(resizer);
>+  controller.mouseEvent(tabView.getElement({type: "group_titleBox", 
>+                         parent: activeGroup}), 0, 0, {type: "mousemove"});
>+  controller.mouseUp(resizer);

That's nicely done! The only part you should change in those lines, please get a reference to the title box before you call any of the mouse events.

>+  controller.click(tabView.getElement({type: "group_stackExpander",
>+                                       parent: activeGroup}));

Please define the element before the click call.

>+  // Close the stack Expander by pressing Escape
>+  controller.keypress(null, 'VK_ESCAPE', {});

Here I miss an important bit. There is no check that the "fan view" has been opened. Looks like it would require a new element added to getElements in the TabView module. Do you think, you can get this done? If not please file a new bug and mark your one as blocking on its implementation.

>+  // Sleep for 300ms to ensure that the tab view is still open
>+  // When tabView closes, it takes a while for isOpen to change
>+  controller.sleep(300);

Please don't use sleep here. Instead a waitFor should be used, which then will wait until the fan view has been closed.

>+  controller.assertJS("subject.isOpen==true", { isOpen: tabView.isOpen });

As mentioned in the other reviews, please use assert() here.

Comment 3

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12848 revision 1

revised automated script. Incorporated changes suggested.

The call to sleep is to ensure that the tab view doesn't close. Since it takes for a while for isOpen to change to false if tab view closes.

Comment 4

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

Comment on attachment 499463 [details] [diff] [review]
Automation script for testcase 12848 revision 1

>diff --git a/firefox/testPanorama/testFanView.js b/firefox/testPanorama/testFanView.js

Something, which I forgot the last time. To be consistent with our shared module we should place tests for Panorama inside testTabView and not testPanorama. 

>+function setupModule() {

As given in the other review, the module parameter has to be added back.

>+  // Perform test twice

Can you please mention in the comment why it has to be performed twice? Otherwise you can move the code to its own function, which gets called from inside the test function. That's more elegant.

>+  for ( var i=0; i < 2; i++ ) {

Independent from my last statement, I only want to mention that we are using spaces around all operators, which also included '='.

>+    var resizer = tabView.getElement({type: "group_resizer", 
>+      parent: activeGroup});
>+    var titleBox = tabView.getElement({type: "group_titleBox", 
>+      parent: activeGroup});
[..]
>+    // Click on the stack Expander
>+    var stackExpander = tabView.getElement({type: "group_stackExpander",
>+      parent: activeGroup});

nit: We are a bit unclear here how to format it as best as possible. But when using this way, it's more obvious to place the closing brackets in their own line and use new lines for all the parameters (don't stick the first one at the end of the first line.

>+    // Check that fan view closes
>+    controller.waitFor(function () {
>+      return tabView.getElement({type: "overlay"})==undefined});

You will have to use the triple operator to compare both values. Also please separate the getElement call from the comparison, and fix the formatting, especially the closing brackets of the waitFor function. Beside that it would be great to have the handling of the fan directly inside the TabView API. But this can be filed as a follow-up bug.

>+    // Sleep for 300ms to ensure that the tab view is still open
>+    // When tabView closes, it takes a while for isOpen to change
>+    controller.sleep(300);
>+    // Check that we remain in tab view
>+    controller.assert(function () { return tabView.isOpen==true });

You can remove the second comment, the first one already makes it clear. Also please fix the formatting of the assert function call, and create a constant for the 300ms at the top.

>+	var fullScreen = new elementslib.Elem(controller.menus['view-menu'].fullScreenItem);
>+	controller.click(fullScreen);

An important fact this test is missing now is the tearDownModule() function. We absolutely have to make sure to reset the fullscreen setting at the end of the test. For the above code you could also use the new Menu API. It would look like: controller.mainMenu.click("#fullScreenItem");

>+      // Fan View overlay element
>+      case "overlay":
>+        nodeCollector.queryNodes(".overlay");
>+        break;

We should give it a better name as overlay, which is really general. Something like fan_pane could work.

Comment 5

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12848 revision 2

revised automation script.

Comment 6

[u279076]

Comment on attachment 500242 [details] [diff] [review]
Automation script for testcase 12848 revision 2

>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
Please use 2011.

>+// Include the required modules
>+var Tabs = require("../../shared-modules/tabs");
>+var TabView = require("../../shared-modules/tabview");
All variable names should begin lower-case.

>+const WAIT_TIME = 300;
Use TIMEOUT -- please refer to the styleguide.

>+  tabView = new TabView.tabView(controller);
You'll have to use a different variable name here -- I suggest activeTabView.


>+function teardownModule() {
>+  var view = controller.window.document.defaultView;
>+  var state = view.fullScreen;
>+  if ( state === 'true' ) {
>+    controller.mainMenu.click("#fullScreenItem");
>+  }
>+}
Please comment this code.

>+    var elem = new elementslib.Name(controller.tabs.activeTab, page.name);
>+    controller.assertNode(elem);
Please do not use "elem" as a variable name -- make it more meaningful.

>+  // Perform test twice for normal and full screen
>+  for ( var i=0; i < 2; i++ ) {
Please refer to the styleguide for formatting for-loops.

>+    fanViewTest();
Please do not extract the functional code of a test into another function -- keep it within the test function.

Comment 7

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12848 revision 3

I have revised the automation script according to changes required by the
review. The teardown module is to make sure that we are not in full screen at
the end of the test. I previously extracted the test according to the review
done by Henrik. I have made changes and removed the fanViewTest function.

Comment 8

[u279076]

Comment on attachment 502377 [details] [diff] [review]
Automation script for testcase 12848 revision 3

>+// Include the required modules
>+var tabs = require("../../shared-modules/tabs");
>+var tabView = require("../../shared-modules/tabview");

Capitalize the first letter please.

>+	// Check that the local test page opened successfully
>+    var pageName = new elementslib.Name(controller.tabs.activeTab, page.name);

Please fix the indentation on the comment.

>+	// Open new Tab
>+    tabBrowser.openTab();
>+  });

Here too.

>+    // Get the resizer and hold left click on it
>+    // Move mouse to title box position and release click to resize group
>+    var resizer = activeTabView.getElement({
>+      type: "group_resizer",
>+      parent: activeGroup
>+    });
>+    var titleBox = activeTabView.getElement({
>+      type: "group_titleBox",
>+      parent: activeGroup
>+    });
>+    activeTabView.controller.mouseDown(resizer);
>+    activeTabView.controller.mouseEvent(titleBox, 0, 0, {type: "mousemove"});
>+    activeTabView.controller.mouseUp(resizer);

This is a bug chunk of code.  Is it possible for you to break your comment up to document each block?

>+    // Check that fan view closes
>+    controller.waitFor(function () {
>+      return undefined === activeTabView.getElement({
>+        type: "fan_pane"
>+      });
>+    });

Please refer to the waitFor() documentation.  You should at least include an error message.

>+
>+    // Sleep to ensure that the tab view is still open
>+    controller.sleep(TIMEOUT);
>+    // Check that we remain in tab view
>+    controller.assert(function () {
>+      return activeTabView.isOpen==true
>+    });

Try using a waitFor() instead of a sleep() and assert().

>diff --git a/shared-modules/tabview.js b/shared-modules/tabview.js
>  * Contributor(s):
>  *   Henrik Skupin <[e-mail]>
>+ *   Clay Earl Uyenghua <[e-mail]>

This should probably be @gmail.com

Comment 9

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12848 revision 4

Implemented necessary changes. Changed assert() calls to waitFor(). Separated big chunk of code and commented each block.

Comment 10

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12848 revision 4

I missed the coding style for the last waitFor() call.

Comment 11

[u279076]

Comment on attachment 503084 [details] [diff] [review]
Automation script for testcase 12848 revision 4

>diff --git a/firefox/testTabView/testFanView.js b/firefox/testTabView/testFanView.js
>+function teardownModule() {
>+  // If we are still on full screen then exit full screen

Please add the module parameter.

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

url should be URL since it's an acronym.

>+    // Do a mouse down(hold click) on resizer element,
>+    // move mouse to title box position and release click to resize group
>+    activeTabView.controller.mouseDown(resizer);
>+    activeTabView.controller.mouseEvent(titleBox, 0, 0, {type: "mousemove"});
>+    activeTabView.controller.mouseUp(resizer);

I would break the comment up so each mouse-event gets a comment.

>+    // Check if fan view is open
>+    controller.waitFor(function () {
>+      return undefined !== activeTabView.getElement({
>+        type: "fan_pane"
>+      });
>+    }, null, null, "Expected Fan View to open");

Please update the error message to display a "got" and "expected" value.

>+    // Check that fan view closes
>+    controller.waitFor(function () {
>+      return undefined === activeTabView.getElement({
>+        type: "fan_pane"
>+      });
>+    }, null, null, "Expected Fan View to close");

Same here.

>+    // Check that we remain in tab view
>+    controller.waitFor(function () {
>+      return activeTabView.isOpen === true
>+    }, null, null, "Expected Tab View to remain open");

Same here.

Comment 12

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12848 revision 5

Implemented necessary changes.

Comment 13

[u279076]

Comment on attachment 504929 [details] [diff] [review]
Automation script for testcase 12848 revision 5

>diff --git a/firefox/testTabView/testFanView.js b/firefox/testTabView/testFanView.js
>+function teardownModule(module) {
>+  // If we are still on full screen then exit full screen
>+  var view = controller.window.document.defaultView;
>+  var state = view.fullScreen;
>+  if ( state === 'true' ) {
>+    controller.mainMenu.click("#fullScreenItem");
>+  }
>+}

Booleans should not be enclosed in quotes.

>+    // Do a mouse down(hold click) on resizer element
>+    activeTabView.controller.mouseDown(resizer);
>+    // Move mouse to title box position
>+    activeTabView.controller.mouseEvent(titleBox, 0, 0, {type: "mousemove"});
>+    // Release mouse down on resize group
>+    activeTabView.controller.mouseUp(resizer);

Can you please separate each of these by a newline?

>+    // Check if fan view is open
>+    controller.waitFor(function () {
>+      return undefined !== activeTabView.getElement({
>+        type: "fan_pane"
>+      });
>+    }, "Expected Fan View to open - didn't expect " + undefined + ", got "
>+    + activeTabView.getElement({
>+      type: "fan_pane"
>+    }));

Try refactoring this so that activeTabView.getElement is assigned to a variable within the waitFor() before the return. Also, the variable you are checking should be the left-side.

>+    // Check that fan view closes
>+    controller.waitFor(function () {
>+      return undefined === activeTabView.getElement({
>+        type: "fan_pane"
>+      });
>+    }, "Expected Fan View to close - expected " + undefined +", got "
>+    + activeTabView.getElement({
>+      type: "fan_pane"
>+    }));

Same here.

>+    // Check that we remain in tab view
>+    controller.waitFor(function () {
>+      return activeTabView.isOpen === true
>+    }, "Expected Tab View to remain open - got " + activeTabView.isOpen
>+    + ", expected " + true);

We don't need got/expected for boolean.

Comment 14

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12848 revision 6

Implemented necessary changes.

Comment 15

[u279076]

Comment on attachment 507373 [details] [diff] [review]
Automation script for testcase 12848 revision 6

This looks good to me.  Henrik, can you spotcheck in case I might have missed something?  Thanks.

Comment 16

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

Comment on attachment 507373 [details] [diff] [review]
Automation script for testcase 12848 revision 6

>+const TIMEOUT = 300;

Looks like it is not necessary anymore.

>+const PERFORM_TEST = 2;

I still think that constant it not necessary.

>+  {URL: LOCAL_TEST_FOLDER + 'layout/mozilla_organizations.html',
>+    name: 'summary'}

Is there a reason for the wrapping?

>+function teardownModule(module) {
>+  // If we are still on full screen then exit full screen
>+  var view = controller.window.document.defaultView;
>+  var state = view.fullScreen;
>+  if ( state === true ) {
>+    controller.mainMenu.click("#fullScreenItem");
>+  }

Some comments as the ones on the new tab test apply here.

>+function testFanView() {
[..]
>+    // Do a mouse down(hold click) on resizer element

nit: please add a space before the brackets

>+    activeTabView.controller.mouseDown(resizer);
>+
>+    // Move mouse to title box position
>+    activeTabView.controller.mouseEvent(titleBox, 0, 0, {type: "mousemove"});
>+
>+    // Release mouse down on resize group
>+    activeTabView.controller.mouseUp(resizer);

I think we should wait for Mozmill 1.5.2 and directly use the new drag and drop feature.

>+    controller.waitFor(function () {
>+      var fanPane = activeTabView.getElement({
>+        type: "fan_pane"
>+      });
>+      return fanPane !== undefined
>+    }, "Expected Fan View to open - got ["
>+    + activeTabView.getElement({
>+      type: "fan_pane"

You should declare fanPane right before the waitFor call. Then updating its value from within the closure function will also make the last value available for the message. That gives a much cleaner way.

>+    }) + "], didn't expect [" + undefined + "]");

Same changes to the brackets please as mentioned earlier on the other patches.

>+    // Check that fan view closes
>+    controller.waitFor(function () {
>+      var fanPane = activeTabView.getElement({
>+        type: "fan_pane"
>+      });
>+      return fanPane === undefined
>+    }, "Expected Fan View to close - got ["
>+    + activeTabView.getElement({
>+      type: "fan_pane"
>+    }) + "], expected [" + undefined + "]");

Same here for both mentioned comments from above.

>+    // Check that we remain in tab view
>+    controller.waitFor(function () {
>+      return activeTabView.isOpen === true

You can reduce that to 'return activeTabView.isOpen;'

>+    // Close tab view and click on full screen for next test
>+    activeTabView.close();
>+    controller.mainMenu.click("#fullScreenItem");

Please test that we really have entered or left the fullscreen mode.

Comment 17

[u279076]

This test requires drag&drop -- blocked by bug 629165.

Comment 18

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

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.