1002394 - Test failure "Add-ons Manager has been opened" across addons tests

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Andreea Matei [:AndreeaMatei]]

All tests that used the opening method from addons.js failed over weekend on all platforms. I have to investigate if this is a new regression or we had issues with AMO.

Affected line:
http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/addons.js#l161

This failed with Aurora only. I'll try to reproduce and come back with details.

http://mozmill-daily.blargon7.com/#/functional/report/db3ef7ec039e4c6b255196b62828bc67
http://mozmill-daily.blargon7.com/#/remote/report/db3ef7ec039e4c6b255196b6282819b7

Comment 1

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

I don't see why this should be a problem with AMO. The functional tests don't make use of it. What I could think of is that the opening/closing tabs changes have been backported to aurora.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

This doesn't reproduce with en-US build but it does with the zh-CN, perhaps with other locales too. What I can obviously see is that on en-US the Addons-manager is opened in a new tab, and on zh-CN in the same tab, that's why it doesn't have an animation. 
I will investigate this and try to find the root of this behavior.

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

This is the culprit 
http://hg.mozilla.org/releases/l10n/mozilla-aurora/zh-CN/rev/3d76f9148714#l1.12
The preference for start-up page was removed, if I set the preference in test, the test will pass.
I guess this is something desired, and we should fix this in our tests, by checking if the add-ons manager is opened in a new tab or in the current on.

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

Actually I think this is rather a bug in FF, because this failed only on zh-CN.
http://mozmill-daily.blargon7.com/#/functional/failure?app=All&branch=All&platform=All&from=2014-04-26&to=&test=%2FrestartTests%2FtestAddons_uninstallTheme%2Ftest3.js&func=testThemeIsUninstalled

If we download the latest Aurora zh-CN it will open with the first tab being "about:blank" instead of "about:home" or anything else set as a localized preference.
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-aurora-l10n/firefox-30.0a2.zh-CN.linux-x86_64.tar.bz2

Comment 5

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

Every locale should actually be affected. Does this only happen for the start page? Have you tried the same with another locale? I still think we have to improve our addons module code here.

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

This is the only locale it failed in our testruns, I also checked de and en-US.
I think I can make a fix in our tests to check if add-ons manager is opened in the same tab to don't wait for event!

Comment 7

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

You will have to remove the start page pref for those other locales too. Otherwise you wont see it. Does it only happen for about:blank? Or are other about: pages affected too?

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

