Re: [kvm-unit-tests PATCH 3/3] add timeout support

["Radim Krčmář" <rkrcmar@redhat.com>]

2015-12-21 13:45-0600, Andrew Jones:
> On Mon, Dec 21, 2015 at 06:04:20PM +0100, Radim Krčmář wrote:
>> 2015-12-17 14:10-0600, Andrew Jones:
>> > diff --git a/run_tests.sh b/run_tests.sh
>> > @@ -21,6 +21,7 @@ function run()
>> > +    local timeout="${9:-$TIMEOUT}"
>> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
>> > @@ -97,8 +98,12 @@ if [ "\$QEMU" ]; then
>> > +if [ "$timeout" ]; then
>> > +	timeout_cmd='timeout --foreground $timeout'
>> 
>> Both would be nicer if they took the TIMEOUT variable as an override.
> 
> Everything already takes TIMEOUT as an override, i.e.
> 
> TIMEOUT=3 ./run_tests.sh
> 
> and
> 
> TIMEOUT=3 arm/run arm/test.flat
> 
> will both already set a timeout for any test that didn't have a timeout
> set in unittests.cfg, or wasn't run with run()/unittests.cfg.

Tests made with mkstandalone.sh ignore the TIMEOUT variable ...

>                                                               Or, did
> you mean that you'd prefer TIMEOUT to override the timeout in
> unittests.cfg?

... and yes, I think that we could have a
- global timeout for all tests.  Something far longer than any tests
  should take (2 minutes?).  To automatically handle random hangs.

- per-test timeout in unittests.cfg.  When the test is known to timeout
  often and the usual time to fail is much shorter than the global
  default.  (Shouldn't be used much.)

- TIMEOUT variable.  It has to override the global timeout and I think
  that if we are ever going to use it, it's because we want something
  weird.  Like using `TIMEOUT=0 ./run_tests.sh` to disable all
  timeouts, prolonging/shortening timeouts because of a special
  configuration, ...
  Because we should improve our defaults otherwise.

  (I'd probably allow something as evil as `eval`ing the TIMEOUT, for
   unlikely stuff like TIMEOUT='$(( ${timeout:-10} / 2 ))')

>                That does make some sense, in the case the one in the
> config is longer than desired, however it also makes sense the way I
> have it now when the one in the config is shorter than TIMEOUT (the
> fallback default). I think I like it this way better.

Ok, the difference was negligible to begin with.

>> We already don't do that for accel and the patch seems ok in other
>> regards,
> 
> Hmm, for accel I see a need for a patch allowing us to do
> 
> ACCEL=?? ./run_tests.sh

Btw. why do we have ACCEL when the project is *kvm*_unit_tests?

> as I already have for TIMEOUT. Also, for both I should add a
> mkstandalone patch allowing
> 
> TIMEOUT=? ACCEL=? make standalone

I'd also handle TIMEOUT/ACCEL in resulting standalone tests.