Some of the test will hang when run. This lets the hanging tests die, and lets you get on with your life.
The bots run under an overall timeout, but it's much more valuable to catch single tests that hang, so I think this is a good idea. We'll need to check that timeout is available first though. timeout is non-standard - it's on FreeBSD & Linux, but not OS X or I suspect other operating systems.
This appears to be equivalent to the previous code, but using subprocess instead of os. If this is the only reason in the entire file for importing subprocess, then can we change it back to os.system to avoid importing the extra module? Or alternatively, if the previous call to os.system was the only reason in the entire file for importing os, then keep the subprocess.call but remove the import of os.system.
Other than that, LGTM
os.system didn't return the correct exit code (124). And I believe
subprocess.call is intended to replace os.system anyway according to this
Fair enough, no need to change anything then.
Weird though that os.system doesn't return the same value, I always thought
os.system(x) was equivalent to subprocess.call(x, shell=True).
Is GNU coreutils already a requirement on FreeBSD or OS X? Because I don't
think timeout exists on FreeBSD, but gtimeout does. Same goes for OS X.
Ah, a mistake on my part. We have a BSD licensed timeout implementation in FreeBSD-current, but it is not yet in the stable/10 branch or in the 10.1 release.
I guess there's a possibility a test on Windows could return 124 and mean
something completely different than timeout. Also 124 is a magic number
that people won't understand unless they check the man page for timeout and
Can you move the return code checking into the try_timeout function, and
then return an enum like eTimedOut, eFailed, eSucceeded?
Other than that, looks good.