1088561 - Add automated test for plugins in different states

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

https://moztrap.mozilla.org/manage/case/11506/
In order to have full coverage for the moztrap TC above we have to enhance the testFlashCookie.js to:
1 disable the plugin
2 set a new cookie value
  -> cookie value remains unchanged

http://hg.mozilla.org/qa/mozmill-tests/file/36d8bcd21096/firefox/tests/remote/testPrivateBrowsing/testFlashCookie.js

Comment 1

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

Those additional steps have nothing to do with the Flash Cookie test. It should be added to a general plugin related test, which we most likely have under restartTests.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

Then in /restartTests/testAddons_pluginDisabledAfterRestart we can enforce Flash if it's enabled and use http://mozqa.com/data/firefox/plugins/flash/cookies/flash_cookie.htm to check if plugin it works or it doesn't.

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

In tests testAddons_pluginDisabledAfterRestart:
1 I check if we have flash installed, if we do I chose it as the tested plugin.
2 I use flash_cookie page to set a coockie(proves flash is enabled and works)
3 I disable flash
4 I try to set a cookie again and I expect to fail

For testing this we need to access remote flash object, so the test has renamed to remote. 
http://mozmill-crowd.blargon7.com/#/remote/report/db1e72a89500c1dbca9e703f712e5bba

This is an tests enhancement and it doesn't touch the libraries.

Comment 4

[Andrei Eftimie]

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

Review of attachment 8511940 [details] [diff] [review]:
-----------------------------------------------------------------

Something is not quite right here.
Even if I have no flash on the system, this test is all green. 

I don't think this needs to be combined with this particular test. We're diluting this tests value.
The only other tests that require Flash are endurance tests.

With this I'd say to have a new test for this.

It can be a functional test, I don't see anything blocking us to not use remote resources.
I see flash_cookie.html is only available in testcase data.
Is there a reason we can't use this locally?

If not we should find another way of testing the plugin that can be used locally

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Andrei Eftimie from comment #4)
> I don't think this needs to be combined with this particular test. We're
> diluting this tests value.
> The only other tests that require Flash are endurance tests.
> 
> With this I'd say to have a new test for this.
Okey, so after a deeper thought I think there should be two different test function to check that Flash works and that it doesn't while disabled, and to skip those test functions if we don't have the Flash:
http://mozmill-crowd.blargon7.com/#/remote/report/db1e72a89500c1dbca9e703f713b848b
http://mozmill-crowd.blargon7.com/#/remote/report/db1e72a89500c1dbca9e703f713e4d0d
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
passed	/restartTests/testAddons_pluginDisabledAfterRestart/test1.js	testFlashPluginWorks	
passed	/restartTests/testAddons_pluginDisabledAfterRestart/test1.js	testDisablePlugin	
passed	/restartTests/testAddons_pluginDisabledAfterRestart/test2.js	testFlashPluginDoesNotWork	
passed	/restartTests/testAddons_pluginDisabledAfterRestart/test2.js	testPluginDisabled
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
skipped	/restartTests/testAddons_pluginDisabledAfterRestart/test1.js	testFlashPluginWorks	
No enabled Flash plugin detected
passed	/restartTests/testAddons_pluginDisabledAfterRestart/test1.js	testDisablePlugin	
skipped	/restartTests/testAddons_pluginDisabledAfterRestart/test2.js	testFlashPluginDoesNotWork	
No enabled Flash plugin detected
passed	/restartTests/testAddons_pluginDisabledAfterRestart/test2.js	testPluginDisabled

In this way we keep the same style we have for other flash-skipped tests. If we would make a new test we would duplicate exactly the steps we do in this tests and those are exactly the one required in moztrap from comment 0. By modifying this tests we would add 1-2 seconds, if we will make a new test we would add 12-13 seconds to a regular ran. 


> It can be a functional test, I don't see anything blocking us to not use
> remote resources.
> I see flash_cookie.html is only available in testcase data.
> Is there a reason we can't use this locally?
If we want to make this functional we would need to add the http://mozqa.com/data/firefox/plugins/flash/cookies/ to http://hg.mozilla.org/qa/mozmill-tests/file/default/data/cookies and the we could also make the remote/testPrivateBrowsing/testFlashCookie a functional test. I would rather do that in follow-up bug than in this one.

Henrik, what do you think we should make this a new test or we can enhance this one? Can we make it remote or should we make it functional by including the flash_cookie in mozmill-test/data?

