Alex Lakatos[:AlexLakatos] Assignee
	Description • 13 years ago

Tracking of mozmill test creation for installing multiple extensions

Litmus test: https://litmus.mozilla.org/show_test.cgi?id=16977

	u415141
	Comment 1 • 13 years ago

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/15725657

	Alex Lakatos[:AlexLakatos] Assignee
	Comment 2 • 13 years ago

I'm getting an error:
Bug 671315 - Mozmill test for installing multiple extensions
I'm getting an error: 
ERROR | Test Failure: {"exception": {"stack": "AddonsManager_isAddonInstalled([object Proxy])@resource://mozmill/stdlib/securable-module.js -> file:///home/alexlakatos/Desktop/addonsmanager/mozmill-tests/lib/addons.js:344\n@:0\n", "message": "addon.getNode is not a function", "fileName": "resource://mozmill/stdlib/securable-module.js -> file:///home/alexlakatos/Desktop/addonsmanager/mozmill-tests/lib/addons.js", "name": "TypeError", "lineNumber": 344}}
TEST-UNEXPECTED-FAIL | /home/alexlakatos/Desktop/addonsmanager/mozmill-tests/tests/functional/restartTests/testInstallMultipleExtensions/test2.js | testCheckMultipleExtensionsAreInstalled

	u279076
	Comment 3 • 13 years ago

I get a different error:
"AddonsManager_categories: Categories could not be found.", addons.js:643

I ran your test with Firefox 5.0 on mozilla-release branch.

	Alex Lakatos[:AlexLakatos] Assignee
	Comment 4 • 13 years ago

On mozilla-release, the Extensions pane has a different ID. You will have to change 
>+  am.setCategory({category: am.getCategoryById({id: "extension"})});
to
>+  am.setCategory({category: am.getCategoryById({id: "extensions"})});

or simply run the current patch on the mozilla-beta branch with Firefox 6 Beta

	u279076
	Comment 5 • 13 years ago

Sorry, I can't help you more to debug this issue until I am back from vacation on Thursday. Henrik or Geo, can you help Alex debug this issue?

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 6 • 13 years ago

I do not see an issue here. Problem is simply that we should hide the real category id in the API to prevent situations and backport confusion like that. See bug 671514.

	Alex Lakatos[:AlexLakatos] Assignee
	Comment 7 • 13 years ago

Found and fixed the error mentioned in comment #2

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07
	Comment 8 • 13 years ago

Comment on attachment 546525 [details] [diff] [review]
patch v1.0

+    controller.assert(function () {
+      return addonIsInstalled;
+    }, "Add-on has been installed - got '" + addonIsInstalled + 
+        "', expected 'true'");
+  }

Please use new-style asserts as documented in https://developer.mozilla.org/en/Mozmill_Tests#Logging_Test_Results -- note that the got/expected stuff is handled for you, so you won't need that in the message.

There's a lot of stuff being added to persisted that I don't see deleted at the end of the string of tests. I'm unsure of the requirement here, but saw other tests doing that. 

Henrik, can you comment?

Aside from that, things look pretty good. The assert is the main blocker, plus whatever Henrik says about cleaning up persisted.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 9 • 13 years ago

Yes, the persisted object has to be cleaned-up in teardown.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 10 • 13 years ago

Comment on attachment 546525 [details] [diff] [review]
patch v1.0

>+++ b/tests/functional/restartTests/testInstallMultipleExtensions/test1.js

Similar to my other review. Please use the Litmus subgroup as prefix for the folder name.

>+  // Store the AMO preview site
>+  persisted.amoPreviewSite = addons.AMO_PREVIEW_SITE;

No need to store the constant in persisted. You always have access to it via the addons module.

