[PATCH v1] automation: fix false success in qemu tests

victorm.lira@amd.com posted 1 patch 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/e4de45759723b28713ef205335c4d79b9e7074b7.1724904269.git.victorm.lira@amd.com
.../build/ubuntu/xenial-xilinx.dockerfile     |  1 +
automation/gitlab-ci/test.yaml                |  2 ++
.../scripts/{qemu-key.exp => console.exp}     | 27 +++++++++++--------
automation/scripts/qemu-alpine-x86_64.sh      | 13 ++++++---
automation/scripts/qemu-smoke-dom0-arm32.sh   | 15 +++++++----
automation/scripts/qemu-smoke-dom0-arm64.sh   | 15 +++++++----
.../scripts/qemu-smoke-dom0less-arm32.sh      | 15 +++++++----
.../scripts/qemu-smoke-dom0less-arm64.sh      | 15 +++++++----
automation/scripts/qemu-smoke-ppc64le.sh      | 13 ++++++---
automation/scripts/qemu-smoke-riscv64.sh      | 13 ++++++---
automation/scripts/qemu-smoke-x86-64.sh       | 13 ++++++---
automation/scripts/qemu-xtf-dom0less-arm64.sh | 13 ++++++---
.../scripts/xilinx-smoke-dom0-x86_64.sh       | 22 +++++++--------
.../scripts/xilinx-smoke-dom0less-arm64.sh    | 21 ++++++++-------
14 files changed, 127 insertions(+), 71 deletions(-)
rename automation/scripts/{qemu-key.exp => console.exp} (50%)
[PATCH v1] automation: fix false success in qemu tests
Posted by victorm.lira@amd.com 3 months ago
From: Victor Lira <victorm.lira@amd.com>

Fix flaw in qemu-*.sh tests that producess a false success. The following
lines produces success despite the "expect" script producing nonzero exit
status.

    set +e
...
    ./automation/scripts/qemu-key.exp | sed 's/\r\+$//'
    (end of file)

The "set +e" is sometimes needed for cleanup such as powering off hardware
after running a test.

Fixes the CI failure introduced by 95764a0817.

Update xilinx-smoke tests to use the "expect" utility for early exit from
tests. Generalize the variable names in the script "qemu-key.exp" to be
used by both QEMU and hardware tests. Add a missing "-continue_timer" flag
for the expect script to properly time out. Add "expect" to xilinx
dockerfile.

Signed-off-by: Victor Lira <victorm.lira@amd.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.org
---
 .../build/ubuntu/xenial-xilinx.dockerfile     |  1 +
 automation/gitlab-ci/test.yaml                |  2 ++
 .../scripts/{qemu-key.exp => console.exp}     | 27 +++++++++++--------
 automation/scripts/qemu-alpine-x86_64.sh      | 13 ++++++---
 automation/scripts/qemu-smoke-dom0-arm32.sh   | 15 +++++++----
 automation/scripts/qemu-smoke-dom0-arm64.sh   | 15 +++++++----
 .../scripts/qemu-smoke-dom0less-arm32.sh      | 15 +++++++----
 .../scripts/qemu-smoke-dom0less-arm64.sh      | 15 +++++++----
 automation/scripts/qemu-smoke-ppc64le.sh      | 13 ++++++---
 automation/scripts/qemu-smoke-riscv64.sh      | 13 ++++++---
 automation/scripts/qemu-smoke-x86-64.sh       | 13 ++++++---
 automation/scripts/qemu-xtf-dom0less-arm64.sh | 13 ++++++---
 .../scripts/xilinx-smoke-dom0-x86_64.sh       | 22 +++++++--------
 .../scripts/xilinx-smoke-dom0less-arm64.sh    | 21 ++++++++-------
 14 files changed, 127 insertions(+), 71 deletions(-)
 rename automation/scripts/{qemu-key.exp => console.exp} (50%)

