[XEN PATCH 0/9] CI: Fixes for tools/tests and junit and other

[XEN PATCH 0/9] CI: Fixes for tools/tests and junit and other

[Anthony PERARD]

From: Anthony PERARD <anthony.perard@vates.tech>

Patch series available in this git branch:
https://xenbits.xenproject.org/git-http/people/aperard/xen-unstable.git br.ci-tools-tests-junit-fixes-v1

All the *-tools-tests-* are currently only checking automatically if the
machine as booted. The only way to find out if one of the tools/tests failed is
to read the console output by hand.

Also, tests-rangeset always return success even if there's a failure.

Fix all that, and add some improvement, and add SELECTED_JOBS_ONLY to
analyze.yaml

How it looks:
success:
    https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1850830262/test_report
failure:
    https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1850830637/test_report

Cheers,

Anthony PERARD (9):
  CI: Add SELECTED_JOBS_ONLY to analyze.yaml
  tools/tests: Fix return value of test-rangeset
  CI: Fix status check for tools/tests
  CI: Ignore run-tools-test return value
  CI: Have the gitlab job fail on tools/tests failure
  CI: Upload junit result as artefact
  CI: Use CDATA avoid the need to escape tests outputs
  CI: Workaround extra content in junit
  CI: Add timing to junit

 automation/gitlab-ci/analyze.yaml    |  3 +++
 automation/gitlab-ci/test.yaml       |  1 +
 automation/scripts/qubes-x86-64.sh   | 18 ++++++++++++++++--
 automation/scripts/run-tools-tests   | 11 +++++++----
 tools/tests/rangeset/test-rangeset.c |  2 +-
 5 files changed, 28 insertions(+), 7 deletions(-)

-- 
Anthony PERARD

[XEN PATCH 1/9] CI: Add SELECTED_JOBS_ONLY to analyze.yaml

[Anthony PERARD]

From: Anthony PERARD <anthony.perard@vates.tech>

Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
---
 automation/gitlab-ci/analyze.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/automation/gitlab-ci/analyze.yaml b/automation/gitlab-ci/analyze.yaml
index 35ff3620cf..5b00b9f25c 100644
--- a/automation/gitlab-ci/analyze.yaml
+++ b/automation/gitlab-ci/analyze.yaml
@@ -29,6 +29,9 @@
   rules:
     - if: $CI_PIPELINE_SOURCE == "schedule"
       when: never
+    - if: $SELECTED_JOBS_ONLY && $CI_JOB_NAME =~ $SELECTED_JOBS_ONLY
+    - if: $SELECTED_JOBS_ONLY
+      when: never
     - if: $WTOKEN && $CI_PROJECT_PATH =~ /^xen-project\/people\/.*$/
       when: manual
       allow_failure: true
-- 
Anthony PERARD

[XEN PATCH 2/9] tools/tests: Fix return value of test-rangeset

[Anthony PERARD]

From: Anthony PERARD <anthony.perard@vates.tech>

Otherwise, failed tests are ignored by automated test.

Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
---
 tools/tests/rangeset/test-rangeset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/tests/rangeset/test-rangeset.c b/tools/tests/rangeset/test-rangeset.c
index 8b580e14eb..c14a908b4f 100644
--- a/tools/tests/rangeset/test-rangeset.c
+++ b/tools/tests/rangeset/test-rangeset.c
@@ -215,7 +215,7 @@ int main(int argc, char **argv)
         }
     }
 
