Page MenuHomePhabricator

5 minute timeout for tests
ClosedPublic

Authored by chaoren on Nov 21 2014, 3:49 PM.

Details

Summary

Some of the test will hang when run. This lets the hanging tests die, and lets you get on with your life.

Diff Detail

Event Timeline

chaoren retitled this revision from to 5 minute timeout for tests.
chaoren updated this object.
chaoren edited the test plan for this revision. (Show Details)
chaoren added a reviewer: vharron.
chaoren added a subscriber: Unknown Object (MLST).
emaste added a subscriber: emaste.Nov 21 2014, 4:03 PM
emaste added inline comments.
test/dosep.py
15

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.

zturner added inline comments.
test/dosep.py
15

Certainly won't work on Windows. We should find another way to do this.

sas added a subscriber: sas.Dec 1 2014, 10:58 AM
chaoren updated this revision to Diff 16897.Dec 3 2014, 4:14 PM

Only enable for linux for now, since the problem doesn't affect OS X and Windows (yet). Also marks which tests timed out.

zturner accepted this revision.Dec 3 2014, 4:32 PM
zturner added a reviewer: zturner.
zturner added inline comments.
test/dosep.py
54

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

This revision is now accepted and ready to land.Dec 3 2014, 4:32 PM

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
https://docs.python.org/2/library/subprocess.html#module-subprocess.

There are other uses of the os module, so it can't be removed either.

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

Anyway, LGTM

emaste added inline comments.Dec 3 2014, 4:53 PM
test/dosep.py
34

please enable for FreeBSD too

chaoren updated this revision to Diff 16906.EditedDec 3 2014, 6:41 PM
chaoren edited edge metadata.

Enabling for FreeBSD as well.

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.

emaste edited edge metadata.Dec 4 2014, 10:33 AM

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 think it'll be better if we just try timeout, then gtimeout, and default
to the original if neither exists. Do you know if there are any targets
other than Windows with a timeout command that does something completely
different http://ss64.com/nt/timeout.html?

chaoren edited edge metadata.

Timeout if command exists.

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

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.

chaoren updated this revision to Diff 16956.Dec 4 2014, 3:03 PM

Move timeout magic number into try_timeout. Windows shouldn't have to deal with it anymore.

looks good.

vharron closed this revision.Dec 4 2014, 4:34 PM