917541 - Selectively enable experimental form-control prefs in mochitests, rather than hardcoding them for all mochitests in prefs_general.js

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Daniel Holbert [:dholbert]]

Right now, we have some experimental DOM prefs enabled in our mochitest profiles -- i.e. enabled for *all of our mochitests*:
> 11 user_pref("dom.experimental_forms", true); // on for testing
> 12 user_pref("dom.forms.number", true); // on for testing
> 13 user_pref("dom.forms.color", true); // on for testing
http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js#11

If we followed this strategy for other experimental features, we'd have a handful of other prefs hardcoded in there, too. (e.g. the various experimental layout.css.* prefs, among others).

But we don't use this strategy for experimental layout prefs, and I don't think we should for DOM prefs, either, because it means we end up running *all of our mochitests* using a substantially different configuration from what our users run.

Of course, it makes sense to run *specific* mochitests with these prefs enabled (e.g. the tests that exercise these features, and perhaps a few others), so we should enable the prefs with SpecialPowers in those tests, and we should take them out of prefs_general.js. 

(unless the prefs need to be enabled at startup time to function properly or something - in which case, we should mention that in prefs_general.js to explain why we need them on there.)

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> But we don't use this strategy for experimental layout prefs, and I don't
> think we should for DOM prefs, either, because it means we end up running
> *all of our mochitests* using a substantially different configuration from
> what our users run.

("substantially different" is relative, of course; right now, these prefs don't make things *too* different. The point is that if we did this for all our experimental stuff, *then* it'd be quite substantially different. So, better to selectively enable in a targeted way, for the tests that matter, unless there's a reason we can't.)

Comment 2

[Masatoshi Kimura [:emk]]

Entirely agree. test_interface can't test the availability of GamePad APIs on release builds due to this bug.

Comment 3

[Mounir Lamouri (:mounir)]

The three preferences you pointed are actually forms preferences instead of DOM preferences. The reason why they are enabled by default are multiple. My first guess was to enable them for every test that needed them but this is a huge hassle (there are a ton of forms tests and many of them will test multiple input types). Also, moving the pref as on by default would duplicate that hassle. Finally, those preferences end up enabled in some platforms (like experimental_forms) or some builds (Aurora/Night vs Beta/Release) so having our full test suite considering them prevent bugs that would happen only if they are enabled. You could say that the opposite is true though ;).

In a nutshell, those three preferences are enabled on purpose in the test profiles but generally speaking, we do not have a policy of adding preferences in the test profile for DOM experimental features. Most features I review get the pref enabled in the test file (recently: datastore and promises).

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Mounir Lamouri (:mounir) from comment #3)
> The three preferences you pointed are actually forms preferences instead of
> DOM preferences.

OK -- I updated the summary to mention form controls instead of "DOM".

> My
> first guess was to enable them for every test that needed them but this is a
> huge hassle (there are a ton of forms tests and many of them will test
> multiple input types).

A bit of a hassle, yes, but it's gotten easier -- IIRC it should just require a call to SpecialPowers.pushPrefEnv() in each test where you want to exercise the new code, before the nodes are appended to the DOM, I think.

> Also, moving the pref as on by default would duplicate that hassle.

Well, at that point, the SpecialPowers.pushPrefEnv() calls would just become unnecessary (but harmless).  There's no rush to remove them -- they can be removed at our leisure, when we're confident that the pref is staying on for good.

> Finally, those preferences end up enabled in some
> platforms (like experimental_forms) or some builds (Aurora/Night vs
> Beta/Release) so having our full test suite considering them prevent bugs
> that would happen only if they are enabled.

Yeah... ideally it'd be great to be able to run all the relevant tests twice, with the pref on (to make sure the new code is working) *and* off (to make sure that the configuration we're shipping to our users is working). Both configurations are worth testing. (though it's worth erring on the side of being concerned with the currently-shipping configuration, IMHO).

> In a nutshell, those three preferences are enabled on purpose in the test
> profiles but generally speaking, we do not have a policy of adding
> preferences in the test profile for DOM experimental features.

Understood. If this chunk doesn't grow much beyond 3 prefs, I'll be happy. (Though I still think it'd be worth scoping those prefs to only be pushPrefEnv()'d in the tests that actually exercise their code, if anyone is up for doing that.)

Comment 5

[Mounir Lamouri (:mounir)]

There are ~60 test files in content/html/content/tests/forms/ that might, for many of them, require some prefs to be turned on.
There are ~400 test files in content/html/content/tests/ that might, for some of them require some prefs to be turned on.
In addition of that, there are tests related to toolkit/ that will take into account some input type.

If all the tests that behave differently for some input type are not running when a new type is enabled, a feature might be broken/missing and we will see that only when the pref will be enabled by default. Checking manually all of those tests to check if it is needed to enable the feature would take a long time. Enabling the feature locally to check which test break, and fix them would be faster but still take quite a lot of time (the author might need to add the .pushPref() call to dozens of test files.

(Note that the reason why enabling the pref changes something is that many tests I wrote and reviewed will break if a new type is supported so the author of the patch will fix the test, thus make the feature work as expected.)

Comment 6

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven't been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.