-    return 0;
+    return ret_code;
 }
 
 /*
-- 
Anthony PERARD

[XEN PATCH 3/9] CI: Fix status check for tools/tests

[Anthony PERARD]

From: Anthony PERARD <anthony.perard@vates.tech>

Without "pipefail", $? have the exit value of `tee`, which should
always be 0. But instead of using "pipefail", do collect the value of
from the test with $PIPESTATUS.

Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
---

Notes:
    Without the next patch "CI: Ignore run-tools-test return value", this
    just mean that if one test fail, the script `qubes-x86-64.sh` will
    detect a ERROR-Timeout because the expected string is never printed.

 automation/scripts/run-tools-tests | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
index 2bca1589db..695ed77e46 100755
--- a/automation/scripts/run-tools-tests
+++ b/automation/scripts/run-tools-tests
@@ -20,7 +20,7 @@ for f in "$1"/*; do
     echo "Running $f"
     printf '  <testcase name="%s">\n' "$f" >> "$xml_out"
     "$f" 2>&1 | tee /tmp/out
-    ret=$?
+    ret=${PIPESTATUS[0]}
     if [ "$ret" -ne 0 ]; then
         echo "FAILED: $f"
         failed+=" $f"
-- 
Anthony PERARD

[XEN PATCH 4/9] CI: Ignore run-tools-test return value

[Anthony PERARD]

From: Anthony PERARD <anthony.perard@vates.tech>

The main script expect to find the string "$passed" or it just
timeout. Then, it doesn't try to download the junit file.
So ignore the return value of run-tools-test for now.

This mean the job will not fail, but at least we should have the junit
files with the error messages.

Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
---

Notes:
    On failure from a tests after this patch, the gitlab job succeed. But
    the junit file is collected.

 automation/scripts/qubes-x86-64.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
index 2750d24eba..046137a4a6 100755
--- a/automation/scripts/qubes-x86-64.sh
+++ b/automation/scripts/qubes-x86-64.sh
@@ -135,10 +135,11 @@ done
     ### tests: tools-tests-pv, tools-tests-pvh
     "tools-tests-pv"|"tools-tests-pvh")
         retrieve_xml=1
-        passed="test passed"
+        passed="run-tools-test over"
         domU_check=""
         dom0_check="
-/root/run-tools-tests /usr/lib/xen/tests /tmp/tests-junit.xml && echo \"${passed}\"
+/root/run-tools-tests /usr/lib/xen/tests /tmp/tests-junit.xml ||:
+echo \"${passed}\"
 nc -l -p 8080 < /tmp/tests-junit.xml >/dev/null &
 "
         if [ "${test_variant}" = "tools-tests-pvh" ]; then
-- 
Anthony PERARD

[XEN PATCH 5/9] CI: Have the gitlab job fail on tools/tests failure

[Anthony PERARD]

From: Anthony PERARD <anthony.perard@vates.tech>

We can't rely on an exit value from `run-tools-tests` since we only
have the console output. `console.exp` only look for success or it
times out. We could parse the console output, but the junit is more
concise. Also check if we have it or fail as well.

Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
---
 automation/scripts/qubes-x86-64.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
index 046137a4a6..7a4c5ae489 100755
--- a/automation/scripts/qubes-x86-64.sh
+++ b/automation/scripts/qubes-x86-64.sh
@@ -298,6 +298,13 @@ TEST_RESULT=$?
 
 if [ -n "$retrieve_xml" ]; then
     nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
+    # Findout if one of the test failed
+    if ! grep -q '</testsuites>' tests-junit.xml; then
+        echo "ERROR: tests-junit.xml is incomplete or missing."
+        TEST_RESULT=1
+    elif grep -q '</failure>' tests-junit.xml; then
+        TEST_RESULT=1
+    fi
 fi
 
 exit "$TEST_RESULT"
-- 
Anthony PERARD

[XEN PATCH 6/9] CI: Upload junit result as artefact

[Anthony PERARD]

From: Anthony PERARD <anthony.perard@vates.tech>

This allow to investigate the file in cases of error. Not all
jobs that extend ".adl-x86-64" are creating a "tests-junit.xml" but
Gitlab only produce a warning when the file isn't found.

Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
---
 automation/gitlab-ci/test.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 842cecf713..2b4b8c021b 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -155,6 +155,7 @@
     paths:
       - smoke.serial
       - '*.log'
+      - tests-junit.xml
     when: always
   rules:
     - if: $SELECTED_JOBS_ONLY && $CI_JOB_NAME =~ $SELECTED_JOBS_ONLY
-- 
Anthony PERARD

[XEN PATCH 7/9] CI: Use CDATA avoid the need to escape tests outputs

[Anthony PERARD]

From: Anthony PERARD <anthony.perard@vates.tech>

This is easier than escaping individual characters, especially '&'
and '<' which are problematic if present.

We might still need to escape ']]>' if this string is present in the
test output, but hopefully not.

Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
---
 automation/scripts/run-tools-tests | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
index 695ed77e46..852c1cfbcf 100755
--- a/automation/scripts/run-tools-tests
+++ b/automation/scripts/run-tools-tests
@@ -25,9 +25,9 @@ for f in "$1"/*; do
         echo "FAILED: $f"
         failed+=" $f"
         printf '   <failure type="failure" message="binary %s exited with code %d">\n' "$f" "$ret" >> "$xml_out"
-        # TODO: could use xml escaping... but current tests seems to
-        # produce sane output
+        printf '<![CDATA[' >> "$xml_out"
         cat /tmp/out >> "$xml_out"
+        printf ']]>' >> "$xml_out"
         printf '   </failure>\n' >> "$xml_out"
     else
         echo "PASSED"
-- 
Anthony PERARD

[XEN PATCH 8/9] CI: Workaround extra content in junit

[Anthony PERARD]

From: Anthony PERARD <anthony.perard@vates.tech>

Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
---
 automation/scripts/qubes-x86-64.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
index 7a4c5ae489..6ab8412f45 100755
--- a/automation/scripts/qubes-x86-64.sh
+++ b/automation/scripts/qubes-x86-64.sh
@@ -298,6 +298,12 @@ TEST_RESULT=$?
 
 if [ -n "$retrieve_xml" ]; then
     nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
+    # Workaround duplicated data been received
+    sed -i.old '/^<\/testsuites>/q' tests-junit.xml > /dev/null
+    extra_line_in_junit=$(($(wc -l < tests-junit.xml.old) - $(wc -l < tests-junit.xml)))
+    if [ $extra_line_in_junit -gt 0 ]; then
+        echo "WARNING: Found $extra_line_in_junit too many lines in junit."
+    fi
     # Findout if one of the test failed
     if ! grep -q '</testsuites>' tests-junit.xml; then
         echo "ERROR: tests-junit.xml is incomplete or missing."
-- 
Anthony PERARD

[XEN PATCH 9/9] CI: Add timing to junit

[Anthony PERARD]

From: Anthony PERARD <anthony.perard@vates.tech>

Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
---
 automation/scripts/run-tools-tests | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
index 852c1cfbcf..e38cc4068c 100755
--- a/automation/scripts/run-tools-tests
+++ b/automation/scripts/run-tools-tests
@@ -18,9 +18,12 @@ for f in "$1"/*; do
         continue
     fi
     echo "Running $f"
-    printf '  <testcase name="%s">\n' "$f" >> "$xml_out"
+    time_start=$EPOCHREALTIME
     "$f" 2>&1 | tee /tmp/out
     ret=${PIPESTATUS[0]}
+    time_end=$EPOCHREALTIME
+    time_test="$(bc <<<"$time_end - $time_start")"
+    printf '  <testcase name="%s" time="%f">\n' "$f" "$time_test" >> "$xml_out"
     if [ "$ret" -ne 0 ]; then
         echo "FAILED: $f"
         failed+=" $f"
-- 
Anthony PERARD

Re: [XEN PATCH 7/9] CI: Use CDATA avoid the need to escape tests outputs

[Andrew Cooper]

On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
> index 695ed77e46..852c1cfbcf 100755
> --- a/automation/scripts/run-tools-tests
> +++ b/automation/scripts/run-tools-tests
> @@ -25,9 +25,9 @@ for f in "$1"/*; do
>          echo "FAILED: $f"
>          failed+=" $f"
>          printf '   <failure type="failure" message="binary %s exited with code %d">\n' "$f" "$ret" >> "$xml_out"
> -        # TODO: could use xml escaping... but current tests seems to
> -        # produce sane output
> +        printf '<![CDATA[' >> "$xml_out"
>          cat /tmp/out >> "$xml_out"
> +        printf ']]>' >> "$xml_out"

I think you want a \n on this printf.

I'd also suggest leaving a TODO for "escape ]]> if necessary".

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [XEN PATCH 2/9] tools/tests: Fix return value of test-rangeset

[Andrew Cooper]

On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> From: Anthony PERARD <anthony.perard@vates.tech>
>
> Otherwise, failed tests are ignored by automated test.

Fixes 7bf777b42cad

> Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [XEN PATCH 3/9] CI: Fix status check for tools/tests

[Andrew Cooper]

On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> From: Anthony PERARD <anthony.perard@vates.tech>
>
> Without "pipefail", $? have the exit value of `tee`, which should
> always be 0. But instead of using "pipefail", do collect the value of
> from the test with $PIPESTATUS.
>
> Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [XEN PATCH 1/9] CI: Add SELECTED_JOBS_ONLY to analyze.yaml

[Andrew Cooper]

On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> From: Anthony PERARD <anthony.perard@vates.tech>
>
> Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [XEN PATCH 6/9] CI: Upload junit result as artefact

[Andrew Cooper]

On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> From: Anthony PERARD <anthony.perard@vates.tech>
>
> This allow to investigate the file in cases of error. Not all
> jobs that extend ".adl-x86-64" are creating a "tests-junit.xml" but
> Gitlab only produce a warning when the file isn't found.

I'm not sure what you're trying to say here.

~Andrew

Re: [XEN PATCH 4/9] CI: Ignore run-tools-test return value

[Andrew Cooper]

On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> index 2750d24eba..046137a4a6 100755
> --- a/automation/scripts/qubes-x86-64.sh
> +++ b/automation/scripts/qubes-x86-64.sh
> @@ -135,10 +135,11 @@ done
>      ### tests: tools-tests-pv, tools-tests-pvh
>      "tools-tests-pv"|"tools-tests-pvh")
>          retrieve_xml=1
> -        passed="test passed"
> +        passed="run-tools-test over"
>          domU_check=""
>          dom0_check="
> -/root/run-tools-tests /usr/lib/xen/tests /tmp/tests-junit.xml && echo \"${passed}\"
> +/root/run-tools-tests /usr/lib/xen/tests /tmp/tests-junit.xml ||:
> +echo \"${passed}\"
>  nc -l -p 8080 < /tmp/tests-junit.xml >/dev/null &
>  "
>          if [ "${test_variant}" = "tools-tests-pvh" ]; then

I noticed this too while hacking on XTF support.  Also of note, I'm not
sure we want to be saying done to the expect script before sending the
results.

The underlying problem is that ${passed} isn't really a pass message;
it's a done+pass message, and there is no way to say "failed" to the
expect script, leaving timeout as the only option of signalling a failure.

I don't think we want to be ignoring errors from run-tools-tests
specifically, but arranging this is hard.  You always need something at
an outer level judging whether there was a good test result.

For XTF, the way I did this was to declare that anything didn't match
"^(Success|Error|Crash|Fail).*" on the final line of the console log to
be deemed to be Crash (because it's usually an uncaught exception).

For this, the best I can think of is to have a fixed outer script which
runs "./test/script && echo $success || echo $failure".  The internal
set -e will cause most unexpected conditions to be an error.

~Andrew

Re: [XEN PATCH 4/9] CI: Ignore run-tools-test return value

[Marek Marczykowski-Górecki]

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

On Tue, Jun 03, 2025 at 02:26:31PM +0100, Andrew Cooper wrote:
> On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> > diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> > index 2750d24eba..046137a4a6 100755
> > --- a/automation/scripts/qubes-x86-64.sh
> > +++ b/automation/scripts/qubes-x86-64.sh
> > @@ -135,10 +135,11 @@ done
> >      ### tests: tools-tests-pv, tools-tests-pvh
> >      "tools-tests-pv"|"tools-tests-pvh")
> >          retrieve_xml=1
> > -        passed="test passed"
> > +        passed="run-tools-test over"
> >          domU_check=""
> >          dom0_check="
> > -/root/run-tools-tests /usr/lib/xen/tests /tmp/tests-junit.xml && echo \"${passed}\"
> > +/root/run-tools-tests /usr/lib/xen/tests /tmp/tests-junit.xml ||:
> > +echo \"${passed}\"
> >  nc -l -p 8080 < /tmp/tests-junit.xml >/dev/null &
> >  "
> >          if [ "${test_variant}" = "tools-tests-pvh" ]; then
> 
> I noticed this too while hacking on XTF support.  Also of note, I'm not
> sure we want to be saying done to the expect script before sending the
> results.

No, expect needs to be notified before, otherwise retrieving the file
won't start.

> The underlying problem is that ${passed} isn't really a pass message;
> it's a done+pass message, and there is no way to say "failed" to the
> expect script, leaving timeout as the only option of signalling a failure.

Well, that can be changed, by adding yet another message that it looks
for, and handling it with exit 1 instead of exit 0.

> I don't think we want to be ignoring errors from run-tools-tests
> specifically, but arranging this is hard.  You always need something at
> an outer level judging whether there was a good test result.

For those tests specifically, if changing to always say success, you
need to ensure that both finding "failure" in xml and not getting the
xml at all (or getting it malformed) is treated as a failure.

> For XTF, the way I did this was to declare that anything didn't match
> "^(Success|Error|Crash|Fail).*" on the final line of the console log to
> be deemed to be Crash (because it's usually an uncaught exception).
> 
> For this, the best I can think of is to have a fixed outer script which
> runs "./test/script && echo $success || echo $failure".  The internal
> set -e will cause most unexpected conditions to be an error.
> 
> ~Andrew

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [XEN PATCH 5/9] CI: Have the gitlab job fail on tools/tests failure

[Andrew Cooper]

On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> From: Anthony PERARD <anthony.perard@vates.tech>
>
> We can't rely on an exit value from `run-tools-tests` since we only
> have the console output. `console.exp` only look for success or it
> times out. We could parse the console output, but the junit is more
> concise. Also check if we have it or fail as well.
>
> Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
> ---
>  automation/scripts/qubes-x86-64.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> index 046137a4a6..7a4c5ae489 100755
> --- a/automation/scripts/qubes-x86-64.sh
> +++ b/automation/scripts/qubes-x86-64.sh
> @@ -298,6 +298,13 @@ TEST_RESULT=$?
>  
>  if [ -n "$retrieve_xml" ]; then
>      nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
> +    # Findout if one of the test failed
> +    if ! grep -q '</testsuites>' tests-junit.xml; then
> +        echo "ERROR: tests-junit.xml is incomplete or missing."
> +        TEST_RESULT=1
> +    elif grep -q '</failure>' tests-junit.xml; then
> +        TEST_RESULT=1
> +    fi
>  fi
>  
>  exit "$TEST_RESULT"

A couple of things.

From my experimentation with junit,
https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1849342222/test_report?job_name=kbl-xtf-x86-64-gcc-debug
we can also use </error> for classification.  I'm also very disappointed
in Gitlab classifying <warning> as success.

Not for this patch, but for XTF I need to be able to express "tolerable
failure".  (All branches of Xen will run the same tests, and we don't
have OSSTest to deem "fail never passed" as non-blocking.)

Even if the job passes overall, I want tolerable failures to show up in
the UI, so I have to use <failure> in junit.xml.  But that means needing
to be more selective, and I don't have a good idea of how to do this. 
(I have one terrible idea, which is </failure type=tolerable"> which
will escape that grep, but it feels like (ab)buse of XML.)

~Andrew

Re: [XEN PATCH 5/9] CI: Have the gitlab job fail on tools/tests failure

[Marek Marczykowski-Górecki]

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

On Tue, Jun 03, 2025 at 02:41:50PM +0100, Andrew Cooper wrote:
> On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> > From: Anthony PERARD <anthony.perard@vates.tech>
> >
> > We can't rely on an exit value from `run-tools-tests` since we only
> > have the console output. `console.exp` only look for success or it
> > times out. We could parse the console output, but the junit is more
> > concise. Also check if we have it or fail as well.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
> > ---
> >  automation/scripts/qubes-x86-64.sh | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> > index 046137a4a6..7a4c5ae489 100755
> > --- a/automation/scripts/qubes-x86-64.sh
> > +++ b/automation/scripts/qubes-x86-64.sh
> > @@ -298,6 +298,13 @@ TEST_RESULT=$?
> >  
> >  if [ -n "$retrieve_xml" ]; then
> >      nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
> > +    # Findout if one of the test failed
> > +    if ! grep -q '</testsuites>' tests-junit.xml; then
> > +        echo "ERROR: tests-junit.xml is incomplete or missing."
> > +        TEST_RESULT=1
> > +    elif grep -q '</failure>' tests-junit.xml; then
> > +        TEST_RESULT=1
> > +    fi
> >  fi
> >  
> >  exit "$TEST_RESULT"
> 
> A couple of things.
> 
> From my experimentation with junit,
> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1849342222/test_report?job_name=kbl-xtf-x86-64-gcc-debug
> we can also use </error> for classification.  I'm also very disappointed
> in Gitlab classifying <warning> as success.
> 
> Not for this patch, but for XTF I need to be able to express "tolerable
> failure".  (All branches of Xen will run the same tests, and we don't
> have OSSTest to deem "fail never passed" as non-blocking.)
> 
> Even if the job passes overall, I want tolerable failures to show up in
> the UI, so I have to use <failure> in junit.xml.  But that means needing
> to be more selective, and I don't have a good idea of how to do this. 
> (I have one terrible idea, which is </failure type=tolerable"> which
> will escape that grep, but it feels like (ab)buse of XML.)

But that automation/ dir (including the run-tools-tests script) is
per-branch, so you can specify there which tests should be considered
failure and which just warning, no? It will require few more bits in the
script, but fundamentally shouldn't be a problem?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [XEN PATCH 5/9] CI: Have the gitlab job fail on tools/tests failure

[Andrew Cooper]

On 03/06/2025 2:44 pm, Marek Marczykowski-Górecki wrote:
> On Tue, Jun 03, 2025 at 02:41:50PM +0100, Andrew Cooper wrote:
>> On 03/06/2025 1:42 pm, Anthony PERARD wrote:
>>> From: Anthony PERARD <anthony.perard@vates.tech>
>>>
>>> We can't rely on an exit value from `run-tools-tests` since we only
>>> have the console output. `console.exp` only look for success or it
>>> times out. We could parse the console output, but the junit is more
>>> concise. Also check if we have it or fail as well.
>>>
>>> Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
>>> ---
>>>  automation/scripts/qubes-x86-64.sh | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
>>> index 046137a4a6..7a4c5ae489 100755
>>> --- a/automation/scripts/qubes-x86-64.sh
>>> +++ b/automation/scripts/qubes-x86-64.sh
>>> @@ -298,6 +298,13 @@ TEST_RESULT=$?
>>>  
>>>  if [ -n "$retrieve_xml" ]; then
>>>      nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
>>> +    # Findout if one of the test failed
>>> +    if ! grep -q '</testsuites>' tests-junit.xml; then
>>> +        echo "ERROR: tests-junit.xml is incomplete or missing."
>>> +        TEST_RESULT=1
>>> +    elif grep -q '</failure>' tests-junit.xml; then
>>> +        TEST_RESULT=1
>>> +    fi
>>>  fi
>>>  
>>>  exit "$TEST_RESULT"
>> A couple of things.
>>
>> From my experimentation with junit,
>> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1849342222/test_report?job_name=kbl-xtf-x86-64-gcc-debug
>> we can also use </error> for classification.  I'm also very disappointed
>> in Gitlab classifying <warning> as success.
>>
>> Not for this patch, but for XTF I need to be able to express "tolerable
>> failure".  (All branches of Xen will run the same tests, and we don't
>> have OSSTest to deem "fail never passed" as non-blocking.)
>>
>> Even if the job passes overall, I want tolerable failures to show up in
>> the UI, so I have to use <failure> in junit.xml.  But that means needing
>> to be more selective, and I don't have a good idea of how to do this. 
>> (I have one terrible idea, which is </failure type=tolerable"> which
>> will escape that grep, but it feels like (ab)buse of XML.)
> But that automation/ dir (including the run-tools-tests script) is
> per-branch, so you can specify there which tests should be considered
> failure and which just warning, no? It will require few more bits in the
> script, but fundamentally shouldn't be a problem?
>

XTF is in a separate repo and does not have branches.

Now consider
https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=d965e2ee07c56c341d8896852550914d87ea5374,
and testing it.

Anything I put into XTF to test it will pass on staging but fail on the
older stable-* trees.  In due course it will get backported to the
bugfix branches, but it likely won't get fixed on the security-only
branches.

Furthermore, I need to not change xen.git to make this work, and older
branches need to pick up newer XTF automatically.  (XSA tests *are*
expected to pass everywhere once the issue is public.)

My current plan is to have logic of the form:

if ( xenver >= $STAGING )
    xtf_failure(...);
else
    xtf_success("Expected Failure:" ...);

where $STAGING moves when backports get done.  I still want the failures
to show up in the Tests UI.

~Andrew

Re: [XEN PATCH 8/9] CI: Workaround extra content in junit

[Andrew Cooper]

On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> From: Anthony PERARD <anthony.perard@vates.tech>
>
> Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
> ---
>  automation/scripts/qubes-x86-64.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
> index 7a4c5ae489..6ab8412f45 100755
> --- a/automation/scripts/qubes-x86-64.sh
> +++ b/automation/scripts/qubes-x86-64.sh
> @@ -298,6 +298,12 @@ TEST_RESULT=$?
>  
>  if [ -n "$retrieve_xml" ]; then
>      nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
> +    # Workaround duplicated data been received
> +    sed -i.old '/^<\/testsuites>/q' tests-junit.xml > /dev/null
> +    extra_line_in_junit=$(($(wc -l < tests-junit.xml.old) - $(wc -l < tests-junit.xml)))
> +    if [ $extra_line_in_junit -gt 0 ]; then
> +        echo "WARNING: Found $extra_line_in_junit too many lines in junit."
> +    fi

Is this the cause of
https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1849342222/test_report
getting a row of 0's for ADL ?

Why are we getting duplicate data?  nc is running in TCP mode, not UDP,
so it's not that.

~Andrew

Re: [XEN PATCH 9/9] CI: Add timing to junit

[Andrew Cooper]

On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> From: Anthony PERARD <anthony.perard@vates.tech>
>
> Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  automation/scripts/run-tools-tests | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
> index 852c1cfbcf..e38cc4068c 100755
> --- a/automation/scripts/run-tools-tests
> +++ b/automation/scripts/run-tools-tests
> @@ -18,9 +18,12 @@ for f in "$1"/*; do
>          continue
>      fi
>      echo "Running $f"
> -    printf '  <testcase name="%s">\n' "$f" >> "$xml_out"
> +    time_start=$EPOCHREALTIME
>      "$f" 2>&1 | tee /tmp/out
>      ret=${PIPESTATUS[0]}
> +    time_end=$EPOCHREALTIME
> +    time_test="$(bc <<<"$time_end - $time_start")"
> +    printf '  <testcase name="%s" time="%f">\n' "$f" "$time_test" >> "$xml_out"

I'd suggest $time_delta rather than $time_test.

~Andrew

Re: [XEN PATCH 5/9] CI: Have the gitlab job fail on tools/tests failure

[Andrew Cooper]

On 03/06/2025 3:09 pm, Andrew Cooper wrote:
> On 03/06/2025 2:44 pm, Marek Marczykowski-Górecki wrote:
>> On Tue, Jun 03, 2025 at 02:41:50PM +0100, Andrew Cooper wrote:
>>> On 03/06/2025 1:42 pm, Anthony PERARD wrote:
>>>> From: Anthony PERARD <anthony.perard@vates.tech>
>>>>
>>>> We can't rely on an exit value from `run-tools-tests` since we only
>>>> have the console output. `console.exp` only look for success or it
>>>> times out. We could parse the console output, but the junit is more
>>>> concise. Also check if we have it or fail as well.
>>>>
>>>> Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
>>>> ---
>>>>  automation/scripts/qubes-x86-64.sh | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/automation/scripts/qubes-x86-64.sh b/automation/scripts/qubes-x86-64.sh
>>>> index 046137a4a6..7a4c5ae489 100755
>>>> --- a/automation/scripts/qubes-x86-64.sh
>>>> +++ b/automation/scripts/qubes-x86-64.sh
>>>> @@ -298,6 +298,13 @@ TEST_RESULT=$?
>>>>  
>>>>  if [ -n "$retrieve_xml" ]; then
>>>>      nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
>>>> +    # Findout if one of the test failed
>>>> +    if ! grep -q '</testsuites>' tests-junit.xml; then
>>>> +        echo "ERROR: tests-junit.xml is incomplete or missing."
>>>> +        TEST_RESULT=1
>>>> +    elif grep -q '</failure>' tests-junit.xml; then
>>>> +        TEST_RESULT=1
>>>> +    fi
>>>>  fi
>>>>  
>>>>  exit "$TEST_RESULT"
>>> A couple of things.
>>>
>>> From my experimentation with junit,
>>> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1849342222/test_report?job_name=kbl-xtf-x86-64-gcc-debug
>>> we can also use </error> for classification.  I'm also very disappointed
>>> in Gitlab classifying <warning> as success.
>>>
>>> Not for this patch, but for XTF I need to be able to express "tolerable
>>> failure".  (All branches of Xen will run the same tests, and we don't
>>> have OSSTest to deem "fail never passed" as non-blocking.)
>>>
>>> Even if the job passes overall, I want tolerable failures to show up in
>>> the UI, so I have to use <failure> in junit.xml.  But that means needing
>>> to be more selective, and I don't have a good idea of how to do this. 
>>> (I have one terrible idea, which is </failure type=tolerable"> which
>>> will escape that grep, but it feels like (ab)buse of XML.)
>> But that automation/ dir (including the run-tools-tests script) is
>> per-branch, so you can specify there which tests should be considered
>> failure and which just warning, no? It will require few more bits in the
>> script, but fundamentally shouldn't be a problem?
>>
> XTF is in a separate repo and does not have branches.
>
> Now consider
> https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=d965e2ee07c56c341d8896852550914d87ea5374,
> and testing it.
>
> Anything I put into XTF to test it will pass on staging but fail on the
> older stable-* trees.  In due course it will get backported to the
> bugfix branches, but it likely won't get fixed on the security-only
> branches.
>
> Furthermore, I need to not change xen.git to make this work, and older
> branches need to pick up newer XTF automatically.  (XSA tests *are*
> expected to pass everywhere once the issue is public.)
>
> My current plan is to have logic of the form:
>
> if ( xenver >= $STAGING )
>     xtf_failure(...);
> else
>     xtf_success("Expected Failure:" ...);
>
> where $STAGING moves when backports get done.  I still want the failures
> to show up in the Tests UI.

P.S. I'm planning to teach ./xtf-runner how to write out a junit.xml
directly.  I'm not interested in interpreting python's stdout and
writing xml in shell...

~Andrew

Re: [XEN PATCH 5/9] CI: Have the gitlab job fail on tools/tests failure

[Anthony PERARD]

On Tue, Jun 03, 2025 at 02:41:50PM +0100, Andrew Cooper wrote:
> On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> >  if [ -n "$retrieve_xml" ]; then
> >      nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
> > +    # Findout if one of the test failed
> > +    if ! grep -q '</testsuites>' tests-junit.xml; then
> > +        echo "ERROR: tests-junit.xml is incomplete or missing."
> > +        TEST_RESULT=1
> > +    elif grep -q '</failure>' tests-junit.xml; then
> > +        TEST_RESULT=1
> > +    fi
> >  fi
> >  
> >  exit "$TEST_RESULT"
> 
> A couple of things.
> 
> From my experimentation with junit,
> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1849342222/test_report?job_name=kbl-xtf-x86-64-gcc-debug
> we can also use </error> for classification.  I'm also very disappointed
> in Gitlab classifying <warning> as success.

According to the documentation [1] which point to this junit xml format [2]
the only elements (and path) are:
    testsuites.testsuite.testcase.failure
"error" or "warning" don't exist.
There's the attributes `type` in <failure> but this isn't explained how
it's used.

But I guess if we follow the link in [2], go through web.archive.org, we
can find [3] which has "skipped", "error", "failure", but still no
"warning".


[1] https://docs.gitlab.com/ci/testing/unit_test_reports/#unit-test-reporting-workflow
[2] https://www.ibm.com/docs/en/developer-for-zos/16.0?topic=formats-junit-xml-format
[3] https://github.com/windyroad/JUnit-Schema/blob/master/JUnit.xsd


> Not for this patch, but for XTF I need to be able to express "tolerable
> failure".  (All branches of Xen will run the same tests, and we don't
> have OSSTest to deem "fail never passed" as non-blocking.)

According to [1], there's a notion of "Existing failures", but that
might show up only on merge request.

> Even if the job passes overall, I want tolerable failures to show up in
> the UI, so I have to use <failure> in junit.xml.  But that means needing
> to be more selective, and I don't have a good idea of how to do this. 
> (I have one terrible idea, which is </failure type=tolerable"> which
> will escape that grep, but it feels like (ab)buse of XML.)

At the moment, `run-tools-tests` write '<failure type="failure"' on
failure, so we could grep on that instead event if it is sligtly more
fragile. I've choosen to grep on '</failure>' at first because that's
much less likely to be written differently, while the attributes in the
tag could be written in a different order.

Then, we can always use `sed` and extract the "type" to check it:
sed -n 's/.*<failure \(\|.* \)type="\([^"]\+\)" .*/\2/p' tests-junit.xml | while read type; do
  case $type in
    failure) echo fail;;
    tolerable) echo ok;;
    *) echo error unknwon type $type;;
  esac
