[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:
> > > 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.


http://reviews.llvm.org/D14706