diff --git a/automation/build/ubuntu/xenial-xilinx.dockerfile b/automation/build/ubuntu/xenial-xilinx.dockerfile
index f03d62e8bd..6107d8b771 100644
--- a/automation/build/ubuntu/xenial-xilinx.dockerfile
+++ b/automation/build/ubuntu/xenial-xilinx.dockerfile
@@ -20,6 +20,7 @@ RUN apt-get update && \
         git \
         gzip \
         file \
+        expect \
         && \
         apt-get autoremove -y && \
         apt-get clean && \
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 3b339f387f..e3ebe37459 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -84,6 +84,7 @@
   variables:
     CONTAINER: ubuntu:xenial-xilinx
     LOGFILE: qemu-smoke-xilinx.log
+    TEST_TIMEOUT: 120
   artifacts:
     paths:
       - smoke.serial
@@ -103,6 +104,7 @@
     LOGFILE: xilinx-smoke-x86_64.log
     XEN_CMD_CONSOLE: "console=com2 com2=115200,8n1,0x2F8,4"
     TEST_BOARD: "crater"
+    TEST_TIMEOUT: 500
   artifacts:
     paths:
       - smoke.serial
diff --git a/automation/scripts/qemu-key.exp b/automation/scripts/console.exp
similarity index 50%
rename from automation/scripts/qemu-key.exp
rename to automation/scripts/console.exp
index 787f1f08cb..2cf31e7ac0 100755
--- a/automation/scripts/qemu-key.exp
+++ b/automation/scripts/console.exp
@@ -1,23 +1,29 @@
 #!/usr/bin/expect -f

