894883 - Failure "The Add-ons Manager has been opened in a second tab - '1' should equal '2'' in /testAddons/testManagerKeyboardShortcut.js

Status: RESOLVED FIXED

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

Description

[Cosmin Malutan, [:cosmin-malutan]]

Happened today on Windows 7 x64 with Aurora

Report
http://mozmill-ci.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947a837a0

I couldn't reproduce it locally neither manual or as testrun :
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947abc7a0

Comment 1

[Andreea Matei [:AndreeaMatei]]

I haven't seen this again on the old mozmill-ci, neither on mozmill-daily. Updating the url so we can keep track.
Please reopen if needed.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

Happened again on Windows 7 with Release zh-CN
http://mozmill-release.blargon7.com/#/functional/report/b7ef1fb3d9703aeaf2c46e07d2e74eeb

Comment 3

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

Is it reproducible with this locale?

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

Yes, I will investigate this now.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

Hi, I investigated this and the reason why it fails is that the browser it checks if the tab is empty or not, and if it is will open the addonsManager tab in the same tab:
Here it checks the tab:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6266
Here it checks the url of tab:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#46

We can solve this in different ways:
http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testAddons/testManagerKeyboardShortcut.js#l33
1 - By doing  a checking like the browser does before opening addons manager so if the page appears to be empty the tabs count we check should still be 1
2 - Before opening the addons manager, we could navigate to "mozilla.org" so the page will never be empty and the tabs count will be 2 every time if the addons manager tabs opens correctly
3 - By checking the active tab url to be "about:addons"

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

I personally prefer the last one.

Comment 7

[Andreea Matei [:AndreeaMatei]]

Second option is out of the question, the first I feel is too complicated for what this test is supposed to do. We have 2 behaviors when we open a page from a loaded tab or an empty tab, but it's tabs general behavior, not related to addons manager.

The last sounds good for what the test has to do.

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572eb5e7bd
http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572eb42def
http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572eb60e89

Comment 9

[Andreea Matei [:AndreeaMatei]]

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

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

Please make the commit message more general next time and add flags for what branches are affected here to backport.

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

Comment 10

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

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

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

::: tests/functional/testAddons/testManagerKeyboardShortcut.js
@@ +29,5 @@
>  */
>  function testKeyboardShortcut() {
>    addonsManager.open({type: "shortcut"});
>  
> +  expect.equal(controller.tabs.activeTab.URL, "about:addons",

Please change that in a follow-up to ´.location´. URL is used internally and is not part of the DOM spec. Thanks.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: follow-up patch

Here is the follow up patch. 
Thanks.

Comment 12

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 794618 [details] [diff] [review]
follow-up patch

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

Follow up landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/0a436bcfec07 (default)

Please flag the affected branches here and create a combined patch if necessary. Thanks.

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

This patch lands clearly on Esr, Aurora, Release and Beta and it fixes the bug

Comment 14

[Andreea Matei [:AndreeaMatei]]

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

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

http://hg.mozilla.org/qa/mozmill-tests/rev/55f48bc3f527 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/179c92b9fc4a (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/830178d0815d (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/67d7968d462c (esr17)

Thanks Cosmin!