931715 - Test failure "The add-on has been correctly installed - got 'false'" in /restartTests/testInstallAddonWithoutEULA/test1.js

Status: RESOLVED FIXED

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

Description

[mario garbi]

This started failing over the weekend on Windows NT 6.1.7601 and Windows NT 5.1.2600:

http://mozmill-daily.blargon7.com/#/remote/report/95c9c51e8c2d9f14917fc749dddc5d18
http://mozmill-daily.blargon7.com/#/remote/report/8a42e4b8f277f9c0268b061eb8013bde

Comment 1

[mario garbi]

I can reproduce it on the failing machine, will try to reproduce locally.

Comment 2

[mario garbi]

To bring an update here. I cannot reproduce this locally and on the failing machine the failures are intermittent. Might be that we are too fast and the Extensions tab isn't getting populated soon enough.

Comment 3

[mario garbi]

Attachment: waitForAMO.patch

The recent failures were due to the fact that we were not waiting for AMO to populate the Extensions tab. Tested this on the failing machine and it works fine now.

Comment 4

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

Sorry, but I totally don't understand that. Why do we have to wait for AMO here?

Comment 5

[Andrei Eftimie]

Comment on attachment 823375 [details] [diff] [review]
waitForAMO.patch

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

Good catch Mario.
I'm wondering if this should go in addons.js

Please fix the mentioned nits.
I'll land this after the merge on both default and mozilla-aurora