>+function testInstallMultipleExtensions() {
>+  for each(addon in persisted.addons) {
>+    // Store a reference to the current add-on in 'persisted.currentAddon'
>+    persisted.currentAddon = addon;

Why? You will directly overwrite it in the next iteration. Also you should use forEach.

>+    var addToFirefox = new elementslib.Selector(controller.window.document, ".install-button a");

This is content and not chrome. So the document has to be controller.tabs.activeTab.

>+    md.start(handleInstallAddonDialog);

This is now part of the addons module. No need to declare the method in the test anymore.

>+    controller.click(addToFirefox);
>+    md.waitForDialog(TIMEOUT_DOWNLOAD);
>+    controller.keypress(null , 'VK_ESCAPE', {});

Why do you have to press escape? 

>+const TIMEOUT_DOWNLOAD = 25000;

We do not download anything in this test. So this constant is not necessary.

>+const ADDONS = [
>+  {name: "Add-on Compatibility Reporter",
>+   id: "compatibility@addons.mozilla.org",
>+   url: addons.AMO_PREVIEW_SITE + "/firefox/addon/15003/"},
>+  {name: "Mozilla QA Companion",
>+   id: "{667e9f3d-0096-4d2b-b171-9a96afbabe20}",
>+   url: addons.AMO_PREVIEW_SITE + "/firefox/addon/5428/"}
>+];

This is part of the persisted object. No need to redeclare the array in the second test.

>+  // Store the AMO preview site
>+  persisted.amoPreviewSite = addons.AMO_PREVIEW_SITE;
>+
>+  // Whitelist add the AMO preview site
>+  addons.addToWhiteList(persisted.amoPreviewSite);
>+
>+  // Store the addons object in 'persisted.addons'
>+  persisted.addons = ADDONS;

Same for all those code.

>+  for each(addon in persisted.addons) {

As mentioned above use forEach

>+    persisted.currentAddon = addon;

Not necessary. It's not used.

>+    controller.assert(function () {
>+      return addonIsInstalled;
>+    }, "Add-on has been installed - got '" + addonIsInstalled + 
>+        "', expected 'true'");

As Geo has mentioned, please use the new assertions module.

	Alex Lakatos[:AlexLakatos] Assignee
	Comment 11 • 13 years ago

Changed the assert and addressed all of Henrik's comments

> Why do you have to press escape? 
There is a doorhanger notification that appears and I'm pressing escape to dismiss it

	Geo Mealer [:geo] -- This account is inactive after 2015-07-07
	Comment 12 • 13 years ago

Comment on attachment 546779 [details] [diff] [review]
patch v2.0

Code looks great, thanks for all the changes. I guess the only outstanding question is how are we dealing with the extension/extensions thing?

As I understand it, this patch is for default, aurora, beta; and I still need a slightly modified one for release. Is that correct?

r+'ing, holding off on checkin until the proper branches to land on are verified.

	u279076
	Comment 13 • 13 years ago

Alex, can you confirm what branches this patch tests PASS? This is a new test so I'm guessing we want to to work on default, aurora, beta, and release.

	Alex Lakatos[:AlexLakatos] Assignee
	Comment 14 • 13 years ago

This patch runs fine on release and beta, but fails on default and aurora because the addons are not compatible.
Do we want to change the test to use some addons from http://mozqa.com/data/firefox/addons/extensions/ (we may have to build them) or just skip this test on aurora and default?
Or maybe search for other two compatible addons on addons.allizom.org ?

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 15 • 13 years ago

We want to use add-ons in this order:

1. From the local system (served via HTTPd)
2. From mozqa.com
3. From AMO

Whereby the add-on from option 1 should be the same as for option 2.

	u279076
	Comment 16 • 13 years ago

Please prioritize fixing the dependency. We will have to retest/review the patch on this bug once the dependency is resolved.

	Alex Lakatos[:AlexLakatos] Assignee
	Comment 17 • 13 years ago

changed the test to use the local addons

	u279076
	Comment 18 • 13 years ago

(In reply to Alex Lakatos from comment #17)
> Created attachment 563419 [details] [diff] [review] [diff] [details] [review]

Does this patch depend on further check-ins from bug 685515?

	Alex Lakatos[:AlexLakatos] Assignee
	Comment 19 • 13 years ago

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #18)
> Does this patch depend on further check-ins from bug 685515?
No, everything that was needed for this test already landed.

	u279076
	Comment 20 • 13 years ago

Comment on attachment 563419 [details] [diff] [review]
patch v3.0 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/73962be86c4f (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/11c1c1743126 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/e4bf8605a01f (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/51217e0157be (mozilla-release)

	u279076
	Comment 21 • 13 years ago

Please check tomorrow's testrun set bug to verified if passing, reopen if failing.