done
But that maybe going a bit too far :-)

Cheers,

-- 
Anthony PERARD

Re: [XEN PATCH 6/9] CI: Upload junit result as artefact

[Anthony PERARD]

On Tue, Jun 03, 2025 at 02:13:59PM +0100, Andrew Cooper wrote:
> On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> > From: Anthony PERARD <anthony.perard@vates.tech>
> >
> > This allow to investigate the file in cases of error. Not all
> > jobs that extend ".adl-x86-64" are creating a "tests-junit.xml" but
> > Gitlab only produce a warning when the file isn't found.
> 
> I'm not sure what you're trying to say here.

I guess I can shorten the message to (with some rewording):
    "This allows to investigate the junit file in cases of parse error."

I guess it doesn't matter if a gitlab job is expected to produce
"tests-junit.xml" or not.

Thanks,

-- 
Anthony PERARD

Re: [XEN PATCH 8/9] CI: Workaround extra content in junit

[Anthony PERARD]

On Tue, Jun 03, 2025 at 03:12:32PM +0100, Andrew Cooper wrote:
> On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> >  if [ -n "$retrieve_xml" ]; then
> >      nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
> > +    # Workaround duplicated data been received
> > +    sed -i.old '/^<\/testsuites>/q' tests-junit.xml > /dev/null
> > +    extra_line_in_junit=$(($(wc -l < tests-junit.xml.old) - $(wc -l < tests-junit.xml)))
> > +    if [ $extra_line_in_junit -gt 0 ]; then
> > +        echo "WARNING: Found $extra_line_in_junit too many lines in junit."
> > +    fi
> 
> Is this the cause of
> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1849342222/test_report
> getting a row of 0's for ADL ?

