924077 - Write mozmill metro test for opening and closing tabs

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Andreea Matei [:AndreeaMatei]]

This test should open and close tabs through all the options metro mode offers (shortkey, tabs container, add/close tab button).

Open a new tab from the new tab button:
https://bug831928.bugzilla.mozilla.org/attachment.cgi?id=703498

Close a tab
https://bug831925.bugzilla.mozilla.org/attachment.cgi?id=703496

Comment 1

[mario garbi]

Attachment: newOpenCloseTab_MetroTest.patch

This requires some changes to the libraries that should be made in their respective bugs but in order to be tested I made the changes here also. I have added a few cases for opening and closing tabs and created the test that opens and close tabs using the overlay button, the shortcut and the toolbar button.

Any feedback is welcomed and as I said it's not a final version, if there are better ways of doing it I am happy to change it.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 830089 [details] [diff] [review]
newOpenCloseTab_MetroTest.patch

Review of attachment 830089 [details] [diff] [review]:
-----------------------------------------------------------------

In general it's fine. But there are still some things to further think about. Also is there a Moztrap test this test is covering? The meta information should be added.

::: metrofirefox/lib/ui/tabs.js
@@ +197,5 @@
>          break;
> +	  case "overlayButton":
> +        var newTabButton = this.getElement({type: "overlayPlus"});
> +        newTabButton.tap();
> +        break;	

Not sure what you are trying to do here. getElements() doesn't exist to trigger actions on elements but retrieving them.

@@ +254,5 @@
>          var win = new findElement.MozMillElement("Elem", this._controller.window);
>          var cmdKey = utils.getEntity(this.dtds, "closeTab.key");
>          win.keypress(cmdKey, {accelKey: true});
>          break;
> +	  case "shortcut2":

I wouldn't call it that way. It should be reflect that is uses a function key. Is there no DTD entity for it?

Also lots of tabs used in your patch.

::: metrofirefox/tests/functional/testTabbedBrowsing/testMetro_openCloseTab.js
@@ +21,5 @@
> +  controller.waitForPageLoad();
> +  
> +  // Open a tab using the overlay button
> +  tabBrowser.openTab("overlayButton");
> +  assert.equal(tabBrowser.length, 2, "Tab has been opened");

Opend how? Please add more details to the message.

@@ +34,5 @@
> +
> +  // Open and close tab using new tab button
> +  tabBrowser.openTab("newTabButton");
> +  assert.equal(tabBrowser.length, 2, "Tab has been opened");
> +  tabBrowser.closeTab("shortcut");

What about the close button?

Comment 3

[mario garbi]

Attachment: openCloseTab.patch

Remove the library changes from the patch due to Andreeas' patch and tested locally using mozmill-sandbox:

http://mozmill-sandbox.blargon7.com/#/functional/report/94e33fe3d7ec0be6dbbfa0a702ebeaa7
http://mozmill-sandbox.blargon7.com/#/functional/report/94e33fe3d7ec0be6dbbfa0a702ebe54a
http://mozmill-sandbox.blargon7.com/#/functional/report/94e33fe3d7ec0be6dbbfa0a702ebe0d6
http://mozmill-sandbox.blargon7.com/#/functional/report/94e33fe3d7ec0be6dbbfa0a702ebdfb1

Comment 4

[Andrei Eftimie]

Comment on attachment 8363518 [details] [diff] [review]
openCloseTab.patch

Review of attachment 8363518 [details] [diff] [review]:
-----------------------------------------------------------------

This needs an update to match the updated UI lib.

::: metrofirefox/tests/functional/testTabbedBrowsing/manifest.ini
@@ +1,1 @@
> +[testMetro_openCloseTab.js]

I would leave "metro" out of the name, we are in the metro subfolder, it should be clear.

::: metrofirefox/tests/functional/testTabbedBrowsing/testMetro_openCloseTab.js
@@ +9,5 @@
> +var tabs = require("../../../lib/ui/tabs");
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.tabBrowser = new tabs.Tabs(aModule.controller);

