733367 - Mozmill remote test for verifying theme installation from addons-dev.allizom.org

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

Litmus ID 15115
Open https://addons-dev.allizom.org/en-US/firefox/addon/lavafox-v1-purple/?src=cb-dl-rating
Click on "Add to Firefox"
Click "allow"
Verify that a window opens with a button that says "Install Now"

Comment 1

[Remus Pop (:RemusPop)]

Attachment: patch v1 (all branches)

I have defined the theme to install in the THEME object so we can work easy with it. 
After that it visits the theme url and clicks the button "Add to Firefox". After installation, in test2 we verify the theme is installed.

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 603260 [details] [diff] [review]
patch v1 (all branches)


>
>diff --git a/tests/remote/restartTests/testAddons_installTheme/test1.js b/tests/remote/restartTests/testAddons_installTheme/test1.js
>new file mode 100644
>--- /dev/null
>+++ b/tests/remote/restartTests/testAddons_installTheme/test1.js

I would rename the test folder something other than testAddons_installTheme, more suggestive would be
testAmo_installTheme, because this is what we are practically doing here


>+
>+  // Whitelist add AMO
>+  addons.addToWhiteList(AMO_DOMAIN);

Do we want to whitelist AMO or specifically click on "Allow" button, because this is the exact user behavior. 
If we are targeting to test that button also, we should click on it instead of using backend API to whitelist add AMO. 

>+  
>+  // Store the theme in the persisted object
>+  persisted.theme = THEME;
>+
>+  tabs.closeAllTabs(controller);
>+}
>+
>+/**
>+ * Test installing a theme
>+ */
>+function testInstallTheme() {
>+  // Go to theme url and perform install
>+  controller.open(persisted.theme.url);
>+  controller.waitForPageLoad();

Please add a custom increased TIMEOUT for waitForPageLoad because AMO loads very slow sometimes. We are risking Timeout 
errors here. In fact, I got one while locally testing the patch. 

>+    
>+  var installLink = new elementslib.Selector(controller.tabs.activeTab, 
>+                                             ".install-button");
>+  var md = new modalDialog.modalDialog(addonsManager.controller.window);
>+  
>+  md.start(addons.handleInstallAddonDialog);
>+  controller.click(installLink);
>+  md.waitForDialog(TIMEOUT_DOWNLOAD); 
>+}

Marlena wants an extra-check to be sure that the modal dialog appears. 
Afaik, our API already does that in the modal dialog handler, but still, if we want 
a UI check for it, we should do that 

Test Requirement: Verify that a window opens with a button that says "Install Now"
This is only IMHO, Marlena please jump in. 

Otherwise the patch is good, its a good start for the remote tests. Thanks Remus!
With those changes, it's going to have my + and we can ask Anthony further to take a 
look and checkin our first AMO remote test. ^^

Comment 3

[Remus Pop (:RemusPop)]

Attachment: patch v2 (all branches)

Changed the test to click on allow button.
Other than that there is no need to recheck if the modal-dialog appears. The test fails if it doesn't. The same happens with the install button inside the modal-dialog.

Added REMOTE_TIMEOUT and set it to 30 seconds. No need to use this in waitForPageLoad because the default is 30 seconds.

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 604377 [details] [diff] [review]
patch v2 (all branches)

Given the short notice request for these tests I can allow the 
Xpath for the "Allow" button, in exchange for whitelist adding amo. 

r+ 

Over to Anthony

Comment 5

[Remus Pop (:RemusPop)]

Attachment: patch v2 (all branches)

Sorry, only intended to change to 30 seconds of timeout. Now it's fixed.

Comment 6

[Remus Pop (:RemusPop)]

Attachment: patch v2 (all branches)

Comment 7

[Remus Pop (:RemusPop)]

Again, sorry, forgot to refresh.

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 604385 [details] [diff] [review]
patch v2 (all branches)

