[PATCH] D14706: [lit] Implement support of per test timeout in lit.

[Jonathan Roelofs via llvm-commits]

jroelofs added inline comments.

================
Comment at: utils/lit/tests/Inputs/per_test_timeout/slow.py:8
@@ +7,2 @@
+print("slow program")
+time.sleep(6)
----------------
delcypher wrote:
> jroelofs wrote:
> > delcypher wrote:
> > > jroelofs wrote:
> > > > delcypher wrote:
> > > > > If I did `lit.py tests/Inputs/per_test_timeout/` lit would try to run the `.py` scripts (in addition to the `.txt`) which would hang. 
> > > > > 
> > > > > I've changed `lit.cfg` just to run the `.txt` scripts. I was mistaken about needing `lit.cfg` trickery here.
> > > > > I thought that would also prevent the `.py` scripts from being in the `.txt` test cases that invoke lit (which is why I mentioned the `lit.cfg` trickery) but if you manually specify the test case names it doesn't seem to matter that `.py`` isn't in `config.suffixes`.
> > > > > 
> > > > > The latest patch I just uploaded should address this.
> > > > Ohh, I see what you were running into now. I don't think we have to worry about running lit on that particular test directory without the requisite `--timeout` flag.
> > > > 
> > > > > I've changed lit.cfg just to run the .txt scripts.
> > > > 
> > > > Running `lit.py tests/Inputs/per_test_timeout/` **should** hang, but running `lit.py tests/Inputs/per_test_timeout/ --timeout=1` should not.
> > > If this is the case why did the patch you sent to post with the subject `[lit] RFC: Per test timeout` have `timeout.py` invoke lit inside lit with the `--timeout` flag? If you expect the caller to use `--timeout=` then having an additional test case that invokes lit on the other test cases with that flag seems redundant.
> > > 
> > > In my current implementation the "lit invoking lit" (`timeout_*.txt` tests) tests are not redundant because they invoke lit in three different ways
> > > 
> > > * Use internal shell, set timeout on command line
> > > * Use external shell, set timeout on command line
> > > * Use internal shell, set timeout in configuration file (i.e. don't use command line flag)
> > > 
> > > I can revert the small change I made to `lit.py` but this begs the question
> > > 
> > > * Who is running these tests?
> > > * How does the person running these tests know how to run them correctly and interpret their results?
> > > If this is the case why did the patch you sent to post with the subject [lit] RFC: Per test timeout have timeout.py invoke lit inside lit with the --timeout flag? If you expect the caller to use --timeout= then having an additional test case that invokes lit on the other test cases with that flag seems redundant.
> > 
> > Because that's how you're supposed to test lit features? Each of the python files directly in `path/to/llvm/utils/lit/tests` is itself a testcase for lit (some (all?) of which have their own testsuite in the Inputs folder), and that's where I expect the caller to know to have the `--timeout=` arg.  Let me know if I can clarify this any better... it's a little hard to explain.
> > 
> > > In my current implementation the "lit invoking lit" (timeout_*.txt tests) tests are not redundant because they invoke lit in three different ways
> > 
> > > Use internal shell, set timeout on command line
> > > Use external shell, set timeout on command line
> > > Use internal shell, set timeout in configuration file (i.e. don't use command line flag)
> > 
> > Each of those three ways of invoking lit to test this feature should be its own file in `path/to/llvm/utils/lit/tests`... i.e. move them up one directory.
> > 
> > > Who is running these tests?
> > 
> > People who make changes to lit should be running them. It'd be ideal if they got run as part of llvm's `make check`, but that isn't the case currently.  I've been meaning to put in a patch for this, so thanks for reminding me.
> > 
> > > How does the person running these tests know how to run them correctly and interpret their results?
> > 
> > The correct way to run the lit testsuite for lit, is to run `lit.py path/to/llvm/utils/lit/tests`. The results are inretpreted by the top-level testsuite's tests, and if lit is not broken, they should all pass.
> Thanks for clarifying. I had completely misunderstood how the tests were meant to be run (and consequently how they were meant to be written).
> 
> I'll try to rewrite the tests.
Ok, cool... glad we're on the same page now.

One thing I just thought of: you'll need to set up the top-level tests such that they detect whether `import psutil` fails, so they can decide whether to test `--timeout=` at all.  I'm not sure what the best way to do that would be, but here's an idea to start from: set an 'available feature' based on the presence of psutil, and then add `REQUIRES: python-psutil` in them.


http://reviews.llvm.org/D14706