620536 - Mozmill test for Panorama cancel tab search

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=12840

Reproducible: Always

Steps to Reproduce:
1. Open several tabs and enter group view
2. Click the groups search icon
3. press escape
4. press escape or ctrl/cmd(OS X) + e


Expected Results:  
1. Group view should be exited and returned to Browser View.
2. The tab showing should be the starting tab from step 1.

Comment 1

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12840

Comment 2

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

Comment on attachment 498936 [details] [diff] [review]
Automation script for testcase 12840

Clay, thanks for your interest in helping out with automated Panorama tests. Because this patch is your first one, I will check it closely and will try to point to all necessary resources, you will need to work on the review comments. If you have questions feel free to ping me at any time on IRC. 

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

Contributor names have to be intended by 2 spaces like the code in general.

>+var tabs = require("../../shared-modules/tabs");
>+var tabViews = require("../../shared-modules/tabview");
>+var utils = require("../../shared-modules/utils");

We haven't added this yet to the style guide, so you aren't aware of. But we want to use a starting capital letters for module variables. Also please remove the 's' from tabViews, so it will be "TabView".

>+const TIMEOUT = 5000;

This value is the default timeout for all our waitFor functions. So instead of passing in TIMEOUT to those functions, you can skip the parameter.

>+var setupModule = function(module) {

Given our style guide we are using named functions. How that will look like you can see here:
https://developer.mozilla.org/en/Mozmill_Tests/Mozmill_Style_Guide#Naming

>+var testCancelTabSearch = function() {

The same applies to your test function.

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

You are using a closure function here, which also doesn't have a name. To minimize confusion while reading the code, there should be a single blank between function and the brackets: "function (page)".

>+  // Select a tab before we open tabView
>+  controller.tabs.selectTabIndex(TAB_INDEX);

Instead of the MozMillController.tabs API you should use the tabBrowser.selectedIndex property to get/set the current tab index. See our documentation: https://developer.mozilla.org/en/Mozmill_Tests/Shared_Modules/TabbedBrowsingAPI/tabBrowser#Attributes

With the automated test I would even go a bit further and select the second tab. That way we can also make sure that the index doesn't get decreased when leaving Panorama.

>+  // Get the search button element and click
>+  var searchButton = tabView.getElement({type: "search_button",
>+										parent: undefined});

If no parent is needed, simply skip the object member. It defaults to undefined.

>+  // Provide the offset since button sometimes gets partially covered
>+  controller.click(searchButton,12,10);

This really sounds like a bug in Panorama. Do you know if it is already known? You can query for all open bugs here:

https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&component=TabCandy

If that's the case we have to add an "// XXX: Bug 123456 - Description" comment. Also if multiple parameters are used, add a blank right behind the comma.

>+  // Check to see if search is displayed when clicking on the search button
>+  var search = new elementslib.ID(controller.window.document, "search");

If the search pane is not available from the TabView module, it will have to be added to the getElements function in the "search elements" area. We don't want to see element instantiation inside the tests, and it is one of our primary goals at the moment to move all those instances over to the shared modules. 

>+  utils.assertElementDisplayed(controller, search, true);

You should use our existent Utils.assertElementVisible() function. If the CSS property, you have to use, is not supported yet, it will have to be added. This function is not perfect at the moment.

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

Instead of controller you should use tabView.controller.keypress()

>+  // Close Tab View by pressing Escape
>+  controller.keypress(null, 'VK_ESCAPE', {});

Same here.

>+  //check if tab displayed is same tab before we opened tabView
>+  controller.assertJS("subject.activeTabIndex==subject.tabIndex",
>+					{ activeTabIndex: controller.tabs.activeTabIndex,
>+					  tabIndex: TAB_INDEX });

With Mozmill 1.5 we now have the assert() function, which will make it much easier to use and to read. It's a closure and has access to all the variables declared in the current function scope. For an example see:

http://hg.mozilla.org/qa/mozmill-tests/file/43f2fb3d66b9/shared-modules/software-update.js#l248

>+function assertElementDisplayed(controller, elem, expectedDisplay) {
>+  var element = elem.getNode();

What is the element.nodeName of the search pane? It should simply be added as another case to the assertElementVisible function.

Overall great work. The test is nicely documented and you even got in touch with shared modules. I'm really looking forward to the revised test.

Comment 3

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12840 revision 1

revised script

Comment 4

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12840 revision 1

revised automation script. 

The tab we select is the second tab before opening the tab view. After closing tab view, we check to see if the same index is focused.

I used controller.tabs.activeTabIndex instead of tabBrowser.selectedIndex since there is a problem accessing the tabBrowser's selectedIndex after opening the tab view.

Comment 5

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

Comment on attachment 499419 [details] [diff] [review]
Automation script for testcase 12840 revision 1

>+function setupModule() {

With the last patch you have removed the module parameter from this function. Please add it back because it will be necessary in the future.

>+  // Get the search button element and click
>+  // XXX: Bug 594811 - You shouldn't be able to drag tab groups 
>+  // over the search button

This is more bug 598375, which will hopefully be fixed soon.

>+  // Provide the offset since button sometimes gets partially covered
>+  controller.click(tabView.getElement({type: "search_button"}), 12, 10);

Here you have moved the code to retrieve the element into the function. Please move it out, as what you had before. Also we do not know how much of the button could be overlayed across platforms. So we should probably click on [width-1/height-1]. You can retrieve the box boundaries via getNode().boxObject.

>+  // Check if search is closed and not displayed
>+  Utils.assertElementVisible(controller, search, false);

Looks good!

>+  //check if tab index displayed is same tab before we opened tabView
>+  controller.assert(function () {
>+                    return TAB_INDEX==controller.tabs.activeTabIndex });
>+}

The indentation of the function block should be 2 blanks. That means 'return' has to start with the 'n' from controller. The closing brackets also have to be on their own line. See the first code block: https://developer.mozilla.org/en/Mozmill_Tests/Mozmill_Style_Guide#Iteration

>+++ b/shared-modules/tabview.js
>       // Search elements
>+      case "search":
>+	    nodeCollector.queryNodes("#search");
>+        break;

To make it more obvious which part this element covers, we should rename it to search_pane.

>+++ b/shared-modules/utils.js
>   switch (element.nodeName) {
>     case 'panel':
>       visible = (element.state == 'open');
>       break;
>+    case 'DIV':

To make this case work in all cases we will have to operate on lower case characters. Otherwise 'div' will fail.

>+      var style = controller.window.getComputedStyle(element, '');
>+      var state = style.getPropertyValue('display');
>+      visible = (state != 'none');
>+      break;

For now it looks fine, but we should make some more improvements to this function and the possible cases. Would you mind filing a new bug for that?

With the above comments addressed we should be ready to land this patch. But I would still like to pass it through another review. Thanks!

Comment 6

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12840 revision 2

Revised automation script.

Filed a bug for improving the assertElementVisible function of Utils.
Bug 621983 - Utils.assertElementVisible improvement and other possible cases
https://bugzilla.mozilla.org/show_bug.cgi?id=621983

Comment 7

[u279076]

Comment on attachment 500246 [details] [diff] [review]
Automation script for testcase 12840 revision 2

>+ * Portions created by the Initial Developer are Copyright (C) 2010
This should probably be updated to 2011 now.

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

>+  tabView = new TabView.tabView(controller);
Since you are already using "tabView" for the API, I suggest using something like "activeTabView" as the name of this variable.

>+    var elem = new elementslib.Name(controller.tabs.activeTab, page.name);
>+    controller.assertNode(elem);
Please use a more meaningful variable name than "elem".  Since you are checking for the Page Name, perhaps "pageName" would be better.

>+  // Get the search button element and click
>+  // XXX: Bug 598375 - Tab group displays over the top of the tab search
>+  // button in panorama view.
>+  // Provide the offset since button sometimes gets partially covered
>+  var searchButton = tabView.getElement({
>+    type: "search_button"
>+  });
>+  var rect = searchButton.getNode().getBoundingClientRect();
>+  var offsetX = rect.width / 2;
>+  var offsetY = rect.height / 2
>+  controller.click(searchButton, offsetX, offsetY);
offsetX and offsetY default to width/2 and height/2, please refer to the source code for the controller object.

Also, in the future, please only ask for review from myself.  I'll hand off to Henrik for further review.  Thanks.

Comment 8

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12840 revision 3

I have revised the automation scripts based on the previous review.

Comment 9

[u279076]

Comment on attachment 502372 [details] [diff] [review]
Automation script for testcase 12840 revision 3

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

I know I asked you to remove the Capital style naming.  After some internal team discussions, we've decided to go back to that.  Please revert this change.  Sorry.

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

Seems like you are missing teardownModule().  All tests should have this even if nothing is done.

>+  //check if tab index displayed is same tab before we opened activeTabView
>+  controller.assert(function () {
>+    return TAB_INDEX == controller.tabs.activeTabIndex
>+  });

This should be ===

>diff --git a/shared-modules/tabview.js b/shared-modules/tabview.js
>+ *   Clay Earl Uyenghua <[e-mail]>

I believe you meant @gmail.com

>diff --git a/shared-modules/utils.js b/shared-modules/utils.js
>+ *   Clay Earl Uyenghua <[e-mail]>

Same here.

>+    case 'div':
>+      var style = controller.window.getComputedStyle(element, '');
>+      var state = style.getPropertyValue('display');
>+      visible = (state != 'none');
>+      break;

This should be !==

Comment 10

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12840 revision 4

I have incorporated the required changes. Sorry for the mix-up of the email. I forgot to change the domain.

Comment 11

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12840 revision 4

Changed assert() calls to waitFor().

Comment 12

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

(In reply to comment #9)
> Seems like you are missing teardownModule().  All tests should have this even
> if nothing is done.

We shouldn't add empty functions to tests, and we haven't done this in the past. It would only add more complexity for nothing. Once a test needs to clean-up settings after the testrun it's absolutely valuable to have a teardownModule() instance.

Comment 13

[u279076]

Comment on attachment 503057 [details] [diff] [review]
Automation script for testcase 12840 revision 4

>diff --git a/firefox/testTabView/testCancelTabSearch.js b/firefox/testTabView/testCancelTabSearch.js
>+function teardownModule() {
>+}

Don't forget to include the "module" parameter.

>+  LOCAL_TEST_PAGES.forEach(function (page) {
>+    controller.open(page.url);

Forgot to mention this earlier, but any variable names which are acronyms should be in all caps.  So in this case, page.URL

>+  //check if tab index displayed is same tab before we opened activeTabView

Make sure your comment starts with a capital letter and follows the // with a space.

>+  controller.waitFor(function () {
>+    return TAB_INDEX === controller.tabs.activeTabIndex
>+  }, null, null, "Tab shown is not the expected tab");
>+}

Looks better.  Can you tweak the message to read something like this:
"Tab shown is not the expected tab - got tabIndex " + controller.tabs.activeTabIndex + ", expected tabIndex " + TAB_INDEX

We like our error messages to have as much detail as possible to make it easier for debugging.

Comment 14

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

(In reply to comment #13)
> >+  controller.waitFor(function () {
> >+    return TAB_INDEX === controller.tabs.activeTabIndex
> >+  }, null, null, "Tab shown is not the expected tab");

As a side-node because Anthony hasn't mentioned it, the message is the second parameter of the waitFor method, and not the forth.

> Looks better.  Can you tweak the message to read something like this:
> "Tab shown is not the expected tab - got tabIndex " +
> controller.tabs.activeTabIndex + ", expected tabIndex " + TAB_INDEX

Also the order of the operands for the comparison in the closure function has to be the same as the one used in the message. It will reduce confusion, as what we have noticed in another test in the last days.

Comment 15

[u279076]

Thanks for the clarification, Henrik.

Comment 16

[u279076]

(In reply to comment #14)
> (In reply to comment #13)
> > >+  controller.waitFor(function () {
> > >+    return TAB_INDEX === controller.tabs.activeTabIndex
> > >+  }, null, null, "Tab shown is not the expected tab");
> 
> As a side-node because Anthony hasn't mentioned it, the message is the second
> parameter of the waitFor method, and not the forth.

According to our documentation, this is wrong:
https://developer.mozilla.org/en/Mozmill/Mozmill_Controller_Object#waitFor%28%29

Comment 17

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

(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > >+  controller.waitFor(function () {
> > > >+    return TAB_INDEX === controller.tabs.activeTabIndex
> > > >+  }, null, null, "Tab shown is not the expected tab");
> > 
> > As a side-node because Anthony hasn't mentioned it, the message is the second
> > parameter of the waitFor method, and not the forth.
> 
> According to our documentation, this is wrong:
> https://developer.mozilla.org/en/Mozmill/Mozmill_Controller_Object#waitFor%28%29

Thanks! The docs where outdated. I have updated them. Also the timeout and delay parameters are optional and don't have to necessarily be specified anymore.

Comment 18

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12840 revision 5

Revised automation script. Implemented necessary changes.

Comment 19

[u279076]

Comment on attachment 504922 [details] [diff] [review]
Automation script for testcase 12840 revision 5

>diff --git a/firefox/testTabView/testCancelTabSearch.js b/firefox/testTabView/testCancelTabSearch.js
>+  // Check if tab index displayed is same tab before we opened activeTabView
>+  controller.waitFor(function () {
>+    return TAB_INDEX === controller.tabs.activeTabIndex
>+  }, "Tab shown is not the expected tab - expected tabIndex "
>+  + TAB_INDEX + ", got tabIndex" + controller.tabs.activeTabIndex);
>+}

Just a couple of nits.  Can you please change around the order of the conditional?  The constant should be the right-side.  Also, your message should be a positive reflection of the expected state.

For example:

return controller.tabs.activeTabIndex === TAB_INDEX;
}, "Tab loaded after switching from Panorama view: got tab[" + 
   controller.tabs.activeTabIndex + "], expected tab[" + TAB_INDEX + "]");

Comment 20

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12840 revision 6

Implemented necessary changes.

Comment 21

[u279076]

Comment on attachment 507371 [details] [diff] [review]
Automation script for testcase 12840 revision 6

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

Comment 22

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

Comment on attachment 507371 [details] [diff] [review]
Automation script for testcase 12840 revision 6

>+const LOCAL_TEST_PAGES = [
[..]
>+  {URL: LOCAL_TEST_FOLDER + 'layout/mozilla_organizations.html', 
>+    name: 'summary'}

Why that wrapping of the last property?

>+function testCancelTabSearch() {
[..]
>+  // Check if tab index displayed is same tab before we opened activeTabView
>+  controller.waitFor(function () {
>+    return controller.tabs.activeTabIndex === TAB_INDEX
>+  }, "Tab index shown after switching from Panorama view: got tab index ["
>+  + controller.tabs.activeTabIndex + "], expected tab index ["
>+  + TAB_INDEX + "]");

Please correct the message to "Index of selected tab has not been changed - got '%val%', expected '%other_val%'".

With that fixed you have my r+.

Anthony, once the patch has been updated please make sure it get tested on all platforms before we check it in.

Comment 23

[u279076]

Clay, please make Henrik's revisions in comment 22 and attach a patch.  Once you've done that, I'll test it and land it if it passes.  Thanks.

Comment 24

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12840 revision 7

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

The wrapping of the last property is because there is a 80-character-per-line limit rule. If I don't wrap it it goes over this rule.

I have attached a patch with the changes for the message.

Comment 25

[Clay Earl Uyenghua]

Attachment: Automation script for testcase 12840 revision 7 [backed-out]

Resubmitting patch, previously attached old patch.

Comment 26

[u279076]

Comment on attachment 509355 [details] [diff] [review]
Automation script for testcase 12840 revision 7 [backed-out]

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

Comment 27

[u279076]

Comment on attachment 509355 [details] [diff] [review]
Automation script for testcase 12840 revision 7 [backed-out]

Backed out patch:
http://hg.mozilla.org/qa/mozmill-tests/rev/c5f6859719e8 [default]

Comment 28

[u279076]

Backed out due to failure:
http://mozmill-release.brasstacks.mozilla.com/#/general/failure?test=firefox%2FtestTabView%2FtestCancelTabSearch.js&func=testCancelTabSearch

Clay, please investigate this failure using the test scripts in mozmill-automation.

Comment 29

[u279076]

I'm going to take this bug. Clay, thanks for your work on it so far.  Can you
please shift focus to bug 624892?

Thanks

Comment 30

[u279076]

Attachment: Patch v2

Refactored the test based on recent changes and fixed the problem which was happening before.

Comment 31

[Aaron Train [:aaronmt]]

Comment on attachment 517945 [details] [diff] [review]
Patch v2

>+const LOCAL_TEST_PAGES = [
>+  { URL: LOCAL_TEST_FOLDER + 'layout/mozilla.html', name: 'community'},
>+  {URL: LOCAL_TEST_FOLDER + 'layout/mozilla_community.html', name: 'history'},
>+  {URL: LOCAL_TEST_FOLDER + 'layout/mozilla_projects.html', name: 'summary'},
>+  {URL: LOCAL_TEST_FOLDER + 'layout/mozilla_organizations.html', name: 'summary'}
>+];
Nit: Upper case 'name' as well as it's a constant too. As well, move the 'community' line back one space.

>+  // Select the one of the tabs
>+  tabBrowser.selectedIndex = TAB_INDEX;

// Select the first tab

Overall, looks good

Comment 32

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

(In reply to comment #31)

Just a quick drive-by:

> >+  {URL: LOCAL_TEST_FOLDER + 'layout/mozilla_organizations.html', name: 'summary'}
> >+];
> Nit: Upper case 'name' as well as it's a constant too. As well, move the
> 'community' line back one space.

URL is capitalized because it's an acronym. We don't use capital letters for object members otherwise.

Comment 33

[u279076]

(In reply to comment #31)
> Nit: Upper case 'name' as well as it's a constant too. As well, move the
> 'community' line back one space.

As Henrik already stated, upper-case only applies to acronyms.


> >+  // Select the one of the tabs
> >+  tabBrowser.selectedIndex = TAB_INDEX;
> 
> // Select the first tab

I'd like to refrain from using specificity in comments when dealing with constants.  Any change to the value of TAB_INDEX would require a change to your proposed comment.  My comment is more inclusive for any potential changes to TAB_INDEX.

Comment 34

[u279076]

Comment on attachment 517945 [details] [diff] [review]
Patch v2

Based on post-review feedback, I'm marking this r+ and moving it over to Henrik for followup.  Thanks Aaron.

Comment 35

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

Comment on attachment 517945 [details] [diff] [review]
Patch v2

The refactoring is already ongoing. So please wait until later today and update the patch with the current locations of modules and test files.

Comment 36

[u279076]

Aaron, now that the refactoring is done, please resubmit your patch for review by Henrik with the necessary changes.

Comment 37

[u279076]

(In reply to comment #36)
> Aaron, now that the refactoring is done, please resubmit your patch for review
> by Henrik with the necessary changes.

Sorry, I got confused with the bug numbers...ignore this.

Comment 38

[u279076]

Attachment: Patch v2.1

Includes refactored structure.

Comment 39

[u279076]

Attachment: Patch v2.1

Wrong patch...this is the right one.

Comment 40

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

Comment on attachment 518593 [details] [diff] [review]
Patch v2.1

>+ * Portions created by the Initial Developer are Copyright (C) 2010

nit: Please update it to 2011.

>+  // Close the search pane by pressing Escape
>+  activeTabView.controller.keypress(null, 'VK_ESCAPE', {});
[..]
>+  // Close the Tab Groups view
>+  activeTabView.close()

Not sure if we have taken that much care of it for the already landed tests, but closing of the tab groups view should definitely be forced in the teardownModule function. Otherwise we can completely break our remaining tests. Similar to other areas we should add a force parameter which closes the view via the back-end.

Comment 41

[u279076]

Attachment: Patch v2.2

Nits addressed -- not sure if adding the teardownModule() sufficiently resolves your concerns in comment 40.

Comment 42

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

Comment on attachment 519930 [details] [diff] [review]
Patch v2.2

>+  // Check the currently active tab has not changed
>+  controller.waitFor(function () {
>+    return controller.tabs.activeTabIndex === TAB_INDEX
>+  }, "Index of selected tab has not been changed - got'" +
>+     controller.tabs.activeTabIndex + "', expected '" + TAB_INDEX + "'");

This should be a simple assert. There is nothing we would have to wait for. With that change r=me.

Comment 43

[u279076]

Attachment: Patch v2.3 [backed-out]

Comment 44

[u279076]

Comment on attachment 520028 [details] [diff] [review]
Patch v2.3 [backed-out]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/79b802181078 [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/082c596a1d20 [mozilla2.0]

Comment 45

[u279076]

Need to back out the test due to a new failure.

When exiting Panorama, the last tab in the order is selected (instead of the 2nd tab). This is only reproducible on Windows XP when run using the python scripts.

Comment 46

[u279076]

Comment on attachment 520028 [details] [diff] [review]
Patch v2.3 [backed-out]

http://hg.mozilla.org/qa/mozmill-tests/rev/4f443538e729 [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/b06e8074cc3c [mozilla2.0]

Comment 47

[u279076]

This is reproducible on all platforms when run using:

mozmill -b <build> -t mozmill-tests/tests/functional/

I'll work to narrow down the set of tests causing this failure.

Comment 48

[u279076]

The problem is to do with testOpenTabInForeground.js which executes immediately before this test. If you only execute these two tests, the test fails. I don't know why, but we are unable to change the selected tab.  testOpenTabInForeground appears to be closing cleanly.  I'm not sure what could be going on here.

Comment 49

[u279076]

Attachment: Patch v3

I've found a solution/workaround.

Instead of...
-------------
tabBrowser.selectedIndex = TAB_INDEX;


I wrap it in a waitFor():
-------------------------
// Select the one of the tabs
controller.waitFor(function() {
  tabBrowser.selectedIndex = TAB_INDEX;
  return tabBrowser.selectedIndex === TAB_INDEX;
}, "Tab has been selected: got '" + tabBrowser.selectedIndex +
   "', expected '" + TAB_INDEX + "'");

Please advise, Henrik.

Comment 50

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

As long as we do not know why it fails I would assume there is a bug and putting a workaround into our test is not the right solution. Can you please come up with a really simplified testcase which exhibits this behavior? Can you reproduce it manually?

Comment 51

[u279076]

(In reply to comment #50)
> As long as we do not know why it fails I would assume there is a bug and
> putting a workaround into our test is not the right solution. Can you please
> come up with a really simplified testcase which exhibits this behavior? Can you
> reproduce it manually?

It's not reproducible manually. I'll see if I can come up with a minimized automated test case today.

Comment 52

[u279076]

Attachment: Minimized test

I've come up with a minimized test -- the symptom is different but I believe it is the same issue.  This test will fail to click the close button on the 2nd tab.

I think this is a race condition. When I added a second tabBrowser.selectedIndex = TAB_INDEX to testCancelTabSearch I found that the test passed.  Going through the minimization process, I discovered a similar problem to be found in testOpenInForeground for closing the 2nd tab.

If you uncomment the sleep() on line 36, this minimized test passes.

Comment 53

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

Comment on attachment 521284 [details]
Minimized test

Can you please add some asserts to check for the states? Do it in two passes, the first with sleep and the second without. I simply would like to run the test and see the results without having to know tab really has to be closed. Also a patch would be nice.

Comment 54

[u279076]

Attachment: Patch: minimized testcase

Here is a patch as requested.  I've added the request to run once with sleep and once without. I did not include your request for more asserts -- I'm not sure exactly what you want to assert? There is already a built in assert with tabBrowser.closeTab() which is failing.

Please tell me EXACTLY what you want asserted.

Comment 55

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

I will fix this remaining failure over on bug 646021.

Comment 56

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

Comment on attachment 521545 [details] [diff] [review]
Patch: minimized testcase

There is also no feedback necessary for testcases.

Comment 57

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

Anthony, please test again but now with bug 646021 fixed, this test should pass on all platforms.

Comment 58

[u279076]

(In reply to comment #57)
> Anthony, please test again but now with bug 646021 fixed, this test should pass
> on all platforms.

Test still fails the same as before.  The only way I can get this to work is with the workaround in comment 49.

Comment 59

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

Attachment: Updated minimized testcase

The minimized testcase works after incorporating the recent changes from the TabBrowser class. If the Panorama test still doesn't work I need a new minimized testcase to work with.

Comment 60

[u279076]

I think I've worked out the issue...

Before we were using tabBrowser.closeTab(); in this test.  After changing this to tabBrowser.closeTab("closeButton"); the test passes.  If this is the acceptable way, I'll post a patch.

Henrik?

Comment 61

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

Why does tabBrowser.closeTab() not work? It will use the file menu's entry to close the tab.

Comment 62

[u279076]

(In reply to comment #61)
> Why does tabBrowser.closeTab() not work? It will use the file menu's entry to
> close the tab.

I don't know WHY it doesn't work -- all I know is it just doesn't work.

Comment 63

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

So this comment applies to my updated version of the minimized testcase. While this could be a separate issue, you should check your patch v3 if it works now.

Comment 64

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

Do we have any update?

Comment 65

[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.