Closed Bug 835383 Opened 11 years ago Closed 11 years ago

Add test that installs an addon without EULA directly from addons.mozilla.org

Categories

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

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect

Tracking

(firefox27 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox27 --- fixed

People

(Reporter: daniela.p98911, Assigned: mario.garbi)

References

Details

(Whiteboard: [sprint2013-36])

Attachments

(2 files, 4 obsolete files)

testAddonWithoutEULA_v4.patch
7.48 KB, patch
andrei
: review-
Details | Diff | Splinter Review
testAddonWithoutEULA_v5.patch
7.48 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review 
A test that verifies the installation of an addon without EULA directly from addons.mozilla.org needs to be added.
Priority: -- → P3
Whiteboard: [sprint2013-36]
Comment on attachment 771576 [details] [diff] [review]
patch v1.0

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

::: lib/addons.js
@@ +170,5 @@
> +  getInstallButton : function AddonsManager_getInstallButton(controller) {
> +    return new elementslib.Selector(controller.window.document,
> +                                    ".install-button");
> +  },
> +

Why should we have this separate from the getElement()? I think it will be enough to have the elements there.

Also, I saw in Daniela's patch from bug 732353 that she used this 'a.button.eula.go' for the "continue to install" part.
Attachment #771576 - Flags: review?(andreea.matei) → review-
The element we use (button) is located on addons.mozilla.org webpage not in Addons Manager therefore it might cause confusion to place it in the same function that gathers the UI elements of AddonsManager.
If this is not a problem I have no problem in placing this new element there. Daniela uses an element from Discovery Pane while I use one from the webpage as requested in the bug title.
How should I proceed here? Leave the function as it is, move it back to the test to avoid confusion or move it entirely into the getElement() function of addons.js?
I figured it will be used already in 2 tests now and may be more to come, but you have a point with the addons manager UI. Still, I think it's the same class name.

Henrik/Dave, what do you think, should we leave them in the tests or have another class/method in addons lib to gather addon related elements?
Status: NEW → ASSIGNED
I would prefer a small UI object to represent the addon page on AMO. This would limit any confusion and duplication. What do you think, Henrik?
Flags: needinfo?(hskupin)
We haven't decided yet if we want to create ui objects for content pages. So far we only do that for chrome and in-content chrome pages. You might want to put this as an roundtable item to our  meeting today.
Flags: needinfo?(hskupin)
Comment on attachment 777811 [details] [diff] [review]
testAddonWithoutEULA_1807.patch

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

::: lib/addons.js
@@ +1209,5 @@
> +/**
> + * @class Class to handle the addons.mozilla.org webpage
> + * @constructor
> + * @param {MozMillController} aBrowserController 
> + *                            Controller of the browser window

Whitespace here and the description goes under the {Type}

@@ +1254,5 @@
> +        throw new Error(arguments.callee.name + ": Unknown element type - " + spec.type);
> +    }
> +
> +    return (nodeCollector.elements.length > 0) ? nodeCollector.elements[0] : undefined;
> +  }

We should have getElements and getElement like in the other classes.

::: tests/remote/testAddons/manifest.ini
@@ +1,2 @@
>  [testSearchAddons.js]
> +[testInstallAddonWithoutEULA.js]

Please move this above.
Attachment #777811 - Flags: review?(andreea.matei) → review-
Attached patch testAddonWithoutEULA_2307.patch (obsolete) — — Splinter Review 
Made the changes requested, functionality isn't changed, just the way we handle the elements gathering.
Attachment #777811 - Attachment is obsolete: true
Attachment #780272 - Flags: review?(andreea.matei)
Comment on attachment 780272 [details] [diff] [review]
testAddonWithoutEULA_2307.patch

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

Looks good to me. Dave, mind checking this quickly before I land it? Not sure if we have other preferences for the class name, maybe AddonsWebsite?
Attachment #780272 - Flags: review?(dave.hunt)
Attachment #780272 - Flags: review?(andreea.matei)
Attachment #780272 - Flags: review+
Comment on attachment 780272 [details] [diff] [review]
testAddonWithoutEULA_2307.patch

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

::: lib/addons.js
@@ +1211,5 @@
> + * @constructor
> + * @param {MozMillController} aBrowserController
> + *        Controller of the browser window
> + */
> +function AMOWebpage(aBrowserController) {

This is more specifically an addon page from AMO. I would suggest renaming to AMOAddonPage or even RemoteAddonPage.

::: tests/remote/testAddons/testInstallAddonWithoutEULA.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

We should use strict for all new files, as per bug 822459

@@ +22,5 @@
> +  tabs.closeAllTabs(aModule.controller);
> +}
> +
> +function teardownModule(aModule) {
> +  addonsManager.close();

Shouldn't this be aModule.addonsManager?

@@ +33,5 @@
> +function testInstallAddonWithEULA() {
> +  controller.open(ADDON.url);
> +  controller.waitForPageLoad();
> +
> +  var amo = new addons.AMOWebpage(controller);

I would rename this to addonPage.
Attachment #780272 - Flags: review?(dave.hunt) → review-
Attachment #788895 - Flags: review?(andreea.matei)
Attachment #788895 - Flags: review?(andrei.eftimie)
Blocks: 835296
Comment on attachment 788895 [details] [diff] [review]
testAddonWithoutEULA_1208.patch

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

Lets get both this and bug 835296 landed, now that we have all dependencies fixed.

Looks good, seems to work fine.
(passes on mozmill 2.0 with bug 885221 applied)

We usually don't want to push new changes on Friday, but lets get this in before the merge on Monday.

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

::: lib/addons.js
@@ +1264,5 @@
> +   *                  value    - Value of the attribute to filter
> +   *                             [optional - default: ""]
> +   *                  parent   - Parent of the to find element
> +   *                             [optional - default: document]
> +   *

We should update these JSDoc comments to match how we are writing them now.
Since they are this way through the whole file lets do a followup to update all of them.

Would you mind opening a new bug for us to refactor all these comments with the new rules?
Attachment #788895 - Flags: review?(andrei.eftimie)
Attachment #788895 - Flags: review?(andreea.matei)
Attachment #788895 - Flags: review+
Attachment #788895 - Flags: checkin+
I'm assuming we won't backport this, so marking as Resolved.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-firefox26: --- → fixed
Resolution: --- → FIXED
We already have Bug 882137 for the JSDoc re-factoring across all mozmill-tests repository.
Backed out:
http://hg.mozilla.org/qa/mozmill-tests/rev/61365962694b

I took the decision to back this out.

The test works fine, but would fail in tandem with bug 835296.
The problem seems slippery enough that I would like to see them both pass correctly before landing them.

We might need to run this as a Restart test? I see Nightly Tester Tools requiring a restart.

Even better maybe we should have both these tests in the same patch (as they have some common dependency in addons.js)
Status: RESOLVED → REOPENED
status-firefox26: fixed → affected
Resolution: FIXED → ---
Yes, that has to be a restart test. We have to clean-up / reset the profile after the test.
Comment on attachment 805301 [details] [diff] [review]
testAddonWithoutEULA_v4.patch

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

Please fix the remaining issue, I think we're good after that.

::: tests/remote/restartTests/testInstallAddonWithoutEULA/test1.js
@@ +31,5 @@
> +  // Bug 867217
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  // Remove condition when transitioned to 2.0
> +  if ("restartApplication" in aModule.controller) {
> +    aModule.controller.restartApplication();

Lets reset the profile here to make sure we don't remain with the addon installed (this might break other tests down the chain)

Use stopApplication(true) to clear the profile.
Attachment #805301 - Flags: review?(andrei.eftimie)
Attachment #805301 - Flags: review?(andreea.matei)
Attachment #805301 - Flags: review-
Made the changes, thank you Andrei for pointing it out, should have used stop in the first place, lesson learned.

http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228cfad7c0
http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228cfada4e
Attachment #808547 - Flags: review?(andrei.eftimie)
Attachment #808547 - Flags: review?(andreea.matei)
I would like these 2 bugs heavily tested under 2.0 as well, as I've seen we have an issue with testNavigateFTP.js when these patches are applied. Thanks.
Comment on attachment 808547 [details] [diff] [review]
testAddonWithoutEULA_v5.patch

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

This fails for me with 2.0 in teardownModule, "controller.waitForPageLoad(): Timeout waiting for page loaded."

I assume it's related to closeAllTabs(). Please check it.
Attachment #808547 - Flags: review?(andrei.eftimie)
Attachment #808547 - Flags: review?(andreea.matei)
Attachment #808547 - Flags: review-
Comment on attachment 808547 [details] [diff] [review]
testAddonWithoutEULA_v5.patch

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

That was due to my mozmill-automation not being on hotfix-2.0. Works fine.

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/6802a7cb7a1c (default)
Attachment #808547 - Flags: review- → review+
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
status-firefox26: affected → ---
status-firefox27: --- → fixed
Resolution: --- → FIXED
I am looking over the two failures we had last night to see if they were caused by something on our side or by a network issue/ page unavailable.
I tried to reproduce it both locally and on the failing Windows machine but I wasn't able. The timeout happens when we are waiting for the remote page to load so I'm not sure that this is a test problem, most likely the page was unavailable at that time or there was a network issue:

Failure reports
http://mozmill-daily.blargon7.com/#/remote/report/6ec6776efe900da3fd2b64a750509c62
http://mozmill-daily.blargon7.com/#/remote/report/6ec6776efe900da3fd2b64a750545f90

Investigation reports:
http://mozmill-crowd.blargon7.com/#/remote/report/6ec6776efe900da3fd2b64a7505c849e
http://mozmill-crowd.blargon7.com/#/remote/report/6ec6776efe900da3fd2b64a7505c8a7a
http://mozmill-crowd.blargon7.com/#/remote/report/6ec6776efe900da3fd2b64a7505c7990
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: