969303 - Add Mozmill test for reopening a recently closed tab via the shortcut (Shift+Accel+T)

Status: RESOLVED FIXED

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

Description

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

Similar to the existing test for http://hg.mozilla.org/qa/mozmill-tests/file/223aa4873b0f/firefox/tests/functional/testSessionStore/testUndoTabFromContextMenu.js we should have a test which exercises the keyboard shortcut Shift+Accel+T. This is a request because that shortcut most likely regressed in a couple of locales for Firefox 27.0. See bug 934231.

What should be done here is that we have to check mochitests, if the context menu case is already covered there. If yes, we can replace this test with the one for shortcut. Otherwise we might want to rename and enhance the test so we can cover both scenarios.

Comment 1

[Otilia Anica]

Cosmin will look into this.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

I looked over mochitest http://hg.mozilla.org/mozilla-central/file/default/browser/base/content/test/general
I couldn't find a test for tabs context menu to undo a closed tab.
I enhanced and renamed our test so we will cover both, the context menu and the shortcut.  

If this will get an r+ I will bring reports for more locales.
Report:
http://mozmill-crowd.blargon7.com/#/functional/report/e35f22a68fec3f6d144713f9ab0022da

Comment 3

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

WE are seeing more and more reports for this. Andreea and Andrei, why is this review waiting for 13 days now?

Comment 4

[Andrei Eftimie]

Comment on attachment 8372319 [details] [diff] [review]
patch_v1.0

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

You missed changing the name in the manifest file.

The test currently fails on OSX.
I can see a context menu remaining open then when we try to press Cmd+T to open a new tab we actually reach `Bookmark All Tabs` which responds to the T shortcut.

Comment 5

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

Comment on attachment 8372319 [details] [diff] [review]
patch_v1.0

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

