Open Bug 857081 Opened 7 years ago Updated 2 years ago

Test harness should report error for misspelled manifest conditions


(Testing :: Mozbase, defect, P3)



(Not tracked)


(Reporter: Irving, Unassigned)



(Whiteboard: [mozbase])

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"
 # Bug 676992: test consistently fails on Android
 fail-if = os == "android"
+# 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"
 # 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.
Component: XPCShell Harness → Mozbase
QA Contact: hskupin
Whiteboard: [mozbase]
So I would advocate the following:
- ManifestParser should have a (class-level) list of what attributes it provides/expects; from 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.
Blocks: 860005
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
Summary: Test harness should report error for misspelled xpcshell.ini manifest conditions → Test harness should report error for misspelled manifest conditions
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.