669286 - Mozmill test for Enable/Disable Extensions

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

Tracking mozmill test creation for enable/disable extensions. 
Litmus testcase: https://litmus.mozilla.org/show_test.cgi?id=15626"

Comment 1

[u415141]

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

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Attachment: WIP v1.0

Requesting feedback from Anthony. 

* Is this approach correct? 

* The test is passing for beta and release branches 

* The litmus test requires more to do, but not all can be covered by Mozmill 
  (for e.g we will not want to test the addon functionality when enabled, just check if it's enabled). Therefore, i tend to think this litmus test will be partially covered by Mozmill and will still require manual checks. 

What's your opinion? Thanks

Comment 3

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

You should use the ACR extension to verify the disabled/enabled state.

Comment 4

[u279076]

Comment on attachment 544808 [details] [diff] [review]
WIP v1.0

>+// Include required modules
>+var addons = require("../../../../lib/addons");
>+var modalDialog = require("../../../../lib/modal-dialog");
>+var tabs = require("../../../../lib/tabs");
>+var {assert, expect} = require("../../../../lib/assertions");

Make this alphabetical by module name.

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

I really don't like variable names like "am". Can you make this addonsManager?

>+  // User initiated restart
>+  controller.startUserShutdown(TIMEOUT_USERSHUTDOWN, true);
>+  controller.sleep(TIMEOUT_RESTART);
>+  controller.window.Application.restart(); 

I'll defer to Henrik for whether this is the desired wait we want to handle the User Intiated Restart -- historically we try to avoid usage of sleep() wherever possible. We always try to waitFor() a particular state than doing a generic sleep().

>+}
>+
>+

Remove the extra newlines.


Same comments apply to the other test files in this patch. Also, I notice you use assert instead of expect in your test. To be clear, you should only use assert when you want your test to quit on fail. In other words, if the rest of a test is dependent on a check passing you should use an assert; if the test can functionally continue even though a check has failed you should use an expect.

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

(In reply to comment #3)
> You should use the ACR extension to verify the disabled/enabled state.

This is not very clear to me right know. Can you please be more specific ? 

* Where can I find this extension? 
* I understand this will always be compatible. You meantioned on IRC something about using controller.window.ACR? can you please elaborate a bit? 

I would really appreciate, thanks! :)

Comment 6

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