You'll need to update your code as this has changed in the UI lib.

@@ +19,5 @@
> +
> +  var tabsLength = tabBrowser.length;
> +
> +  // Open a tab using the overlay plus button and close it with shortcut keys
> +  tabBrowser.openTab("sideNewTab");

Also check these names, they have also changed.

Comment 5

[mario garbi]

Attachment: openCloseTab_v3.patch

Updated the patch including the suggestions in the previous review.

Comment 6

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8367296 [details] [diff] [review]
openCloseTab_v3.patch

Review of attachment 8367296 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenCloseTab.js
@@ +12,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.tabBrowser = new tabs.TabBrowser(aModule.controller);
> +}
> +
> +function testMetroOpenCloseTabs() {

I miss the js documentation. Also the name shouldn't need Metro as we are in the metro part of the repo.

@@ +20,5 @@
> +  var tabsLength = tabBrowser.length;
> +
> +  // Open a tab using the overlay plus button and close it with shortcut keys
> +  tabBrowser.openTab("sidebarButton");
> +  assert.equal(tabBrowser.length, tabsLength + 1, "Tab has been opened");

I'd like to see the messages more complete, to specify through what was the action e.g "Tab has been opened via the sidebar button".

@@ +25,5 @@
> +  tabBrowser.closeTab();
> +  assert.equal(tabBrowser.length, tabsLength, "Tab has been closed");
> +
> +  // Open tab using shortcut keys and close it with shortcut keys
> +  tabBrowser.openTab("shortcut");

This is the default value if none is given. We should also add the check for shortcut2.

@@ +34,5 @@
> +  // Open tab using new tab button and close it using the close button
> +  tabBrowser.openTab("newTabButton");
> +  assert.equal(tabBrowser.length, tabsLength + 1, "Tab has been opened");
> +  tabBrowser.closeTab("button", undefined);
> +  assert.equal(tabBrowser.length, tabsLength, "Tab has been closed");

We could use a forEach to open the tabs and one to close them through all the methods.

Comment 7

[Daniel Gherasim]

Attachment: openCloseTab_v4.patch

Updated the patch with the requirements from the last review.

Comment 8

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8369936 [details] [diff] [review]
openCloseTab_v4.patch

Review of attachment 8369936 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenCloseTabs.js
@@ +7,5 @@
> +// Include required modules
> +var { assert } = require("../../../../lib/assertions");
> +var tabs = require("../../../lib/ui/tabs");
> +
> +const OPEN_CLOSE_TABS_ELEMENTS = {

I'd call this METHODS or just ELEMENTS.

@@ +9,5 @@
> +var tabs = require("../../../lib/ui/tabs");
> +
> +const OPEN_CLOSE_TABS_ELEMENTS = {
> +  // Ways to open the tab via tabBrowser class
> +  "openTab": ["default", "sidebarButton", "shortcut2", "newTabButton"],

What is default? And it should have "shortcut" added too, alphabetically sorted please.

@@ +11,5 @@
> +const OPEN_CLOSE_TABS_ELEMENTS = {
> +  // Ways to open the tab via tabBrowser class
> +  "openTab": ["default", "sidebarButton", "shortcut2", "newTabButton"],
> +  // Ways to close the tab via tabBrowser class
> +  "closeTab": ["default", "default", "default", "button"]

Same here about default, I don't see why it's duplicated anyway. This should have the methods in the library for closing the tab.

@@ +33,5 @@
> +
> +  // Check if the number of close and open tab ways is equal
> +  assert.equal(OPEN_CLOSE_TABS_ELEMENTS.openTab.length,
> +               OPEN_CLOSE_TABS_ELEMENTS.closeTab.length,
> +               "Number of open and close tab ways is equal");

Why would this be necessary?

@@ +39,5 @@
> +  // Get the initial number of opened tabs
> +  var tabsLength = tabBrowser.length;
> +
> +  // Open tabs through all defined ways
> +  OPEN_CLOSE_TABS_ELEMENTS.openTab.forEach(function(el) {

aElement as parameter and a space after function.

@@ +43,5 @@
> +  OPEN_CLOSE_TABS_ELEMENTS.openTab.forEach(function(el) {
> +    if(el !== "default")
> +      tabBrowser.openTab(el, undefined);
> +    else
> +      tabBrowser.openTab();

This if/else won't be necessary if we use all the methods in the array. The default one is one of the methods.

@@ +55,5 @@
> +  OPEN_CLOSE_TABS_ELEMENTS.closeTab.forEach(function(el) {
> +    if(el !== "default")
> +      tabBrowser.closeTab(el);
> +    else
> +      tabBrowser.closeTab();

Same here.

Comment 9

[Daniel Gherasim]

Attachment: openCloseTab_v5.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #8)
> Comment on attachment 8369936 [details] [diff] [review]
> openCloseTab_v4.patch
> 
> Review of attachment 8369936 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenCloseTabs.js
> @@ +9,5 @@
> > +var tabs = require("../../../lib/ui/tabs");
> > +
> > +const OPEN_CLOSE_TABS_ELEMENTS = {
> > +  // Ways to open the tab via tabBrowser class
> > +  "openTab": ["default", "sidebarButton", "shortcut2", "newTabButton"],
> 
> What is default? And it should have "shortcut" added too, alphabetically
> sorted please.

Removed default and added "shortcut" instead.

> @@ +33,5 @@
> > +
> > +  // Check if the number of close and open tab ways is equal
> > +  assert.equal(OPEN_CLOSE_TABS_ELEMENTS.openTab.length,
> > +               OPEN_CLOSE_TABS_ELEMENTS.closeTab.length,
> > +               "Number of open and close tab ways is equal");
> 
> Why would this be necessary?

That was needed to ensure that we close them all or we don't have more closeTab methods than openTabs.

I have changed the code now a bit & the rest of the tabs are closed in teardownModule. 

Shouldn't we have a check to ensure number of close tab methods is less than or equal to the open tab methods ?

Comment 10

[Andreea Matei [:AndreeaMatei]]

(In reply to daniel.gherasim from comment #9)
> Shouldn't we have a check to ensure number of close tab methods is less than
> or equal to the open tab methods ?

Not really, we care about testing the available open/close methods. Doesn't matter how many each has, if tabs remain opened, they get closed in teardown.

Comment 11

[Andrei Eftimie]

Comment on attachment 8371345 [details] [diff] [review]
openCloseTab_v5.patch

Review of attachment 8371345 [details] [diff] [review]:
-----------------------------------------------------------------

There are some problems with the tabOpen and tabClose methods.

I don't think we're correctly waiting for the tabs to open/close.
Right now this test goes too fast that's why it is appearing to pass (I am not sure, it seems like our listeners get confused, we attach all of them, and the first tab that opens triggers all of them), if you slow it down a bit you will see that it fails.

For testing add some sleep() calls before/after you open/close tabs and you'll notice the issues.

::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenCloseTabs.js
@@ +7,5 @@
> +// Include required modules
> +var { assert } = require("../../../../lib/assertions");
> +var tabs = require("../../../lib/ui/tabs");
> +
> +const ELEMENTS = {

These are not elements, but different methods by which we are opening/closing tabs.

@@ +8,5 @@
> +var { assert } = require("../../../../lib/assertions");
> +var tabs = require("../../../lib/ui/tabs");
> +
> +const ELEMENTS = {
> +  // Methods to open the tab via tabBrowser class

I don't think these comment is necessary

@@ +10,5 @@
> +
> +const ELEMENTS = {
> +  // Methods to open the tab via tabBrowser class
> +  "openTab": ["newTabButton", "shortcut", "shortcut2", "sidebarButton"],
> +  // Methods to close the tab via tabBrowser class

Same

@@ +28,5 @@
> + * Test open and close tabs functionality
> + */
> +function testOpenCloseTabs() {
> +  controller.open("about:home");
> +  controller.waitForPageLoad();

I was thinking that we don't need to open a page, but without this we can't open a tab with "sidebarButton" since that button is not available on the startScreen.

But any new opened tab is again set to the startScreen, so is we want to test the "sidebarButton" we should test that method first.

@@ +34,5 @@
> +  // Get the initial number of opened tabs
> +  var tabsLength = tabBrowser.length;
> +
> +  // Open tabs through all defined methods
> +  ELEMENTS.openTab.forEach(function (aElement) {

We are not iterating on elements, but on methods

@@ +37,5 @@
> +  // Open tabs through all defined methods
> +  ELEMENTS.openTab.forEach(function (aElement) {
> +    tabBrowser.openTab(aElement);
> +
> +    // Check if the number of opened tabs is incremented by 1

I think its pretty clear what we are doing here.
We could remove this comment.

@@ +39,5 @@
> +    tabBrowser.openTab(aElement);
> +
> +    // Check if the number of opened tabs is incremented by 1
> +    assert.equal(tabBrowser.length, ++tabsLength,
> +                 "Tab has been opened via '" + aElement + "' element");

Please also drop the "' element"

Comment 12

[Daniel Gherasim]

Attachment: openCloseTab_v6.patch

For waiting the tab open I used waitForPageLoad() as a workaround. 
Looks like the TabOpen event doesn't actually wait for the tab animation to finish, so we need to find a solution for this in the library, filled bug 969397 for this.

Comment 13

[Daniel Gherasim]

Attachment: openCloseTab_v6.1.patch

As bug 969397 will get fixed, this test will work without the workaround.

Added the final patch and bugg 969397 as a blocker for this.

Comment 14

[Andrei Eftimie]

Comment on attachment 8373217 [details] [diff] [review]
openCloseTab_v6.1.patch

Review of attachment 8373217 [details] [diff] [review]:
-----------------------------------------------------------------

This has r+ from me with a small update to the error message mentioned below.
Please request a review from Henrik after that update.

::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenCloseTabs.js
@@ +36,5 @@
> +  METHODS.openTab.forEach(function (aMethod) {
> +    tabBrowser.openTab(aMethod);
> +    assert.equal(tabBrowser.length, ++tabsLength,
> +                 "Tab has been opened via '" + aMethod);
> +  });

You forgot a apostrophe in here. Also in the message below.

Comment 15

[Daniel Gherasim]

Attachment: openCloseTab_v6.1.patch

Sorry for missing that.

Uptated now and asked Henrik for final review.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 8373353 [details] [diff] [review]
openCloseTab_v6.1.patch

Review of attachment 8373353 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenCloseTabs.js
@@ +29,5 @@
> +  controller.open("about:home");
> +  controller.waitForPageLoad();
> +
> +  // Get the initial number of opened tabs
> +  var tabsLength = tabBrowser.length;

We should close all open tabs in setup so we should have 1 here.

@@ +35,5 @@
> +  // Open tabs through all defined methods
> +  METHODS.openTab.forEach(function (aMethod) {
> +    tabBrowser.openTab(aMethod);
> +    assert.equal(tabBrowser.length, ++tabsLength,
> +                 "Tab has been opened via '" + aMethod + "'");

I wonder if we could make it an expect, so we can really test all variations and do not abort early and miss other failures.

@@ +43,5 @@
> +  METHODS.closeTab.forEach(function (aMethod) {
> +    tabBrowser.closeTab(aMethod);
> +    assert.equal(tabBrowser.length, --tabsLength,
> +                 "Tab has been closed via '" + aMethod  + "'");
> +  });

Same as above.

Comment 17

[Daniel Gherasim]

Attachment: openCloseTab_v6.2.patch

Thanks.
I updated patch with requested changes now.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 8373987 [details] [diff] [review]
openCloseTab_v6.2.patch

Review of attachment 8373987 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenCloseTabs.js
@@ +44,5 @@
> +  // Close tabs through all defined methods
> +  METHODS.closeTab.forEach(function (aMethod) {
> +    tabBrowser.closeTab(aMethod);
> +    expect.equal(tabBrowser.length, --tabsLength,
> +                 "Tab has been closed via '" + aMethod  + "'");

Given that we make use of expect above, you can't assume that 5 tabs are open now. It could be 4, 3, 2 or even only 1. So we probably would unexpectedly close the browser when trying to close the last open tab.

Comment 19

[Daniel Gherasim]

Attachment: openCloseTab_v6.3.patch

Thanks whimboo,
Regarding closing the last tab, in metro firefox that actually triggers opening a new one. 

So to test the close methods by checking the tabLength parameter we need at least 2 tabs opened so nothig else gets triggered in the way.

I updated the test with this new condition and to always have the right number of opened tabs saved in tabsLength variable ( given that we use expect now ).

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 8374772 [details] [diff] [review]
openCloseTab_v6.3.patch

Review of attachment 8374772 [details] [diff] [review]:
-----------------------------------------------------------------

The latest update totally over-complicates the test. All those additional checks are not necessary. I would do the following:

* Create two distinct test methods in this module. One for open and one for close
** Set the correct pre-conditions
** Run the test and checks if all is fine
** Run cleanup code

Comment 21

[Daniel Gherasim]

Attachment: openCloseTab_v7.patch

Updated pach as requested.

Comment 22

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8375380 [details] [diff] [review]
openCloseTab_v7.patch

Review of attachment 8375380 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenCloseTabs.js
@@ +31,5 @@
> +  var tabsLength = tabBrowser.length;
> +
> +  // Open tabs through all defined methods
> +  METHODS.openTab.forEach(function (aMethod) {
> +    tabBrowser.openTab(aMethod);

On a previous patch you had to open about:home or smth in order to have sidebarButton visible to click on it. Has that been fixed?

@@ +47,5 @@
> +function testCloseTabs() {
> +  // Open some tabs to test the close methods
> +  while(tabBrowser.length <= METHODS.closeTab.length)
> +    tabBrowser.openTab();
> +

We'll have to assert that we have the necessary number of tabs here. It's a must to be able to close them.

@@ +58,5 @@
> +                 "Tab has been closed via '" + aMethod  + "'");
> +    tabsLength = tabBrowser.length;
> +  });
> +
> +  tabBrowser.closeAllTabs();

Basically we expect all tabs to be closed at this point. If that's not the case, we have teardownModule. In testOpenTabs() it was necessary to close them in order to continue with this function.

Comment 23

[Daniel Gherasim]

Attachment: openCloseTab_v7.1.patch

Thanks,

So I updated the patch with the following changes:
* added code to open the about:home page to have the sidebar elements visible first;
* changed the "while" condition when opening tabs so we don't risk an infinite loop if the openTab fails;
* added an assert for testing the number of opened tabs so we can test closing them;

Comment 24

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8378908 [details] [diff] [review]
openCloseTab_v7.1.patch

Review of attachment 8378908 [details] [diff] [review]:
-----------------------------------------------------------------

Please request review from Henrik and Dave next.

::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenCloseTabs.js
@@ +53,5 @@
> +  for(var i = 0; i < METHODS.closeTab.length; i++)
> +    tabBrowser.openTab();
> +
> +  assert.ok(tabBrowser.length > METHODS.closeTab.length,
> +            "Number of opened tabs is ok");

I'd give a better message here like "All required tabs have been opened".

Comment 25

[Daniel Gherasim]

Attachment: openCloseTab_v7.2.patch

Thanks, I updated the patch and asked for final review.

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8380504 [details] [diff] [review]
openCloseTab_v7.2.patch

Review of attachment 8380504 [details] [diff] [review]:
-----------------------------------------------------------------

Changing to f+ because it looks mostly good but I have some questions. Also leaving review flag for Henrik for now.

::: metrofirefox/tests/functional/testTabbedBrowsing/manifest.ini
@@ +1,1 @@
> +[testOpenCloseTabs.js]

Is there a reason these aren't separate test files? As far as I can tell they are not dependent on each others, and by combining them we can't address them independently in the manifest files.

::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenCloseTabs.js
@@ +9,5 @@
> +var tabs = require("../../../lib/ui/tabs");
> +var prefs = require("../../../../firefox/lib/prefs");
> +
> +const METHODS = {
> +  openTab: ["sidebarButton", "newTabButton", "shortcut", "shortcut2"],

What is shortcut2?

@@ +28,5 @@
> +/**
> + * Test open tabs functionality
> + */
> +function testOpenTabs() {
> +  controller.open("about:home");

I'm not familiar with Metro, but is there a reason we can't use about:blank? Also, it would be useful to add a comment to briefly explain why we need to open a page at all.

Comment 27

[Daniel Gherasim]

Attachment: openCloseTab_v7.3.patch

> > +[testOpenCloseTabs.js]
>
> Is there a reason these aren't separate test files? As far as I can tell
> they are not dependent on each others, and by combining them we can't
> address them independently in the manifest files.
>

Hmm, you are right, it may be better to have them as independent tests so if one fails we won't disable both of them.

> > +  openTab: ["sidebarButton", "newTabButton", "shortcut", "shortcut2"],
>
> What is shortcut2?
>

It's ctrl+n, in metrofirefox this shortcut opens a new tab too.
Would it be better to name it shortcutCmd_N ?

> > +  controller.open("about:home");
>
> I'm not familiar with Metro, but is there a reason we can't use about:blank?
> Also, it would be useful to add a comment to briefly explain why we need to
> open a page at all.

about:blank works fine as well & we need this for the sidebar bars to become visible.

Comment 28

[Daniel Gherasim]

Attachment: openCloseTab_v7.4.patch

Forgot to add de 2nd test in the patch.

Comment 29

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to daniel.gherasim from comment #27)
> > > +  openTab: ["sidebarButton", "newTabButton", "shortcut", "shortcut2"],
> >
> > What is shortcut2?
> 
> It's ctrl+n, in metrofirefox this shortcut opens a new tab too.
> Would it be better to name it shortcutCmd_N ?

No, this value should be independent of the actual shortcut. It's fine to leave it as it is, I was just curious. :-)

> > > +  controller.open("about:home");
> >
> > I'm not familiar with Metro, but is there a reason we can't use about:blank?
> > Also, it would be useful to add a comment to briefly explain why we need to
> > open a page at all.
> 
> about:blank works fine as well & we need this for the sidebar bars to become
> visible.

Great, thanks!

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 8383032 [details] [diff] [review]
openCloseTab_v7.4.patch

Review of attachment 8383032 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/tests/functional/testTabbedBrowsing/testCloseTabs.js
@@ +25,5 @@
> +  tabBrowser.closeAllTabs();
> +}
> +
> +/**
> + * Test close tabs functionality

Mind adding this Bug number for easier finding the implementation bug later? That would also apply to the other test.

@@ +30,5 @@
> + */
> +function testCloseTabs() {
> +  // Open some tabs to test the close methods
> +  for(var i = 0; i < METHODS.length; i++)
> +    tabBrowser.openTab();

nit: Please obey the coding styles and put brackets around the above line. I can remember that has been mentioned already. Also add a blank after 'for' and the opening bracket.

@@ +33,5 @@
> +  for(var i = 0; i < METHODS.length; i++)
> +    tabBrowser.openTab();
> +
> +  assert.ok(tabBrowser.length > METHODS.length,
> +            "All required tabs have been opened");

Do we really need this assert? Isn't openTab() checking that the tab has been successfully opened?

@@ +42,5 @@
> +  METHODS.forEach(function (aMethod) {
> +    tabBrowser.closeTab(aMethod);
> +    expect.equal(tabBrowser.length, tabsLength - 1,
> +                 "Tab has been closed via '" + aMethod  + "'");
> +    tabsLength = tabBrowser.length;

tabsLength is only used inside the for loop, so please move its declaration in which also makes this line obsolete.

::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenTabs.js
@@ +30,5 @@
> +/**
> + * Test open tabs functionality
> + */
> +function testOpenTabs() {
> +  // Open any page so the sidebar buttons become visible

Not sure what this comment should tell me. Personally I would prefer if we could open the default start page and not about:blank similar to our other tests.

@@ +41,5 @@
> +  METHODS.forEach(function (aMethod) {
> +    tabBrowser.openTab(aMethod);
> +    expect.equal(tabBrowser.length, tabsLength + 1,
> +                 "Tab has been opened via '" + aMethod + "'");
> +    tabsLength = tabBrowser.length;

Same as for the other test.

Comment 31

[Daniel Gherasim]

Attachment: openCloseTab_v7.5.patch

Updated patch as Henrik requested.

Comment 32

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8386068 [details] [diff] [review]
openCloseTab_v7.5.patch

Review of attachment 8386068 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenTabs.js
@@ +27,5 @@
> +  tabBrowser.closeAllTabs();
> +}
> +
> +/**
> + * Test open tabs functionality - bug 924077

Please put this in the same format as for TODO, bug on the first line and description below. Same for the other test.

@@ +30,5 @@
> +/**
> + * Test open tabs functionality - bug 924077
> + */
> +function testOpenTabs() {
> +  controller.open("about:start");

Hm, about:start as I know doesn't have the sidebar buttons. It's the main page with the containers for bookmarks, top sites..

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 8386068 [details] [diff] [review]
openCloseTab_v7.5.patch

Review of attachment 8386068 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/tests/functional/testTabbedBrowsing/testOpenTabs.js
@@ +27,5 @@
> +  tabBrowser.closeAllTabs();
> +}
> +
> +/**
> + * Test open tabs functionality - bug 924077

"Bug 924077: Test open tabs functionality" would save the second line, which I personally would prefer.

Comment 34

[Daniel Gherasim]

(In reply to Andreea Matei [:AndreeaMatei] from comment #32)
> > +function testOpenTabs() {
> > +  controller.open("about:start");
> 
> Hm, about:start as I know doesn't have the sidebar buttons. It's the main
> page with the containers for bookmarks, top sites..

It also works, tested it manually. So it actually works with any page. The sidebar becomes visible once we have a page to go back to ( given that we also have the back button on the sidebar ). 
That actually looks to me as a UI architecture problem because the *new tab* sidebar button should be visible from the start, somehow handled separately to the *back button*.

Comment 35

[Daniel Gherasim]

Attachment: openCloseTab_v7.6.patch

Uploaded the patch with the changed comments.

Comment 36

[Andrei Eftimie]

Comment on attachment 8386670 [details] [diff] [review]
openCloseTab_v7.6.patch

Review of attachment 8386670 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.

Henrik asked in a previous review if we could use the standard new page instead of hardcoding it.
You didn't mention anything about that.

For desktop Firefox we use `browser.newtab.url` and `browser.startup.homepage`.
I've checked and couldn't find any of the pages we use as being available as a preferences in metro.
So we might as well leave them like they are now.

r+ from me.
Henrik, do you want to have another look?

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 8386670 [details] [diff] [review]
openCloseTab_v7.6.patch

Review of attachment 8386670 [details] [diff] [review]:
-----------------------------------------------------------------

Not sure why I have to review this again for those minor changes (no API work) left to do. You could have landed it with your review.

Comment 38

[Andrei Eftimie]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/e556d0beef64 (default)

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

We are not going to backport those patches.