Come up with a better way to test experimental/in-progress CSS features, without enabling them for testing profiles

NEW
Unassigned

Status

()

Core
CSS Parsing and Computation
P3
normal
2 years ago
a year ago

People

(Reporter: dholbert, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox51 affected)

Details

(Reporter)

Description

2 years ago
Right now, we tend to implement new CSS properties / values by:
 a) Implementing the new behavior behind a pref (off-by-default or non-RELEASE_BUILD by default at first)
 b) Add tests for the new behavior to property_database.js, test_transitions.html, etc. (And maybe some other tests.)
 c) Add the pref to testing/profiles/prefs_general.js so that the new code gets tested.  (Otherwise, we can't be sure the new code works & stays working.)

Part (c) (enabling the pref for mochitests) is problematic because, as dbaron hinted in bug 1297333 comment 5, it means we're not testing the same configuration that we're shipping to release users (in cases where the feature isn't ready to be preffed-on, at least).

On the other hand, if we *don't* do something like part (c), we won't benefit from integration testing -- the property_database.js-based tests won't get run, and we might mistakenly check in untested code (thinking that a green Try run means all's well), or we might check in code that breaks previously-working (but disabled) tests for this new feature without realizing it.

SO: we need to come up with a best-practice way to thoroughly test new CSS features (and benefit from our existing all-properties tests), while still testing the configuration that we intend on shipping to users when the current trunk ends up turning into a release.
(Reporter)

Comment 1

2 years ago
Some ideas below:

Option (1):
 * Enable a bunch of experimental CSS properties' about:config prefs (via some common function "EnableExperimentalCSSFeatures" defined in property_database.js), at the beginning of every property_database.js-based test.  For tests that aim to check each property individually (in isolation), the not-quite-testing-what-we-ship aspect here should be fine.
  PROS: Pretty simple, gets stuff tested by default, and by default, the vast majority of mochitests will still be testing the configuration that we'll actually ship.
  CONS: We still won't be running the property_database.js-based tests in the configuration that we're shipping, which may be more or less of an issue depending on the experimental property/feature.


OPTION (2):
  * Put new pref-controlled CSS Properties into a new array called e.g. gPrefControlledCSSProperties to property_database.js, with one additional "pref" field per property.  (Properties only migrate out of this array when we're ready to pref them on in release builds.)
  * All property_database.js-based tests (which are aiming to test each property in isolation) should do their first normal pass over gCSSProperties, and then do a *second* pass over this second array, preffing on each property in turn and then doing their normal test mechanics.
  PROS: We'll be closer to the ideal of "test what we're shipping".
  CONS: Requires a bit more reworking/rewriting/refactoring of existing test code, probably. Works well for new CSS Properties, but not as well for new pref-controlled values & syntax for existing properties (e.g. "display: flex", "background-image: [hypothetical-new-gradient-expression]")
> c) Add the pref to testing/profiles/prefs_general.js so that the new code gets tested.

I think this part should be inside a #ifndef RELEASE_BUILD so that we test with
experimental features enabled in Nightly/Aurora and disabled in Beta/Release.
(Reporter)

Comment 3

2 years ago
Thanks -- let's call Mats' suggestion "Option 3". Pros/cons of that option, as I see them:

  PROS: It's simple.  It's also an approximation of the configuration that we often intend to ship soonish on Nightly anyway (with an #ifndef RELEASE_BUILD in all.js). And it means that, at least on Beta & Release, we'll be testing the configuration we're shipping to users.
  CONS: We still wouldn't be testing the configuration that we'd [currently, at that point in time] be shipping to Nightly & Aurora users.  [Though we would be testing a configuration that we might be shipping to them "soon", whenever we're ready to move the prefs_general.js "#ifdef RELEASE_BUILD" chunk over into librpref's all.js file.]
(Reporter)

Comment 4

2 years ago
dbaron, how would you feel about us going with mats' suggestion ("Option 3") for bug 1297333 at least (clip path w/ shapes), so we can get that feature continuously & reliably tested as we land its last missing pieces?

And, stepping back from that feature in particular, do you have any general preference among the various options here, or any other ideas for what best-practices would be for testing not-quite-ready-for-default-enabling-in-Nightly CSS properties/syntax going forward?
Flags: needinfo?(dbaron)
I'm ok with that, assuming you test that the #ifdef is working as expected.
Flags: needinfo?(dbaron)
Sadly, I found that testing/profiles/prefs_general.js isn't preprocessed like all.js, so "Option 3" won't work unless we make it go to preprocessor.
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.