::: tests/remote/restartTests/testInstallAddonWithoutEULA/test1.js
@@ +59,5 @@
>    });
>  
>    // Verify the add-on is installed
>    var addon = addonsManager.getAddons({attribute: "name", value: ADDON.name})[0];
> +  assert.waitFor( function () {

nit: extra space

@@ +60,5 @@
>  
>    // Verify the add-on is installed
>    var addon = addonsManager.getAddons({attribute: "name", value: ADDON.name})[0];
> +  assert.waitFor( function () {
> +    return addonsManager.isAddonInstalled({addon: addon})

nit: missing ;

Comment 6

[mario garbi]

Attachment: waitForAMO_v1.patch

Fixed a couple of nits. Sorry about those.

Comment 7

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

Comment on attachment 823384 [details] [diff] [review]
waitForAMO_v1.patch

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

If you haven't seen my comment, I don't understand why we need this.

Comment 8

[mario garbi]

Sorry Henrik, I missed your comment there. Basically after we install the addon we open the AMO Extensions tab and check that the addon is present there. This was handled with assert.ok without waiting for any element and worked fine for all machines. 
For some reasons the new win-8-32-4 and win-8-64-4 machines performed the install much faster than the rest and checked the Extensions tab without waiting to be populated. By using waitFor we can actually see a few "false" values and then a "true" one which indicates a delay there.
I checked if this fixes it on the failing win-8-64-4 machine and we stopped reproducing the issue.

Comment 9

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

(In reply to mario garbi from comment #8)
> Sorry Henrik, I missed your comment there. Basically after we install the
> addon we open the AMO Extensions tab and check that the addon is present
> there. This was handled with assert.ok without waiting for any element and
> worked fine for all machines.

Which AMO extensions tab? This test doesn't touch addons.mozilla.org at all. I think you mean AOM instead. Please be careful with acronyms and use the right ones if you want to stop confusion.

> For some reasons the new win-8-32-4 and win-8-64-4 machines performed the
> install much faster than the rest and checked the Extensions tab without
> waiting to be populated. By using waitFor we can actually see a few "false"
> values and then a "true" one which indicates a delay there.
> I checked if this fixes it on the failing win-8-64-4 machine and we stopped
> reproducing the issue.

I don't see why this should need an additional waitFor call here. The category change should be done once all extensions and their data have been added tot he view. But not earlier. If that is the case here I assume something else is broken here. So no, I'm against this workaround which is like an additional sleep call.

Comment 10

[Andrei Eftimie]

Hmm, lets backtrack a bit.

From what I saw, the code in question:
> addonsManager.isAddonInstalled({addon: addon}); 
Initially returns false

That method checks the following DOM attributes:
> return addon.getNode().getAttribute("remote") == "false" &&
>        addon.getNode().getAttribute("status") == "installed";

That's why I liked the simple proposal to exchange the ok() check with a waitFor() check.
Maybe we should wait in addons.js when we fetch an element (addon) untill it is fully loaded / DOM fully initialised.

****
As a sidenote:
With the move on multiple sides to make Firefox asynchronous I am expecting to see more waitFor() calls needed in the future. Our current architecture is singleThreaded / blocking and we have to workaround async calls in the newer API's

Comment 11

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

What type of add-on is it? Given that the test is under restartTests I feel its not a restartless one, right? So after a restart the add-on should always be installed. 'remote'==true would mean it's not locally available.

Comment 12

[mario garbi]

The test is indeed under restartTests but we only have a single test file as there was no need to actually restart the browser (the installed addon shows in addon manager). We use indeed a remote addon (Nightly Tools) as that was part of the test requirements.
I will continue to looked over what happens here but as far as I can tell visually on the new machines we either get the extensions populated slower or we move faster (the test itself is moving very fast over that part).

Comment 13

[mario garbi]

Attachment: skip.patch

Adding a skip patch until we come with an acceptable fix patch due to the large number of failures.

Comment 14

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 823961 [details] [diff] [review]
skip.patch

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

I miss a proper commit message here, please add it.
For the "skip-if" in the manifest, I think it's "win" for windows. Please check against Mozmill 2.0.

Comment 15

[mario garbi]

Attachment: skip_v1.patch

Added a commit message and fixed the manifest so we correctly skip it if the OS is windows.

Comment 16

[mario garbi]

I have set dumps to better show what happens inside the failing method and we can see that while on Linux we get right values right away on mm-win-7-64-4 machine (where it fails) we need a few moments to install the addon as it's status is showing "installing". This will return a false value for isAddonInstalled() if we simply call it without waiting for the addon to finish installing.

Linux dumps:
TEST-START | /home/mariogarbi/repo/mine/skip/tests/remote/restartTests/testInstallAddonWithoutEULA/test1.js | testInstallAddonWithEULA

 Remote: false

 Status: installed

TEST-PASS | /home/mariogarbi/repo/mine/skip/tests/remote/restartTests/testInstallAddonWithoutEULA/test1.js | test1.js::testInstallAddonWithEULA


mm-win-7-64-4 dumps:
TEST-START | c:\jenkins\workspace\mozilla-central_remote\mozmill-tests\tests\remote\restartTests\testInstallAddonWithoutEULA\test1.js | testInstallAddonWithEULA

 ---- WaitFor iteration ---

 Remote:false

 Status:installing

 ---- WaitFor iteration ---

 Remote:false

 Status:installing

 ---- WaitFor iteration ---

 Remote:false

 Status:installed

TEST-PASS | c:\jenkins\workspace\mozilla-central_remote\mozmill-tests\tests\remote\restartTests\testInstallAddonWithoutEULA\test1.js | test1.js::testInstallAddonWithEULA

Comment 17

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

Comment on attachment 823979 [details] [diff] [review]
skip_v1.patch

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

So some things here:

* Please rename the test so it really follows our naming conventions! It needs a testAddons_ prefix

* Why aren't we restarting the browser for testing that the add-on has been installed? That's the reason why your test is failing. You can't fully test this in a single test given that NTT is not a restartless add-on!

Comment 18

[mario garbi]

Attachment: addonWithoutEulaFix.patch

Thank you Henrik for catching that, I should have created two tests in the first place. 

With this patch I've split the test into two tests and changed the name to match the naming convention. As far as I could tell it also fixes the failures as by the time we restart the browser the addon is present in addons manager.

http://mozmill-crowd.blargon7.com/#/remote/report/8a42e4b8f277f9c0268b061eb8bcd84a

Comment 19

[Andrei Eftimie]

Comment on attachment 824636 [details] [diff] [review]
addonWithoutEulaFix.patch

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

Looks good, only 1 small change needed.

::: tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test1.js
@@ +22,5 @@
> +  aModule.addonsManager = new addons.AddonsManager(aModule.controller);
> +
> +  tabs.closeAllTabs(aModule.controller);
> +
> +  persisted.addonName = ADDON.name;

Please save the whole ADDON object, not just the name.

Comment 20

[mario garbi]

Attachment: addonWithoutEulaFix_v1.patch

Thanks Andrei for catching that, I changed it in this patch as you requested.

Comment 21

[Andrei Eftimie]

Comment on attachment 825781 [details] [diff] [review]
addonWithoutEulaFix_v1.patch

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

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

Comment 22

[Andrei Eftimie]

I think we need to get this in mozilla-aurora as well

Comment 23

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

For consistency also regarding the test name we should probably get this backported to all the branches it exists on.

Comment 24

[Andrei Eftimie]

> For consistency also regarding the test name we should probably get this backported to all
> the branches it exists on.
The test is new, it exists only on Aurora and Nightly

No reason to wait here. All tests are passing on Aurora as well.

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/fcfcedd44496 (mozilla-aurora)