1113192 - Add automated test to check that prefs for openH264 plugin are correctly generated

Status: RESOLVED WONTFIX

(Testing :: Firefox UI Tests, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

For moztrap case 14491.
https://moztrap.mozilla.org/manage/case/14491/

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Plugin openH264 needs internet connection to install itself(step 4), that makes this test a mozmill candidate.

Comment 2

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v1

Comment 3

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8555918 [details] [diff] [review]
v1

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

::: firefox/tests/remote/restartTests/testPreferences_openH264/test2.js
@@ +7,5 @@
> +// Include necessary modules
> +var addons = require("../../../../../lib/addons");
> +var prefs = require("../../../../../lib/prefs");
> +var tabs = require("../../../../lib/tabs");
> +var browser = require("../../../../lib/ui/browser");

Blank line to separate ui libs please.

@@ +19,5 @@
> +}];
> +
> +const PLUGIN_ID = "gmp-gmpopenh264";
> +const PREF_LAST_CATEGORY = "extensions.ui.lastCategory";
> +const TIMEOUT_PLUGIN_INSTALL = 90000;

This is a lot, we might want to look for some "official" intel on how long it takes usually.

@@ +52,5 @@
> +  addonsManager.open();
> +  addonsManager.setCategory({category: addonsManager.getCategoryById({id: "plugin"})});
> +
> +  var plugin = addonsManager.getAddons({attribute: "value", value: PLUGIN_ID})[0];
> +  assert.waitFor(() => plugin.getNode().getAttribute("notification")!="warning",

Spaces before and after operator != and we might want to use double ==.

@@ +61,5 @@
> +  assert.ok(preferencesGenerated(),
> +            "OpenH264 preferences are generated after plugin installation");
> +}
> +
> +function preferencesGenerated() {

jsdoc

@@ +65,5 @@
> +function preferencesGenerated() {
> +  var prefsGenerated = true;
> +
> +  OPENH264_PREFS.forEach(aPref => {
> +    let pref = prefs.getPref(aPref.prefName, aPref.defaultValue);

We use var

@@ +66,5 @@
> +  var prefsGenerated = true;
> +
> +  OPENH264_PREFS.forEach(aPref => {
> +    let pref = prefs.getPref(aPref.prefName, aPref.defaultValue);
> +    prefsGenerated = pref == aPref.defaultValue ? prefsGenerated && false : prefsGenerated && true;

We want this split in 2 lines to keep 80 chars/line.

Comment 4

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v1.1

Updated patch based on review.

Reports:
Linux: http://mozmill-crowd.blargon7.com/#/remote/report/7485d49d3e5bf7c4685a1f7886455957
Windows: http://mozmill-crowd.blargon7.com/#/remote/report/7485d49d3e5bf7c4685a1f78864b8cf9
Mac: http://mozmill-crowd.blargon7.com/#/remote/report/7485d49d3e5bf7c4685a1f78864f7739

Comment 5

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8557061 [details] [diff] [review]
v1.1

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

LGTM. I will land on Monday to be able to monitor results.

Comment 6

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

Comment on attachment 8557061 [details] [diff] [review]
v1.1

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

This test is so simple, that I wonder why it is not getting tested in mochitest? I don't see any comment on this bug yet for this investigation. It also concerns me to see this huge amount of time spent in waiting for it to be installed.

Also I haven't seen any feedback or review request for me yet which was agreed on before new tests getting landed.

::: firefox/tests/remote/restartTests/testPreferences_openH264/test1.js
@@ +6,5 @@
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  // close the browser to start with a new profile for the following test
> +  aModule.controller.stopApplication(true);

Can't this be a new style restart test in a single file?

::: firefox/tests/remote/restartTests/testPreferences_openH264/test2.js
@@ +20,5 @@
> +}];
> +
> +const PLUGIN_ID = "gmp-gmpopenh264";
> +const PREF_LAST_CATEGORY = "extensions.ui.lastCategory";
> +const TIMEOUT_PLUGIN_INSTALL = 90000;

Does it really take up to 90 seconds? If that is the case we should really hard think about if we can take this test! Find a way to get this down to at maximum 30 seconds or so.

@@ +24,5 @@
> +const TIMEOUT_PLUGIN_INSTALL = 90000;
> +
> +function setupModule(aModule) {
> +  aModule.browserWindow = new browser.BrowserWindow();
> +  aModule.controller = aModule.browserWindow.controller;

This is not really necessary.

@@ +26,5 @@
> +function setupModule(aModule) {
> +  aModule.browserWindow = new browser.BrowserWindow();
> +  aModule.controller = aModule.browserWindow.controller;
> +
> +  aModule.tabBrowser = new tabs.tabBrowser(aModule.controller);

Take it by going via the browserWindow.

@@ +71,5 @@
> +
> +  OPENH264_PREFS.forEach(aPref => {
> +    var pref = prefs.getPref(aPref.prefName, aPref.defaultValue);
> +    prefsGenerated = pref == aPref.defaultValue ?
> +                             prefsGenerated && false : prefsGenerated && true;

This is the wrong way to test. What you want here is to check against the default prefs branch.

Comment 7

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v2.patch

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Comment on attachment 8557061 [details] [diff] [review]
> v1.1
> 
> Review of attachment 8557061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This test is so simple, that I wonder why it is not getting tested in
> mochitest? I don't see any comment on this bug yet for this investigation.
I looked on mxr and haven't found tests for this. We thought of it to be mozmill because the real plugin needs internet connection to install. 

> It also concerns me to see this huge amount of time spent in waiting for it
> to be installed.
If the plugin installs sooner, the test won't wait for so long. It depends on the network. Sometimes it installs quicker, sometimes I waited even 10 minutes for it to install (manually, on my local machine). Also, we have even greater timeouts in other tests (300000, 360000 for updates).
 
> ::: firefox/tests/remote/restartTests/testPreferences_openH264/test1.js
> @@ +6,5 @@
> > +
> > +function setupModule(aModule) {
> > +  aModule.controller = mozmill.getBrowserController();
> > +  // close the browser to start with a new profile for the following test
> > +  aModule.controller.stopApplication(true);
> 
> Can't this be a new style restart test in a single file?
We need to restart the browser with a new profile for this test. Having it in a one file restart test, the code after the restart is not ran.

> ::: firefox/tests/remote/restartTests/testPreferences_openH264/test2.js
> @@ +20,5 @@
> > +}];
> > +
> > +const PLUGIN_ID = "gmp-gmpopenh264";
> > +const PREF_LAST_CATEGORY = "extensions.ui.lastCategory";
> > +const TIMEOUT_PLUGIN_INSTALL = 90000;
> 
> Does it really take up to 90 seconds? If that is the case we should really
> hard think about if we can take this test! Find a way to get this down to at
> maximum 30 seconds or so.
As I said earlier, it depends on the network. On my local machine the test fails with a 30s timeout.

Comment 8

[Andreea Matei [:AndreeaMatei]]

Before doing a review, I want to let Henrik decide over the first answers about being a mozmill test and the install time.
Maybe would be good to test this on a staging machine to see there how long it takes to install, out of 10 tries lets say.

Comment 9

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

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #7)
> If the plugin installs sooner, the test won't wait for so long. It depends
> on the network. Sometimes it installs quicker, sometimes I waited even 10
> minutes for it to install (manually, on my local machine). Also, we have
> even greater timeouts in other tests (300000, 360000 for updates).

This is not acceptable. We definitely cannot wait that long in our tests. Keep in mind that when we have multiple tests for the plugin, it would all count up. Maybe we should then make it a single remote test with a couple of test methods.

Not sure if that has been answered somewhere else already, but is there a way to reduce the time until Firefox checks to install the plugin?

Comment 10

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #7)
> > If the plugin installs sooner, the test won't wait for so long. It depends
> > on the network. Sometimes it installs quicker, sometimes I waited even 10
> > minutes for it to install (manually, on my local machine). Also, we have
> > even greater timeouts in other tests (300000, 360000 for updates).
> 
> This is not acceptable. We definitely cannot wait that long in our tests.
> Keep in mind that when we have multiple tests for the plugin, it would all
> count up. Maybe we should then make it a single remote test with a couple of
> test methods.
> 
> Not sure if that has been answered somewhere else already, but is there a
> way to reduce the time until Firefox checks to install the plugin?

I ran the test on staging with this timeout and it fails, the plugin didn't install that quick, so I guess we can't reduce it (instead we would even need to increase it). 
Another solution could be to try to use a mockup instead, but that would mean moving it to the b-c suite...

Comment 11

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

I don't see in your answer that you have checked for a way to get the plugin quicker downloaded and installed.

Comment 12

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v3

(In reply to Henrik Skupin (:whimboo) from comment #6)
> @@ +71,5 @@
> > +
> > +  OPENH264_PREFS.forEach(aPref => {
> > +    var pref = prefs.getPref(aPref.prefName, aPref.defaultValue);
> > +    prefsGenerated = pref == aPref.defaultValue ?
> > +                             prefsGenerated && false : prefsGenerated && true;
> 
> This is the wrong way to test. What you want here is to check against the
> default prefs branch.

It turns out that that doesn't work: getting the preference with prefs.getPref(aPref.prefName, aPref.defaultValue, true) doesn't return the correct preference value (always returns the value provided as default)

Comment 13

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8563440 [details] [diff] [review]
v3

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

Removing myself from review since it's still unclear if we want this with the real plugin.

Comment 14

[Mihaela Velimiroviciu (:mihaelav)]

What's going to happen with this test? Is it going to be rewritten for Marionette?

Comment 15

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

Comment on attachment 8563440 [details] [diff] [review]
v3

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

We are not going to add new mozmill tests. If this is an important test for QE please convert it to Marionette.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 20

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

We are going to remove firefox ui tests, and this hasn't had any activity in the last 5 years.