Mozmill test for changing a theme

VERIFIED FIXED

Status

Mozilla QA
Mozmill Tests
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: vladmaniac, Assigned: vladmaniac)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-restart][mozmill-aom])

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

7 years ago
Tracking of Mozmill test creation for changing a theme. 
Litmus: https://litmus.mozilla.org/show_test.cgi?id=15633
(Assignee)

Updated

7 years ago
Assignee: nobody → vlad.maniac
Status: NEW → ASSIGNED

Comment 1

7 years ago
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/15959583
(Assignee)

Updated

7 years ago
Depends on: 672763
Whiteboard: [mozmill-aom]
Depends on: 685515
(Assignee)

Comment 2

7 years ago
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?
(Assignee)

Updated

7 years ago
Depends on: 691307
(Assignee)

Comment 3

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

Logged bug 691307
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.
(Assignee)

Comment 5

7 years ago
Created attachment 564479 [details] [diff] [review]
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
Attachment #564479 - Flags: feedback?(hskupin)
(Assignee)

Updated

7 years ago
Attachment #564479 - Flags: feedback?(anthony.s.hughes)
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 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.
Attachment #564479 - Flags: feedback?(hskupin)
Attachment #564479 - Flags: feedback?(anthony.s.hughes)
Attachment #564479 - Flags: feedback-
(Assignee)

Comment 8

7 years ago
(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
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.
CCing Dave on this bug. Dave, can you comment in regards to comment 9?
(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?
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.
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.
Depends on: 692862
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.
(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
(Assignee)

Comment 16

7 years ago
(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.
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.
(Assignee)

Comment 18

7 years ago
(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
We do not support Mozmill 2.0 at this time. So you need a dev version of the hotfix-1.5 branch instead.
(Assignee)

Comment 20

7 years ago
Created attachment 571068 [details] [diff] [review]
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)
Attachment #564479 - Attachment is obsolete: true
Attachment #571068 - Flags: review?(gmealer)
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.
Attachment #571068 - Flags: review?(hskupin)
Attachment #571068 - Flags: review?(gmealer)
Attachment #571068 - Flags: feedback?(gmealer)
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.
Attachment #571068 - Flags: feedback?(gmealer) → feedback+
(Assignee)

Comment 23

7 years ago
(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 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: "plain.theme@quality.mozilla.org",
>+   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.
Attachment #571068 - Flags: review?(hskupin) → review-
(Assignee)

Comment 25

7 years ago
Created attachment 571347 [details] [diff] [review]
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.
Attachment #571068 - Attachment is obsolete: true
Attachment #571347 - Flags: review?(hskupin)
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.
Attachment #571347 - Flags: review?(hskupin) → review-
(Assignee)

Comment 27

7 years ago
Created attachment 573510 [details] [diff] [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
Attachment #571347 - Attachment is obsolete: true
Attachment #573510 - Flags: review?(hskupin)
(Assignee)

Comment 28

7 years ago
(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
(Assignee)

Comment 29

7 years ago
Created attachment 573513 [details] [diff] [review]
patch v3.1 all branches

done
Attachment #573510 - Attachment is obsolete: true
Attachment #573513 - Flags: review?(hskupin)
Attachment #573510 - Flags: review?(hskupin)
Attachment #573513 - Flags: review?(hskupin) → review?(anthony.s.hughes)
(Assignee)

Comment 30

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

Will create another patch.
Attachment #573513 - Flags: review?(anthony.s.hughes)
(Assignee)

Comment 31

7 years ago
Created attachment 576459 [details] [diff] [review]
patch v3.2 all branches

Not using parameter 'aModule' anymore, as requested in all other patches
Attachment #573513 - Attachment is obsolete: true
Attachment #576459 - Flags: review?(anthony.s.hughes)
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.
Attachment #576459 - Flags: review?(anthony.s.hughes) → review-
(Assignee)

Comment 33

7 years ago
Created attachment 576709 [details] [diff] [review]
patch v3.3 all branches

Modified to use 'assert'
Attachment #576459 - Attachment is obsolete: true
Attachment #576709 - Flags: review?(anthony.s.hughes)
Whiteboard: [mozmill-aom] → [mozmill-functional][mozmill-aom]
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.
Attachment #576709 - Flags: review?(anthony.s.hughes) → review-
(Assignee)

Comment 35

7 years ago
Created attachment 577591 [details] [diff] [review]
patch v3.4 all branches

No problem - agree with you 

Fixed.
Attachment #576709 - Attachment is obsolete: true
Attachment #577591 - Flags: review?(anthony.s.hughes)
(Assignee)

Comment 36

7 years ago
Created attachment 577592 [details] [diff] [review]
patch v3.4 (all) [checked-in]

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

new one uploaded here
Attachment #577591 - Attachment is obsolete: true
Attachment #577592 - Flags: review?(anthony.s.hughes)
Attachment #577591 - Flags: review?(anthony.s.hughes)
Whiteboard: [mozmill-functional][mozmill-aom] → [mozmill-restart][mozmill-aom]
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)
Attachment #577592 - Attachment description: patch v3.4 all branches → patch v3.4 (all) [checked-in]
Attachment #577592 - Flags: review?(anthony.s.hughes) → review+
Please verify with tomorrow's testrun.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
No fails for this one.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.