Comment 6

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

I'm always for having tests in functional than remote! So lets do it. If we have already a test why not adding that to it? Btw, don't we already have the test for disabling a plugin? I highly think so. Means it would even be a way lesser addition of code.

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.0

Per Henriks input in comment 6:
I added the resources in data so the test would be functional.
This is exactly the test that does that, so enhancing this will require the minimum addition of code for adding full coverage for the moztrap testcase.


With Flash
http://mozmill-crowd.blargon7.com/#/functional/report/3f82829489bbb6f26149da7c58173209
Without Flash
http://mozmill-crowd.blargon7.com/#/functional/report/3f82829489bbb6f26149da7c58190a03

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 1088561.patch

I updated the test so we won't regress with bug	1086507.

Comment 9

[Andrei Eftimie]

Comment on attachment 8513401 [details] [diff] [review]
1088561.patch

Review of attachment 8513401 [details] [diff] [review]:
-----------------------------------------------------------------

::: data/plugins/flash/cookies/flash_cookie.html
@@ +5,5 @@
> +    <meta charset="utf-8">
> +    <script>
> +      function init() {
> +        getCookie();
> +      }

We should wait for bug 1086507 to land (or at least get the changes as they are included atm).

::: firefox/tests/functional/restartTests/testAddons_pluginDisabledAfterRestart/test1.js
@@ +25,5 @@
>        }
>    });
>  
> +  // If we have Flash installed, choose flash and also test functionality
> +  var shockwaveFlash = addons.getInstalledAddons(function (aAddon) {

nit: you could use a fat arrow function here

@@ +28,5 @@
> +  // If we have Flash installed, choose flash and also test functionality
> +  var shockwaveFlash = addons.getInstalledAddons(function (aAddon) {
> +    if (aAddon.isActive && aAddon.type === "plugin" && aAddon.name === "Shockwave Flash")
> +      return aAddon;
> +  })[0];

First try using flash, if we use this plugin, there's no need to also check for other plugins (activePlugins check)

@@ +68,5 @@
> +  var resultField = new elementslib.ID(controller.tabs.activeTab, "result_get");
> +  assert.waitFor(() => resultField.getNode().value === "undefined",
> +                 "Cookie value is undefined.");
> +
> +  var cookieField = new elementslib.ID(controller.tabs.activeTab, "cookieValue");

findElement

@@ +69,5 @@
> +  assert.waitFor(() => resultField.getNode().value === "undefined",
> +                 "Cookie value is undefined.");
> +
> +  var cookieField = new elementslib.ID(controller.tabs.activeTab, "cookieValue");
> +  controller.type(cookieField, COOKIE_VALUE);

cookieFied.keypress()

@@ +72,5 @@
> +  var cookieField = new elementslib.ID(controller.tabs.activeTab, "cookieValue");
> +  controller.type(cookieField, COOKIE_VALUE);
> +
> +  // Set the cookie value
> +  var setCookie = new elementslib.ID(controller.tabs.activeTab, "setCookie");

findElement

same changes to test2 please

@@ +105,5 @@
>    // Check that the plugin is disabled
>    assert.equal(aPlugin.getNode().getAttribute("active"), "false",
>                 persisted.plugin.name + " is disabled");
>  }
> +

Remove this extra newline :)

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 1088561.patch

I added the changes from bug 1086507. 

