[manifestparser] should allow consumers to use arbitrary 'conditional' keys



3 years ago
3 years ago


(Reporter: ahal, Unassigned)


Firefox Tracking Flags

(Not tracked)




3 years ago
Right now there are four conditional keys in manifestparser, run-if (to be removed), skip-if, fail-if and subsuite. These are all handled explicitly in manifestparser itself:

This means anytime we want to add a new key with a value that should get parsed by the ExpressionParser, we need to modify manifestparser and release a new version. This does not scale, and makes it impossible to add conditional values that are specific to a test harness.

For example, "subsuite" is only used by mochitests but is explicitly handled in manifestparser core. This is a slippery slope as manifestparser will quickly become complicated if all harnesses start defining harness specific keys there.

Another example is that we want a "requesttimeoutfactor-if" sort of key in xpcshell. Currently the only way to implement it would be to explicitly handle it in manifestparser core, but no other harnesses would ever make use of it.

I think there are a few ways we could approach this:

1) Assume any key that ends with "-if" has a conditional value.
2) Test to see if the value looks like a valid expression and treat it like one if it does
3) Provide a way for harnesses to explicitly list which keys they want to be conditional.
4) A combination of 1 and 2.

I like the first approach the best because it is simple, intuitive and not *too* automagical. My next favourite approach is the third one. I think #2 and #4 are bad ideas.

Another thing to consider is that "subsuite" has a syntax like:
  subsuite = <value>,<condition>

It may also be worthwhile to support arbitrary "value,condition" keys in manifestparser. This could be done in a follow up though.
One issue is hygiene with respect to extending the manifest parser. It looks like TestManifest is intended to be subclassed by harnesses for specific behavior. The other side of this is being aggressive about failing early when a harness doesn't support a key that has apparent special treatment in the manifestparser. I'm a big fan of proposal 1 above if we keep this in mind and make it hard to misuse.

The other thing is extending the manifest language. A config value now is almost always an expression that evaluates to a single value. This is extended for subsuite, and requesttimeoutfactor-if is another case we would want a manifest value to be a constant along with a condition to determine when that constant applies. It might be worthwhile to extend the expression language so LITERAL,EXPRESSION (or similar) is an entry point to the grammar that results in a tuple applicable for keys like these.
You need to log in before you can comment on or make changes to this bug.