[PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[John Snow]

Instead of using a hardcoded timeout, just rely on Avocado's built-in
test case timeout. This helps avoid timeout issues on machines where 60
seconds is not sufficient.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/avocado/acpi-bits.py | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py
index 8745a58a766..ac13e22dc93 100644
--- a/tests/avocado/acpi-bits.py
+++ b/tests/avocado/acpi-bits.py
@@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self):
         self._vm.launch()
         # biosbits has been configured to run all the specified test suites
         # in batch mode and then automatically initiate a vm shutdown.
-        # sleep for maximum of one minute
-        max_sleep_time = time.monotonic() + 60
-        while self._vm.is_running() and time.monotonic() < max_sleep_time:
-            time.sleep(1)
-
-        self.assertFalse(time.monotonic() > max_sleep_time,
-                         'The VM seems to have failed to shutdown in time')
-
+        # Rely on avocado's unit test timeout.
+        self._vm.wait(timeout=None)
         self.parse_log()
-- 
2.37.3

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[Alex Bennée]

John Snow <jsnow@redhat.com> writes:

> Instead of using a hardcoded timeout, just rely on Avocado's built-in
> test case timeout. This helps avoid timeout issues on machines where 60
> seconds is not sufficient.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/avocado/acpi-bits.py | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py
> index 8745a58a766..ac13e22dc93 100644
> --- a/tests/avocado/acpi-bits.py
> +++ b/tests/avocado/acpi-bits.py
> @@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self):
>          self._vm.launch()
>          # biosbits has been configured to run all the specified test suites
>          # in batch mode and then automatically initiate a vm shutdown.
> -        # sleep for maximum of one minute
> -        max_sleep_time = time.monotonic() + 60
> -        while self._vm.is_running() and time.monotonic() < max_sleep_time:
> -            time.sleep(1)
> -
> -        self.assertFalse(time.monotonic() > max_sleep_time,
> -                         'The VM seems to have failed to shutdown in time')
> -

We might want some wait for consoles as well depending on what is output
during the run.


> +        # Rely on avocado's unit test timeout.
> +        self._vm.wait(timeout=None)
>          self.parse_log()


-- 
Alex Bennée

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]

On Tue, Nov 15, 2022, 5:48 PM Alex Bennée <alex.bennee@linaro.org> wrote:

>
> John Snow <jsnow@redhat.com> writes:
>
> > Instead of using a hardcoded timeout, just rely on Avocado's built-in
> > test case timeout. This helps avoid timeout issues on machines where 60
> > seconds is not sufficient.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  tests/avocado/acpi-bits.py | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py
> > index 8745a58a766..ac13e22dc93 100644
> > --- a/tests/avocado/acpi-bits.py
> > +++ b/tests/avocado/acpi-bits.py
> > @@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self):
> >          self._vm.launch()
> >          # biosbits has been configured to run all the specified test
> suites
> >          # in batch mode and then automatically initiate a vm shutdown.
> > -        # sleep for maximum of one minute
> > -        max_sleep_time = time.monotonic() + 60
> > -        while self._vm.is_running() and time.monotonic() <
> max_sleep_time:
> > -            time.sleep(1)
> > -
> > -        self.assertFalse(time.monotonic() > max_sleep_time,
> > -                         'The VM seems to have failed to shutdown in
> time')
> > -
>
> We might want some wait for consoles as well depending on what is output
> during the run.
>

Ah! there's the catch I was forgetting. I subbed this in and it appeared to
work, so I thought I was home free.

OK, back to the drawing board...


>
> > +        # Rely on avocado's unit test timeout.
> > +        self._vm.wait(timeout=None)
> >          self.parse_log()
>
>
> --
> Alex Bennée
>
>

[-- Attachment #2: Type: text/html, Size: 2691 bytes --]

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[Ani Sinha]

On Wed, Nov 16, 2022 at 2:58 AM John Snow <jsnow@redhat.com> wrote:
>
> Instead of using a hardcoded timeout, just rely on Avocado's built-in
> test case timeout. This helps avoid timeout issues on machines where 60
> seconds is not sufficient.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Ani Sinha <ani@anisinha.ca>

> ---
>  tests/avocado/acpi-bits.py | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py
> index 8745a58a766..ac13e22dc93 100644
> --- a/tests/avocado/acpi-bits.py
> +++ b/tests/avocado/acpi-bits.py
> @@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self):
>          self._vm.launch()
>          # biosbits has been configured to run all the specified test suites
>          # in batch mode and then automatically initiate a vm shutdown.
> -        # sleep for maximum of one minute
> -        max_sleep_time = time.monotonic() + 60
> -        while self._vm.is_running() and time.monotonic() < max_sleep_time:
> -            time.sleep(1)
> -
> -        self.assertFalse(time.monotonic() > max_sleep_time,
> -                         'The VM seems to have failed to shutdown in time')
> -
> +        # Rely on avocado's unit test timeout.
> +        self._vm.wait(timeout=None)

I think this is fine. This just waits until the VM is shutdown on its
own and relies on the avocado framework to timeout if it doesn't. We
do not need to look into the console. The test issues a shutdown from
the VM itself once its done with the batch operations.

>          self.parse_log()
> --
> 2.37.3
>

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[Ani Sinha]

On Wed, Nov 16, 2022 at 4:17 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> John Snow <jsnow@redhat.com> writes:
>
> > Instead of using a hardcoded timeout, just rely on Avocado's built-in
> > test case timeout. This helps avoid timeout issues on machines where 60
> > seconds is not sufficient.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  tests/avocado/acpi-bits.py | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py
> > index 8745a58a766..ac13e22dc93 100644
> > --- a/tests/avocado/acpi-bits.py
> > +++ b/tests/avocado/acpi-bits.py
> > @@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self):
> >          self._vm.launch()
> >          # biosbits has been configured to run all the specified test suites
> >          # in batch mode and then automatically initiate a vm shutdown.
> > -        # sleep for maximum of one minute
> > -        max_sleep_time = time.monotonic() + 60
> > -        while self._vm.is_running() and time.monotonic() < max_sleep_time:
> > -            time.sleep(1)
> > -
> > -        self.assertFalse(time.monotonic() > max_sleep_time,
> > -                         'The VM seems to have failed to shutdown in time')
> > -
>
> We might want some wait for consoles as well depending on what is output
> during the run.

