[PATCH] Add per-test timeouts to lit
Daniel Dunbar
daniel at zuster.org
Tue Dec 9 17:16:00 PST 2014
In addition to the inline comments, could you include some tests that exercise this code?
================
Comment at: utils/lit/lit/Test.py:221
@@ +220,3 @@
+
+class TimeoutException(Exception):
+ def __init__(self, duration):
----------------
I'd rather just change executeCommand to not need this than have it added here.
================
Comment at: utils/lit/lit/main.py:212
@@ -210,1 +211,3 @@
action="store", type=float, default=None)
+ group.add_option("", "--test-timeout", dest="testTimeout", metavar="N",
+ help="Maximum time to spend in any given test (in seconds)",
----------------
Can we just spell this "--timeout", it's pretty obvious. If you were worried about the conflict with --max-time I'd rather rename that one to --max-testing-time or something similar, as its not a commonly used option (and I wonder if anyone other than me ever uses it). :)
================
Comment at: utils/lit/lit/util.py:168
@@ +167,3 @@
+ timeout_timer = threading.Timer(timeout, timeout_handler, [p, timed_out])
+ timeout_timer.start()
+
----------------
Can this code be factored out into a helper class? Then you wouldn't need to use the hack of wrapping timed_out in a list just so you can modify it.
================
Comment at: utils/lit/lit/util.py:170
@@ +169,3 @@
+
+ if timed_out[0]:
+ raise Test.TimeoutException(timeout)
----------------
Technically, you shouldn't need to do this, and it is probably a good safety check *not to*.
If you kill the process, then the p.wait() below should return the status showing that the process was terminated (and if it hangs, there is a problem).
http://reviews.llvm.org/D6584
More information about the llvm-commits
mailing list