Well, the error I had was this one:
"FATAL: Extra content at the end of the document"
https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1848598740/test_report

And indeed, when I managed to dl the junit.xml, there were the end of
the document duplicated many times.

> Why are we getting duplicate data?  nc is running in TCP mode, not UDP,
> so it's not that.

I think Marek talked about some notwork equiment in the middle?
I managed to find in matrix where they were talk about this duplication
of data, well, with `nc` dl for ever, with lots of duplicated data:

https://matrix.to/#/!XcEgmbCouiNWHlGdHk:matrix.org/$OkZmPOandaPy_OVAU8hpoAs14JWHtI6rXYrIZawUqDE?via=matrix.org&via=nitro.chat&via=aperard.fr

from marmarek
> So, it appears to be a bug in pasta - the thing that podman uses to
> proxy traffic out of the container's network namespace. You know, an
> additional network stack in userspace.

I have no idea if it is the same issue, but I had extra content in the
junit file with nearly all my jobs.

Cheers,

-- 
Anthony PERARD

Re: [XEN PATCH 5/9] CI: Have the gitlab job fail on tools/tests failure

[Andrew Cooper]

On 03/06/2025 4:58 pm, Anthony PERARD wrote:
> On Tue, Jun 03, 2025 at 02:41:50PM +0100, Andrew Cooper wrote:
>> On 03/06/2025 1:42 pm, Anthony PERARD wrote:
>>>  if [ -n "$retrieve_xml" ]; then
>>>      nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
>>> +    # Findout if one of the test failed
>>> +    if ! grep -q '</testsuites>' tests-junit.xml; then
>>> +        echo "ERROR: tests-junit.xml is incomplete or missing."
>>> +        TEST_RESULT=1
>>> +    elif grep -q '</failure>' tests-junit.xml; then
>>> +        TEST_RESULT=1
>>> +    fi
>>>  fi
>>>  
>>>  exit "$TEST_RESULT"
>> A couple of things.
>>
>> From my experimentation with junit,
>> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1849342222/test_report?job_name=kbl-xtf-x86-64-gcc-debug
>> we can also use </error> for classification.  I'm also very disappointed
>> in Gitlab classifying <warning> as success.
> According to the documentation [1] which point to this junit xml format [2]
> the only elements (and path) are:
>     testsuites.testsuite.testcase.failure
> "error" or "warning" don't exist.
> There's the attributes `type` in <failure> but this isn't explained how
> it's used.
>
> But I guess if we follow the link in [2], go through web.archive.org, we
> can find [3] which has "skipped", "error", "failure", but still no
> "warning".
>
>
> [1] https://docs.gitlab.com/ci/testing/unit_test_reports/#unit-test-reporting-workflow
> [2] https://www.ibm.com/docs/en/developer-for-zos/16.0?topic=formats-junit-xml-format
> [3] https://github.com/windyroad/JUnit-Schema/blob/master/JUnit.xsd