::: firefox/tests/functional/testSessionStore/testUndoTab.js
@@ +23,5 @@
>  
>  var teardownModule = function(aModule) {
>    utils.closeContentAreaContextMenu(aModule.controller);
> +}
> +function setupTest(aModule) {

This method should be listed after setupModule

@@ +39,5 @@
> +  expect.equal(linkId.getNode().textContent, "1", "Second tab has correct id");
> +
> +  // Check 'Recently Closed Tabs' count, should be 0
> +  var tabCount = sessionStore.getClosedTabCount(aModule.controller);
> +  assert.equal(tabCount, 0, "'Recently Closed Tabs' sub menu has one entry");

Nothing of that should be part of teardownTest. This method only contains teardown code to clean up Firefox.

@@ +94,5 @@
> +
> +  // Restore recently closed tab via shortcut
> +  var cmdKey = utils.getEntity(tabBrowser.getDtds(), "tabCmd.commandkey");
> +  controller.keypress(null, cmdKey, {accelKey: true, shiftKey: true});
> +  controller.waitForPageLoad();

I think we should make this a new method on the tabBrowser class called 'reopen'.

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

I made the changes, also I make use of controller.getMenu to handle the context menu.
Thanks 

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/6062fdddb60e88657bc78ec1f4dfdc5b
http://mozmill-crowd.blargon7.com/#/functional/report/6062fdddb60e88657bc78ec1f4dfd8bc
http://mozmill-crowd.blargon7.com/#/functional/report/6062fdddb60e88657bc78ec1f4dfd1b9

Comment 7

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8380744 [details] [diff] [review]
patch_v2.0

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

::: firefox/lib/tabs.js
@@ +551,5 @@
> +   * Method for reopening the last closed tab
> +   *
> +   * @param  {String} [aEventType="shortcut"] Type of event which triggers the action
> +   */
> +  reopen: function (aEventType) {

reopen: function tabBrowser_reopen(aEventType)

@@ +567,5 @@
> +        break;
> +      case "contextMenu":
> +        var contextMenu = this._controller.getMenu("#tabContextMenu");
> +        contextMenu.select("#context_undoCloseTab", this.getTab());
> +        break;

Let's please switch the cases to be alphabetically sorted.

::: firefox/tests/functional/testSessionStore/testUndoTab.js
@@ +40,3 @@
>  }
>  
>  var testUndoTabFromContextMenu = function() {

Please add js doc

@@ +76,2 @@
>  
> +    // Check for correct id on 2nd tab, should be 1

Indentation is off.

@@ +76,3 @@
>  
> +    // Check for correct id on 2nd tab, should be 1
> +  var linkId = new elementslib.ID(controller.tabs.activeTab, "id");

linkId is declared above, no need for var.

@@ +85,3 @@
>  }
> +
> +var testUndoTabViaShortcut = function() {

Please add js doc for this function

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.1

I fixed the nits, thanks Andreea.

Comment 9

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8389726 [details] [diff] [review]
patch_v2.1

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

Just 2 changes left here. Please also update the peers in the commit message.

::: firefox/lib/tabs.js
@@ +567,5 @@
> +      case "contextMenu":
> +        var contextMenu = this._controller.getMenu("#tabContextMenu");
> +        contextMenu.select("#context_undoCloseTab", this.getTab());
> +        break;
> +      default:

default should be down where we have assert.fail

::: firefox/tests/functional/testSessionStore/testUndoTab.js
@@ +109,5 @@
> +
> +  // Check 'Recently Closed Tabs' count, should be 0
> +  var tabCount = sessionStore.getClosedTabCount(controller);
> +  assert.equal(tabCount, 0, "'Recently Closed Tabs' sub menu has one entry");
> +

Blank line which needs to be removed.

Comment 10

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

Comment on attachment 8389726 [details] [diff] [review]
patch_v2.1

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

Just some drive-by comments to shorten the review cycle...

::: firefox/lib/tabs.js
@@ +553,5 @@
>  
>    /**
> +   * Method for reopening the last closed tab
> +   *
> +   * @param  {String} [aEventType="shortcut"] Type of event which triggers the action

Description in the new line please.

@@ +575,5 @@
> +                                  {accelKey: true, shiftKey: true});
> +        break;
> +
> +        assert.fail("Unknown event type - " + type);
> +    }

You should really work with events here! As coded now the method is totally unstable and doesn't even wait for the tab to be restored.

::: firefox/tests/functional/testSessionStore/testUndoTab.js
@@ +89,5 @@
> +
> +/**
> + * Test 'Undo Close Tab' via shortcut
> + */
> +var testUndoTabViaShortcut = function() {

No var declarations please. Make sure you closely follow the coding styles.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.2

I updated the patch and I listen for events too, and I get ride of var daclaration when defining test functions.
http://mozmill-crowd.blargon7.com/#/functional/report/16dc29d7a411d565992bbd7dc8399d47
http://mozmill-crowd.blargon7.com/#/functional/report/16dc29d7a411d565992bbd7dc8399fc7
http://mozmill-crowd.blargon7.com/#/functional/report/16dc29d7a411d565992bbd7dc839a214

Comment 12

[Andrei Eftimie]

Comment on attachment 8394262 [details] [diff] [review]
patch_v2.2

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

A few small nits, otherwise it looks fine to me.

::: firefox/lib/tabs.js
@@ +887,5 @@
> +
> +    // Add event listener to wait until the tab has been opened
> +    this._controller.window.addEventListener("TabOpen", checkTabOpened);
> +    if (animationObserver.isAnimated) {
> +     this._tabs.getNode().addEventListener("transitionend", checkTabTransitioned);

nit: indentation

@@ +890,5 @@
> +    if (animationObserver.isAnimated) {
> +     this._tabs.getNode().addEventListener("transitionend", checkTabTransitioned);
> +    }
> +    else {
> +     self.transitioned = true;

nit: indentation

@@ +912,5 @@
> +
> +        assert.fail("Unknown event type - " + type);
> +    }
> +
> +    try {

This try block should also encompass the action (the above switch).

@@ +913,5 @@
> +        assert.fail("Unknown event type - " + type);
> +    }
> +
> +    try {
> +      assert.waitFor(function () {

fat arrow please :)

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.0

Thanks for review Andrei I fixed the nits.

Comment 14

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

Comment on attachment 8394777 [details] [diff] [review]
patch_v3.0

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

API changes look fine. Just some smaller things to get fixed.

::: firefox/lib/tabs.js
@@ +872,5 @@
>    /**
> +   * Method for reopening the last closed tab
> +   *
> +   * @param  {String} [aEventType="shortcut"]
> +   *                  Type of event which triggers the action

Align the description with the type identifier.

@@ +913,5 @@
> +
> +          assert.fail("Unknown event type - " + type);
> +      }
> +
> +      assert.waitFor(()=>self.opened && self.transitioned,

Spaces around operators.  Also please put the && operation inside brackets.

::: firefox/tests/functional/testSessionStore/testUndoTab.js
@@ +33,2 @@
>  var teardownModule = function(aModule) {
>    utils.closeContentAreaContextMenu(aModule.controller);

You want this in teardownTest() instead. If we are going to add one more test after the contextual test function, it could fail when the context menu is still open. You should test those negative cases.

@@ +68,5 @@
>    tabCount = sessionStore.getClosedTabCount(controller);
>    assert.equal(tabCount, 1, "'Recently Closed Tabs' sub menu has one entry");
>  
> +  // Restore recently closed tab via context menu
> +  tabBrowser.reopen("contextMenu");

Can we also add the main menu case here?

@@ +74,4 @@
>  
> +  contextMenu.open(tabBrowser.getTab());
> +  contextMenuItem = contextMenu.getItem("#context_undoCloseTab");
> +  assert.ok(contextMenuItem.getNode().disabled, "'Undo Close Tab' is disabled");

Do we have to really re-fetch the element? Can't we use the state of the variable setup earlier?

@@ +88,4 @@
>  }
> +
> +/**
> + * Test 'Undo Close Tab' via shortcut

Please add a bug number

Comment 15

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #14)
> > +  // Restore recently closed tab via context menu
> > +  tabBrowser.reopen("contextMenu");
> 
> Can we also add the main menu case here?
Yes, but I would have to change the library again and add an extra test. Should I do that?

Comment 16

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

It is a valid path to test, isn't it? So I don't understand the question that you have to change the library. We will always have to change libraries. Finally we will have all bits together and call this done.

Comment 17

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.0

I made the changes and I added the test for main-menu case.
Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/6881c00549f3ec04f84ba63c97122805
http://mozmill-crowd.blargon7.com/#/functional/report/6881c00549f3ec04f84ba63c97122be6
http://mozmill-crowd.blargon7.com/#/functional/report/6881c00549f3ec04f84ba63c971222f5

Comment 18

[Andrei Eftimie]

Comment on attachment 8396397 [details] [diff] [review]
patch_v4.0

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

This looks good to me.

Please update the comments with the bug numbers and we can get this landed.

::: firefox/tests/functional/testSessionStore/testUndoTab.js
@@ +36,5 @@
> +  utils.closeContentAreaContextMenu(aModule.controller);
> +}
> +
> +/**
> + * Test 'Undo Close Tab' via context menu

I think we should probably also add the bug number here. Well I'm not sure if it should be this bug or the one that initially added the test...

@@ +109,5 @@
> +
> +}
> +
> +/**
> + * Test 'Undo Close Tab' via main-menu

The bug number is missing.

Comment 19

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.1

Thanks for review Andrei, I added the bug number for the other new tests, and as I discussed on irc with Henrik we don't have to add it for old tests.

Comment 20

[Andrei Eftimie]

Comment on attachment 8397089 [details] [diff] [review]
patch_v4.1

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

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

Comment 21

[Andrei Eftimie]

Since the initial request mentioned a regression in Firefox 27, we should backport this down to Beta.

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

The patch applies cleanly on both aurora and beta.
Thanks

Comment 23

[Andreea Matei [:AndreeaMatei]]

http://hg.mozilla.org/qa/mozmill-tests/rev/b3df54e65503 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/03345c0a2b25 (beta)

Thanks Cosmin!

Comment 24

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

We should even backport this to release, so we can also cover this for possible chem-spill releases.

Comment 25

[Cosmin Malutan, [:cosmin-malutan]]

The patch applies cleanly on release too:
http://mozmill-crowd.blargon7.com/#/functional/report/3f9e4de2c971ca3efecc0cf497065d08
http://mozmill-crowd.blargon7.com/#/functional/report/3f9e4de2c971ca3efecc0cf49706620d
http://mozmill-crowd.blargon7.com/#/functional/report/3f9e4de2c971ca3efecc0cf497066c00

Comment 26

[Andreea Matei [:AndreeaMatei]]

http://hg.mozilla.org/qa/mozmill-tests/rev/03345c0a2b25 (release)