This makes it much harder (impossibly harder, in some cases) to modify how reftest works and is run, e.g. wrt profile settings (we can add a --with-profile option to hack around that when desired, or something similar). See bug for one improvement this would allow...

See bug 417516 where I added a makefile target for mochitest. Ideally we'd reuse automation.py to run reftest if we're going to do this.

This patch adds a "make reftest" target that you can run at the root of your objdir. (Also 'make crashtest') To accomplish this, it adds a runreftest.py script. You can run said script manually from your objdir like: python _tests/reftest/runreftest.py /path/to/reftest.list But if you just want to run all the reftests, just use "make reftest".

Comment on attachment 360321 [details] [diff] [review] add 'make reftest' r?dbaron because this deals with reftest, so I'd like his sign-off.

Comment on attachment 360321 [details] [diff] [review] add 'make reftest' and r?waldo for a once-over on all the Python bits I've touched.

Comment on attachment 360321 [details] [diff] [review] add 'make reftest' >diff --git a/layout/tools/reftest/Makefile.in b/layout/tools/reftest/Makefile.in >+DIST_FILES = install.rdf >+ >+# Used in install.rdf >+DEFINES += \ >+ -DMOZ_APP_VERSION=$(MOZ_APP_VERSION) \ >+ $(NULL) This should be in the ifdef XPI_NAME block so that we don't end up with dist/bin/install.rdf >diff --git a/layout/tools/reftest/install.rdf b/layout/tools/reftest/install.rdf >+ <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id> >+#expand <em:minVersion>__MOZ_APP_VERSION__</em:minVersion> >+#expand <em:maxVersion>__MOZ_APP_VERSION__</em:maxVersion> This is the Firefox install ID. Seems to me we should use the toolkit install ID and gecko version, but perhaps you tried that? Not a blocker, but worth looking into especially since Fennec probably needs something different.

I did not in fact try that, and I should have. I kind of copy-pasted this install.rdf from somewhere else in the tree.

Seems like it would be good if runreftest.py removed the profile when it was done, even when it was Ctrl-C-ed. If that's hard, it's ok to defer it to a later version. Then we can also (probably later) use mktemp to create the profile directory so runreftest.py can be used more than once at the same time. (Then we can also fix the reftest server stuff to try other ports if the port it wants is busy... I've been meaning to do that.) Other than that, this seems fine (though I haven't looked at everything in detail... I don't think I need to).

Right, I really just want your blessing on the general concept.

Comment on attachment 360321 [details] [diff] [review] add 'make reftest' The concept seems fine, though I'd really like us to get back to a state where we can be doing more than one reftest run on the same tree (which we could before the HTTP server stuff landed).

Ok. I will fix your suggestions before landing this, I don't think any of them are difficult. Then presumably you just need to fix the httpd bits to get back there.

Ok, I addressed bsmedberg's comments (using toolkit@mozilla.org App ID in install.rdf, cleaned up Makefile stuff a little), and I also changed runreftest.py to use a temp dir for the profile directory, and remove it in a finally block, per dbaron.

Comment on attachment 360398 [details] [diff] [review] add make reftest, updated per review comments [Checkin: See comment 15 & 19+21+22] >diff --git a/build/automation.py.in b/build/automation.py.in >- log.info("ERROR FAIL Missing 'kill' utility to kill process with pid=%s. Kill it manually !", pid) >+ log.info("TEST-UNEXPECTED-FAIL | Missing 'kill' utility to kill process with pid=%s. Kill it manually !", pid) Kill the almost-trailing space while you're at it, not sure how I missed that when reviewing that line previously... >-def runApp(testURL, env, app, profileDir, extraArgs, utilityPath = DIST_BIN, xrePath = DIST_BIN, certPath = CERTS_SRC_DIR): >+def runApp(testURL, env, app, profileDir, extraArgs, runSSLTunnel = False, utilityPath = DIST_BIN, xrePath = DIST_BIN, certPath = CERTS_SRC_DIR): I'm not really sure why ssltunnel isn't always being run except that it's not necessary, but if that's so I think it's an unwise guess. For now this is good enough, but I expect we'll circle back around and want ssltunnel here eventually, although at the moment I'm not sure how. >+def main(): >+ parser = OptionParser() >+ defaults = {} Not necessary if you aren't going to use it... >+ # browser environment >+ browserEnv = dict(os.environ) >+ >+ # These variables are necessary for correct application startup; change >+ # via the commandline at your own risk. >+ browserEnv["NO_EM_RESTART"] = "1" >+ browserEnv["XPCOM_DEBUG_BREAK"] = "warn" >+ appDir = os.path.dirname(options.app) >+ if automation.UNIXISH: >+ browserEnv["LD_LIBRARY_PATH"] = appDir >+ browserEnv["MOZILLA_FIVE_HOME"] = appDir >+ browserEnv["GNOME_DISABLE_CRASH_DIALOG"] = "1" I'm pretty sure we're going to want --browser-env support here eventually, but probably better as a followup. >diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in > (status, start) = automation.runApp(testURL, browserEnv, options.app, > PROFILE_DIRECTORY, options.browserArgs, >+ runSSLTunnel = True, > utilityPath=options.utilityPath, > xrePath=options.xrePath, > certPath=options.certPath) Seems like it might be a good idea for runSSLTunnel to default to True and then only explicitly override it for reftest.

(In reply to comment #13) > I'm not really sure why ssltunnel isn't always being run except that it's not > necessary, but if that's so I think it's an unwise guess. For now this is good > enough, but I expect we'll circle back around and want ssltunnel here > eventually, although at the moment I'm not sure how. Out of the 3 current consumers of automation.py (+ runreftest.py added here), only Mochitest is actually using SSLtunnel. Seems like a waste to run it for all of the rest. Also, for runreftest.py I didn't want to have to add --utility-path/--cert-path/--xre-path for something that we weren't using. dbaron wasn't keen on making reftest depend on features of automation.py, so I kept the script as simple as possible.

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/e94509d21882

https://developer.mozilla.org/En/Mozilla_automated_testing should be updated

Comment on attachment 360398 [details] [diff] [review] add make reftest, updated per review comments [Checkin: See comment 15 & 19+21+22] http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk?mark=75,79,97-98#72 I'm not familiar with .PHONY, but shouldn't the 2 new targets be added to it ?

Yeah, I missed that. Not a real big deal, I'll get it in a followup at some point.

Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/de81de26738b

(In reply to comment #17) > I'm not familiar with .PHONY, (In reply to comment #19) > http://hg.mozilla.org/releases/mozilla-1.9.1/rev/de81de26738b Ftr, this fixed .PHONY on 1.9.1 (only).

Comment on attachment 360398 [details] [diff] [review] add make reftest, updated per review comments [Checkin: See comment 15 & 19+21+22] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f0ad22379ffc (Bv1-191) Resync' runreftest.py with m-c, after bug 472706

Comment on attachment 360398 [details] [diff] [review] add make reftest, updated per review comments [Checkin: See comment 15 & 19+21+22] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6f36b8b0904b (Cv1-191) Resync' automation.py.in with m-c, missed part