Closed Bug 1113192 Opened 5 years ago Closed 11 days ago

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

Categories

(Testing :: Firefox UI Tests, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: cosmin-malutan, Unassigned)

Details

User Story

1 Launch Firefox with a clean profile.
2 Navigate to about:config and click on "I'll be careful, I promise!" button.
3 Search for gmpopenh264.
 > The following prefs are not generated:
 > media.{0}.lastUpdate
 > media.{0}.version
4 Go to Add-ons Manager -> Plugins and wait for openH264 plugin to install.
5 Repeat steps 2 and 3.
 > After the plugin is installed, the new prefs are properly generated (please see the attachment):
 > media.{0}.lastUpdate
 > media.{0}.version

Attachments

(1 file, 3 obsolete files)

Plugin openH264 needs internet connection to install itself(step 4), that makes this test a mozmill candidate.
Assignee: nobody → mihaela.velimiroviciu
Whiteboard: [sprint]
Attached patch v1 (obsolete) — Splinter Review
Attachment #8555918 - Flags: review?(andreea.matei)
Status: NEW → ASSIGNED
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.
Attachment #8555918 - Flags: review?(andreea.matei) → review-
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.
Attachment #8557061 - Flags: review?(andreea.matei) → review+
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.
Attachment #8557061 - Flags: review-
Attached patch v2.patch (obsolete) — Splinter Review
(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.
Attachment #8557061 - Attachment is obsolete: true
Attachment #8559791 - Flags: review?(andreea.matei)
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.
Flags: needinfo?(hskupin)
(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?
Flags: needinfo?(hskupin)
(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...
I don't see in your answer that you have checked for a way to get the plugin quicker downloaded and installed.
Attached patch v3Splinter Review
(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)
Attachment #8559791 - Attachment is obsolete: true
Attachment #8559791 - Flags: review?(andreea.matei)
Attachment #8563440 - Flags: review?(andreea.matei)
Attachment #8563440 - Flags: feedback?(hskupin)
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.
Attachment #8563440 - Flags: review?(andreea.matei)
What's going to happen with this test? Is it going to be rewritten for Marionette?
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.
Attachment #8563440 - Flags: feedback?(hskupin)
Component: Mozmill Tests → Firefox UI Tests
Whiteboard: [sprint]
Assignee: mihaela.velimiroviciu → nobody
Product: Mozilla QA → Testing
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
No assignee, updating the status.

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

Status: NEW → RESOLVED
Closed: 11 days ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.