(In reply to comment #5)
> * Where can I find this extension? 

It's linked in the Litmus test.

> * I understand this will always be compatible. You meantioned on IRC
> something about using controller.window.ACR? can you please elaborate a bit? 

As also mentioned use the DOM inspector to inspect the browser window after ACR is installed. Then you will see the ACR instance attached to the window.

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0

Uploaded patch with requested changes

Comment 8

[u279076]

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

>+  // Check if the addon is disabled
>+  assert.ok(!addonsManager.isAddonEnabled({addon: addon}), 
>+                                          "The addon is disabled");

Align message with !

Otherwise this looks pretty good. One note, if you only need include the assertion class which you will require to use. In other words, if you don't use "assert" in your test, don't include it; if you don't need "expect" in your test, don't include it.

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1 for release and beta branch

Uploading patch with requested changes. 

This patch is meant for release and beta branch

Comment 10

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1 for release and beta branch

Comment 11

[u279076]

Comment on attachment 545624 [details] [diff] [review]
patch v1.1 for release and beta branch

>+var tabs = require("../../../../lib/tabs");
>+
>+
>+const ADDON_ID = "[e-mail]";

Please only use one line of separation.

>+  tabs.closeAllTabs(controller);
>+}
>+
>+
>+function teardownModule() {

Same here

>+  controller.waitFor(function () {
>+    controller.window.Application.restart(); 
>+  }, "Timeout exceeded in waiting for user initiated restart");

The message should be an indication of expected state, not error. In this case, we should use something like "Application has been restarted".

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

I could be wrong, but I don't think the {} are needed if you are declaring a single variable.

>+var tabs = require("../../../../lib/tabs");
>+
>+
>+const ADDON_ID = "[e-mail]";

Single new line separation
 
>+  // User initiated restart
>+  controller.startUserShutdown(TIMEOUT_USERSHUTDOWN, true);

TIMEOUT_USER_SHUTDOWN is preferred

>+
>+  controller.waitFor(function () {
>+    controller.window.Application.restart(); 
>+  }, "Timeout exceeded in waiting for user initiated restart");

The message should be an indication of expected state, not error. In this case, we should use something like "Application has been restarted".

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

I don't think the {} are needed

>+const TIMEOUT_USERSHUTDOWN = 2000;

TIMEOUT_USER_SHUTDOWN

>+  // Check if addon instance is attached to the window
>+  assert.notEqual(controller.window.ACR, undefined, "The ACR window object exists");

This seems a bit iffy to me. I think we should have an assert.exists and assert.notExists. I'll talk to Henrik about this.

>+  controller.waitFor(function () {
>+    controller.window.Application.restart(); 
>+  }, "Timeout exceeded in waiting for user initiated restart");

The message should be an indication of expected state, not error. In this case, we should use something like "Application has been restarted".

Comment 12

[u279076]

(In reply to comment #11)
> Comment on attachment 545624 [details] [diff] [review] [review]
> patch v1.1 for release and beta branch
>
> >+var {assert} = require("../../../../lib/assertions");
> 
> I could be wrong, but I don't think the {} are needed if you are declaring a
> single variable.

I was wrong with this...the {} are needed -- ignore that comment.

Comment 13

[u279076]

(In reply to comment #12)
> (In reply to comment #11)
> > Comment on attachment 545624 [details] [diff] [review] [review] [review]
> > patch v1.1 for release and beta branch
> >+  // Check if addon instance is attached to the window
> >+  assert.notEqual(controller.window.ACR, undefined, "The ACR window object exists");
>
> This seems a bit iffy to me. I think we should have an assert.exists and assert.notExists. 
> I'll talk to Henrik about this.

Henrik has decided it would be better to have an assert.notOK() for this scenario -- see bug 671433.

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.2 for release and beta branch

Patch with requested comments and changes.
 
This is intended for beta and release branches, please keep in mind it will not pass for aurora and default. Once this has r+, I'll follow up with those as well. 

Please make note of bug 671433 in this situation. Thanks!

Comment 15

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

> > This seems a bit iffy to me. I think we should have an assert.exists and assert.notExists. 
> > I'll talk to Henrik about this.

I'd actually have been for the exist/notExist change. :)

Comment 16

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

Comment on attachment 546476 [details] [diff] [review]
patch v1.2 for release and beta branch

Code looks ok to me, but I'm running into several errors running it, all like:

ERROR | Test Failure: {"exception": {"stack": "AddonsManager_isAddonEnabled([object Proxy])@resource://mozmill/stdlib/securable-module.js -> file:///Users/geo/hg/mozmill-tests/lib/addons.js:321\n@:0\n", "message": "AddonsManager_isAddonEnabled: Add-on not specified.", "fileName": "resource://mozmill/stdlib/securable-module.js -> file:///Users/geo/hg/mozmill-tests/lib/addons.js", "name": "Error", "lineNumber": 321}}

OTOH, I'm not at all sure they're real errors. I may well be running the test incorrectly.

Henrik and I never actually talked about how to run these tests as a "full test run" against something not already checked into Mozmill, and I've never worked on a restart test so I may just be doing something stupid.

So upshot is I can't r+ you yet; you'll have to wait until I talk to Henrik tomorrow and get the process straight.

Comment 17

[Maniac Vlad Florin (:vladmaniac)]

Geo, please run mozmill-restart -t /tests/functional/restartTests/<foldername> 
-b <path to you firefox build - release branch> --show-all --addons <path to locally installed addons> 

You can find the Ids for the extensions i'm using in the test code. (declared as constants). 

This should pass the test for you! :)

Comment 18

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

(In reply to comment #15)
> > > This seems a bit iffy to me. I think we should have an assert.exists and assert.notExists. 
> > > I'll talk to Henrik about this.
> 
> I'd actually have been for the exist/notExist change. :)

For the API rewrite we will have element.exist, which a test can use together with a call to expect.ok. For the old modules please keep using controller.assertNode() and controller.assertNodeNotExist().

Comment 19

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

(In reply to comment #17)
> Geo, please run mozmill-restart -t
> /tests/functional/restartTests/<foldername> 
> -b <path to you firefox build - release branch> --show-all --addons <path to
> locally installed addons> 

No, that's not the final command you have to use here. To fully test your patch you will have to execute the testrun_funcional.py script from the mozmill-automation repository and use the --repository option which has to point to your local clone of the repository. The patch has to be committed or on top of the Mercurial queue.

Comment 20

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

Wait, what's the locally-installed addons thing? Are there other requirements than just running the automation?

Comment 21

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

(In reply to comment #20)
> Wait, what's the locally-installed addons thing? Are there other
> requirements than just running the automation?

This is not a requirement and I'm not sure why it has been used here. --addons is optionally and all tests have to work without this option. It's only available for testing bad interactions between Firefox and installed add-ons.

Comment 22

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

Comment on attachment 546476 [details] [diff] [review]
patch v1.2 for release and beta branch

Based on what Henrik said, there's a fundamental problem in that you're expecting the addon to already be installed. This results in three errors, all saying something like:

ERROR | Test Failure: {"exception": {"stack": "AddonsManager_isAddonEnabled([object Proxy])@resource://mozmill/stdlib/securable-module.js -> file:///Users/geo/hg/mozmill-tests/lib/addons.js:321\n@:0\n", "message": "AddonsManager_isAddonEnabled: Add-on not specified.", "fileName": "resource://mozmill/stdlib/securable-module.js -> file:///Users/geo/hg/mozmill-tests/lib/addons.js", "name": "Error", "lineNumber": 321}}


What I'm hearing is that tests are responsible for installing their own addons; they cannot assume any addons to already be there.

That means in your development, you should never use the --addons command line option. As Henrik says, it's there only to run tests with -additional- addons in order to validate that they aren't breaking existing flows.

The code looks fine to me, but you need the portion that installs and uninstalls the required addon in setup and teardown respectively. 

If there are not existing tests that already do this as an example, please ask Henrik for the preferred methodology here.

Comment 23

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.3

Sorry for this late patch - it's alright now since we have figured a way to install the addon and follow the best practices as well. 

Thanks for being patient

Comment 24

[u279076]

Comment on attachment 558274 [details] [diff] [review]
patch v1.3

Forgive me but I think we want to use asserts in this test instead of expects. My thinking here is what's the point in testing enabling an addon which has not been disabled?

Henrik, what do you think?

Comment 25

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

It doesn't really matter in this case because it's a restart test and we always run all the test modules. But yes, please change it to assert so that we are safe for the future.

Comment 26

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.4 all branches

Added patch with fixed nit. 

Also, fixed user initiated shutdown problems. Tested on all branches with testrun_functional.py script. I have no errors locally. 

Henrik, i would be most grateful if I can get your feedback on this one. Thanks

Comment 27

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

Comment on attachment 559098 [details] [diff] [review]
patch v1.4 all branches

>diff --git a/data/addons/icons.xpi b/data/addons/icons.xpi

With Alex's patch on bug 685515 that extension should not be part of your patch anymore.

>+++ b/tests/functional/restartTests/testAddons_enableDisableExtension/test1.js
[..]
>+const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../../data/");
>+const LOCAL_TEST_ADDON = LOCAL_TEST_FOLDER + "addons/icons.xpi";
[..]
>+++ b/tests/functional/restartTests/testAddons_enableDisableExtension/test2.js
[..]
>+const ADDON_ID = "[e-mail]";

Please assign the id and the URL to the persisted object as what we are doing in all the other tests.

Comment 28

[u279076]

Comment on attachment 559098 [details] [diff] [review]
patch v1.4 all branches

Canceling review until you can implement changes from Henrik

Comment 29

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.5 all branches (checked-in)

Fixed. Hope its fine now. 

Asking f? from whimboo first

Comment 30

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

Comment on attachment 559413 [details] [diff] [review]
patch v1.5 all branches (checked-in)

Looks fine to me. Please mark older patches as obsolete. Thanks.

Comment 31

[u279076]

Comment on attachment 559413 [details] [diff] [review]
patch v1.5 all branches (checked-in)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/c4895a875256 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/57365d41adb8 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/a6d0b0505c48 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/4a03d8e3961b (mozilla-release)

Comment 32

[u279076]

Please check tomorrow's testrun and update this bug accordingly.

Comment 33

[u279076]

Missing dependency bug 685515 has caused a failure in our testruns.

Comment 34

[u279076]

Reopening until I can fix the missing dependency on bug 685515.

Comment 35

[u279076]

Attachment: Patch v2.0 (all) [checked-in]

Patch to update the path to the test addon.

Comment 36

[u279076]

Comment on attachment 560666 [details] [diff] [review]
Patch v2.0 (all) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/558f77449596 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/3d2cec5dc144 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/c42c8ea4fe87 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/14b5bb1802ae (mozilla-release)

Comment 37

[u279076]

Reopening as this test is still failing, though intermittently:
http://mozmill-release.brasstacks.mozilla.com/#/functional/failure?branch=8.0&platform=Windows%20NT&from=2011-09-21&to=2011-09-21&test=%2FrestartTests%2FtestAddons_enableDisableExtension%2Ftest4.js&func=test4.js%3A%3AtestEnabledAddon

Comment 38

[u279076]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #37)

Vlad, please investigate. If this is due to a dependency, please make that a higher priority.

Comment 39

[u279076]

I'm calling this bug resolved FIXED because the test was already checked in and we have another bug addressing failures in this test. See bug 688375 for future reference.