(In reply to Andrei Eftimie from comment #9)
> > +
> > +  var cookieField = new elementslib.ID(controller.tabs.activeTab, "cookieValue");
> > +  controller.type(cookieField, COOKIE_VALUE);
> 
> cookieFied.keypress()
It won't work, keypress needs to be called for each latter:
https://github.com/mozilla/mozmill/blob/8cad8438b6f6237f449add334f97d731448baff7/mozmill/mozmill/extension/resource/driver/controller.js#L1052

Comment 11

[Andrei Eftimie]

(In reply to Cosmin Malutan from comment #10)
> It won't work, keypress needs to be called for each latter:
> https://github.com/mozilla/mozmill/blob/
> 8cad8438b6f6237f449add334f97d731448baff7/mozmill/mozmill/extension/resource/
> driver/controller.js#L1052

Ah sorry, it's called `sendKeys`

Comment 12

[Andrei Eftimie]

Comment on attachment 8514216 [details] [diff] [review]
1088561.patch

Review of attachment 8514216 [details] [diff] [review]:
-----------------------------------------------------------------

The whole addon query & filter block is really hard to follow.

It would be great to also merge these 2 tests into 1, but that's probably out of scope of this issue.
This would help disable all relevant tests directly (in case of no-flash, or no active addon at all), and reduce setup/teardown cruft quite a lot.

::: firefox/tests/functional/restartTests/testAddons_pluginDisabledAfterRestart/test1.js
@@ +40,5 @@
> +    }
> +    else {
> +      testDisablePlugin.__force_skip__= "No enabled plugins detected"
> +    }
> +    testFlashPluginWorks.__force_skip__= "No enabled Flash plugin detected";

This whole block is still really hard to read.

I wonder if we should directly save the retrieved addon in `persisted.addon` and use that in the setupModule checks, in test1 and in test2 so we eliminate the need of all other persisted object.

I've though a lot on how to improve this, may I offer a solution?
>  // Try using Flash if available
>  aModule.plugin = addons.getInstalledAddons(filterFlash)[0] ||
>                   addons.getInstalledAddons(filterActiveAddon)[0];
>
>  if (!aModule.plugin) {
>    testDisablePlugin.__force_skip__= "No enabled plugins detected";
>    testFlashPluginWorks.__force_skip__= "No enabled plugins detected";
>  }
>  else if (aModule.plugin.name !== "Shockwave Flash") {
>    testFlashPluginWorks.__force_skip__= "No enabled Flash plugin detected";
>  }

And make helper methods from the filter functions (add a jsdoc comment please)
> function filterFlash(aAddon) {
>   if (aAddon.isActive && aAddon.type === "plugin" && aAddon.name === "Shockwave Flash") {
>     return aAddon;
>   }
> }
> 
> function filterActive(aAddon) {
>   if (aAddon.isActive && aAddon.type === "plugin" && !aAddon.blocklistState) {
>     return aAddon;
>   }
> }

::: firefox/tests/functional/restartTests/testAddons_pluginDisabledAfterRestart/test2.js
@@ +20,5 @@
>  function setupModule(aModule) {
>    aModule.controller = mozmill.getBrowserController();
>  
>    // Skip test if we have no plugins
>    if (persisted.enabledPlugins < 1) {

We would check `persisted.addon`

@@ +24,5 @@
>    if (persisted.enabledPlugins < 1) {
>      testPluginDisabled.__force_skip__ = "No enabled plugins detected";
>      teardownModule.__force_skip__ = "No enabled plugins detected";
>    }
> +  if (!persisted.isFlash) {

We would check `persisted.addon.name === "Shockwave Flash"`

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1

Tu summarize the changes:
* I added the flash_cookie locally so the test remains functional
* I made the restart-test as a single-file
* I enhanced the test so it checks for flash, and if it founds it it tests it's functionality otherwise it will go with any other active plugin.

The reports are with both flash installed and uninstalled 
http://mozmill-crowd.blargon7.com/#/functional/report/24f9dc1b051c99c8247bed4e78d58691
http://mozmill-crowd.blargon7.com/#/functional/report/24f9dc1b051c99c8247bed4e78d72035

Comment 14

[Andrei Eftimie]

Comment on attachment 8514888 [details] [diff] [review]
patch v2.1

Review of attachment 8514888 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Good we can have this test as a functional one.
Please also file a bug to get the the remote/testPrivateBrowsing/testFlashCookie.js test into a functional one (if it isn't filed already).

Adding a review flag for Henrik, mostly for the addition of the swf files into mozmill-tests.
They take up 88kb, which I find reasonable for having a couple remote tests made into functional ones.

Comment 15

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

Comment on attachment 8514888 [details] [diff] [review]
patch v2.1

Review of attachment 8514888 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/tests/functional/testAddons/manifest.ini
@@ +1,5 @@
>  [parent:../manifest.ini]
>  
>  [testManagerKeyboardShortcut.js]
>  [testPluginDisableAffectsAboutPlugins.js]
> +[testPluginInDiffrentStates.js]

Spelling error. This is 'different'

::: firefox/tests/functional/testAddons/testPluginInDiffrentStates.js
@@ +6,5 @@
>  
>  // Include required modules
> +var addons = require("../../../../lib/addons");
> +var tabs = require("../../../lib/tabs");
> +var prefs = require("../../../lib/prefs");

Please keep alphabetical order. I think that this should be clear meanwhile.

@@ +8,5 @@
> +var addons = require("../../../../lib/addons");
> +var tabs = require("../../../lib/tabs");
> +var prefs = require("../../../lib/prefs");
> +
> +const PREF_LAST_CATEGORY = "extensions.ui.lastCategory";

nit: Try to keep the BASE_URL block as first constant definition.

@@ +52,5 @@
>    tabs.closeAllTabs(aModule.controller);
>  }
>  
> +function teardownTest(aModule) {
> +  aModule.controller.restartApplication(persisted.tests.shift());

I would propose that we keep the same restart logic as we have in other tests. Removing entries is not something which will work with state machines. So please check the other single-file restart tests, and do it similarly.

Also what about when the last test was run? You do a restart here but not a stopApplication as included in teardownModule.

@@ +69,5 @@
> +  aModule.controller.stopApplication(true);
> +}
> +
> +/**
> + * If the operated plugin is flash, check that it works when enabled

nit: 'If the chosen plugin is Flash'

@@ +88,5 @@
> +  var setCookie = findElement.ID(controller.tabs.activeTab, "setCookie");
> +  setCookie.click();
> +
> +  assert.equal(resultField.getNode().value, TEST_DATA.cookieValue,
> +               "Cookie value is displayed.");

We really do not need a waitFor here?

@@ +103,5 @@
>      category: addonsManager.getCategoryById({id: "plugin"})
>    });
>  
> +  var plugin = addonsManager.getAddons({attribute: "value",
> +                                         value: persisted.plugin.id})[0];

nit: Indentation is off.

@@ +110,3 @@
>  
>    // Check that the plugin is disabled
> +  assert.ok(!addonsManager.isAddonEnabled({addon: plugin}),

getAttribute() returns a string and not a boolean.

@@ +114,3 @@
>  }
> +
> +// If the operated plugin is flash, check that it doesn't work when disabled

nit: 'chosen'

@@ +114,4 @@
>  }
> +
> +// If the operated plugin is flash, check that it doesn't work when disabled
> +function testFlashPluginDoesNotWork() {

I would propose a better name like 'testFlashPluginInactive'.

@@ +126,5 @@
> +  setCookie.click();
> +
> +  var resultField = findElement.ID(controller.tabs.activeTab, "result_get");
> +  assert.notEqual(resultField.getNode().value, TEST_DATA.cookieValue,
> +                  "Cookie value is not displayed.");

Wouldn't this better need a waitFor?

@@ +156,5 @@
> + * Function for filtering flash
> + */
> +function filterFlash(aAddon) {
> +  if (aAddon.isActive &&
> +      aAddon.type === "plugin" &&

What about blocklistState here?

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

(In reply to Henrik Skupin (:whimboo) from comment #15)
> >  
> > +function teardownTest(aModule) {
> > +  aModule.controller.restartApplication(persisted.tests.shift());
> 
> I would propose that we keep the same restart logic as we have in other
> tests. Removing entries is not something which will work with state
> machines. So please check the other single-file restart tests, and do it
> similarly.
> 
> Also what about when the last test was run? You do a restart here but not a
> stopApplication as included in teardownModule.
We can't have the same restart logic as in a tests that is completely different. In this test if we don't have flash we wan't to skip test functions 1 and 3. If in function 1 we call restartApplication with tests 2, it will mess-up when tests 1 is skipped, because it won't know which testfunction to run next. The solution here is to call restartApplication in teardownTest and keep the state of the remaining tests in the persisted object. I've done something similar in a chrome test:
https://github.com/mozilla/gecko-dev/blob/master/browser/extensions/pdfjs/test/browser_pdfjs_navigation.js#L147

> >    // Check that the plugin is disabled
> > +  assert.ok(!addonsManager.isAddonEnabled({addon: plugin}),
> 
> getAttribute() returns a string and not a boolean.
Method isAddonEnabled returns a boolean, for sure.

http://mozmill-crowd.blargon7.com/#/functional/report/1e5e44bea8f3e17708194a929a38759d

Comment 17

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

(In reply to Cosmin Malutan from comment #16)
> We can't have the same restart logic as in a tests that is completely
> different. In this test if we don't have flash we wan't to skip test
> functions 1 and 3. If in function 1 we call restartApplication with tests 2,
> it will mess-up when tests 1 is skipped, because it won't know which
> testfunction to run next. The solution here is to call restartApplication in
> teardownTest and keep the state of the remaining tests in the persisted

Well, I don't see why it should not be possible. You already have an if condition in setupTest, which checks if the test needs to be disabled. If that is the case you could perfectly set the next test to run to 'testPluginDisabled', which will then be picked up by teardownTest.

> object. I've done something similar in a chrome test:
> https://github.com/mozilla/gecko-dev/blob/master/browser/extensions/pdfjs/
> test/browser_pdfjs_navigation.js#L147

We are not browser-chrome here, and we should try to stay with what we currently have. If it doesn't fit due to some reason, lets discuss a better strategy. But I do not like the stripping of elements from a test array. Keeping an index might be a solution, and should also work for the software update tests.

> > >    // Check that the plugin is disabled
> > > +  assert.ok(!addonsManager.isAddonEnabled({addon: plugin}),
> > 
> > getAttribute() returns a string and not a boolean.
> Method isAddonEnabled returns a boolean, for sure.

My fault. I didn't notice that you moved away from getAttribute in that patch.

Comment 18

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

Comment on attachment 8516513 [details] [diff] [review]
patch v3.0

Review of attachment 8516513 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/tests/functional/testAddons/testPluginInDifferentStates.js
@@ +53,5 @@
>    tabs.closeAllTabs(aModule.controller);
>  }
>  
> +function teardownTest(aModule) {
> +  var nextTest = persisted.tests.shift();

As said in my previous comment, it's not something I like also regarding consistency. Lets get this sorted out before shooting up more and more review requests. This only delays the landing of the patch. An in-time discussion is always more fruitful.

@@ +92,5 @@
> +  var setCookie = findElement.ID(controller.tabs.activeTab, "setCookie");
> +  setCookie.click();
> +
> +  assert.waitFor(() => resultField.getNode().value === TEST_DATA.cookieValue,
> +               "Cookie value is displayed.");

nit: indentation is off.

Comment 19

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> Well, I don't see why it should not be possible. You already have an if
> condition in setupTest, which checks if the test needs to be disabled. If
> that is the case you could perfectly set the next test to run to
> 'testPluginDisabled', which will then be picked up by teardownTest.
But we still want to see that the test was skipped if there was no flash, we don't want to just by pass the tests if we don't have the flash installed, right? 

> currently have. If it doesn't fit due to some reason, lets discuss a better
> strategy. But I do not like the stripping of elements from a test array.
> Keeping an index might be a solution, and should also work for the software
> update tests.
Keeping an index sounds reasonable to me.

(In reply to Henrik Skupin (:whimboo) from comment #18)
> As said in my previous comment, it's not something I like also regarding
> consistency. Lets get this sorted out before shooting up more and more
> review requests. This only delays the landing of the patch. An in-time
> discussion is always more fruitful.
As I mentioned above, in order to settle for an implementation we have to decide if:
1 In case we don't have flash installed we wan't to skip the test function so they appear in report as skipped, or we should ignore them.
2 We wan't to keep an test index so we can set the next test to ran in set-up?

Needs to be mentioned, when we hit a skipped test function it won't ran anything so it's critical to set the text function either in setupTest or in teardownTest.

Comment 20

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

(In reply to Cosmin Malutan from comment #19)
> (In reply to Henrik Skupin (:whimboo) from comment #17)
> > Well, I don't see why it should not be possible. You already have an if
> > condition in setupTest, which checks if the test needs to be disabled. If
> > that is the case you could perfectly set the next test to run to
> > 'testPluginDisabled', which will then be picked up by teardownTest.
> But we still want to see that the test was skipped if there was no flash, we
> don't want to just by pass the tests if we don't have the flash installed,
> right?

Are we not able to mark the test as skipped in its setupTest module? That should be doable.

Comment 21

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> > But we still want to see that the test was skipped if there was no flash, we
> > don't want to just by pass the tests if we don't have the flash installed,
> > right?
> 
> Are we not able to mark the test as skipped in its setupTest module? That
> should be doable.
This is exactly what the patch does at the moment, what I try to state here is the reason I need to set the next test function in setupTest or teardownTest modules.
So what is left to do here is to set the test index on persisted object instead of removing entries?

Comment 22

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

Comment on attachment 8516513 [details] [diff] [review]
patch v3.0

Review of attachment 8516513 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/tests/functional/testAddons/testPluginInDifferentStates.js
@@ +41,5 @@
> +}
> +
> +function setupTest(aModule) {
> +  // Skip tests that require flash if no installation found
> +  if (persisted.plugin.name !== "Shockwave Flash") {

So you also want to run this code only after the restart when testFlashPluginInactive() should be executed, and not each time as right now.

So if you would specify this function name as next test before the restart, you can do an if check here and skip the test if necessary. Inside the if condition you can also set the next test to be executed. So we have an extra restart in between but that shouldn't hurt us.

Comment 23

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.1

So I removed the tests array and add the restartApplication in tests functions, when the functions are skipped the setup will be ran only twice. This looks much more clear than before and we saved two restarts. 
The only thing is that we have to skip the flash tests in setupTest module so it will know which tests are skipped after each restart.

Thanks Andrei and Henrik ;)

Comment 24

[Andrei Eftimie]

Comment on attachment 8518910 [details] [diff] [review]
patch v3.1

Review of attachment 8518910 [details] [diff] [review]:
-----------------------------------------------------------------

Works great. r+ from me with the 2 nits fixed

Ask a final review from Henrik please.

::: firefox/tests/functional/testAddons/testPluginInDifferentStates.js
@@ +93,5 @@
>      category: addonsManager.getCategoryById({id: "plugin"})
>    });
>  
> +  var plugin = addonsManager.getAddons({attribute: "value",
> +                                         value: persisted.plugin.id})[0];

nit: there's an extra space

@@ +131,5 @@
> +  // Open the Add-ons Manager
> +  addonsManager.open();
> +
> +  var plugin = addonsManager.getAddons({attribute: "value",
> +                                       value: persisted.plugin.id})[0];

nit: missing whitespace

Comment 25

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.2

Fixed those nits.

Comment 26

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

Cosmin, can you please mark attachments with declined reviews as obsolete in the future? The list of patches here is getting confusing. Thanks.

Comment 27

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

Ups, sorry. Please disregard my last comment.

Comment 28

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

Comment on attachment 8520639 [details] [diff] [review]
patch v3.2

Review of attachment 8520639 [details] [diff] [review]:
-----------------------------------------------------------------

I do not understand why the restart logic has been moved from the teardownTest function into the test itself. This will not work at all. It has to stay in that teardown method, otherwise the restart will not be executed if something in the test fails.

Comment 29

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.3

(In reply to Henrik Skupin (:whimboo) from comment #28)
> I do not understand why the restart logic has been moved from the
> teardownTest function into the test itself. This will not work at all. It
> has to stay in that teardown method, otherwise the restart will not be
> executed if something in the test fails.
I'm not sure I got this right on the first time, but now I do the restart in the teardownTest and I set the next test in test-itself, it works both when skipped or enabled.
The restart logic is exactly the same as in update tests, it's just skips the flash tests in setupTest.

http://mozmill-crowd.blargon7.com/#/functional/report/b67428b6e502c230ff85b6a4c421594f
http://mozmill-crowd.blargon7.com/#/functional/report/b67428b6e502c230ff85b6a4c4231a20

Comment 30

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

Comment on attachment 8521374 [details] [diff] [review]
patch v3.3

Review of attachment 8521374 [details] [diff] [review]:
-----------------------------------------------------------------

Looks way better but please check my inline comments. There is one which prevents me to give a r+.

::: firefox/tests/functional/testAddons/testPluginInDifferentStates.js
@@ +56,5 @@
>  function teardownModule(aModule) {
> +  prefs.preferences.clearUserPref(PREF_LAST_CATEGORY);
> +
> +  // Enable the plugin that was disabled
> +  addons.enableAddon(persisted.plugin.id);

This may not be safe enough. We will have to do this via an API call. Also enableAddon expects a dict and not an id.

@@ +76,5 @@
> +  controller.waitForPageLoad();
> +
> +  // Wait for the getCookie() function to type undefined in the #result_get field
> +  var resultField = findElement.ID(controller.tabs.activeTab, "result_get");
> +  assert.waitFor(() => resultField.getNode().value !== "",

nit: missing brackets around the comparison.

@@ +86,5 @@
> +  // Set the cookie value
> +  var setCookie = findElement.ID(controller.tabs.activeTab, "setCookie");
> +  setCookie.click();
> +
> +  assert.waitFor(() => resultField.getNode().value === TEST_DATA.cookieValue,

nit: missing brackets around the comparison.

@@ +109,1 @@
>    // Disable the plugin

nit: I would exchange those two lines. A comment should always be on top but not in the middle.

@@ +128,5 @@
> +  var setCookie = findElement.ID(controller.tabs.activeTab, "setCookie");
> +  setCookie.click();
> +
> +  var resultField = findElement.ID(controller.tabs.activeTab, "result_get");
> +  assert.waitFor(() => resultField.getNode().value !== TEST_DATA.cookieValue,

nit: missing brackets around the comparison.

@@ +140,5 @@
> +  // Open the Add-ons Manager
> +  addonsManager.open();
> +
> +  var plugin = addonsManager.getAddons({attribute: "value",
> +                                        value: persisted.plugin.id})[0];

nit: please keep the right indentation for those blocks.

Comment 31

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #30)
> @@ +56,5 @@
> >  function teardownModule(aModule) {
> > +  prefs.preferences.clearUserPref(PREF_LAST_CATEGORY);
> > +
> > +  // Enable the plugin that was disabled
> > +  addons.enableAddon(persisted.plugin.id);
> 
> This may not be safe enough. We will have to do this via an API call. Also
> enableAddon expects a dict and not an id.
Why is not safe? what API? It expects a string, otherwise the test would fail.
http://hg.mozilla.org/qa/mozmill-tests/file/b8934759b1f6/lib/addons.js#l1626

Comment 32

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

Oh sorry. Due to the lack of ui and backend separation in this module I was looking at the first method with this name, which is part of the AddonManager class. So this is fine then.

Comment 33

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.4

I fixed the nits.

Thanks


http://mozmill-crowd.blargon7.com/#/functional/report/a6906174787fc0d722108aa1faa2d4cc

Comment 34

[Andrei Eftimie]

Comment on attachment 8523695 [details] [diff] [review]
patch v3.4

Review of attachment 8523695 [details] [diff] [review]:
-----------------------------------------------------------------

Please fix the manifest file.

::: firefox/tests/functional/testAddons/manifest.ini
@@ -1,5 @@
>  [parent:../manifest.ini]
>  
>  [testEnableDisableUndoUninstall.js]
> -[testInstallUninstallHardBlocklistedExtension.js]
> -[testInstallUninstallSoftBlocklistedExtension.js]

Why have you removed these tests? They are needed.

Comment 35

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.0

Sorry for that, I guess I removed them by mistake while fixing a conflict.

Comment 36

[Andrei Eftimie]

Comment on attachment 8529049 [details] [diff] [review]
patch v4.0

Review of attachment 8529049 [details] [diff] [review]:
-----------------------------------------------------------------

Landed: 
https://hg.mozilla.org/qa/mozmill-tests/rev/8aa90ed61ff4 (default)

Comment 37

[Andrei Eftimie]

We'll want this on Aurora (to get into beta with the upcoming merge)

Comment 38

[Andrei Eftimie]

Transplanted to Aurora:
https://hg.mozilla.org/qa/mozmill-tests/rev/82425e3f2675 (mozilla-aurora)

(minor conflict in the manifest file)

Comment 39

[Andreea Matei [:AndreeaMatei]]

The manifest from restartTests still has the test, Daniela will add a follow up patch.

Comment 40

[Daniela Domnici]

Attachment: removeFromManifest

testAddons_pluginDisabledAfterRestart removed from restartTests/manifest

Comment 41

[Daniela Domnici]

Attachment: Aurora_removeFromManifest

testAddons_pluginDisabledAfterRestart remove from manifest

Comment 42

[Daniela Domnici]

Attachment: beta_removeFromManifest

Removed testAddons_pluginDisabledAfterRestart from manifest

Comment 43

[Daniela Domnici]

Attachment: nightly_updatedPatch

updated patch for nightly branch

Comment 44

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8535648 [details] [diff] [review]
nightly_updatedPatch

Review of attachment 8535648 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/16b50427082b (default)

Comment 45

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8535642 [details] [diff] [review]
Aurora_removeFromManifest

Review of attachment 8535642 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/158c52f9dd33 (aurora)

Comment 46

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8535646 [details] [diff] [review]
beta_removeFromManifest

Review of attachment 8535646 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/d7c28636e9dc (beta)
Thanks Daniela.