I was also very disappointed with the documentation, which seems to be
almost non-existent.

But after some source diving:
https://gitlab.com/gitlab-org/gitlab-foss/-/blob/master/lib/gitlab/ci/parsers/test/junit.rb#L69-83

So anything which isn't recognised is deemed to be success.  Lovely :(

I'm also still none the wiser as to why Skip's system_out is a formatted
json blob, unlike the others.


>
>
>> Not for this patch, but for XTF I need to be able to express "tolerable
>> failure".  (All branches of Xen will run the same tests, and we don't
>> have OSSTest to deem "fail never passed" as non-blocking.)
> According to [1], there's a notion of "Existing failures", but that
> might show up only on merge request.
>
>> Even if the job passes overall, I want tolerable failures to show up in
>> the UI, so I have to use <failure> in junit.xml.  But that means needing
>> to be more selective, and I don't have a good idea of how to do this. 
>> (I have one terrible idea, which is </failure type=tolerable"> which
>> will escape that grep, but it feels like (ab)buse of XML.)
> At the moment, `run-tools-tests` write '<failure type="failure"' on
> failure, so we could grep on that instead event if it is sligtly more
> fragile. I've choosen to grep on '</failure>' at first because that's
> much less likely to be written differently, while the attributes in the
> tag could be written in a different order.
>
> Then, we can always use `sed` and extract the "type" to check it:
> sed -n 's/.*<failure \(\|.* \)type="\([^"]\+\)" .*/\2/p' tests-junit.xml | while read type; do
>   case $type in
>     failure) echo fail;;
>     tolerable) echo ok;;
>     *) echo error unknwon type $type;;
>   esac
> done
> But that maybe going a bit too far :-)