actually I think you won't get anything on the console since grub is
not configured to use the serial console.  I tried "-serial stdio" a
while back without any output.

>
>
> > +        # Rely on avocado's unit test timeout.
> > +        self._vm.wait(timeout=None)
> >          self.parse_log()
>
>
> --
> Alex Bennée

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[Alex Bennée]

Ani Sinha <ani@anisinha.ca> writes:

> On Wed, Nov 16, 2022 at 4:17 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Instead of using a hardcoded timeout, just rely on Avocado's built-in
>> > test case timeout. This helps avoid timeout issues on machines where 60
>> > seconds is not sufficient.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  tests/avocado/acpi-bits.py | 10 ++--------
>> >  1 file changed, 2 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py
>> > index 8745a58a766..ac13e22dc93 100644
>> > --- a/tests/avocado/acpi-bits.py
>> > +++ b/tests/avocado/acpi-bits.py
>> > @@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self):
>> >          self._vm.launch()
>> >          # biosbits has been configured to run all the specified test suites
>> >          # in batch mode and then automatically initiate a vm shutdown.
>> > -        # sleep for maximum of one minute
>> > -        max_sleep_time = time.monotonic() + 60
>> > -        while self._vm.is_running() and time.monotonic() < max_sleep_time:
>> > -            time.sleep(1)
>> > -
>> > -        self.assertFalse(time.monotonic() > max_sleep_time,
>> > -                         'The VM seems to have failed to shutdown in time')
>> > -
>>
>> We might want some wait for consoles as well depending on what is output
>> during the run.
>
> actually I think you won't get anything on the console since grub is
> not configured to use the serial console.  I tried "-serial stdio" a
> while back without any output.

Grub is certainly capable of serial output but I think the grub.cfg
needs changes to support that. It would definitely be an improvement if
we could enable serial output because currently the test is totally mute
while running which is unlike every other test in avocado.

>
>>
>>
>> > +        # Rely on avocado's unit test timeout.
>> > +        self._vm.wait(timeout=None)
>> >          self.parse_log()
>>
>>
>> --
>> Alex Bennée


-- 
Alex Bennée

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[Ani Sinha]

