Created attachment 295599 [details] Non-working mostly-transcription, for backup ...and correct one of the greatest mistakes I've ever made in not rewriting in a sane language when I had the chance earlier.
Created attachment 295608 [details] [diff] [review] Mostly-working patch with known problems This seems to work on OS X, but for whatever reason it's showing the same errors the debug tinderboxen on MozillaTest are currently showing -- no idea why I see it with a Python script but not a Perl script. The only other known problem I'm aware of is that the server isn't properly killed on Windows, but there's not much I can do about that for the moment -- need bsmedberg's killableprocess to handle that. I'm using the serverShutdown handler for now to tell the server to go away, which is sufficient in most cases. This doesn't change the existing Perl version, and the two can coexist peacefully for the moment until we can do a s/runtests\.pl/runtests\.py/g. It's a mostly straightforward translation, and it's probably not especially idiomatic Python. Unless we end up wanting killableprocess, this is 2.4-capable, so it's fine for tinderboxen. I'm pretty sure there are problems with this patch, but I'm not sure I'd see many of them as I haven't coded Python that often. Also, the improvements I'd want to make go beyond just conversion, and it seems better to make this about just translating from Perl to Python so as not to confuse things too much.
Created attachment 295609 [details] [diff] [review] With a couple more tweaks
Created attachment 296092 [details] [diff] [review] Fully working now! The passwordmgr problem was because the profile path I was passing on the command line was a relative path; changing it to an absolute path fixed the problem.
Attachment #296092 - Flags: review?
Attachment #296092 - Flags: review? → review?(rcampbell)
Comment on attachment 296092 [details] [diff] [review] Fully working now! >Index: testing/mochitest/Makefile.in >-_SERV_FILES = runtests.pl \ >+_SERV_FILES = \ >+ runtests.pl \ >+ runtests.py \ > gen_template.pl \ > redirect.html \ > $(topsrcdir)/netwerk/test/httpserver/httpd.js \ >+ $(topsrcdir)/tools/footprint/leak-gauge.pl \ > $(NULL) I couldn't be bothered to remove this last addition before creating the patch; pretend it's not there.
Created attachment 296104 [details] [diff] [review] I hate Windows
heh. Give me a couple of days to play with this, Jeff. Thanks for the patch(es)!
I played with this last night on my windows machine and it seemed to do what we need it to. I had problems running with --file-level and --log-file options (mochitest came up, but just hung there even with --autorun). And I didn't test --appname (which I will) or the --chrome, --browser-chrome options. --autorun, --console-level and --close-when-done all worked as expected though. one nit at the end of the file: +################## +# Do everything. # +################## + +main() should replace with: if __name__ == '__main__': main() It's a standard pythonism and makes it easier to deal with in an interactive environment for debugging. More to follow...
--log-file and --log-level work for me on OS X; I'll have to fire up the Windows VM to check there, I guess. Looking at the code, tho, I should probably be urlencoding the query parameters somehow (and need to double-check that would get undone on the other side); look for that in a later patch. What's the newest version of Python that can be required to run this? I thought I'd been running 2.4 in testing, but since |A if C else B| is a 2.5ism, I clearly couldn't have been doing so. All versions of MozillaBuild, OS X 10.4, and recent Linux distros all ship 2.5, which has been out since September 2006.
In 1.9 we should work with python 2.3 In moz2 we will require python 2.4 I don't know much about "A if C else B" but I think you can get a similar result with "A or B"... sayrer is this correct?
Ugh. The ternary syntax I can live without, but 2.3 claims not to have subprocess: http://www.python.org/doc/2.3/modindex.html
OS X 10.4 ships with an out-dated 2.3 version of python. All the build machines install 2.4 at a minimum as part of the "refplatform". 2.3 should be considered too old and I wouldn't worry about that. We want a target of 2.4 or greater so we can wedge killableprocess.py in here. We'll have to wait for ternary syntax.
Where does runtests.pl (to-be-py) live? If this is part of the standard unit-test framework, we should continue to support python 2.3... if this is something particular to Talos, then we can use whatever version of python we want.
It's for mochitest.
no, as Ted says, this is going to live next to runtests.pl in mozilla/testing/mochitest. I was thinking about adding killableprocess.py in alongside it, or possibly putting it into a tools/python/killableprocess package we could install. Any opinions?
Ordinary developers are expected to run runtests.pl to run the mochitests locally, according to http://developer.mozilla.org/en/docs/Mochitest#Running_Mochitest Therefore I think we should stay 2.3-compatible... we could of course import subprocess/killableprocess conditionally for newer pythons that support it.
The point is that I find the existing Perl file hard to read and hard to edit. Every time I have to edit it, I have to relearn the Perl necessary to do so. I don't want to have to do that, I'm pretty sure sayrer doesn't want to have to do that, and I want to be able to use a language I can read and know reasonably well to implement Mochitesting functionality. I don't know what to say if this can't replace the Perl version; I'm willing to endure pain, even considerable pain, to make it work in 2.3, but I don't know how possible that is staying inside the standard Python distribution. If it is, I will gladly take that over being forced to touch Perl any time I need to change things here, even if it requires considerable effort now. If it isn't...I'd rather not consider that possibility until forced to do so. I can't describe how bad I feel about being forced to use subpar, unusable technology because we can't use something new that's not insane. Please don't take this as exaggeration; it absolutely is not.
The python 2.3 requirement is mac/unix only... we can require python 2.5 (mozillabuild) on Windows. So for *nix-only, it looks pretty trivial to import popen2 and use the Popen3 object to get the pid.
http://docs.python.org/lib/module-popen2.html for reference
Wow, Jeff. And I thought I disliked runtests.pl. :) Those are good suggestions, Ben. This should be pretty easy to make backwards compatible. Then, in a year or two we can just rip out the 2.3 requirements (and start porting forward to python 3.0). Also, there's no reason we can't keep runtests.pl in the tree and just consider it deprecated from this point forward. People can use what they feel comfortable with or what works. I think the majority will move to using the python version as it is much more understandable. Thanks for your work on this, Jeff!
Created attachment 299475 [details] [diff] [review] Abstract process management to cater to obsolete versions of Python I've only tested this on Mac, but the Windows parts were just copy-paste so I don't expect any bustage there. As for the Perl version, I wrote this because I can't stand hacking on this in Perl. As soon as this is in and tinderboxen are converted to use this, I'd like to see us either remove the Perl version or temporarily replace it with a file that prints a warning message and then calls the Python one, which would be removed in a few months once everyone knows what's up. Maintaining two of these isn't a good use of anyone's time.
alright, I'm going to spend this afternoon after the meetings playing with this. I agree we should move to deprecate runtests.pl, though I'm reluctant to rip it out entirely right away.
Comment on attachment 299475 [details] [diff] [review] Abstract process management to cater to obsolete versions of Python looks good.
Attachment #299475 - Flags: review?(rcampbell) → review+
Committed, and added a note to the topic in #developers about this -- so hopefully we'll get some feedback on this. Ideally it's effectively a noop for developers, but we'll see...
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Mass move of the rest of the Mochitest bugs from Core:Testing to Testing:Mochitest. Filter on MochitestMassMove to ignore.
Component: Testing → Mochitest
Product: Core → Testing
QA Contact: testing → mochitest
You need to log in before you can comment on or make changes to this bug.