[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