Re: [PATCH 3/3] automation: use expect to run QEMU

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Stefano,

On 10/08/2024 08:59, Stefano Stabellini wrote:
> Use expect to invoke QEMU so that we can terminate the test as soon as
> we get the right string in the output instead of waiting until the
> final timeout.
This is a great improvement.

> 
> For timeout, instead of an hardcoding the value, use a Gitlab CI
> variable "QEMU_TIMEOUT" that can be changed depending on the latest
> status of the Gitlab CI runners.
What is the current value of QEMU_TIMEOUT? I don't see it being mentioned 
anywhere.
Could tests define this env var if needed? What would be the precedence then?
I'm thinking that some tests don't need a very big timeout that otherwise is 
necessary
for longer tests (e.g. xtf test does not need to wait 720s). 

> 
> Note that for simplicity in the case of dom0less test, the script only
> checks for the $passed string. That is because the dom0 prompt and the
> $passed string could happen in any order making it difficult to check
> with expect which checks for strings in a specific order.
I use expect in my local testing too and I search for both strings. More below.

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> ---
>  automation/scripts/qemu-alpine-x86_64.sh      | 15 +++----
>  automation/scripts/qemu-key.ex                | 42 +++++++++++++++++++
>  automation/scripts/qemu-smoke-dom0-arm32.sh   | 15 ++++---
>  automation/scripts/qemu-smoke-dom0-arm64.sh   | 15 ++++---
>  .../scripts/qemu-smoke-dom0less-arm32.sh      | 14 +++----
>  .../scripts/qemu-smoke-dom0less-arm64.sh      | 14 +++----
>  automation/scripts/qemu-smoke-ppc64le.sh      | 12 +++---
>  automation/scripts/qemu-smoke-riscv64.sh      | 12 +++---
>  automation/scripts/qemu-smoke-x86-64.sh       | 14 +++----
>  automation/scripts/qemu-xtf-dom0less-arm64.sh | 14 +++----
>  10 files changed, 97 insertions(+), 70 deletions(-)
>  create mode 100755 automation/scripts/qemu-key.ex
> 
> diff --git a/automation/scripts/qemu-alpine-x86_64.sh 
> b/automation/scripts/qemu-alpine-x86_64.sh
> index 8e398dcea3..24b23a573c 100755
> --- a/automation/scripts/qemu-alpine-x86_64.sh
> +++ b/automation/scripts/qemu-alpine-x86_64.sh
> @@ -77,18 +77,15 @@ EOF
>  # Run the test
>  rm -f smoke.serial
>  set +e
> -timeout -k 1 720 \
> -qemu-system-x86_64 \
> +export qemu_cmd="qemu-system-x86_64 \
Usually (even respected by analyze.sh) exported env variables are written in 
capital letters

>      -cpu qemu64,+svm \
>      -m 2G -smp 2 \
>      -monitor none -serial stdio \
>      -nographic \
>      -device virtio-net-pci,netdev=n0 \
> -    -netdev user,id=n0,tftp=binaries,bootfile=/pxelinux.0 |& \
> -        # Remove carriage returns from the stdout output, as gitlab
> -        # interface chokes on them
> -        tee smoke.serial | sed 's/\r//'
> +    -netdev user,id=n0,tftp=binaries,bootfile=/pxelinux.0"
>  
> -set -e
> -(grep -q "Domain-0" smoke.serial && grep -q "BusyBox" smoke.serial) || exit 1
> -exit 0
> +export qemu_log="smoke.serial"
> +export log_msg="Domain-0"
> +export passed="BusyBox"
NIT: one empty line here for readability

> +./automation/scripts/qemu-key.ex
> diff --git a/automation/scripts/qemu-key.ex b/automation/scripts/qemu-key.ex
NIT: usually expect script have '.exp' extension.

> new file mode 100755
> index 0000000000..569ef2781f
> --- /dev/null
> +++ b/automation/scripts/qemu-key.ex
> @@ -0,0 +1,42 @@
> +#!/usr/bin/expect -f
> +
> +set timeout $env(QEMU_TIMEOUT)
> +
> +log_file -a $env(qemu_log)
> +
> +match_max 10000
> +
> +eval spawn $env(qemu_cmd)
> +
> +expect_after {
> +    -re "(.*)\r" {
> +        exp_continue
> +    }
> +    timeout {send_user "ERROR-Timeout!\n"; exit 1}
> +    eof {send_user "ERROR-EOF!\n"; exit 1}
> +}
> +
> +if {[info exists env(uboot_cmd)]} {
> +    expect "=>"
> +
> +    send "$env(uboot_cmd)\r"
> +}
> +
> +if {[info exists env(log_msg)]} {
> +    expect "$env(log_msg)"
> +}
> +
> +if {[info exists env(xen_cmd)]} {
> +    send "$env(xen_cmd)\r"
> +}
Does not seem to be used at all. For now, I'd suggest to drop it.

> +
> +if {[info exists env(passed)]} {
> +    expect {
> +        "$env(passed)" {
> +            exit 0
> +        }
> +    }
> +}
As said above, in my local testing, I search for both strings (after all we 
should test that dom0 works too) as follows:

if {[info exists env(dom0_passed)] && [info exists env(domu_passed)]} {
    expect {
        "$env(dom0_passed)" {
            expect "$env(domu_passed)"
            exit 0
        }
        "$env(domu_passed)" {
            expect "$env(dom0_passed)"
            exit 0
        }
    }
}

Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal