Re: [PATCH 1/5] tests/functional: Add a decorator for skipping long running tests

[Thomas Huth]

On 24/01/2025 16.28, Daniel P. Berrangé wrote:
> On Fri, Jan 24, 2025 at 03:15:25PM +0100, Thomas Huth wrote:
> > Some tests have a very long runtime and might run into timeout
> > issues e.g. when QEMU has been compiled with --enable-debug.
> > Add a decorator for marking them more easily and document the
> > corresponding environment variable that is used to enable the
> > tests.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   docs/devel/testing/functional.rst        |  8 ++++++++
> >   tests/functional/qemu_test/__init__.py   |  2 +-
> >   tests/functional/qemu_test/decorators.py | 14 ++++++++++++++
> >   tests/functional/test_arm_quanta_gsj.py  |  5 +++--
> >   4 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/docs/devel/testing/functional.rst 
> > b/docs/devel/testing/functional.rst
> > index ae238ed3fc..7d9396b696 100644
> > --- a/docs/devel/testing/functional.rst
> > +++ b/docs/devel/testing/functional.rst
> > @@ -351,5 +351,13 @@ the code snippet below:
> >   Tests should not live in this state forever and should either be fixed
> >   or eventually removed.
> >   
> > +QEMU_TEST_TIMEOUT_EXPECTED
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +Tests that have a very long runtime and might run into timeout issues
> > +e.g. if the QEMU binary has been compiled with debugging options enabled.
> > +To avoid these timeout issues by default and to save some precious CPU
> > +cycles during normal testing, such tests are disabled by default unless
> > +the QEMU_TEST_TIMEOUT_EXPECTED environment variable has been set.
> > +
> >   
> >   .. _unittest: https://docs.python.org/3/library/unittest.html
> > diff --git a/tests/functional/qemu_test/__init__.py 
> > b/tests/functional/qemu_test/__init__.py
> > index da1830286d..b1a19d2a4b 100644
> > --- a/tests/functional/qemu_test/__init__.py
> > +++ b/tests/functional/qemu_test/__init__.py
> > @@ -14,7 +14,7 @@
> >   from .testcase import QemuBaseTest, QemuUserTest, QemuSystemTest
> >   from .linuxkernel import LinuxKernelTest
> >   from .decorators import skipIfMissingCommands, skipIfNotMachine, \
> > -    skipFlakyTest, skipUntrustedTest, skipBigDataTest, \
> > +    skipFlakyTest, skipUntrustedTest, skipBigDataTest, skipLongRuntime, \
>
> s/Runtime/RunningTime/, but actually in terms of naming
> convention, 'skipSlowTest' would fit better.

Ack, that sounds better, indeed.

> > diff --git a/tests/functional/qemu_test/decorators.py 
> > b/tests/functional/qemu_test/decorators.py
> > index df088bc090..8f311e5309 100644
> > --- a/tests/functional/qemu_test/decorators.py
> > +++ b/tests/functional/qemu_test/decorators.py
> > @@ -86,6 +86,20 @@ def skipBigDataTest():
> >       return skipUnless(os.getenv('QEMU_TEST_ALLOW_LARGE_STORAGE'),
> >                         'Test requires large host storage space')
> >   
> > +'''
> > +Decorator to skip execution of tests which have a really long
> > +runtime (and might e.g. time out if QEMU has been compiled with
> > +debugging enabled) unless the $QEMU_TEST_TIMEOUT_EXPECTED
> > +environment variable is set
> > +
> > +Example:
> > +
> > +  @skipLongRuntime()
> > +'''
> > +def skipLongRuntime():
> > +    return skipUnless(os.getenv('QEMU_TEST_TIMEOUT_EXPECTED'),
> > +                      'Test has a very long runtime and might time out')
> > +
>
> You're preserving the existnig env var which is good,
> but I have a little niggling desire to unify the
> naming conventions:
>
>    skipFlakyTest     -> $QEMU_TEST_ALLOW_FLAKY
>    skipUntrustedTest -> $QEMU_TEST_ALLOW_UNTRUSTED
>    skipBigDataTest   -> $QEMU_TEST_ALLOW_BIG_DATA
>    skipSlowTest      -> $QEMU_TEST_ALLOW_SLOW
>
> Could be a separate patch though if you like the idea.

I like the idea, some of the others are already starting with 
QEMU_TEST_ALLOW_, so the renaming makes sense to me, too. I'll change this 
patch in v2, but I'll leave the others for now - feel free to send a patch 
for them, if not I'll tackle them sometime later.

 Thomas