On Wed, Nov 16, 2022 at 3:07 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Ani Sinha <ani@anisinha.ca> writes:
>
> > On Wed, Nov 16, 2022 at 4:17 AM Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >>
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > Instead of using a hardcoded timeout, just rely on Avocado's built-in
> >> > test case timeout. This helps avoid timeout issues on machines where 60
> >> > seconds is not sufficient.
> >> >
> >> > Signed-off-by: John Snow <jsnow@redhat.com>
> >> > ---
> >> >  tests/avocado/acpi-bits.py | 10 ++--------
> >> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >> >
> >> > diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py
> >> > index 8745a58a766..ac13e22dc93 100644
> >> > --- a/tests/avocado/acpi-bits.py
> >> > +++ b/tests/avocado/acpi-bits.py
> >> > @@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self):
> >> >          self._vm.launch()
> >> >          # biosbits has been configured to run all the specified test suites
> >> >          # in batch mode and then automatically initiate a vm shutdown.
> >> > -        # sleep for maximum of one minute
> >> > -        max_sleep_time = time.monotonic() + 60
> >> > -        while self._vm.is_running() and time.monotonic() < max_sleep_time:
> >> > -            time.sleep(1)
> >> > -
> >> > -        self.assertFalse(time.monotonic() > max_sleep_time,
> >> > -                         'The VM seems to have failed to shutdown in time')
> >> > -
> >>
> >> We might want some wait for consoles as well depending on what is output
> >> during the run.
> >
> > actually I think you won't get anything on the console since grub is
> > not configured to use the serial console.  I tried "-serial stdio" a
> > while back without any output.
>
> Grub is certainly capable of serial output but I think the grub.cfg
> needs changes to support that. It would definitely be an improvement if
> we could enable serial output because currently the test is totally mute
> while running which is unlike every other test in avocado.

sounds reasonable. bits seems to have its own way to set up grub
serial port redirect and I tried something quickly but it didn't quite
work. Need to spend more time looking at it.

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[Ani Sinha]

On Wed, Nov 16, 2022 at 6:02 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Wed, Nov 16, 2022 at 3:07 PM Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> >
> > Ani Sinha <ani@anisinha.ca> writes:
> >
> > > On Wed, Nov 16, 2022 at 4:17 AM Alex Bennée <alex.bennee@linaro.org> wrote:
> > >>
> > >>
> > >> John Snow <jsnow@redhat.com> writes:
> > >>
> > >> > Instead of using a hardcoded timeout, just rely on Avocado's built-in
> > >> > test case timeout. This helps avoid timeout issues on machines where 60
> > >> > seconds is not sufficient.
> > >> >
> > >> > Signed-off-by: John Snow <jsnow@redhat.com>
> > >> > ---
> > >> >  tests/avocado/acpi-bits.py | 10 ++--------
> > >> >  1 file changed, 2 insertions(+), 8 deletions(-)
> > >> >
> > >> > diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py
> > >> > index 8745a58a766..ac13e22dc93 100644
> > >> > --- a/tests/avocado/acpi-bits.py
> > >> > +++ b/tests/avocado/acpi-bits.py
> > >> > @@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self):
> > >> >          self._vm.launch()
> > >> >          # biosbits has been configured to run all the specified test suites
> > >> >          # in batch mode and then automatically initiate a vm shutdown.
> > >> > -        # sleep for maximum of one minute
> > >> > -        max_sleep_time = time.monotonic() + 60
> > >> > -        while self._vm.is_running() and time.monotonic() < max_sleep_time:
> > >> > -            time.sleep(1)
> > >> > -
> > >> > -        self.assertFalse(time.monotonic() > max_sleep_time,
> > >> > -                         'The VM seems to have failed to shutdown in time')
> > >> > -
> > >>
> > >> We might want some wait for consoles as well depending on what is output
> > >> during the run.
> > >
> > > actually I think you won't get anything on the console since grub is
> > > not configured to use the serial console.  I tried "-serial stdio" a
> > > while back without any output.
> >
> > Grub is certainly capable of serial output but I think the grub.cfg
> > needs changes to support that. It would definitely be an improvement if
> > we could enable serial output because currently the test is totally mute
> > while running which is unlike every other test in avocado.
>
> sounds reasonable. bits seems to have its own way to set up grub
> serial port redirect and I tried something quickly but it didn't quite
> work. Need to spend more time looking at it.

just when I wrote this, it seems my hack finally worked! Will need
more testing before pushing.

commit 58513f19ac7b537da0769a732ff0d93d6d93d3b0 (HEAD -> qemu-bits)
Author: Ani Sinha <ani@anisinha.ca>
Date:   Wed Nov 16 17:37:51 2022 +0530

    serial port redirection test

    Signed-off-by: Ani Sinha <ani@anisinha.ca>