-if {[info exists env(QEMU_TIMEOUT)]} {
-    set timeout $env(QEMU_TIMEOUT)
+if {[info exists env(TEST_TIMEOUT)]} {
+    set timeout $env(TEST_TIMEOUT)
 } else {
     set timeout 1500
 }

-log_file -a $env(QEMU_LOG)
+log_file -a $env(TEST_LOGFILE)

 match_max 10000

-eval spawn $env(QEMU_CMD)
+eval spawn $env(TEST_CMD_START)

 expect_after {
     -re "(.*)\r" {
-        exp_continue
+        exp_continue -continue_timer
+    }
+    timeout {
+        send_error "ERROR-Timeout!\n"
+        exit 1
+    }
+    eof {
+        send_error "ERROR-EOF!\n"
+        exit 1
     }
-    timeout {send_error "ERROR-Timeout!\n"; exit 1}
-    eof {send_error "ERROR-EOF!\n"; exit 1}
 }

 if {[info exists env(UBOOT_CMD)]} {
@@ -28,22 +34,21 @@ if {[info exists env(UBOOT_CMD)]} {

 if {[info exists env(LOG_MSG)]} {
     expect {
-        "$env(PASSED)" {
+        "$env(TEST_PASS_MSG)" {
             expect "$env(LOG_MSG)"
             exit 0
         }
         "$env(LOG_MSG)" {
-            expect "$env(PASSED)"
+            expect "$env(TEST_PASS_MSG)"
             exit 0
         }
     }
 }

 expect {
-    "$env(PASSED)" {
+    "$env(TEST_PASS_MSG)" {
         exit 0
     }
 }

 expect eof
-
diff --git a/automation/scripts/qemu-alpine-x86_64.sh b/automation/scripts/qemu-alpine-x86_64.sh
index 42a89e86b0..e9a1577af7 100755
--- a/automation/scripts/qemu-alpine-x86_64.sh
+++ b/automation/scripts/qemu-alpine-x86_64.sh
@@ -77,7 +77,7 @@ EOF
 # Run the test
 rm -f smoke.serial
 set +e
-export QEMU_CMD="qemu-system-x86_64 \
+export TEST_CMD_START="qemu-system-x86_64 \
     -cpu qemu64,+svm \
     -m 2G -smp 2 \
     -monitor none -serial stdio \
@@ -85,8 +85,13 @@ export QEMU_CMD="qemu-system-x86_64 \
     -device virtio-net-pci,netdev=n0 \
     -netdev user,id=n0,tftp=binaries,bootfile=/pxelinux.0"

-export QEMU_LOG="smoke.serial"
+export TEST_LOGFILE="smoke.serial"
 export LOG_MSG="Domain-0"
-export PASSED="BusyBox"
+export TEST_PASS_MSG="BusyBox"

-./automation/scripts/qemu-key.exp | sed 's/\r\+$//'
+# Capture test result using expect utility.
+rm -f console
+mkfifo console
+sed 's/\r\+$//' < console &
+./automation/scripts/console.exp > console
+exit $?
diff --git a/automation/scripts/qemu-smoke-dom0-arm32.sh b/automation/scripts/qemu-smoke-dom0-arm32.sh
index fd64b19358..dc2ed1f968 100755
--- a/automation/scripts/qemu-smoke-dom0-arm32.sh
+++ b/automation/scripts/qemu-smoke-dom0-arm32.sh
@@ -78,7 +78,7 @@ bash imagebuilder/scripts/uboot-script-gen -t tftp -d . -c config

 rm -f ${serial_log}
 set +e
-export QEMU_CMD="./qemu-system-arm \
+export TEST_CMD_START="./qemu-system-arm \
    -machine virt \
    -machine virtualization=true \
    -smp 4 \
@@ -92,8 +92,13 @@ export QEMU_CMD="./qemu-system-arm \
    -bios /usr/lib/u-boot/qemu_arm/u-boot.bin"

 export UBOOT_CMD="virtio scan; dhcp; tftpb 0x40000000 boot.scr; source 0x40000000"
-export QEMU_LOG="${serial_log}"
+export TEST_LOGFILE="${serial_log}"
 export LOG_MSG="Domain-0"
-export PASSED="/ #"
-
-../automation/scripts/qemu-key.exp | sed 's/\r\+$//'
+export TEST_PASS_MSG="/ #"
+
+# Capture test result using expect utility.
+rm -f console
+mkfifo console
+sed 's/\r\+$//' < console &
+../automation/scripts/console.exp > console
+exit $?
diff --git a/automation/scripts/qemu-smoke-dom0-arm64.sh b/automation/scripts/qemu-smoke-dom0-arm64.sh
index e0cea742b0..8ecddae555 100755
--- a/automation/scripts/qemu-smoke-dom0-arm64.sh
+++ b/automation/scripts/qemu-smoke-dom0-arm64.sh
@@ -94,7 +94,7 @@ bash imagebuilder/scripts/uboot-script-gen -t tftp -d binaries/ -c binaries/conf
 # Run the test
 rm -f smoke.serial
 set +e
-export QEMU_CMD="./binaries/qemu-system-aarch64 \
+export TEST_CMD_START="./binaries/qemu-system-aarch64 \
     -machine virtualization=true \
     -cpu cortex-a57 -machine type=virt \
     -m 2048 -monitor none -serial stdio \
@@ -105,8 +105,13 @@ export QEMU_CMD="./binaries/qemu-system-aarch64 \
     -bios /usr/lib/u-boot/qemu_arm64/u-boot.bin"

 export UBOOT_CMD="virtio scan; dhcp; tftpb 0x40000000 boot.scr; source 0x40000000"
-export QEMU_LOG="smoke.serial"
+export TEST_LOGFILE="smoke.serial"
 export LOG_MSG="Domain-0"
-export PASSED="BusyBox"
-
-./automation/scripts/qemu-key.exp | sed 's/\r\+$//'
+export TEST_PASS_MSG="BusyBox"
+
+# Capture test result using expect utility.
+rm -f console
+mkfifo console
+sed 's/\r\+$//' < console &
+./automation/scripts/console.exp > console
+exit $?
diff --git a/automation/scripts/qemu-smoke-dom0less-arm32.sh b/automation/scripts/qemu-smoke-dom0less-arm32.sh
index e824cb7c2a..2625efc163 100755
--- a/automation/scripts/qemu-smoke-dom0less-arm32.sh
+++ b/automation/scripts/qemu-smoke-dom0less-arm32.sh
@@ -131,7 +131,7 @@ bash imagebuilder/scripts/uboot-script-gen -t tftp -d . -c config
 # Run the test
 rm -f ${serial_log}
 set +e
-export QEMU_CMD="./qemu-system-arm \
+export TEST_CMD_START="./qemu-system-arm \
     -machine virt \
     -machine virtualization=true \
     -smp 4 \
@@ -145,8 +145,13 @@ export QEMU_CMD="./qemu-system-arm \
     -bios /usr/lib/u-boot/qemu_arm/u-boot.bin"

 export UBOOT_CMD="virtio scan; dhcp; tftpb 0x40000000 boot.scr; source 0x40000000"
-export QEMU_LOG="${serial_log}"
+export TEST_LOGFILE="${serial_log}"
 export LOG_MSG="${dom0_prompt}"
-export PASSED="${passed}"
-
-../automation/scripts/qemu-key.exp | sed 's/\r\+$//'
+export TEST_PASS_MSG="${passed}"
+
+# Capture test result using expect utility.
+rm -f console
+mkfifo console
+sed 's/\r\+$//' < console &
+../automation/scripts/console.exp > console
+exit $?
diff --git a/automation/scripts/qemu-smoke-dom0less-arm64.sh b/automation/scripts/qemu-smoke-dom0less-arm64.sh
index f42ba5d196..a9ab92d8bf 100755
--- a/automation/scripts/qemu-smoke-dom0less-arm64.sh
+++ b/automation/scripts/qemu-smoke-dom0less-arm64.sh
@@ -205,7 +205,7 @@ bash imagebuilder/scripts/uboot-script-gen -t tftp -d binaries/ -c binaries/conf
 # Run the test
 rm -f smoke.serial
 set +e
-export QEMU_CMD="./binaries/qemu-system-aarch64 \
+export TEST_CMD_START="./binaries/qemu-system-aarch64 \
     -machine virtualization=true \
     -cpu cortex-a57 -machine type=virt,gic-version=$gic_version \
     -m 2048 -monitor none -serial stdio \
@@ -216,8 +216,13 @@ export QEMU_CMD="./binaries/qemu-system-aarch64 \
     -bios /usr/lib/u-boot/qemu_arm64/u-boot.bin"

 export UBOOT_CMD="virtio scan; dhcp; tftpb 0x40000000 boot.scr; source 0x40000000"
-export QEMU_LOG="smoke.serial"
+export TEST_LOGFILE="smoke.serial"
 export LOG_MSG="Welcome to Alpine Linux"
-export PASSED="${passed}"
-
-./automation/scripts/qemu-key.exp | sed 's/\r\+$//'
+export TEST_PASS_MSG="${passed}"
+
+# Capture test result using expect utility.
+rm -f console
+mkfifo console
+sed 's/\r\+$//' < console &
+./automation/scripts/console.exp > console
+exit $?
diff --git a/automation/scripts/qemu-smoke-ppc64le.sh b/automation/scripts/qemu-smoke-ppc64le.sh
index 594f92c19c..74ffca00c6 100755
--- a/automation/scripts/qemu-smoke-ppc64le.sh
+++ b/automation/scripts/qemu-smoke-ppc64le.sh
@@ -11,7 +11,7 @@ machine=$1
 rm -f ${serial_log}
 set +e

-export QEMU_CMD="qemu-system-ppc64 \
+export TEST_CMD_START="qemu-system-ppc64 \
     -bios skiboot.lid \
     -M $machine \
     -m 2g \
@@ -22,7 +22,12 @@ export QEMU_CMD="qemu-system-ppc64 \
     -serial stdio \
     -kernel binaries/xen"

-export QEMU_LOG="${serial_log}"
-export PASSED="Hello, ppc64le!"
+export TEST_LOGFILE="${serial_log}"
+export TEST_PASS_MSG="Hello, ppc64le!"

-./automation/scripts/qemu-key.exp | sed 's/\r\+$//'
+# Capture test result using expect utility.
+rm -f console
+mkfifo console
+sed 's/\r\+$//' < console &
+./automation/scripts/console.exp > console
+exit $?
diff --git a/automation/scripts/qemu-smoke-riscv64.sh b/automation/scripts/qemu-smoke-riscv64.sh
index c2595f657f..d1631b9633 100755
--- a/automation/scripts/qemu-smoke-riscv64.sh
+++ b/automation/scripts/qemu-smoke-riscv64.sh
@@ -6,14 +6,19 @@ set -ex
 rm -f smoke.serial
 set +e

-export QEMU_CMD="qemu-system-riscv64 \
+export TEST_CMD_START="qemu-system-riscv64 \
     -M virt \
     -smp 1 \
     -nographic \
     -m 2g \
     -kernel binaries/xen"

-export QEMU_LOG="smoke.serial"
-export PASSED="All set up"
+export TEST_LOGFILE="smoke.serial"
+export TEST_PASS_MSG="All set up"

-./automation/scripts/qemu-key.exp | sed 's/\r\+$//'
+# Capture test result using expect utility.
+rm -f console
+mkfifo console
+sed 's/\r\+$//' < console &
+./automation/scripts/console.exp > console
+exit $?
diff --git a/automation/scripts/qemu-smoke-x86-64.sh b/automation/scripts/qemu-smoke-x86-64.sh
index 3440b1761d..1703b96e9e 100755
--- a/automation/scripts/qemu-smoke-x86-64.sh
+++ b/automation/scripts/qemu-smoke-x86-64.sh
@@ -16,12 +16,17 @@ esac

 rm -f smoke.serial
 set +e
-export QEMU_CMD="qemu-system-x86_64 -nographic -kernel binaries/xen \
+export TEST_CMD_START="qemu-system-x86_64 -nographic -kernel binaries/xen \
         -initrd xtf/tests/example/$k \
         -append \"loglvl=all console=com1 noreboot console_timestamps=boot $extra\" \
         -m 512 -monitor none -serial stdio"

-export QEMU_LOG="smoke.serial"
-export PASSED="Test result: SUCCESS"
+export TEST_LOGFILE="smoke.serial"
+export TEST_PASS_MSG="Test result: SUCCESS"

-./automation/scripts/qemu-key.exp | sed 's/\r\+$//'
+# Capture test result using expect utility.
+rm -f console
+mkfifo console
+sed 's/\r\+$//' < console &
+./automation/scripts/console.exp > console
+exit $?
diff --git a/automation/scripts/qemu-xtf-dom0less-arm64.sh b/automation/scripts/qemu-xtf-dom0less-arm64.sh
index 4042fe5060..77a46e3e79 100755
--- a/automation/scripts/qemu-xtf-dom0less-arm64.sh
+++ b/automation/scripts/qemu-xtf-dom0less-arm64.sh
@@ -51,7 +51,7 @@ bash imagebuilder/scripts/uboot-script-gen -t tftp -d binaries/ -c binaries/conf
 # Run the test
 rm -f smoke.serial
 set +e
-export QEMU_CMD="./binaries/qemu-system-aarch64 \
+export TEST_CMD_START="./binaries/qemu-system-aarch64 \
     -machine virtualization=true \
     -cpu cortex-a57 -machine type=virt \
     -m 2048 -monitor none -serial stdio \
@@ -62,7 +62,12 @@ export QEMU_CMD="./binaries/qemu-system-aarch64 \
     -bios /usr/lib/u-boot/qemu_arm64/u-boot.bin"

 export UBOOT_CMD="virtio scan; dhcp; tftpb 0x40000000 boot.scr; source 0x40000000"
-export QEMU_LOG="smoke.serial"
-export PASSED="${passed}"
+export TEST_LOGFILE="smoke.serial"
+export TEST_PASS_MSG="${passed}"

-./automation/scripts/qemu-key.exp | sed 's/\r\+$//'
+# Capture test result using expect utility.
+rm -f console
+mkfifo console
+sed 's/\r\+$//' < console &
+./automation/scripts/console.exp > console
+exit $?
diff --git a/automation/scripts/xilinx-smoke-dom0-x86_64.sh b/automation/scripts/xilinx-smoke-dom0-x86_64.sh
index 4559e2b9ee..14999956a8 100755
--- a/automation/scripts/xilinx-smoke-dom0-x86_64.sh
+++ b/automation/scripts/xilinx-smoke-dom0-x86_64.sh
@@ -27,7 +27,6 @@ memory = 512
 vif = [ "bridge=xenbr0", ]
 disk = [ ]
 '
-TIMEOUT_SECONDS=200

 # Select test variant.
 if [ "${TEST}" = "ping" ]; then
@@ -125,20 +124,21 @@ boot

 # Power cycle board and collect serial port output.
 SERIAL_DEV="/dev/serial/${TEST_BOARD}"
-SERIAL_CMD="cat ${SERIAL_DEV} | tee smoke.serial | sed 's/\r//'"
 sh /scratch/gitlab-runner/${TEST_BOARD}.sh 2
 sleep 5
 sh /scratch/gitlab-runner/${TEST_BOARD}.sh 1
 sleep 5
 set +e
 stty -F ${SERIAL_DEV} 115200
-timeout -k 1 ${TIMEOUT_SECONDS} nohup sh -c "${SERIAL_CMD}"
-sh /scratch/gitlab-runner/${TEST_BOARD}.sh 2
-
-set -e
-
-if grep -q "${PASS_MSG}" smoke.serial; then
-    exit 0
-fi

-fatal "Test failed"
+# Capture the test result and power off board before exiting.
+export TEST_PASS_MSG="${PASS_MSG}"
+export TEST_CMD_START="cat ${SERIAL_DEV}"
+export TEST_LOGFILE="smoke.serial"
+rm -f console
+mkfifo console
+sed 's/\r\+$//' < console &
+./automation/scripts/console.exp > console
+TEST_RESULT=$?
+sh "/scratch/gitlab-runner/${TEST_BOARD}.sh" 2
+exit ${TEST_RESULT}
diff --git a/automation/scripts/xilinx-smoke-dom0less-arm64.sh b/automation/scripts/xilinx-smoke-dom0less-arm64.sh
index 18aa07f0a2..a6ad37a887 100755
--- a/automation/scripts/xilinx-smoke-dom0less-arm64.sh
+++ b/automation/scripts/xilinx-smoke-dom0less-arm64.sh
@@ -137,13 +137,16 @@ cd $START
 SERIAL_DEV="/dev/serial/zynq"
 set +e
 stty -F ${SERIAL_DEV} 115200
-timeout -k 1 120 nohup sh -c "cat ${SERIAL_DEV} | tee smoke.serial | sed 's/\r//'"

-# stop the board
-cd /scratch/gitlab-runner
-bash zcu102.sh 2
-cd $START
-
-set -e
-(grep -q "^Welcome to Alpine Linux" smoke.serial && grep -q "${passed}" smoke.serial) || exit 1
-exit 0
+# Capture the test result and power off board before exiting.
+export TEST_PASS_MSG="${passed}"
+export LOG_MSG="Welcome to Alpine Linux"
+export TEST_CMD_START="cat ${SERIAL_DEV}"
+export TEST_LOGFILE="smoke.serial"
+rm -f console
+mkfifo console
+sed 's/\r\+$//' < console &
+./automation/scripts/console.exp > console
+TEST_RESULT=$?
+sh /scratch/gitlab-runner/zcu102.sh 2
+exit ${TEST_RESULT}
--
2.25.1
Re: [PATCH v1] automation: fix false success in qemu tests
Posted by Michal Orzel 3 months ago
Hi Victor,

On 29/08/2024 06:09, victorm.lira@amd.com wrote:
> 
> 
> From: Victor Lira <victorm.lira@amd.com>
> 
> Fix flaw in qemu-*.sh tests that producess a false success. The following
> lines produces success despite the "expect" script producing nonzero exit
> status.
> 
>     set +e
> ...
>     ./automation/scripts/qemu-key.exp | sed 's/\r\+$//'
>     (end of file)
> 
> The "set +e" is sometimes needed for cleanup such as powering off hardware
> after running a test.
> 
> Fixes the CI failure introduced by 95764a0817.
You may want to use "Fixes" tag. See docs/process/sending-patches.pandoc.

> 
> Update xilinx-smoke tests to use the "expect" utility for early exit from
> tests. Generalize the variable names in the script "qemu-key.exp" to be
> used by both QEMU and hardware tests. Add a missing "-continue_timer" flag
> for the expect script to properly time out. Add "expect" to xilinx
> dockerfile.
This commit tries to do 2 thins at once:
 - fix "set +e" issue
 - make xilinx tests use expect

These should be 2 separate patches. The first patch should only remove "set +e" from qemu patches.
There is no need for fifo in qemu tests. You should use that only in xilinx tests where we cannot remove "set +e".

~Michal
Re: [PATCH v1] automation: fix false success in qemu tests
Posted by Lira, Victor M 3 months ago
Hello Michal,

Unfortunately only removing "set +e" did not fix the issue as the test 
still will always pass.
See here (line 90):
https://gitlab.com/xen-project/people/sstabellini/xen/-/jobs/7700210695

I think we will need to use the fifo or the Bash "pipefail" function.

> You may want to use "Fixes" tag. See docs/process/sending-patches.pandoc.
> This commit tries to do 2 thins at once:
>   - fix "set +e" issue
>   - make xilinx tests use expect
>
> These should be 2 separate patches. The first patch should only remove "set +e" from qemu patches.
> There is no need for fifo in qemu tests. You should use that only in xilinx tests where we cannot remove "set +e".
>
> ~Michal

OK, I will review the doc and create separate patches for fixing the 
qemu and xilinx tests.


Victor
Re: [PATCH v1] automation: fix false success in qemu tests
Posted by Stefano Stabellini 3 months ago
On Thu, 28 Aug 2024, Lira, Victor M wrote:
> Hello Michal,
> 
> Unfortunately only removing "set +e" did not fix the issue as the test still
> will always pass.
> See here (line 90):
> https://gitlab.com/xen-project/people/sstabellini/xen/-/jobs/7700210695
> 
> I think we will need to use the fifo or the Bash "pipefail" function.

pipefail is better if possible


> > You may want to use "Fixes" tag. See docs/process/sending-patches.pandoc.
> > This commit tries to do 2 thins at once:
> >   - fix "set +e" issue
> >   - make xilinx tests use expect
> > 
> > These should be 2 separate patches. The first patch should only remove "set
> > +e" from qemu patches.
> > There is no need for fifo in qemu tests. You should use that only in xilinx
> > tests where we cannot remove "set +e".
> > 
> > ~Michal
> 
> OK, I will review the doc and create separate patches for fixing the qemu and
> xilinx tests.
> 
> 
> Victor
> 
>
Re: [PATCH v1] automation: fix false success in qemu tests
Posted by Lira, Victor M 3 months ago
While developing a patch to update xilinx tests to use the "expect"
utility I found that with the current use of "expect" in the *qemu-* 
*scripts,
it is possible for the test to produce a *false success *if the expect
script returns nonzero due to the following lines

     set +e
...
     ./automation/scripts/qemu-key.exp | sed 's/\r\+$//'
     (end of file)

The first disables exiting on errors which is needed for cleanup in some
cases such as turning off power in a hardware test. However, here the
script exits 0 on reaching the end of file.

This is a serious flaw in our testing system as the result of the test is
essentially ignored.

I have been able produce such a false success here by setting a random 
passed
message and short timeout (line 142).
https://gitlab.com/xen-project/people/sstabellini/xen/-/jobs/7693194287#L142

I have sent a patch that fixes this using the same method from my xilinx
patch but suggestions are welcome. For qemu tests it may be enough to
instead just remove "set +e".