672480 - Mozmill test for changing a theme

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

Tracking of Mozmill test creation for changing a theme. 
Litmus: https://litmus.mozilla.org/show_test.cgi?id=15633

Comment 1

[u415141]

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

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

As the litmus test requires, https://litmus.mozilla.org/show_test.cgi?id=15633
this test needs to deal with two themes. 

Tried a patch involving plain.jar - the testing theme we have under /data folder and 
the firefox default theme - which is somehow erased from the Themes list (Appearance category) in the Mozmill Firefox profile. 

Whether we get the default theme back on for the mozmill firefox profile, or we need to have another testing theme besides Plain Theme to get this test completed. 

Which is the best approach?

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

Well I've moved forward and asked Alex for another theme under mozmill-test/data folder 

Logged bug 691307

Comment 4

[u279076]

Sorry I missed this last week. I don't see why this needs 2 themes. The Litmus test only requires that you install a theme and switch to it. Please explain before doing any work on bug 691307.

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

Attachment: wip patch 1.0

Well the reason I filed the dependency bug is that I cannot access the default theme in the test - it is not present in the Appearance Category from AOM 

I've explained above then received no answer so I've falsely taken the liberty of trying to deblock this test somehow. 

I've tested locally under Ubuntu 11.04 with all branches and get the same result. 

I've put a controller.sleep(4000); in the test to see clearly that the default theme is not present. 

Did I do something or do we have a problem with this caused by something else?
Please help. Thanks

Comment 6

[u279076]

Can you do some more debugging around the issue locally? Is the default theme missing only under Ubuntu? Is it missing under any other tests? Is it missing when we just run mozmill -b <build>?

Sorry, I didn't have time to test your WIP patch yet.

Comment 7

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

Comment on attachment 564479 [details] [diff] [review]
wip patch 1.0

>+function testInstallTheme() {
[..]
>+  md.start(addons.handleInstallAddonDialog);
>+  controller.click(installLink);
>+  md.waitForDialog(TIMEOUT_DOWNLOAD); 
>+}

You know that Firefox gets restarted right after the Install button has been clicked? Not sure at this point, what happens in such a case. Could be a race condition. You have to wait for the final step in installing the theme. For now simply add a sleep for testing purposes and check if that helps.

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> Comment on attachment 564479 [details] [diff] [review] [diff] [details] [review]
> wip patch 1.0
> 
> >+function testInstallTheme() {
> [..]
> >+  md.start(addons.handleInstallAddonDialog);
> >+  controller.click(installLink);
> >+  md.waitForDialog(TIMEOUT_DOWNLOAD); 
> >+}
> 
> You know that Firefox gets restarted right after the Install button has been
> clicked? Not sure at this point, what happens in such a case. Could be a
> race condition. You have to wait for the final step in installing the theme.
> For now simply add a sleep for testing purposes and check if that helps.

Well the line 

>+  md.waitForDialog(TIMEOUT_DOWNLOAD);

waits for the modal dialog to install the theme and having the TIMEOUT_DOWNLOAD set manually takes care of possible timeout failures. It did not fail so far on any tests. i don't really get it why this is a problem. 
the problem is in test2.js where the default theme is not seen 

nevertheless, if you run the mozmill profile ('mozmill' from command line) the default theme is not there in the list

Comment 9

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

Ah, I see. The problem here is that we disabled the installation of add-ons shipped with the application. We got massive problems with TestPilot during update tests. As it looks like we do not differentiate between add-on distributions and app wide default add-ons. Can you please get in contact with Dave and ask him, if there is a way to allow the installation separately for both types? If yes, we have to patch Mozrunner.

Comment 10

[u279076]

CCing Dave on this bug. Dave, can you comment in regards to comment 9?

Comment 11