diff --git a/python/init.py b/python/init.py
index 8fde344..1e36d51 100644
--- a/python/init.py
+++ b/python/init.py
@@ -85,7 +85,7 @@ def early_init():
         except Exception as e:
             print "Error parsing Serial Port Console Redirect (SPCR) table:"
             print e
-
+    serial_cmd = "serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1"
     with import_annotation("os"):
         import os
     with init_annotation("os"):

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 2599 bytes --]

On Tue, Nov 15, 2022, 10:24 PM Ani Sinha <ani@anisinha.ca> wrote:

> On Wed, Nov 16, 2022 at 2:58 AM John Snow <jsnow@redhat.com> wrote:
> >
> > Instead of using a hardcoded timeout, just rely on Avocado's built-in
> > test case timeout. This helps avoid timeout issues on machines where 60
> > seconds is not sufficient.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Reviewed-by: Ani Sinha <ani@anisinha.ca>
>

Alex's critique is valid, though: the way vm.wait() works is to
immediately  terminate the serial console connection as it prepares for the
VM to shut down. I forgot about this.

(For historical reasons, it does this to avoid deadlocks when the pipe
fills.)

I think we definitely do want to make sure we watch the console *while* we
wait for it to shut down, which is not a feature QEMUMachine really offers
right now in a meaningful way.

I need to make some more drastic changes to machine.py, but in the meantime
I can revise this patch to do something a bit smarter so we get console
logging while we wait. This is a use case worth supporting.

(Thanks for writing new and interesting tests!)