There's xq which might be able to do this more nicely, and is packaged
in alpine.

~Andrew

Re: [XEN PATCH 1/9] CI: Add SELECTED_JOBS_ONLY to analyze.yaml

[Stefano Stabellini]

On Tue, 3 Jun 2025, Andrew Cooper wrote:
> On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> > From: Anthony PERARD <anthony.perard@vates.tech>
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [XEN PATCH 3/9] CI: Fix status check for tools/tests

[Stefano Stabellini]

On Tue, 3 Jun 2025, Andrew Cooper wrote:
> On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> > From: Anthony PERARD <anthony.perard@vates.tech>
> >
> > Without "pipefail", $? have the exit value of `tee`, which should
> > always be 0. But instead of using "pipefail", do collect the value of
> > from the test with $PIPESTATUS.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [XEN PATCH 7/9] CI: Use CDATA avoid the need to escape tests outputs

[Stefano Stabellini]

On Tue, 3 Jun 2025, Andrew Cooper wrote:
> On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> > diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
> > index 695ed77e46..852c1cfbcf 100755
> > --- a/automation/scripts/run-tools-tests
> > +++ b/automation/scripts/run-tools-tests
> > @@ -25,9 +25,9 @@ for f in "$1"/*; do
> >          echo "FAILED: $f"
> >          failed+=" $f"
> >          printf '   <failure type="failure" message="binary %s exited with code %d">\n' "$f" "$ret" >> "$xml_out"
> > -        # TODO: could use xml escaping... but current tests seems to
> > -        # produce sane output
> > +        printf '<![CDATA[' >> "$xml_out"
> >          cat /tmp/out >> "$xml_out"
> > +        printf ']]>' >> "$xml_out"
> 
> I think you want a \n on this printf.
> 
> I'd also suggest leaving a TODO for "escape ]]> if necessary".
> 
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [XEN PATCH 8/9] CI: Workaround extra content in junit

[Stefano Stabellini]

On Tue, 3 Jun 2025, Anthony PERARD wrote:
> On Tue, Jun 03, 2025 at 03:12:32PM +0100, Andrew Cooper wrote:
> > On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> > >  if [ -n "$retrieve_xml" ]; then
> > >      nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
> > > +    # Workaround duplicated data been received
> > > +    sed -i.old '/^<\/testsuites>/q' tests-junit.xml > /dev/null
> > > +    extra_line_in_junit=$(($(wc -l < tests-junit.xml.old) - $(wc -l < tests-junit.xml)))
> > > +    if [ $extra_line_in_junit -gt 0 ]; then
> > > +        echo "WARNING: Found $extra_line_in_junit too many lines in junit."
> > > +    fi
> > 
> > Is this the cause of
> > https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1849342222/test_report
> > getting a row of 0's for ADL ?
> 
> Well, the error I had was this one:
> "FATAL: Extra content at the end of the document"
> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1848598740/test_report
> 
> And indeed, when I managed to dl the junit.xml, there were the end of
> the document duplicated many times.

Wouldn't it better to do |sort|uniq to dedup the file?

Re: [XEN PATCH 9/9] CI: Add timing to junit

[Stefano Stabellini]

On Tue, 3 Jun 2025, Anthony PERARD wrote:
> From: Anthony PERARD <anthony.perard@vates.tech>
> 
> Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
> ---
>  automation/scripts/run-tools-tests | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
> index 852c1cfbcf..e38cc4068c 100755
> --- a/automation/scripts/run-tools-tests
> +++ b/automation/scripts/run-tools-tests
> @@ -18,9 +18,12 @@ for f in "$1"/*; do
>          continue
>      fi
>      echo "Running $f"
> -    printf '  <testcase name="%s">\n' "$f" >> "$xml_out"
> +    time_start=$EPOCHREALTIME
>      "$f" 2>&1 | tee /tmp/out
>      ret=${PIPESTATUS[0]}
> +    time_end=$EPOCHREALTIME
> +    time_test="$(bc <<<"$time_end - $time_start")"

As it looks like no other scripts need bc at the moment but we already
rely on awk (automation/scripts/xilinx-smoke-dom0less-arm64.sh) I'd
prefer this version:

time_test="$(awk "BEGIN {print $time_end - $time_start}")"


> +    printf '  <testcase name="%s" time="%f">\n' "$f" "$time_test" >> "$xml_out"
>      if [ "$ret" -ne 0 ]; then
>          echo "FAILED: $f"
>          failed+=" $f"
> -- 
> Anthony PERARD
> 

Re: [XEN PATCH 8/9] CI: Workaround extra content in junit

[Marek Marczykowski-Górecki]

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

On Tue, Jun 03, 2025 at 11:29:11AM -0700, Stefano Stabellini wrote:
> On Tue, 3 Jun 2025, Anthony PERARD wrote:
> > On Tue, Jun 03, 2025 at 03:12:32PM +0100, Andrew Cooper wrote:
> > > On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> > > >  if [ -n "$retrieve_xml" ]; then
> > > >      nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
> > > > +    # Workaround duplicated data been received
> > > > +    sed -i.old '/^<\/testsuites>/q' tests-junit.xml > /dev/null
> > > > +    extra_line_in_junit=$(($(wc -l < tests-junit.xml.old) - $(wc -l < tests-junit.xml)))
> > > > +    if [ $extra_line_in_junit -gt 0 ]; then
> > > > +        echo "WARNING: Found $extra_line_in_junit too many lines in junit."
> > > > +    fi
> > > 
> > > Is this the cause of
> > > https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1849342222/test_report
> > > getting a row of 0's for ADL ?
> > 
> > Well, the error I had was this one:
> > "FATAL: Extra content at the end of the document"
> > https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1848598740/test_report
> > 
> > And indeed, when I managed to dl the junit.xml, there were the end of
> > the document duplicated many times.
> 
> Wouldn't it better to do |sort|uniq to dedup the file?

I don't think XML structure will survive it...

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [XEN PATCH 8/9] CI: Workaround extra content in junit

[Stefano Stabellini]

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

On Tue, 3 Jun 2025, Marek Marczykowski-Górecki wrote:
> On Tue, Jun 03, 2025 at 11:29:11AM -0700, Stefano Stabellini wrote:
> > On Tue, 3 Jun 2025, Anthony PERARD wrote:
> > > On Tue, Jun 03, 2025 at 03:12:32PM +0100, Andrew Cooper wrote:
> > > > On 03/06/2025 1:42 pm, Anthony PERARD wrote:
> > > > >  if [ -n "$retrieve_xml" ]; then
> > > > >      nc -w 10 "$SUT_ADDR" 8080 > tests-junit.xml </dev/null
> > > > > +    # Workaround duplicated data been received
> > > > > +    sed -i.old '/^<\/testsuites>/q' tests-junit.xml > /dev/null
> > > > > +    extra_line_in_junit=$(($(wc -l < tests-junit.xml.old) - $(wc -l < tests-junit.xml)))
> > > > > +    if [ $extra_line_in_junit -gt 0 ]; then
> > > > > +        echo "WARNING: Found $extra_line_in_junit too many lines in junit."
> > > > > +    fi
> > > > 
> > > > Is this the cause of
> > > > https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1849342222/test_report
> > > > getting a row of 0's for ADL ?
> > > 
> > > Well, the error I had was this one:
> > > "FATAL: Extra content at the end of the document"
> > > https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1848598740/test_report
> > > 
> > > And indeed, when I managed to dl the junit.xml, there were the end of
> > > the document duplicated many times.
> > 
> > Wouldn't it better to do |sort|uniq to dedup the file?
> 
> I don't think XML structure will survive it...

Ops, good point.

Re: [XEN PATCH 9/9] CI: Add timing to junit

[Anthony PERARD]

On Tue, Jun 03, 2025 at 11:35:22AM -0700, Stefano Stabellini wrote:
> On Tue, 3 Jun 2025, Anthony PERARD wrote:
> > From: Anthony PERARD <anthony.perard@vates.tech>
> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
> > ---
> >  automation/scripts/run-tools-tests | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
> > index 852c1cfbcf..e38cc4068c 100755
> > --- a/automation/scripts/run-tools-tests
> > +++ b/automation/scripts/run-tools-tests
> > @@ -18,9 +18,12 @@ for f in "$1"/*; do
> >          continue
> >      fi
> >      echo "Running $f"
> > -    printf '  <testcase name="%s">\n' "$f" >> "$xml_out"
> > +    time_start=$EPOCHREALTIME
> >      "$f" 2>&1 | tee /tmp/out
> >      ret=${PIPESTATUS[0]}
> > +    time_end=$EPOCHREALTIME
> > +    time_test="$(bc <<<"$time_end - $time_start")"
> 
> As it looks like no other scripts need bc at the moment but we already
> rely on awk (automation/scripts/xilinx-smoke-dom0less-arm64.sh) I'd
> prefer this version:
> 
> time_test="$(awk "BEGIN {print $time_end - $time_start}")"

You mean I have to choose between busybox and busybox?

$ ls -l $(which bc awk)
lrwxrwxrwx    1 root     root            12 Feb 13 23:19 /usr/bin/awk -> /bin/busybox
lrwxrwxrwx    1 root     root            12 Feb 13 23:19 /usr/bin/bc -> /bin/busybox

:-)

I guess it doesn't really matter.

One difference though:
$ awk "BEGIN {print $time_end - $time_start}"
3.28798
$ bc <<<"$time_end - $time_start"
3.287982

awk is less precise, but I guess that doesn't matter as well, gitlab UI
isn't going to show the extra digits.

So I guess I can change to use `awk` instead, just in case for some
reason `bc` isn't present, since `awk` seems mandatory for many of our
scripts.

Thanks,

-- 
Anthony PERARD

Re: [XEN PATCH 9/9] CI: Add timing to junit

[Andrew Cooper]

On 04/06/2025 9:48 am, Anthony PERARD wrote:
> On Tue, Jun 03, 2025 at 11:35:22AM -0700, Stefano Stabellini wrote:
>> On Tue, 3 Jun 2025, Anthony PERARD wrote:
>>> From: Anthony PERARD <anthony.perard@vates.tech>
>>>
>>> Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
>>> ---
>>>  automation/scripts/run-tools-tests | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
>>> index 852c1cfbcf..e38cc4068c 100755
>>> --- a/automation/scripts/run-tools-tests
>>> +++ b/automation/scripts/run-tools-tests
>>> @@ -18,9 +18,12 @@ for f in "$1"/*; do
>>>          continue
>>>      fi
>>>      echo "Running $f"
>>> -    printf '  <testcase name="%s">\n' "$f" >> "$xml_out"
>>> +    time_start=$EPOCHREALTIME
>>>      "$f" 2>&1 | tee /tmp/out
>>>      ret=${PIPESTATUS[0]}
>>> +    time_end=$EPOCHREALTIME
>>> +    time_test="$(bc <<<"$time_end - $time_start")"
>> As it looks like no other scripts need bc at the moment but we already
>> rely on awk (automation/scripts/xilinx-smoke-dom0less-arm64.sh) I'd
>> prefer this version:
>>
>> time_test="$(awk "BEGIN {print $time_end - $time_start}")"
> You mean I have to choose between busybox and busybox?
>
> $ ls -l $(which bc awk)
> lrwxrwxrwx    1 root     root            12 Feb 13 23:19 /usr/bin/awk -> /bin/busybox
> lrwxrwxrwx    1 root     root            12 Feb 13 23:19 /usr/bin/bc -> /bin/busybox
>
> :-)
>
> I guess it doesn't really matter.
>
> One difference though:
> $ awk "BEGIN {print $time_end - $time_start}"
> 3.28798
> $ bc <<<"$time_end - $time_start"
> 3.287982
>
> awk is less precise, but I guess that doesn't matter as well, gitlab UI
> isn't going to show the extra digits.
>
> So I guess I can change to use `awk` instead, just in case for some
> reason `bc` isn't present, since `awk` seems mandatory for many of our
> scripts.
>
> Thanks,

bc is a normal posix utility just like awk is, so there's no change in
dependencies caused by this.

Furthermore, it is the right tool for this job in a way that awk isn't. 
Besides the reduction in precision, see xen/tools/check-endbr.sh for the
number of ways we found that awk malfunctions with large numbers.

I firmly suggest continuing to use bc here.

~Andrew

Re: [XEN PATCH 9/9] CI: Add timing to junit

[Marek Marczykowski-Górecki]

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

On Wed, Jun 04, 2025 at 11:57:11AM +0100, Andrew Cooper wrote:
> On 04/06/2025 9:48 am, Anthony PERARD wrote:
> > On Tue, Jun 03, 2025 at 11:35:22AM -0700, Stefano Stabellini wrote:
> >> On Tue, 3 Jun 2025, Anthony PERARD wrote:
> >>> From: Anthony PERARD <anthony.perard@vates.tech>
> >>>
> >>> Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
> >>> ---
> >>>  automation/scripts/run-tools-tests | 5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
> >>> index 852c1cfbcf..e38cc4068c 100755
> >>> --- a/automation/scripts/run-tools-tests
> >>> +++ b/automation/scripts/run-tools-tests
> >>> @@ -18,9 +18,12 @@ for f in "$1"/*; do
> >>>          continue
> >>>      fi
> >>>      echo "Running $f"
> >>> -    printf '  <testcase name="%s">\n' "$f" >> "$xml_out"
> >>> +    time_start=$EPOCHREALTIME
> >>>      "$f" 2>&1 | tee /tmp/out
> >>>      ret=${PIPESTATUS[0]}
> >>> +    time_end=$EPOCHREALTIME
> >>> +    time_test="$(bc <<<"$time_end - $time_start")"
> >> As it looks like no other scripts need bc at the moment but we already
> >> rely on awk (automation/scripts/xilinx-smoke-dom0less-arm64.sh) I'd
> >> prefer this version:
> >>
> >> time_test="$(awk "BEGIN {print $time_end - $time_start}")"
> > You mean I have to choose between busybox and busybox?
> >
> > $ ls -l $(which bc awk)
> > lrwxrwxrwx    1 root     root            12 Feb 13 23:19 /usr/bin/awk -> /bin/busybox
> > lrwxrwxrwx    1 root     root            12 Feb 13 23:19 /usr/bin/bc -> /bin/busybox
> >
> > :-)
> >
> > I guess it doesn't really matter.
> >
> > One difference though:
> > $ awk "BEGIN {print $time_end - $time_start}"
> > 3.28798
> > $ bc <<<"$time_end - $time_start"
> > 3.287982
> >
> > awk is less precise, but I guess that doesn't matter as well, gitlab UI
> > isn't going to show the extra digits.
> >
> > So I guess I can change to use `awk` instead, just in case for some
> > reason `bc` isn't present, since `awk` seems mandatory for many of our
> > scripts.
> >
> > Thanks,
> 
> bc is a normal posix utility just like awk is, so there's no change in
> dependencies caused by this.

Linux requires bc for building, and I _very_ often find systems where it
isn't installed by default (by awk is)...
Anyway, that's probably irrelevant in the CI container that has busybox
for both.

> Furthermore, it is the right tool for this job in a way that awk isn't. 
> Besides the reduction in precision, see xen/tools/check-endbr.sh for the
> number of ways we found that awk malfunctions with large numbers.
> 
> I firmly suggest continuing to use bc here.
> 
> ~Andrew

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [XEN PATCH 9/9] CI: Add timing to junit

[Stefano Stabellini]

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

On Wed, 4 Jun 2025, Marek Marczykowski-Górecki wrote:
> On Wed, Jun 04, 2025 at 11:57:11AM +0100, Andrew Cooper wrote:
> > On 04/06/2025 9:48 am, Anthony PERARD wrote:
> > > On Tue, Jun 03, 2025 at 11:35:22AM -0700, Stefano Stabellini wrote:
> > >> On Tue, 3 Jun 2025, Anthony PERARD wrote:
> > >>> From: Anthony PERARD <anthony.perard@vates.tech>
> > >>>
> > >>> Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
> > >>> ---
> > >>>  automation/scripts/run-tools-tests | 5 ++++-
> > >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/automation/scripts/run-tools-tests b/automation/scripts/run-tools-tests
> > >>> index 852c1cfbcf..e38cc4068c 100755
> > >>> --- a/automation/scripts/run-tools-tests
> > >>> +++ b/automation/scripts/run-tools-tests
> > >>> @@ -18,9 +18,12 @@ for f in "$1"/*; do
> > >>>          continue
> > >>>      fi
> > >>>      echo "Running $f"
> > >>> -    printf '  <testcase name="%s">\n' "$f" >> "$xml_out"
> > >>> +    time_start=$EPOCHREALTIME
> > >>>      "$f" 2>&1 | tee /tmp/out
> > >>>      ret=${PIPESTATUS[0]}
> > >>> +    time_end=$EPOCHREALTIME
> > >>> +    time_test="$(bc <<<"$time_end - $time_start")"
> > >> As it looks like no other scripts need bc at the moment but we already
> > >> rely on awk (automation/scripts/xilinx-smoke-dom0less-arm64.sh) I'd
> > >> prefer this version:
> > >>
> > >> time_test="$(awk "BEGIN {print $time_end - $time_start}")"
> > > You mean I have to choose between busybox and busybox?
> > >
> > > $ ls -l $(which bc awk)
> > > lrwxrwxrwx    1 root     root            12 Feb 13 23:19 /usr/bin/awk -> /bin/busybox
> > > lrwxrwxrwx    1 root     root            12 Feb 13 23:19 /usr/bin/bc -> /bin/busybox
> > >
> > > :-)
> > >
> > > I guess it doesn't really matter.
> > >
> > > One difference though:
> > > $ awk "BEGIN {print $time_end - $time_start}"
> > > 3.28798
> > > $ bc <<<"$time_end - $time_start"
> > > 3.287982
> > >
> > > awk is less precise, but I guess that doesn't matter as well, gitlab UI
> > > isn't going to show the extra digits.
> > >
> > > So I guess I can change to use `awk` instead, just in case for some
> > > reason `bc` isn't present, since `awk` seems mandatory for many of our
> > > scripts.
> > >
> > > Thanks,
> > 
> > bc is a normal posix utility just like awk is, so there's no change in
> > dependencies caused by this.
> 
> Linux requires bc for building, and I _very_ often find systems where it
> isn't installed by default (by awk is)...
> Anyway, that's probably irrelevant in the CI container that has busybox
> for both.

Yes, I see the same. Honestly, I wouldn't mind having a dependecy on bc,
it is common enough and available enough, it is just that we are not
doing a great job at tracking this type of dependencies in test scripts,
so I just thought the less we have the better and in this case there is
an easy solution. Anyway, this is not the hill I am hill I am going to
die on.