Closed Bug 1088614 Opened 7 years ago Closed 7 years ago

Add test to undo the add-on removal.

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(firefox35 fixed, firefox36 fixed, firefox37 fixed)

RESOLVED FIXED
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: cosmin-malutan, Assigned: mihaelav)

Details

(Whiteboard: [sprint])

Attachments

(3 files, 4 obsolete files)

No description provided.
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
OS: Linux → All
Hardware: x86_64 → All
Summary: Add test to Undo the add-on removal. → Add test to undo the add-on removal.
Assignee: nobody → mihaela.velimiroviciu
Status: NEW → ASSIGNED
Whiteboard: [sprint]
Attached patch v1 (obsolete) — Splinter Review
I created a new test which installs, uninstalls and then "undoes" the uninstall and checks that the addon remains installed.
Attachment #8511986 - Flags: review?(andrei.eftimie)
Attachment #8511986 - Flags: review?(andreea.matei)
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.
Attachment #8511986 - Flags: review?(andrei.eftimie)
Attachment #8511986 - Flags: review?(andreea.matei)
Attachment #8511986 - Flags: review-
Attached patch v2 (obsolete) — Splinter Review
Attachment #8511986 - Attachment is obsolete: true
Attachment #8513512 - Flags: review?(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?
Attachment #8513512 - Flags: review?(andrei.eftimie) → review-
Attached patch v2.1 (obsolete) — Splinter Review
(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".
Attachment #8513512 - Attachment is obsolete: true
Attachment #8514292 - Flags: review?(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
Attachment #8514292 - Flags: review?(andrei.eftimie) → review+
Attached patch v2.2Splinter Review
(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+
Attachment #8514292 - Attachment is obsolete: true
Attachment #8514966 - Flags: review+
Attachment #8514966 - Flags: checkin?(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)
Attachment #8514966 - Flags: checkin?(andrei.eftimie) → checkin+
Thanks Mihaela.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(mihaela.velimiroviciu)
(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.
Flags: needinfo?(mihaela.velimiroviciu)
Attachment #8516662 - Flags: review?(andrei.eftimie)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
Attachment #8516662 - Flags: review?(andrei.eftimie)
Attachment #8516662 - Flags: review+
Attachment #8516662 - Flags: checkin+
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The v2.2 patch was not applied on beta. For beta I have applied a new patch.
Attached patch beta_V1 (obsolete) — Splinter Review
Attachment #8532407 - Flags: review?(mihaela.velimiroviciu)
Attachment #8532407 - Flags: review?(andreea.matei)
Attached patch beta_V2Splinter Review
Attachment #8532407 - Attachment is obsolete: true
Attachment #8532407 - Flags: review?(mihaela.velimiroviciu)
Attachment #8532407 - Flags: review?(andreea.matei)
Attachment #8532490 - Flags: review?(mihaela.velimiroviciu)
Attachment #8532490 - Flags: review?(andreea.matei)
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!
Attachment #8532490 - Flags: review?(mihaela.velimiroviciu)
Attachment #8532490 - Flags: review?(andreea.matei)
Attachment #8532490 - Flags: review+
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You`re welcome :)
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.