> > ---
> >  tests/avocado/acpi-bits.py | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py
> > index 8745a58a766..ac13e22dc93 100644
> > --- a/tests/avocado/acpi-bits.py
> > +++ b/tests/avocado/acpi-bits.py
> > @@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self):
> >          self._vm.launch()
> >          # biosbits has been configured to run all the specified test
> suites
> >          # in batch mode and then automatically initiate a vm shutdown.
> > -        # sleep for maximum of one minute
> > -        max_sleep_time = time.monotonic() + 60
> > -        while self._vm.is_running() and time.monotonic() <
> max_sleep_time:
> > -            time.sleep(1)
> > -
> > -        self.assertFalse(time.monotonic() > max_sleep_time,
> > -                         'The VM seems to have failed to shutdown in
> time')
> > -
> > +        # Rely on avocado's unit test timeout.
> > +        self._vm.wait(timeout=None)
>
> I think this is fine. This just waits until the VM is shutdown on its
> own and relies on the avocado framework to timeout if it doesn't. We
> do not need to look into the console. The test issues a shutdown from
> the VM itself once its done with the batch operations.


Still, if it fails, we want to see the output, right? It's very frustrating
if it doesn't, especially in an automated pipeline.


> >          self.parse_log()
> > --
> > 2.37.3
> >
>
>

[-- Attachment #2: Type: text/html, Size: 4108 bytes --]

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[Ani Sinha]

[-- Attachment #1: Type: text/plain, Size: 3020 bytes --]

On Wed, Nov 16, 2022 at 11:31 PM John Snow <jsnow@redhat.com> wrote:

>
>
> On Tue, Nov 15, 2022, 10:24 PM Ani Sinha <ani@anisinha.ca> wrote:
>
>> On Wed, Nov 16, 2022 at 2:58 AM John Snow <jsnow@redhat.com> wrote:
>> >
>> > Instead of using a hardcoded timeout, just rely on Avocado's built-in
>> > test case timeout. This helps avoid timeout issues on machines where 60
>> > seconds is not sufficient.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> Reviewed-by: Ani Sinha <ani@anisinha.ca>
>>
>
> Alex's critique is valid, though: the way vm.wait() works is to
> immediately  terminate the serial console connection as it prepares for the
> VM to shut down. I forgot about this.
>
> (For historical reasons, it does this to avoid deadlocks when the pipe
> fills.)
>
> I think we definitely do want to make sure we watch the console *while* we
> wait for it to shut down, which is not a feature QEMUMachine really offers
> right now in a meaningful way.
>

Maybe  we can push your current patch while we consider these console
logging enhancements for the next release window. Console logging woikd
require some changes in bits and some more testing. I'm not sure if I'll
have time for it immediately at present.


> I need to make some more drastic changes to machine.py, but in the
> meantime I can revise this patch to do something a bit smarter so we get
> console logging while we wait. This is a use case worth supporting.
>
> (Thanks for writing new and interesting tests!)
>
>
>> > ---
>> >  tests/avocado/acpi-bits.py | 10 ++--------
>> >  1 file changed, 2 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py
>> > index 8745a58a766..ac13e22dc93 100644
>> > --- a/tests/avocado/acpi-bits.py
>> > +++ b/tests/avocado/acpi-bits.py
>> > @@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self):
>> >          self._vm.launch()
>> >          # biosbits has been configured to run all the specified test
>> suites
>> >          # in batch mode and then automatically initiate a vm shutdown.
>> > -        # sleep for maximum of one minute
>> > -        max_sleep_time = time.monotonic() + 60
>> > -        while self._vm.is_running() and time.monotonic() <
>> max_sleep_time:
>> > -            time.sleep(1)
>> > -
>> > -        self.assertFalse(time.monotonic() > max_sleep_time,
>> > -                         'The VM seems to have failed to shutdown in
>> time')
>> > -
>> > +        # Rely on avocado's unit test timeout.
>> > +        self._vm.wait(timeout=None)
>>
>> I think this is fine. This just waits until the VM is shutdown on its
>> own and relies on the avocado framework to timeout if it doesn't. We
>> do not need to look into the console. The test issues a shutdown from
>> the VM itself once its done with the batch operations.
>
>
> Still, if it fails, we want to see the output, right? It's very
> frustrating if it doesn't, especially in an automated pipeline.
>
>
>> >          self.parse_log()
>> > --
>> > 2.37.3
>> >
>>
>>

[-- Attachment #2: Type: text/html, Size: 4999 bytes --]

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[Ani Sinha]

On Thu, Nov 17, 2022 at 5:24 AM Ani Sinha <ani@anisinha.ca> wrote:
>
>
>
> On Wed, Nov 16, 2022 at 11:31 PM John Snow <jsnow@redhat.com> wrote:
>>
>>
>>
>> On Tue, Nov 15, 2022, 10:24 PM Ani Sinha <ani@anisinha.ca> wrote:
>>>
>>> On Wed, Nov 16, 2022 at 2:58 AM John Snow <jsnow@redhat.com> wrote:
>>> >
>>> > Instead of using a hardcoded timeout, just rely on Avocado's built-in
>>> > test case timeout. This helps avoid timeout issues on machines where 60
>>> > seconds is not sufficient.
>>> >
>>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>> Reviewed-by: Ani Sinha <ani@anisinha.ca>
>>
>>
>> Alex's critique is valid, though: the way vm.wait() works is to immediately  terminate the serial console connection as it prepares for the VM to shut down. I forgot about this.
>>
>> (For historical reasons, it does this to avoid deadlocks when the pipe fills.)
>>
>> I think we definitely do want to make sure we watch the console *while* we wait for it to shut down, which is not a feature QEMUMachine really offers right now in a meaningful way.
>
>
> Maybe  we can push your current patch while we consider these console logging enhancements for the next release window. Console logging woikd require some changes in bits and some more testing. I'm not sure if I'll have time for it immediately at present.
>
>>
>> I need to make some more drastic changes to machine.py, but in the meantime I can revise this patch to do something a bit smarter so we get console logging while we wait. This is a use case worth supporting.
>>
>> (Thanks for writing new and interesting tests!)

Spoke to John on IRC. Seems this patch using vm.wait() is safe for
this release as I do not use the console o/p in the test and do not
call vm.set_console().
When we enable the console output, some additional work will need to
be done for the QemuMachine library to make sure we avoid races when
we call vm.wait() with _early_cleanup().

>>
>>>
>>> > ---
>>> >  tests/avocado/acpi-bits.py | 10 ++--------
>>> >  1 file changed, 2 insertions(+), 8 deletions(-)
>>> >
>>> > diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py
>>> > index 8745a58a766..ac13e22dc93 100644
>>> > --- a/tests/avocado/acpi-bits.py
>>> > +++ b/tests/avocado/acpi-bits.py
>>> > @@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self):
>>> >          self._vm.launch()
>>> >          # biosbits has been configured to run all the specified test suites
>>> >          # in batch mode and then automatically initiate a vm shutdown.
>>> > -        # sleep for maximum of one minute
>>> > -        max_sleep_time = time.monotonic() + 60
>>> > -        while self._vm.is_running() and time.monotonic() < max_sleep_time:
>>> > -            time.sleep(1)
>>> > -
>>> > -        self.assertFalse(time.monotonic() > max_sleep_time,
>>> > -                         'The VM seems to have failed to shutdown in time')
>>> > -
>>> > +        # Rely on avocado's unit test timeout.
>>> > +        self._vm.wait(timeout=None)
>>>
>>> I think this is fine. This just waits until the VM is shutdown on its
>>> own and relies on the avocado framework to timeout if it doesn't. We
>>> do not need to look into the console. The test issues a shutdown from
>>> the VM itself once its done with the batch operations.
>>
>>
>> Still, if it fails, we want to see the output, right? It's very frustrating if it doesn't, especially in an automated pipeline.
>>
>>>
>>> >          self.parse_log()
>>> > --
>>> > 2.37.3
>>> >
>>>

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[Ani Sinha]

On Wed, Nov 16, 2022 at 8:54 AM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Wed, Nov 16, 2022 at 2:58 AM John Snow <jsnow@redhat.com> wrote:
> >
> > Instead of using a hardcoded timeout, just rely on Avocado's built-in
> > test case timeout. This helps avoid timeout issues on machines where 60
> > seconds is not sufficient.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Reviewed-by: Ani Sinha <ani@anisinha.ca>
>
> > ---
> >  tests/avocado/acpi-bits.py | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)

Thomas, since you are picking up my other patch, maybe you want to
also pick this one up as well if you have not sent out your PR
already?

> >
> > diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py
> > index 8745a58a766..ac13e22dc93 100644
> > --- a/tests/avocado/acpi-bits.py
> > +++ b/tests/avocado/acpi-bits.py
> > @@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self):
> >          self._vm.launch()
> >          # biosbits has been configured to run all the specified test suites
> >          # in batch mode and then automatically initiate a vm shutdown.
> > -        # sleep for maximum of one minute
> > -        max_sleep_time = time.monotonic() + 60
> > -        while self._vm.is_running() and time.monotonic() < max_sleep_time:
> > -            time.sleep(1)
> > -
> > -        self.assertFalse(time.monotonic() > max_sleep_time,
> > -                         'The VM seems to have failed to shutdown in time')
> > -
> > +        # Rely on avocado's unit test timeout.
> > +        self._vm.wait(timeout=None)
>
> I think this is fine. This just waits until the VM is shutdown on its
> own and relies on the avocado framework to timeout if it doesn't. We
> do not need to look into the console. The test issues a shutdown from
> the VM itself once its done with the batch operations.
>
> >          self.parse_log()
> > --
> > 2.37.3
> >

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[Thomas Huth]

On 18/11/2022 05.05, Ani Sinha wrote:
> On Wed, Nov 16, 2022 at 8:54 AM Ani Sinha <ani@anisinha.ca> wrote:
>>
>> On Wed, Nov 16, 2022 at 2:58 AM John Snow <jsnow@redhat.com> wrote:
>>>
>>> Instead of using a hardcoded timeout, just rely on Avocado's built-in
>>> test case timeout. This helps avoid timeout issues on machines where 60
>>> seconds is not sufficient.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> Reviewed-by: Ani Sinha <ani@anisinha.ca>
>>
>>> ---
>>>   tests/avocado/acpi-bits.py | 10 ++--------
>>>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> Thomas, since you are picking up my other patch, maybe you want to
> also pick this one up as well if you have not sent out your PR
> already?

Sorry, too late, it's already merged:

https://gitlab.com/qemu-project/qemu/-/commit/1b7a07c4414323d985e89c4e7

  Thomas

Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[Ani Sinha]

On Fri, Nov 18, 2022 at 1:33 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 18/11/2022 05.05, Ani Sinha wrote:
> > On Wed, Nov 16, 2022 at 8:54 AM Ani Sinha <ani@anisinha.ca> wrote:
> >>
> >> On Wed, Nov 16, 2022 at 2:58 AM John Snow <jsnow@redhat.com> wrote:
> >>>
> >>> Instead of using a hardcoded timeout, just rely on Avocado's built-in
> >>> test case timeout. This helps avoid timeout issues on machines where 60
> >>> seconds is not sufficient.
> >>>
> >>> Signed-off-by: John Snow <jsnow@redhat.com>
> >>
> >> Reviewed-by: Ani Sinha <ani@anisinha.ca>
> >>
> >>> ---
> >>>   tests/avocado/acpi-bits.py | 10 ++--------
> >>>   1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > Thomas, since you are picking up my other patch, maybe you want to
> > also pick this one up as well if you have not sent out your PR
> > already?
>
> Sorry, too late, it's already merged:
>
> https://gitlab.com/qemu-project/qemu/-/commit/1b7a07c4414323d985e89c4e7

Ok if you generate another PR with simple misc fixes, please let me know.