[Dave Townsend [:mossop]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Ah, I see. The problem here is that we disabled the installation of add-ons
> shipped with the application. We got massive problems with TestPilot during
> update tests. As it looks like we do not differentiate between add-on
> distributions and app wide default add-ons. Can you please get in contact
> with Dave and ask him, if there is a way to allow the installation
> separately for both types? If yes, we have to patch Mozrunner.

The pref "extensions.installDistroAddons" should not affect whether the default theme is correctly installed but would stop TestPilot from getting installed. Changing "extensions.enabledScopes" could break the default theme, but would not affect TestPilot. Which pref did you change?

Comment 12

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

In Mozmill we are currently using extensions.enabledScopes to only install extensions from the profile:

https://github.com/mozautomation/mozmill/blob/hotfix-1.5/mozrunner/mozrunner/__init__.py#L319

Seems like that breaks us here. Dave, which preferences we would have to set to achieve the following:

* Block installation of global add-ons and distribution add-ons
* Allow installation of add-ons in the profile and the application folder

Once I have that information I will file a new bug for mozrunner. Thanks.

Comment 13

[Dave Townsend [:mossop]]

extensions.installDistroAddons=false will disable installing distribution add-ons
extensions.enabledScopes=5 will disable all extension locations except for the profile and the application (where the default theme is).

Some of the partner repacks may still be using the application location for their bundled add-ons. In that case there is currently no way to not install those add-ons but keep the default theme.

Comment 14

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

Thanks Dave. I have filed bug 692862 to get it fixed in Mozmill.

For the time being would the following workaround solve it? We could re-set those prefs right before the first restart. Do we check those other locations again each time Firefox starts? In this case the Default theme should be present for the check in test2.js.

Comment 15

[Dave Townsend [:mossop]]

(In reply to Henrik Skupin (:whimboo) from comment #14)
> Thanks Dave. I have filed bug 692862 to get it fixed in Mozmill.
> 
> For the time being would the following workaround solve it? We could re-set
> those prefs right before the first restart. Do we check those other
> locations again each time Firefox starts? In this case the Default theme
> should be present for the check in test2.js.

We check for distribution add-ons only when Firefox has been upgraded. We rescan all enabled locations on every startup, so yes I think what you're suggesting should work

Comment 16

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Townsend (:Mossop) from comment #13)
> extensions.installDistroAddons=false will disable installing distribution
> add-ons
> extensions.enabledScopes=5 will disable all extension locations except for
> the profile and the application (where the default theme is).
> 
> Some of the partner repacks may still be using the application location for
> their bundled add-ons. In that case there is currently no way to not install
> those add-ons but keep the default theme.

I've tried to change those prefs in the test but had no luck retrieving the default theme in test2.js 

  // Set pref for add-ons
  prefs.preferences.setPref("extensions.installDistroAddons", 
                            'false');

  prefs.preferences.setPref("extensions.enabledScopes", 5);

This is the sample code i used. If anything else is needed for clarifications, i will asap provide.

Comment 17

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

That's suspicious. I don't have time for deeper investigation. So we probably have to wait with this test until the underlying Mozmill issue has been fixed and a new release pushed to pypi.

Comment 18

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> That's suspicious. I don't have time for deeper investigation. So we
> probably have to wait with this test until the underlying Mozmill issue has
> been fixed and a new release pushed to pypi.

Thanks Henrik for a fast dependency fix. Still, the mozmill version with the fix needs to land 
At the moment, the patch can be tested using a dev version of mozmill, meaning 2.0rc1, which can be build locally following the instructions from heree https://github.com/mozautomation/mozmill 

When the mozmill version containing the fix will be landed, a fast patch will follow up and we can close this bug also

Comment 19

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

We do not support Mozmill 2.0 at this time. So you need a dev version of the hotfix-1.5 branch instead.

Comment 20

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1 all branches

The dependency is fixed so here is the first patch 

Details: 

test1.js - installs the plain-theme from the local folder using the install script 
test2.js - verifies that plain-theme is installed and enables the default theme (now we have two themes so we can test changing) 
test3.js - verifies default theme is active and it is the first one on the list (as the litmus test requires)

Comment 21

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 571068 [details] [diff] [review]
patch v1 all branches

I'm changing this one to Henrik for review. I know he's asked you to split, but once you've started the review process with someone it's best to stick with them as it's their concerns you need to satisfy.

I will look at this sometime today and give any feedback I can. Approval should come from Henrik though.

Re: the split, I'll clarify in email but go ahead and favor me for code where Henrik has not already started review. Favor Henrik to continue any reviews he's already begun.

Comment 22

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 571068 [details] [diff] [review]
patch v1 all branches

This looks OK to me. I'm unsure what our rules are on local data, so still would like Henrik's confirmation.

Comment 23

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Geo Mealer [:geo] from comment #22)
> Comment on attachment 571068 [details] [diff] [review] [diff] [details] [review]
> patch v1 all branches
> 
> This looks OK to me. I'm unsure what our rules are on local data, so still
> would like Henrik's confirmation.

Makes sense

Comment 24

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

Comment on attachment 571068 [details] [diff] [review]
patch v1 all branches

>+var prefs = require("../../../../lib/prefs");



>+const LOCAL_INSTALL_SCRIPT = "addons/install.html?addon=";
>+const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../../data/");

As mentioned by Anthony on another bug we don't want to have a separate const for the install page. Please make it identical to all the other tests with local test pages.

>+const THEME = [
>+  {name: "Theme (Plain)",
>+   id: "[e-mail]",
>+   url: LOCAL_TEST_FOLDER + LOCAL_INSTALL_SCRIPT + "themes/plain.jar"},
>+  {name: "Default",
>+   id: "{972ce4c6-7e08-4474-a285-3208198ce6fd}"}
>+];


>+function setupModule() {
>+  controller = mozmill.getBrowserController();
>+  addonsManager = new addons.AddonsManager(controller);

As mentioned on another bug we have to start making use of aModule as parameter for setupModule and teardownModule. Currently we are creating global variables which is not ideal. Please add the parameter and use it for variables, e.g. aModule.controller. 

>+  // Whitelist add the local test folder
>+  addons.addToWhiteList(LOCAL_TEST_FOLDER);

We only want to whitelist the domain and not the whole URL.

>+function testInstallTheme() {
[..]
>+  md.start(addons.handleInstallAddonDialog);
>+  controller.click(installLink);
>+  md.waitForDialog(TIMEOUT_DOWNLOAD); 

Whenever it is possible we should trigger an user-initiated restart.

>+var addons = require("../../../../lib/addons");
>+var {assert} = require("../../../../lib/assertions");
>+var tabs = require("../../../../lib/tabs");


>+  assert.ok(addonsManager.isAddonInstalled({addon: plainTheme}), 
>+            "The theme " + persisted.theme[0].id + " is installed");

Please use single quotes around the theme id.

>+   // Restart the browser using restart prompt
>+  var restartLink = addonsManager.getElement({type: "listView_restartLink", 
>+                                              parent: defaultTheme});
>+
>+  controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true);
>+
>+  controller.click(restartLink); 

nit: You can remove those blank lines. There is no need to separate this code.

>+var tabs = require("../../../../lib/tabs");

No need to include tabs in test3.

>+  // Verify the default theme is active
>+  var defaultTheme = addonsManager.getAddons({attribute: "value", 
>+                                              value: persisted.theme[1].id})[0];
>+
>+  assert.ok(defaultTheme.getNode().getAttribute("active"), "true");

Why an assert? There is no action involved so it can be an expect call.

>+  // Verify the default theme is the first one of the list
>+  var themeList = addonsManager.getAddons({attribute: "type", value: "theme"});
>+  var firstThemeId = themeList[0].getNode().getAttribute("value");
>+
>+  expect.ok(firstThemeId, persisted.theme[1].id);

Well, we shouldn't do such a check. Instead we have to check in test2 and test3 if the theme is active and not only installed.

Comment 25

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2 all branches

All changes addressed 

One observation: Did not created aModule parameter for teardownModule because I do not create any global variables here in this test. Only applies for setupModule here. Will do it for sure in further situations if it's necessary.

Comment 26

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

Comment on attachment 571347 [details] [diff] [review]
patch v2 all branches

>+const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../../data/");
[..]
>+  // Whitelist add the local test folder
>+  addons.addToWhiteList("http://localhost:43336");

We don't want to have hard-coded hostnames in the test. Instead use the information from LOCAL_TEST_FOLDER.

>+/**
>+ * Verifies the theme is installed
>+ */
>+function testThemeIsInstalled() {

That should be "installed and enabled"

>+  // Verify the plain-theme is active
>+  expect.equal(plainTheme.getNode().getAttribute("active"), "true");

Also please use 'enabled' here. It's more obvious when comparing the UI as back-end attributes.

>+  addonsManager.enableAddon({addon: defaultTheme});
>+
>+   // Restart the browser using restart prompt
>+  var restartLink = addonsManager.getElement({type: "listView_restartLink", 
>+                                              parent: defaultTheme});

Can we have an extra check as in test1.js to ensure that the default theme has been marked to be enabled?

>+function teardownModule() {  
>+  delete persisted.theme;  
>+
>+  addonsManager.close();

This has to be aModule.addonsManager

>+/*
>+ * Verify we changed to the default theme
>+ */
>+function testChangedTheme() {

The function name should be updated to be more specific of which theme we are referring to here.

Comment 27

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3 all branches

Addressed all revision requests except:

We do not check for default theme to be pending for enable in test1.js

Comment 28

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #27)
> Created attachment 573510 [details] [diff] [review] [diff] [details] [review]
> patch v3 all branches
> 
> Addressed all revision requests except:
> 
> We do not check for default theme to be pending for enable in test1.js

Ups - "as in test1.js" so I will correct with a follow up patch asap. sorry

Comment 29

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.1 all branches

done

Comment 30

[Maniac Vlad Florin (:vladmaniac)]

The review will be a - because Anthony does not agree with using 'aModule' 

Will create another patch.

Comment 31

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.2 all branches

Not using parameter 'aModule' anymore, as requested in all other patches

Comment 32

[u279076]

Comment on attachment 576459 [details] [diff] [review]
patch v3.2 all branches

>+  // Verify the default theme is active
>+  var defaultTheme = addonsManager.getAddons({attribute: "value", 
>+                                              value: persisted.theme[1].id})[0];
>+
>+  expect.equal(defaultTheme.getNode().getAttribute("active"), "true");
>+}

The final assertion of a test should always be an assert.

Comment 33

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.3 all branches

Modified to use 'assert'

Comment 34

[u279076]

Comment on attachment 576709 [details] [diff] [review]
patch v3.3 all branches

>+  // Verify that plain-theme is marked to be enabled
>+  expect.equal(plainTheme.getNode().getAttribute("pending"), "enable");

>+  // Verify the plain-theme is enabled
>+  expect.ok(addonsManager.isAddonEnabled({addon: plainTheme}), 
>+            "The theme '" + persisted.theme[0].id + "' is enabled");

>+  // Verify that default theme is marked to be enabled
>+  expect.equal(defaultTheme.getNode().getAttribute("pending"), "enable");

I think these will need to be an assert since the rest of the test assumes this is true.

Sorry for missing this last time around.

Comment 35

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.4 all branches

No problem - agree with you 

Fixed.

Comment 36

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.4 (all) [checked-in]

re-tested that patch and somehow does not apply cleanly. 

new one uploaded here

Comment 37

[u279076]

Comment on attachment 577592 [details] [diff] [review]
patch v3.4 (all) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/11fde9f412d2 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/8da0705e1377 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/5c455a9ecd5c (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/0ff23a50f881 (mozilla-release)

Comment 38

[u279076]

Please verify with tomorrow's testrun.

Comment 39

[Remus Pop (:RemusPop)]

No fails for this one.