Reftests currently have two ways of passing in options: command line arguments, that are only used on desktop, and via prefs, which is only used on mobile. This has several disadvantages, notably unnecessary complexity since the implementation adds preprocessor directives to the reftest.js file, and makes it much easier to break things on mobile when making changes on desktop. As part of bug 1181516 I want to pass in a number of manifests and the associated test filters for each. This is easy to do via prefs because I can json serialize a structure like {manifest1:[filter1-1, filter1-2], manifest2:[filter2-1]} and read it in reftest.js. I could do the same thing via the command line, I suppose, but it seems perverse to make two implementations when no one is actually ever going to write this by hand. It also seems strange to have this read from the prefs when everything else is read from the command line. Therefore I propose simply removing commandline support and always passing data into the harness via prefs. The only problem I can see is if someone is running the binary with reftest options set directly, for some reason, without going through mach or at least runreftest.py. Hopefully no one is doing that, but…

I know dbaron has said historically that he uses "firefox -reftest" directly, so I'd defer to him, but it's possible that having this particular case require the Python harness wouldn't be bad.

I've never liked requiring runreftest.py because running firefox -reftest directly allows more flexibility with debugging tools. But I guess I'm ok with dropping support for the command line way of passing things to reftest. (Are there any command line options today other than "-reftest <manifest>"? Is it hard to keep just that?)

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #2) > I've never liked requiring runreftest.py because running firefox -reftest > directly allows more flexibility with debugging tools. Interesting. Do you have examples of tasks that you find hard to do using the harness and so tend to use the binary for directly? These might be workflows we want to improve for everyone. > But I guess I'm ok > with dropping support for the command line way of passing things to reftest. > (Are there any command line options today other than "-reftest <manifest>"? > Is it hard to keep just that?) I think that's the only one. Removing it is just easier because there doesn't need to be logic about how the multiple ways of specifying reftests interact. If no one's using it that makes for ε less complexity to maintain in the harness. But if it's something that will make you more productive I can make it work.

Nothing in particular that I know of that's broken right now -- just that the interesting cases seem to regress, and I have the sense that anytime I want to try something interesting (like, say, use a debugging tool that requires passing a quoted argument through the various python scripts, or wanting to pass an environment variable to firefox but not to the python stuff (or, say, an xpcshell process that it starts) for, say, the nsTraceRefcnt-based leak debugging tools) that there's a decent chance I'll have to debug something that's broken along the pipeline because things have regressed. I'm ok with removing it.

Bug 1181520 - Remove support for passing in reftest arguments via the command line. This unifies how reftests are invoked across desktop and mobile, and paves the way for introducing more complex datatypes that are unreasonable to express on the command line.

Bug 1181516 - Allow reftests to take paths to multiple directories containing tests on the command line. This makes reftest command line arguments behave more like other test suites, so we can use a simple unified syntax for e.g. |mach try|. The patch also reworks the command line argument parsing to use argparse rather than optparse, and causes mach to reuse the same parser as the suite.

Bug 1193223 - Add reftest support to mach test.

Bug 1181516 - Better support for providing a directory name and discovering reftests under that directory

Bug 1193224 - Remove vestigial --tests-root-dir option from xpcshell tests.

Bug 1193257 - Make xpcshell harness command line arguments path filters for tests

Bug 1193257, 1181516 - Update unittest mozconfigs for all platforms

Bug 1188730 - Exit with a warning and status code 0 if we don't find any mochitests to run. This can happen, in particular with the M-oth job on buildbot in combination with test selection through |mach try| without being an error. That job is a particular problem because it runs several mochitest-based suites of which only a subset may have tests selected

Bug 1193215 - Support for passing test directories through mach try. This adds support for web-platform-tests to mach try. It changes the implementation so that instead of passing paths to manifests, the user passes arbitary paths in the source tree, and tests under that path are run, with test discovery mainly left to the harness.

Bug 1193264 - Add support for saving and reusing try strings in mach try Adds --save and --preset arguments that can be used to store and reuse frequently used try strings.

Bug 1181520 - Remove support for passing in reftest arguments via the command line. This unifies how reftests are invoked across desktop and mobile, and paves the way for introducing more complex datatypes that are unreasonable to express on the command line.

Comment on attachment 8646443 [details] MozReview Request: Bug 1181520 - Remove support for passing in reftest arguments via the command line. https://reviewboard.mozilla.org/r/15753/#review14077 ::: layout/tools/reftest/reftest.js (Diff revision 1) > -#if BOOTSTRAP 1) do we still use bootstrap mode for android? 2) does this work on android? 3) removing if/else appears like we are keeping a lot of code from both parts. Can we reduce the code to just the else block?

https://reviewboard.mozilla.org/r/15753/#review14263 ::: layout/tools/reftest/reftest.js (Diff revision 1) > -#if BOOTSTRAP This works on android on Try at least. It's not surprising because we are mostly using the code that was already used on mobile and dropping the desktop-specific part. I think the fact that we have kept some of the code from the else clause is the bug that needs to be fixed.

https://reviewboard.mozilla.org/r/15753/#review14077 > 1) do we still use bootstrap mode for android? > 2) does this work on android? > 3) removing if/else appears like we are keeping a lot of code from both parts. Can we reduce the code to just the else block? This works on android on Try at least. It's not surprising because we are mostly using the code that was already used on mobile and dropping the desktop-specific part. I think the fact that we have kept some of the code from the else clause is the bug that needs to be fixed.

Comment on attachment 8646443 [details] MozReview Request: Bug 1181520 - Remove support for passing in reftest arguments via the command line. Bug 1181520 - Remove support for passing in reftest arguments via the command line. This unifies how reftests are invoked across desktop and mobile, and paves the way for introducing more complex datatypes that are unreasonable to express on the command line.

Comment on attachment 8646443 [details] MozReview Request: Bug 1181520 - Remove support for passing in reftest arguments via the command line. https://reviewboard.mozilla.org/r/15753/#review15761 thanks, this is looking good.

sorry had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5a8d86582839 since one of this changes caused https://treeherder.mozilla.org/logviewer.html#?job_id=13953993&repo=mozilla-inbound

Had to back it out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0a54cadfd0 for failures on Android 4.0 API11 reftests that look like this https://treeherder.mozilla.org/logviewer.html#?job_id=14037560&repo=mozilla-inbound

Backed out for breaking Android 4.0 debug reftests https://hg.mozilla.org/integration/mozilla-inbound/rev/d90bfbc8688a

Looks like Android 4.0 debug reftests are scheduled on inbound but not on try.. that's kind of bad.