[PATCH 14/39] tests/functional: require non-NULL success_message for console wait

Alex Bennée posted 39 patches 1 day, 23 hours ago
Only 38 patches received!
[PATCH 14/39] tests/functional: require non-NULL success_message for console wait
Posted by Alex Bennée 1 day, 23 hours ago
From: Daniel P. Berrangé <berrange@redhat.com>

When waiting for expected output, the 'success_message' is a mandatory
parameter, with 'failure_message' defaulting to None.

The code has logic which indicates it was trying to cope with
'success_message' being None and 'failure_message' being non-None but
it does not appear able to actually do anything useful. The check for
'success_message is None' will break out of the loop before any check
for 'failure_message' has been performed.

IOW, for practcal purposes 'success_message' must be non-None unless
'send_string' is set. Assert this expectation and simplify the loop
logic.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20241121154218.1423005-15-berrange@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/functional/qemu_test/cmd.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/functional/qemu_test/cmd.py b/tests/functional/qemu_test/cmd.py
index 98722a9cf6..f6c4e4dda1 100644
--- a/tests/functional/qemu_test/cmd.py
+++ b/tests/functional/qemu_test/cmd.py
@@ -81,6 +81,8 @@ def is_readable_executable_file(path):
 def _console_interaction(test, success_message, failure_message,
                          send_string, keep_sending=False, vm=None):
     assert not keep_sending or send_string
+    assert success_message or send_string
+
     if vm is None:
         vm = test.vm
     console = vm.console_file
@@ -95,7 +97,7 @@ def _console_interaction(test, success_message, failure_message,
                 send_string = None # send only once
 
         # Only consume console output if waiting for something
-        if success_message is None and failure_message is None:
+        if success_message is None:
             if send_string is None:
                 break
             continue
@@ -107,7 +109,7 @@ def _console_interaction(test, success_message, failure_message,
         if not msg:
             continue
         console_logger.debug(msg)
-        if success_message is None or success_message in msg:
+        if success_message in msg:
             break
         if failure_message and failure_message in msg:
             console.close()
@@ -138,6 +140,7 @@ def interrupt_interactive_console_until_pattern(test, success_message,
     :param interrupt_string: a string to send to the console before trying
                              to read a new line
     """
+    assert success_message
     _console_interaction(test, success_message, failure_message,
                          interrupt_string, True)
 
@@ -152,6 +155,7 @@ def wait_for_console_pattern(test, success_message, failure_message=None,
     :param success_message: if this message appears, test succeeds
     :param failure_message: if this message appears, test fails
     """
+    assert success_message
     _console_interaction(test, success_message, failure_message, None, vm=vm)
 
 def exec_command(test, command):
@@ -180,6 +184,7 @@ def exec_command_and_wait_for_pattern(test, command,
     :param success_message: if this message appears, test succeeds
     :param failure_message: if this message appears, test fails
     """
+    assert success_message
     _console_interaction(test, success_message, failure_message, command + '\r')
 
 def get_qemu_img(test):
-- 
2.39.5


Re: [PATCH 14/39] tests/functional: require non-NULL success_message for console wait
Posted by Thomas Huth 1 day, 21 hours ago
On 21/11/2024 17.57, Alex Bennée wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> When waiting for expected output, the 'success_message' is a mandatory
> parameter, with 'failure_message' defaulting to None.
> 
> The code has logic which indicates it was trying to cope with
> 'success_message' being None and 'failure_message' being non-None but
> it does not appear able to actually do anything useful. The check for
> 'success_message is None' will break out of the loop before any check
> for 'failure_message' has been performed.
> 
> IOW, for practcal purposes 'success_message' must be non-None unless
> 'send_string' is set. Assert this expectation and simplify the loop
> logic.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> Message-Id: <20241121154218.1423005-15-berrange@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/functional/qemu_test/cmd.py | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/functional/qemu_test/cmd.py b/tests/functional/qemu_test/cmd.py
> index 98722a9cf6..f6c4e4dda1 100644
> --- a/tests/functional/qemu_test/cmd.py
> +++ b/tests/functional/qemu_test/cmd.py
> @@ -81,6 +81,8 @@ def is_readable_executable_file(path):
>   def _console_interaction(test, success_message, failure_message,
>                            send_string, keep_sending=False, vm=None):
>       assert not keep_sending or send_string
> +    assert success_message or send_string
> +
>       if vm is None:
>           vm = test.vm
>       console = vm.console_file
> @@ -95,7 +97,7 @@ def _console_interaction(test, success_message, failure_message,
>                   send_string = None # send only once
>   
>           # Only consume console output if waiting for something
> -        if success_message is None and failure_message is None:
> +        if success_message is None:
>               if send_string is None:
>                   break
>               continue
> @@ -107,7 +109,7 @@ def _console_interaction(test, success_message, failure_message,
>           if not msg:
>               continue
>           console_logger.debug(msg)
> -        if success_message is None or success_message in msg:
> +        if success_message in msg:
>               break
>           if failure_message and failure_message in msg:
>               console.close()
> @@ -138,6 +140,7 @@ def interrupt_interactive_console_until_pattern(test, success_message,
>       :param interrupt_string: a string to send to the console before trying
>                                to read a new line
>       """
> +    assert success_message
>       _console_interaction(test, success_message, failure_message,
>                            interrupt_string, True)
>   
> @@ -152,6 +155,7 @@ def wait_for_console_pattern(test, success_message, failure_message=None,
>       :param success_message: if this message appears, test succeeds
>       :param failure_message: if this message appears, test fails
>       """
> +    assert success_message
>       _console_interaction(test, success_message, failure_message, None, vm=vm)

This assert is theoretically not needed since we call _console_interaction 
with send_string=None, so if success_message is None, the new assert in 
_console_interaction would trigger instead.

But it also does not hurt to have it here, so fine for me if we keep it.

Reviewed-by: Thomas Huth <thuth@redhat.com>