(In reply to Henrik Skupin (:whimboo) from comment #7)
> You will have to remove the start page pref for those other locales too.
> Otherwise you won't see it. Does it only happen for about:blank? Or are other
> about: pages affected too?
I can't clear that preference as is set at build time, also this only affects zh-CN as it sets this at preference in a different place too as Francesco pointed in bug 1002442 comment 2.
Here is a patch with a workaround where I set that preference in tabs library so it won't change the behaviour of closeAllTabs method, should be removed once the bug gets fixed. I think is better to workaround this issue instead of skipping the test for all locales for just one broken. 
Firstly I tried to change the preference in tabs library but this caused other tests to fail, so I changed at opening about:home in closeAllTabs. 
http://mozmill-crowd.blargon7.com/#/remote/report/db3ef7ec039e4c6b255196b6288d3b64
http://mozmill-crowd.blargon7.com/#/remote/report/db3ef7ec039e4c6b255196b6288d3c54
http://mozmill-crowd.blargon7.com/#/remote/report/db3ef7ec039e4c6b255196b6288d48e7

Comment 9

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

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

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

No, please not such workarounds and especially not in tabs.js. We have to make addons.js failure-prove.

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

I catch the exception and I check Addons Manager, if it hasn't been opened, I forward the exception.
Also I had to edit a test to make it depend on the index of addons manager instead of a hardcoded value when checking if a new tab has been opened.

We would have caught this regression even if Addons Manager wouldn't fail, in tests:
/functional/testTabbedBrowsing/testNewWindow.js
/functional/testPreferences/testRestoreHomepageToDefault.js  

http://mozmill-crowd.blargon7.com/#/remote/report/91f18c799f60f33fc05d03749c00856b
http://mozmill-crowd.blargon7.com/#/remote/report/91f18c799f60f33fc05d03749c007d6d
http://mozmill-crowd.blargon7.com/#/remote/report/91f18c799f60f33fc05d03749c0070ea

Comment 11

[Andrei Eftimie]

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

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

Its a reasonable enhancement, but we might break the wait for category with it in certain cases. Please check those.

::: firefox/lib/addons.js
@@ +173,5 @@
>      }
> +    catch (e) {
> +      // If Addons manager is opened but there is no transitioned flag
> +      // either it was already opened or has opened in the current tab
> +      if (!this.isOpen && !self.transitioned) {

Now we'll only throw if there is no transition and if we have the Addons Managers not open.
In the case of "about:blank" + open AMO (where there is no transition): we will not throw, but the code from the try block for the category load will not run at all. Please check if that is the case indeed. Might an expect.waitFor help in that case? Or do we need to reorganize the code a bit?

::: firefox/tests/remote/testAddons/testSearchAddons.js
@@ +60,5 @@
>    // Verify that clicking the 'Show All Results' link opens a new tab on AMO
>    var allResultsLink = am.getElements({type:"allResults"})[0];
>    controller.click(allResultsLink);
>    assert.waitFor(function () {
> +    return tabBrowser.selectedIndex === amIndex + 1;

This is probably better, but it will fail if AMO is opened in the same tab... this will not happen with the current setup, but might later. I don't have any clear solution at this time so we might go ahead with it.

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.1

I added another try block for checking that amo is opened, so we won't catch the category to.
(In reply to Andrei Eftimie from comment #11)
> >    assert.waitFor(function () {
> > +    return tabBrowser.selectedIndex === amIndex + 1;
> 
> This is probably better, but it will fail if amo is opened in the same
> tab... this will not happen with the current setup, but might later. I don't
> have any clear solution at this time so we might go ahead with it.
Here the amo is already opened, and we have to check the next opened tab, which we open from amo, it's a follow up to the current fix in library.

Comment 13

[Andrei Eftimie]

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

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

Looks good to me.

Henrik?

Comment 14

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

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

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

::: firefox/lib/addons.js
@@ +165,5 @@
> +        // If Addons manager is opened but there is no transitioned flag
> +        // either it was already opened or has opened in the current tab
> +        if (!this.isOpen && !self.transitioned) {
> +          assert.fail(e.message);
> +        }

I do not understand this logic here. Why don't you simply add a check for isOpen in the waitFor() call? That should be totally sufficient.

@@ +174,5 @@
>  
>          var categoryID = this.getCategoryId({category: this.selectedCategory});
>          var timeout = (categoryID === "discover") ? TIMEOUT_REMOTE : undefined;
>  
>          assert.waitFor(function () {

If the AOM was already open we most likely will also fail here in waiting for the page being loaded. There is no load happening.

::: firefox/tests/remote/testAddons/testSearchAddons.js
@@ +54,5 @@
>    assert.ok(resultsLength <= NUMBER_OF_MAX_RESULTS,
>              "Number of addons are less or equal than: " + NUMBER_OF_MAX_RESULTS);
>  
> +  // Addons Manager tab index
> +  var amIndex = tabBrowser.selectedIndex;

Move this down under the next comment in line 61, and call the variable aomIndex. Further I don't see a reason for this extra comment.

@@ +60,5 @@
>    // Verify that clicking the 'Show All Results' link opens a new tab on AMO
>    var allResultsLink = am.getElements({type:"allResults"})[0];
>    controller.click(allResultsLink);
>    assert.waitFor(function () {
> +    return tabBrowser.selectedIndex === amIndex + 1;

Lets make it a fat arrow one.

Comment 15

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #14)
> ::: firefox/lib/addons.js
> @@ +165,5 @@
> > +        // If Addons manager is opened but there is no transitioned flag
> > +        // either it was already opened or has opened in the current tab
> > +        if (!this.isOpen && !self.transitioned) {
> > +          assert.fail(e.message);
> > +        }
> 
> I do not understand this logic here. Why don't you simply add a check for
> isOpen in the waitFor() call? That should be totally sufficient.
This was an enhancement, the flag isOpened is set early and we have to wait for tab to animate before we take action on it. Bug 942711

> @@ +174,5 @@
> >  
> >          var categoryID = this.getCategoryId({category: this.selectedCategory});
> >          var timeout = (categoryID === "discover") ? TIMEOUT_REMOTE : undefined;
> >  
> >          assert.waitFor(function () {
> 
> If the AOM was already open we most likely will also fail here in waiting
> for the page being loaded. There is no load happening.
True, I checked that, I would add a flag if the catch block and skip this waiting if the amo was already opened, what do you say?

Comment 16

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

(In reply to Cosmin Malutan from comment #15)
> > I do not understand this logic here. Why don't you simply add a check for
> > isOpen in the waitFor() call? That should be totally sufficient.
>
> This was an enhancement, the flag isOpened is set early and we have to wait
> for tab to animate before we take action on it. Bug 942711

Sure, what speaks against "return self.transitioned && this.isOpen"?

> > >          var categoryID = this.getCategoryId({category: this.selectedCategory});
> > >          var timeout = (categoryID === "discover") ? TIMEOUT_REMOTE : undefined;
> > >  
> > >          assert.waitFor(function () {
> > 
> > If the AOM was already open we most likely will also fail here in waiting
> > for the page being loaded. There is no load happening.
> True, I checked that, I would add a flag if the catch block and skip this
> waiting if the amo was already opened, what do you say?

I would abort early. I don't see a reason why to make the conditions more complicated. Please add such a test to the lib test too.

Comment 17

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.0

(In reply to Henrik Skupin (:whimboo) from comment #16)
> > for tab to animate before we take action on it. Bug 942711
> 
> Sure, what speaks against "return self.transitioned && this.isOpen"?

> I would abort early. I don't see a reason why to make the conditions more
> complicated. Please add such a test to the lib test too.

I did that, but then it will still fail if there is no transition end, so i still have to catch that and return early.

Thanks for review Henrik, I'm passing this to Andreea.

Comment 18

[Andreea Matei [:AndreeaMatei]]

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

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

LGTM, passing to Henrik.

Comment 19

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

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

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

r=me with the two mentioned issues fixed.

::: firefox/lib/addons.js
@@ +166,5 @@
> +        if (this.isOpen && !self.transitioned) {
> +          return;
> +        }
> +
> +        assert.fail(e.message);

You do not want to throw a new exception. Re-throw the one caught here. This will ensure that the lines of code are not altered.

::: firefox/lib/tests/testAddonsManager.js
@@ +33,5 @@
> +  expect.ok(am.isOpen, "Add-ons Manager is opened.");
> +
> +  am.close();
> +  expect.ok(!am.isOpen, "Add-ons Manager is closed.");
> +}

You might need a teardownTest method to clean-up a possible left-over AOM.

Comment 20

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.1

I fixed the nits, thanks for review.

Comment 21

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

Comment on attachment 8422368 [details] [diff] [review]
patch_v3.1

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

Why a review again? Please see my last comments. Beside that the commit message doesn't list the right people who did reviews on that patch.

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.2

I didn't know what r=me meant. I updated the commit message too. I'm passing this to Andrei and Andreea.

Comment 23

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8422408 [details] [diff] [review]
patch_v3.2

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

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

We want backports for this.

Comment 24

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

(In reply to Cosmin Malutan from comment #22)
> I didn't know what r=me meant. I updated the commit message too. I'm passing
> this to Andrei and Andreea.

Then you should really check the review documentation on MDN:
https://developer.mozilla.org/en-US/docs/Code_Review_FAQ

I would propose that for all of you. So here I already gave my review, and you should carried it over.

Comment 25

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.2[esr24]

Andreea previous patch applies cleanly on all branches except for esr, for which I provide the following patch.

Comment 26

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8422437 [details] [diff] [review]
patch_v3.2[esr24]

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

http://hg.mozilla.org/qa/mozmill-tests/rev/8929c4f34e9b (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/59bedb4c3533 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/654c7a144577 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/87e231ea22cf (esr24)