1088614 - Add test to undo the add-on removal.

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

https://moztrap.mozilla.org/manage/case/11501/
In order to have full coverage for moztrap case above, we have to add step 13, Undo the addon removal in
tests 3 and 4 and to remove them back before the restart.
http://hg.mozilla.org/qa/mozmill-tests/file/36d8bcd21096/firefox/tests/functional/restartTests/testAddons_enableDisableExtension/manifest.ini

Comment 2

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v1

I created a new test which installs, uninstalls and then "undoes" the uninstall and checks that the addon remains installed.

Comment 3

[Andrei Eftimie]

Comment on attachment 8511986 [details] [diff] [review]
v1

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

Comment 1 says to enhance an existing test, and gives `testAddons_enableDisableExtension` as the candidate to use.
Is there a specific reason we need a new test altogether?

Looking at the above mentioned test, it looks like the new test also covers (almost?) everything the initial test is?
If this was the intention, please also remove the initial test, and leave this the new test's name similar to the old one.

+1 for having it in 1 testfile

::: firefox/tests/functional/testAddons/testUndoAddonUninstall.js
@@ +16,5 @@
> +
> +const INSTALL_DIALOG_DELAY = 1000;
> +const TIMEOUT_DOWNLOAD = 25000;
> +
> +const ADDON = {

nit: please move this right below BASE_URL

@@ +66,5 @@
> +  addonsManager.open();
> +
> +  // Get the extensions pane
> +  addonsManager.setCategory({
> +    category: addonsManager.getCategoryById({id: "extension"})

nit: missing semicolon

@@ +79,5 @@
> +            "The addon is installed");
> +
> +  // Remove the addon
> +  addonsManager.removeAddon({addon: addon});
> +

We should add a check here that the addon has been uninstalled. If both `remove` and `undo` get broken at the same time, we won't notice otherwise.

Comment 4

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v2

Comment 5

[Andrei Eftimie]

Comment on attachment 8513512 [details] [diff] [review]
v2

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

This looks really nice!

The test currently fails for me on OSX:
> TEST-START | /Users/andrei.eftimie/work/mozilla/bugs/1088614/mozmill-tests/firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js | testUndoUninstall
> ERROR | Test Failure | {
>   "exception": {
>     "message": "The addon is disabled - got 'false'", 
>     "lineNumber": 122, 
>     "name": "AssertionError", 
>     "fileName": "file:///Users/andrei.eftimie/work/mozilla/bugs/1088614/mozmill-tests/firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js"
>   }
> }
> TEST-UNEXPECTED-FAIL | /Users/andrei.eftimie/work/mozilla/bugs/1088614/mozmill-tests/firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js | testUndoUninstall

::: firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js
@@ +118,5 @@
> +  // Remove the addon
> +  addonsManager.removeAddon({addon: addon});
> +
> +  // Check if the addon is disabled
> +  assert.ok(!addonsManager.isAddonEnabled({addon: addon}), "The addon is disabled");

This is where the test fails for me.
What does removeAddon do? Shouldn't this check be isAddonInstalled?

Comment 6

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v2.1

(In reply to Andrei Eftimie from comment #5)
> Comment on attachment 8513512 [details] [diff] [review]
> v2
> 
> Review of attachment 8513512 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks really nice!
> 
> The test currently fails for me on OSX:
> > TEST-START | /Users/andrei.eftimie/work/mozilla/bugs/1088614/mozmill-tests/firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js | testUndoUninstall
> > ERROR | Test Failure | {
> >   "exception": {
> >     "message": "The addon is disabled - got 'false'", 
> >     "lineNumber": 122, 
> >     "name": "AssertionError", 
> >     "fileName": "file:///Users/andrei.eftimie/work/mozilla/bugs/1088614/mozmill-tests/firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js"
> >   }
> > }
> > TEST-UNEXPECTED-FAIL | /Users/andrei.eftimie/work/mozilla/bugs/1088614/mozmill-tests/firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js | testUndoUninstall

You're right, now it fails for me, as well.

> ::: firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js
> @@ +118,5 @@
> > +  // Remove the addon
> > +  addonsManager.removeAddon({addon: addon});
> > +
> > +  // Check if the addon is disabled
> > +  assert.ok(!addonsManager.isAddonEnabled({addon: addon}), "The addon is disabled");
> 
> This is where the test fails for me.
> What does removeAddon do? Shouldn't this check be isAddonInstalled?

The addon is removed (uninstalled) only after restart; instead, it is marked for uninstall using a "pending" attribute. I updated the test to check for that attribute after "remove" and after "undo".

Comment 7

[Andrei Eftimie]

Comment on attachment 8514292 [details] [diff] [review]
v2.1

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

Looks really nice! Thanks Mihaela.
Please do leave the setDiscoveryPaneURL in place as mentioned inline, otherwise this looks great.

::: firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js
@@ -26,4 @@
>    aModule.controller = mozmill.getBrowserController();
>  
>    aModule.addonsManager = new addons.AddonsManager(aModule.controller);
> -  addons.setDiscoveryPaneURL("about:home");

Please leave this in. Saves at least 3s from my tests.
Leave it in setupModule

Comment 8

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v2.2

(In reply to Andrei Eftimie from comment #7)
> Comment on attachment 8514292 [details] [diff] [review]
> v2.1
> 
> Review of attachment 8514292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks really nice! Thanks Mihaela.
> Please do leave the setDiscoveryPaneURL in place as mentioned inline,
> otherwise this looks great.
> 
> ::: firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js
> @@ -26,4 @@
> >    aModule.controller = mozmill.getBrowserController();
> >  
> >    aModule.addonsManager = new addons.AddonsManager(aModule.controller);
> > -  addons.setDiscoveryPaneURL("about:home");
> 
> Please leave this in. Saves at least 3s from my tests.
> Leave it in setupModule

Done.

Carrying over the r+

Comment 9

[Andrei Eftimie]

Comment on attachment 8514966 [details] [diff] [review]
v2.2

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

https://hg.mozilla.org/qa/mozmill-tests/rev/627b45d4db58 (default)

Comment 10

[Andrei Eftimie]

Thanks Mihaela.

Comment 11

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

Comment on attachment 8514966 [details] [diff] [review]
v2.2

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

::: firefox/tests/functional/testAddons/manifest.ini
@@ +1,3 @@
>  [parent:../manifest.ini]
>  
> +[testEnableDisableUndoUninstall.js]

I'm not sure why this new enhanced checks have been added to this test and not to testAddons_uninstallExtension, where it really belongs way better to.

::: firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js
@@ +127,5 @@
> +               "Addon is no longer marked for uninstall");
> +
> +  // Check that addon is still installed
> +  assert.ok(addonsManager.isAddonInstalled({addon: addon}),
> +            "The addon is uninstalled");

The message here is off from what you are testing. You would like to get this fixed.

Comment 12

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: fix assert message

(In reply to Henrik Skupin (:whimboo) from comment #11)
> Comment on attachment 8514966 [details] [diff] [review]
> v2.2
> 
> Review of attachment 8514966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/tests/functional/testAddons/manifest.ini
> @@ +1,3 @@
> >  [parent:../manifest.ini]
> >  
> > +[testEnableDisableUndoUninstall.js]
> 
> I'm not sure why this new enhanced checks have been added to this test and
> not to testAddons_uninstallExtension, where it really belongs way better to.

That makes sense. I logged Bug 1093598 as follow up for this matter.

> ::: firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js
> @@ +127,5 @@
> > +               "Addon is no longer marked for uninstall");
> > +
> > +  // Check that addon is still installed
> > +  assert.ok(addonsManager.isAddonInstalled({addon: addon}),
> > +            "The addon is uninstalled");
> 
> The message here is off from what you are testing. You would like to get
> this fixed.

Fixed in the attached patch. Thanks for catching it.

Comment 13

[Andrei Eftimie]

Comment on attachment 8516662 [details] [diff] [review]
fix assert message

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

https://hg.mozilla.org/qa/mozmill-tests/rev/44f661d630db (default)

Comment 14

[Andreea Matei [:AndreeaMatei]]

We missed this but we want the full coverage in beta, so this needs backporting. Is already in aurora with the merge. Daniela, can you please check if the patch applies to beta and tests well, so then we can follow up in bug 1093598 down to beta too? 
Thanks!

Comment 15

[Daniela Domnici]

The v2.2 patch was not applied on beta. For beta I have applied a new patch.

Comment 16

[Daniela Domnici]

Attachment: beta_V1

Comment 17

[Daniela Domnici]

Attachment: beta_V2

Comment 18

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8532490 [details] [diff] [review]
beta_V2

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

http://hg.mozilla.org/qa/mozmill-tests/rev/1cd9a60c8851 (beta)

Thanks!

Comment 19

[Daniela Domnici]

You`re welcome :)