There was a lot of good feedback from the post introducing the concept of universal manifest formats for all unit tests. I want to continue the discussion based upon those questions/comments.
Why aren’t we just going to take one of our existing manifest formats and roll it out to the rest of the tests? Why re-invent the wheel?
The most suggestions were for adopting the reftest manifest format across the board. While the reftest manifests are excellent at what they are designed for we are hitting the point where they can’t handle the growth in terms of 32 vs. 64 bit tests, qt vs. gtk vs. android (which all end up in the same ‘linux’ bucket), shell tests vs. full browser, etc. Here’s some of the worst offenders that are already in use:
./js/src/tests/js1_5/extensions/jstests.list:
random-if(xulRuntime.OS=="WINNT"&&!isDebugBuild) skip-if(xulRuntime.OS=="Linux") script regress-342960.js # slow
./js/src/tests/js1_5/Regress/jstests.list:
fails-if(xulRuntime.OS=="WINNT") random-if(xulRuntime.OS=="Linux"&&!XPCOMABI.match(/x86_64/)) skip-if(xulRuntime.OS=="Linux"&&XPCOMABI.match(/x86_64/)) script regress-3649-n.js # No test results on windows, sometimes no test results on 32 bit linux, hangs os/consumes ram/swap on 64bit linux.
./js/src/tests/js1_6/extensions/jstests.list:
fails-if(!xulRuntime.shell&&!isDebugBuild) skip-if(!xulRuntime.shell&&isDebugBuild) script regress-455464-04.js # bug xxx - hangs reftests in debug, ### bug xxx - NS_ERROR_DOM_NOT_SUPPORTED_ERR in opt
./layout/reftests/bugs/reftest.list:
fails-if(MOZ_WIDGET_TOOLKIT=="windows") fails-if(MOZ_WIDGET_TOOLKIT=="cocoa") random-if(MOZ_WIDGET_TOOLKIT!="cocoa"&&MOZ_WIDGET_TOOLKIT!="windows") != 399636-quirks-html.html 399636-quirks-ref.html # windows failure bug 429017, mac failure bug 429019
./modules/plugin/test/reftest/reftest.list:
fails-if(!haveTestPlugin) skip-if(!prefs.getBoolPref("dom.ipc.plugins.enabled")) == pluginproblemui-direction-1.html pluginproblemui-direction-ref.html
It’s all pretty unreadable, and things are just going to get worse as more mobile platforms come online and we have to handle exceptions for each one.
Adding tests is currently easy and I don’t want to lose that.
Totally in agreement here! There may end up being a few more keystrokes, but overall things really aren’t changing much for the simplest case. Something like this:
#old syntax
== file1.html file1-ref.html
#new syntax
{"type" : "=="
"files" : ["file1.html", "file1-ref.html"]
}
What’s the benefit here?
- Greater granularity
- Better benchmarks
- Finer filtering
Make it possible to target specific sections of code for test runs. Specialized manifests would be easy to create around an area of interest. For instance, if I make a manifest that includes all content tests, then I can do something like:
> python runtests.py --manfiest=mycontentmanfiest.json
and it can run only the tests that I want based on my patch.
Enable us to easily benchmark things - run all known [orange] tests, run specific performance intensive tests several times for profiling diagnostic, etc.
Want to enable a test on maemo only? Want to never run a test on android? We want to make this simpler and easier without having to do a bunch of inline javascript hacking.
Why are you picking on reftests?
We chose refest to be used as a proof of concept. We wanted to roll the new manifest format out to reftest first to ensure that everything worked the way that we wanted (kick the tires a bit) and then slowly convert over our other test harnesses.
What about inline js calls like for isDebugBuild or checks against MOZ_WIDGET_TOOLKIT?
We no longer want to support js code in manifest files. We believe that that sort of code should live outside the manifest. Support would be built into the manifest for the states that these calls check (ie, lists of available platforms to test against, lists of build types, native theme types, etc). This way every test would use the same check, and if the check needs to be updated to support a new platform/arch/build/theme/etc it can be done in a central location. This will come in handy for the mobile platforms which would all currently end up identified as ‘linux’.
Where’s the new format?
This is probably the biggest question. Truth is, we aren’t finalized here yet. We are pretty sure that json is the way to go for the flexibility that we want - but we are still very open to suggestions. Right now this is what we are considering:
{ "tests" :
[
{ "type": "==",
"files": ["file1.html", file2.html"],
"skip-if": ["maemo", "android"],
"prefs": {
pref1 : bool1
pref2 : bool2
},
},
{ "type" : "js",
"files" : ["jsfile.html"],
"random-if" : ["winnt"],
"skip-if": ["isdebug"],
}
{ "group" : "withTestplugin",
"plugins": ["plugin1.xpi"],
"prefs": {
enablePlugin : True
}
"tests" :
[
{ "type" : "!=",
"files" : ["file1.html", "file6.html"],
}
]
}
]
}
This format would allow you to easily set prefs either per-test or per-group of tests. There is also a means to install/enable plugins and then group tests together that use that plugin. A lot has been borrowed from reftest manifest (skip-if, random-if) as that is stuff that works - we want to take concepts that work and keep them around. What do you like about it? What don’t you like? What would you like included as an option? Would you prefer xml? yaml? We are in very early stages here and the end result should be a manifest format that everyone is happy with.
I like your idea of replacing “MOZ_WIDGET_TOOLKIT==’cocoa’” with “cocoa”. It would save typing, prevent errors, and make it easier to read manifest entries for tests that have complex conditions. But that’s orthogonal to the syntax of the manifest format.
I like the idea of allowing groups of tests to share a common pref or skip-condition. We could add that to the reftest manifest format using indentation, like in Python or YAML, without changing the syntax for the normal case.
I also like the idea of using a common –filter or –manifest option across all of our test suites. But again, that has nothing to do with the syntax.
How would your manifest format would handle conditions that require && or ||, like the test for bug 399636 requires? Are you planning to allow || and && and ! in “fails-if” and “random-if” conditions, but not arbitrary JavaScript?
Comment by Jesse Ruderman — May 14, 2010 @ 6:02 pm
This is really cool. I think the && and || stuff is in logic like this:
“skip-if”: ["maemo", "android"],
But this is ambiguous as I could assume it is an || condition, yet how would we solve an && condition. I found this exact problem while toying with filtering for fennec mochitests (https://elvis314.wordpress.com/2010/04/27/filtering-mochitests-for-remote-testing/) I ended up coding my implementation to be && which is not always the most useful.
One thing that I would like to know is how would mochitest format look like? Currently this is a os.walk of test_* files in a directory tree. So with no manifest other than entries in a Makefile and using a format above, would we have:
{ “type” : “mochitest”,
“files” : ["mochifile.html"],
“skip-if”: ["isdebug"],
}
Comment by Joel Maher — May 14, 2010 @ 6:35 pm
So for the common case this is a lot more typing than the current reftest format, actually. Worse yet, there’s indentation involved, which can very easily get pretty wacky unless one is careful (which increases the time it takes to add entries even more). Do we have any plan for handling that?
Comment by Boris — May 14, 2010 @ 7:23 pm
How about replacing the token ‘files’ with the type, use an extended JSON that allows the value to be an array and allows comments, and don’t indent array elements, so one could write
[
{"==": ["file1.html", "file1-ref.html"]},
{”==”: ["file2.html", "file2-ref.html"]},
// {”==”: ["file3.html", "file3-ref.html"]},
{”==”: ["file4.html", "file4-ref.html"]},
]
This would keep common cases on one line without indentation, which I think would be a big help.
Comment by Robert O'Callahan — May 15, 2010 @ 4:41 am
I like this plan. YAML is nicer for humans to write than JSON, has plenty of parser support, and it has comments. I would go with that instead.
Comment by Rob Sayre — May 15, 2010 @ 7:42 am
@Boris - the indentation is not required by json, I’ve included it in the examples to make things more readable.
Comment by alice — May 15, 2010 @ 1:22 pm
I think the skip, fails, etc. conditions should remain JS. That keeps it clear what’s an && vs. an ||.
We can use significantly shorter variable names (the current MOZ_WIDGET_TOOLKIT is for backwards compatibility). However, I’m wary of making them too short. Currently things are based on widget because that’s the most likely cause of platform-specific failures in reftests, which mostly test layout and graphics. But if we start using this for other tests, that will likely change. And once we have a qt build (for mobile) there’s a real difference between “this fails with gtk widgets” and “this fails on Linux”.
I’d prefer something like:
fails: true
or
fails: widget == “cocoa”
Comment by David Baron — May 16, 2010 @ 3:17 pm
What would be really good is to have some possibility to say “if the pref foo is (not) true/2/’bar’” in the skip-if or random-if values.
Reftests can currently do that, and we have somewhat strangely done things in other tests for that, like bailing out early from a test when a pref is (not) matched. It would be nice to have a generic way to set this.
Comment by Robert Kaiser — May 19, 2010 @ 9:18 am