[Lldb-commits] [PATCH] D13124: test runner: switch to pure-Python timeout mechanism

[Todd Fiala via lldb-commits]

tfiala added inline comments.

================
Comment at: test/test_runner/lib/process_control.py:214
@@ +213,3 @@
+        """
+        raise Exception("platform needs to implement")
+
----------------
zturner wrote:
> Should this return `False` now that this is not supported on Windows, or do you think this is still ok?  It's probably just of theoretical concern.
Correct - this is not valid if soft terminate isn't supported.  It should return false if soft terminate is not supported, and raise if it is supported but not overriddden, I think.

================
Comment at: test/test_runner/lib/process_control.py:229
@@ +228,3 @@
+        """
+        raise Exception("platform needs to implement")
+
----------------
zturner wrote:
> Change this to:
> 
>     # It's not actually documented what return code Popen.terminate()
>     # passes to TerminateProcess.  Experimentation shows that it's always
>     # 1, but there's no guarantee that's true.  Hopefully this check
>     # works well enough to be sufficient.
>     return returncode != 0
> 
> With these changes I get the following output:
> 
>     d:\src\llvm\tools\lldb\test\test_runner\test>python process_control_tests.py
>     ....ss
>     ----------------------------------------------------------------------
>     Ran 6 tests in 1.208s
> 
>     OK (skipped=2)
> 
> Does that look right to you?
> It's not actually documented what return code Popen.terminate()
passes to TerminateProcess. Experimentation shows that it's always

This is checking the returncode as returned by wait() on the process.  So we're checking the return value of popen_object.wait() here (which should be identical to the popen_object.returncode() immediately after wait() succeeds).

I think for the Windows case, we override this in the WindowsProcessHelper and have that return "True".  (All kills will be hard terminate in this case).  This is really only used by the test cases right now.  Alternatively, we can change it to pass in the popen object, and have us write in a "did a hard kill on this" member variable on the Windows implementation.  Then, we just return "True" if that is set, False otherwise.

For Windows, though, I can put in the returncode != 0.  If the hard kill test works with that, it is fine.  But it will also pass if a process returns a non-zero exit code I think (i.e. return from main() is non-zero).


http://reviews.llvm.org/D13124