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

[Andrew Jones <drjones@redhat.com>]

On Tue, Dec 22, 2015 at 07:02:21PM +0100, Radim Krčmář wrote:
> 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.

OK, I'll do something allowing us to easily enable a long default
timeout.

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

I'd prefer to avoid the evil^Weval stuff... And the timeout duration can
already be a floating point.

> 
> >                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?

arm tests are sometimes tcg only. Hey, we'll take what we get for
arm, as we're sadly missing everything...

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

OK

Thanks,
drew

> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html