Jeff Walden [:Waldo] Reporter
	Description • 16 years ago

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...

	(not currently active) Ted Mielczarek Assignee
	Comment 1 • 16 years ago

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.

	(not currently active) Ted Mielczarek Assignee
	Comment 3 • 16 years ago

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".

	(not currently active) Ted Mielczarek Assignee
	Comment 4 • 16 years ago

Comment on attachment 360321 [details] [diff] [review]
add 'make reftest'

r?dbaron because this deals with reftest, so I'd like his sign-off.

	(not currently active) Ted Mielczarek Assignee
	Comment 5 • 16 years ago

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.

	Benjamin Smedberg
	Comment 6 • 16 years ago

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.

	(not currently active) Ted Mielczarek Assignee
	Comment 7 • 16 years ago

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.

	David Baron :dbaron:
	Comment 8 • 16 years ago

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).

	(not currently active) Ted Mielczarek Assignee
	Comment 9 • 16 years ago

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

	David Baron :dbaron:
	Comment 10 • 16 years ago

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).

	(not currently active) Ted Mielczarek Assignee
	Comment 11 • 16 years ago

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.

	(not currently active) Ted Mielczarek Assignee
	Comment 12 • 16 years ago

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.

	Jeff Walden [:Waldo] Reporter
	Comment 13 • 16 years ago

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.

	(not currently active) Ted Mielczarek Assignee
	Comment 14 • 16 years ago

(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.

	(not currently active) Ted Mielczarek Assignee
	Comment 15 • 16 years ago

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

	Nickolay_Ponomarev
	Comment 16 • 16 years ago

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

	Serge Gautherie (:sgautherie)
	Comment 17 • 16 years ago

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 ?

	(not currently active) Ted Mielczarek Assignee
	Comment 18 • 16 years ago

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

	(not currently active) Ted Mielczarek Assignee
	Comment 19 • 16 years ago

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

	Serge Gautherie (:sgautherie)
	Comment 20 • 16 years ago

(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).

	Serge Gautherie (:sgautherie)
	Comment 21 • 16 years ago

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

	Serge Gautherie (:sgautherie)
	Comment 22 • 16 years ago

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