Closed Bug 969303 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect

Tracking

(firefox28 fixed, firefox29 fixed, firefox30 fixed, firefox31 fixed)

RESOLVED FIXED
Tracking Status
firefox28 --- fixed
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: whimboo, Assigned: cosmin-malutan)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Cosmin will look into this.
Assignee: nobody → cosmin.malutan
Status: NEW → ASSIGNED
Attached patch patch_v1.0 (obsolete) — Splinter Review
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
Attachment #8372319 - Flags: review?(andrei.eftimie)
Attachment #8372319 - Flags: review?(andreea.matei)
WE are seeing more and more reports for this. Andreea and Andrei, why is this review waiting for 13 days now?
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.
Attachment #8372319 - Flags: review?(andrei.eftimie)
Attachment #8372319 - Flags: review?(andreea.matei)
Attachment #8372319 - Flags: review-
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'.
Attachment #8372319 - Flags: review-
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
Attachment #8380744 - Flags: review?(andrei.eftimie)
Attachment #8380744 - Flags: review?(andreea.matei)
Attachment #8380744 - Flags: review-
Attached patch patch_v2.1 (obsolete) — Splinter Review
I fixed the nits, thanks Andreea.
Attachment #8372319 - Attachment is obsolete: true
Attachment #8380744 - Attachment is obsolete: true
Attachment #8389726 - Flags: review?(andreea.matei)
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.
Attachment #8389726 - Flags: review?(andreea.matei) → review-
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.
Attachment #8389726 - Flags: review-
Attached patch patch_v2.2 (obsolete) — Splinter Review
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
Attachment #8389726 - Attachment is obsolete: true
Attachment #8394262 - Flags: review?(andrei.eftimie)
Attachment #8394262 - Flags: review?(andreea.matei)
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 :)
Attachment #8394262 - Flags: review?(andrei.eftimie)
Attachment #8394262 - Flags: review?(andreea.matei)
Attachment #8394262 - Flags: review-
Attached patch patch_v3.0 (obsolete) — Splinter Review
Thanks for review Andrei I fixed the nits.
Attachment #8394262 - Attachment is obsolete: true
Attachment #8394777 - Flags: review?(hskupin)
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
Attachment #8394777 - Flags: review?(hskupin) → review-
(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?
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 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.
Attachment #8396397 - Flags: review?(andrei.eftimie)
Attachment #8396397 - Flags: review?(andreea.matei)
Attachment #8396397 - Flags: review+
Attached patch patch_v4.1Splinter Review
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.
Attachment #8396397 - Attachment is obsolete: true
Attachment #8397089 - Flags: review?(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)
Attachment #8397089 - Flags: review?(andrei.eftimie)
Attachment #8397089 - Flags: review+
Attachment #8397089 - Flags: checkin+
Since the initial request mentioned a regression in Firefox 27, we should backport this down to Beta.
The patch applies cleanly on both aurora and beta.
Thanks
http://hg.mozilla.org/qa/mozmill-tests/rev/b3df54e65503 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/03345c0a2b25 (beta)

Thanks Cosmin!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
We should even backport this to release, so we can also cover this for possible chem-spill releases.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
http://hg.mozilla.org/qa/mozmill-tests/rev/03345c0a2b25 (release)
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.