># HG changeset patch
># User Remus Pop <[e-mail]>
># Date 1331299067 -7200
># Node ID bbda57ad57978cbde33e00154b523f8c2a5a9331
># Parent  aa5be88dc23ef9e2810b4eaa95d6c93c2fd34eae
>Bug 733367 - Mozmill remote test for verifying theme installation r=vlad.maniac, r=ashughes
>
>diff --git a/tests/remote/restartTests/testAmo_installTheme/test1.js b/tests/remote/restartTests/testAmo_installTheme/test1.js
>new file mode 100644
>--- /dev/null
>+++ b/tests/remote/restartTests/testAmo_installTheme/test1.js
>@@ -0,0 +1,86 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is MozMill Test code.
>+ *
>+ * The Initial Developer of the Original Code is the Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2012
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Remus Pop <[e-mail]> (original author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+// Include required modules
>+var addons = require("../../../../lib/addons");
>+var domUtils = require("../../../../lib/dom-utils");
>+var modalDialog = require("../../../../lib/modal-dialog");
>+var tabs = require("../../../../lib/tabs");
>+
>+const AMO_DOMAIN = "https://addons-dev.allizom.org";
>+const REMOTE_TIMEOUT = 30000;
>+
>+const THEME = {
>+  name: "LavaFox V1-Purple",
>+  id: "[e-mail]",
>+  url: AMO_DOMAIN + "/en-US/firefox/addon/lavafox-v1-purple/?src=cb-dl-rating"
>+};
>+
>+function setupModule() {
>+  controller = mozmill.getBrowserController();
>+  addonsManager = new addons.AddonsManager(controller);
>+  
>+  // Store the theme in the persisted object
>+  persisted.theme = THEME;
>+
>+  tabs.closeAllTabs(controller);
>+}
>+
>+/**
>+ * Test installing a theme
>+ */
>+function testInstallTheme() {
>+  // Go to theme url and perform install
>+  controller.open(persisted.theme.url);
>+  controller.waitForPageLoad();
>+    
>+  var installLink = new elementslib.Selector(controller.tabs.activeTab, 
>+                                             ".install-button");
>+  var md = new modalDialog.modalDialog(addonsManager.controller.window);
>+
>+  md.start(addons.handleInstallAddonDialog);
>+  controller.click(installLink);
>+
>+  allowButton = new elementslib.Lookup(controller.window.document,
>+                '/id("main-window")/id("mainPopupSet")/id("notification-popup")'+
>+                '/id("addon-install-blocked-notification")/anon([1])'+
>+                '/{"class":"popup-notification-button-container",'+
>+                  '"pack":"end","align":"center"}/anon({"anonid":"button"})'+
>+                '/anon({"anonid":"button"})');
>+
>+  controller.click(allowButton);
>+  md.waitForDialog(REMOTE_TIMEOUT); 
>+}
>diff --git a/tests/remote/restartTests/testAmo_installTheme/test2.js b/tests/remote/restartTests/testAmo_installTheme/test2.js
>new file mode 100644
>--- /dev/null
>+++ b/tests/remote/restartTests/testAmo_installTheme/test2.js
>@@ -0,0 +1,69 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is MozMill Test code.
>+ *
>+ * The Initial Developer of the Original Code is the Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2012
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Remus Pop <[e-mail]> (original author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+// Include required modules
>+var addons = require("../../../../lib/addons");
>+var {expect} = require("../../../../lib/assertions");
>+var tabs = require("../../../../lib/tabs");
>+
>+function setupModule() {
>+  controller = mozmill.getBrowserController();
>+  addonsManager = new addons.AddonsManager(controller);
>+}
>+
>+function teardownModule() {
>+  delete persisted.theme;
>+
>+  addonsManager.close();
>+}
>+
>+/**
>+ * Verifies the theme is installed
>+ */
>+function testThemeIsInstalled() {
>+  addonsManager.open();
>+  
>+  // Set category to 'Appearance'
>+  addonsManager.setCategory({
>+    category: addonsManager.getCategoryById({id: "theme"})
>+  });
>+
>+  // Verify the theme is installed
>+  var aTheme = addonsManager.getAddons({attribute: "value", value: persisted.theme.id})[0];
>+  var themeIsInstalled = addonsManager.isAddonInstalled({addon: aTheme});
>+
>+  expect.ok(themeIsInstalled, "The theme is successfully installed");
>+}

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

oh...sorry for this huge useless comment

Comment 10

[u279076]

Comment on attachment 604385 [details] [diff] [review]
patch v2 (all branches)

>+++ b/tests/remote/restartTests/testAmo_installTheme/test1.js

nit: testAMO_

>+/**
>+ * Test installing a theme
>+ */

Test installing a theme from AMO

>+function testInstallTheme() {

testInstallThemeFromAMO()

>+  // Go to theme url and perform install
>+  controller.open(persisted.theme.url);
>+  controller.waitForPageLoad();

Please comment each of the blocks of code, not just one comment to cover it all.

>+/**
>+ * Verifies the theme is installed
>+ */

nit: Test the theme has been installed

>+function testThemeIsInstalled() {

nit: testThemeInstalled()

>+  var themeIsInstalled = addonsManager.isAddonInstalled({addon: aTheme});

I don't think this is necessary and can be done within the assertion

>+  expect.ok(themeIsInstalled, "The theme is successfully installed");

Please include the theme name in the message.
Example: "'theme_name' theme has been installed."

Comment 11

[Remus Pop (:RemusPop)]

Attachment: patch v3 (all branches)

All requests addressed.

Comment 12

[Remus Pop (:RemusPop)]

Attachment: patch v3 (all branches)

Corrected a comment.

Comment 13

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

Comment on attachment 605396 [details] [diff] [review]
patch v3 (all branches)

>+const AMO_DOMAIN = "https://addons-dev.allizom.org";

Please keep it as much as possible in sync in how we declare the local URLs.

>+const REMOTE_TIMEOUT = 30000;

This is way too general. What is remote?

>+  allowButton = new elementslib.Lookup(controller.window.document,

You don't want to declare a global variable. Also I think it would be good to have a module with element identifiers because lookup strings like those will otherwise by duplicated a lot.

>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

I would suggest that we start making use of MPL 2.0 right away.

Comment 14

[u279076]

Comment on attachment 605396 [details] [diff] [review]
patch v3 (all branches)

>+/* ***** BEGIN LICENSE BLOCK *****
>+ * ***** END LICENSE BLOCK ***** */

All new tests should use MPL2 from here on out (bug 735355)

>+  allowButton = new elementslib.Lookup(controller.window.document,
>+                '/id("main-window")/id("mainPopupSet")/id("notification-popup")'+
>+                '/id("addon-install-blocked-notification")/anon([1])'+
>+                '/{"class":"popup-notification-button-container",'+
>+                  '"pack":"end","align":"center"}/anon({"anonid":"button"})'+
>+                '/anon({"anonid":"button"})');

This is a really long string. Is there any way this can be cleaned up? Maybe break it into a couple of variables/constants? Can it be moved to the addons shared module?

Comment 15

[Remus Pop (:RemusPop)]

The problem is that the theme will not be immediately be compatible for nightly, when version bumps.
I'll try to find a theme with this requirement, but feel free to leave a comment if you are aware of such.

Comment 16

[Remus Pop (:RemusPop)]

A solution to this could be adding a try-catch block for the "Install anyway button".

Comment 17

[Remus Pop (:RemusPop)]

Attachment: patch v4 (all branches)

Updated to MPL 2.0.

The allow button has been moved to toolbars.js (see dependency). The problem with the theme not being compatible has been solved by adding a helper function in addons.js (see dependency).
Also used some exported constants from addons.js.

Comment 18

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

Comment on attachment 607953 [details] [diff] [review]
patch v4 (all branches)

>+  md.start(addons.handleInstallAddonDialog);
>+  
>+  // Click the install button on the AMO page
>+  controller.click(installButton);
>+  
>+  // "Install Anyway" button appears if addon is not compatible with FF version
>+  addons.handleInstallAnywayButton(controller);

This is crazy. What are you trying to accomplish here? You can't handle both buttons at the same time.

Comment 19

[Remus Pop (:RemusPop)]

"Install anyway" appears after clicking the install button, if the theme is incompatible.

Comment 20

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

Correct - "if" the theme is incompatible, but what otherwise? You do not want to click that button.

Comment 21

[Remus Pop (:RemusPop)]

You're right, but that's what the try-catch is for. It tries to define the button, but it fails, so no error will appear and the test will run normally.

Comment 22

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

Sorry, but I don't see a try/catch in the test and even if I would it's the wrong place. Everything should be wrapped into the helper method.

Comment 23

[Remus Pop (:RemusPop)]

I solved this in a separate bug, bug 737799.

Comment 24

[u279076]

Comment on attachment 607953 [details] [diff] [review]
patch v4 (all branches)

Looks fine to me; pausing for check-in until you and Henrik resolve the current feedback given recent comments.

Also, adding feedback? for Marlena to make sure this tests everything she expects.

Comment 25

[Marlena Compton [:marlenac]]

Comment on attachment 607953 [details] [diff] [review]
patch v4 (all branches)

Can someone walk me through this on skype?

Comment 26

[u279076]

Clearing out old requests - I assume this bug is not going forward.

Comment 27

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

Depends on. If you think it is important to have I would suggest to file a new bug for Firefox UI tests. This one is already too cluttered to be useful anymore.