[PATCH 0/2] Work around various report.xml compatibility issues

[PATCH 0/2] Work around various report.xml compatibility issues

[Theodore Ts'o]

While updating {kvm,gce}-xfstests to work with fstests upstream, we
found that Darrick's changes have led to some incompatibilities.  The
most serious one is that it breaks the ability of the junitxml Python
package from being able to parse the report.xml file.  I've worked
around this in my test appliance in the first patch, via the simple
expedient of simply nuking the xmlns attribute from the testcases tag.
However, this may be a compatibility issue that will impact other
fstests users.

The second issue is one which depending on which xunit/Jenkins schema
you believe, the timestamp attribute may possibly not allow the use of
a timezone.  Again, I've worked around it in my test appliance, but it
might be that this will cause compatibility problem with other xunit
parsers.

I know that the report.xml file is strictly not conformat with the
xunit schema already, as discussed in fstests commit b76a6cdb40b5
("report: derive an xml schema for the xunit report").  However, I
believe that in order to allow fstests users to use xunit parsing
packages without having to write their own custom XML parsers, we
should strive to be compatible with as many xunit tools as possible,
whenever it's reasonably possible.

(It's kind of like HTML; no one actually follows the formal spec, so
HTML authoring tools need to be as compatible as possible as with the
most commmonly used browsers, and browsers need to be as flexible as
possible to support HTML files which are "mostly conformant".  :-)

So while this patch series is against fstests-bld, I'm hoping this can
spark a discussion about whether we should make some changes to
fstests to be more compatible in practice with various xunit file
consumers/parsers which might be out there.

Theodore Ts'o (2):
  test-appliance: edit out xmlns from the result.xml file
  test-appliance: support a timestamp specifier which contains a
    timezone

 test-appliance/files/root/runtests.sh                     | 1 +
 .../usr/lib/python3/dist-packages/gen_results_summary.py  | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.31.0

[PATCH 1/2] test-appliance: edit out xmlns from the result.xml file

[Theodore Ts'o]

Commit b76a6cdb40b5 ("report: derive an xml schema for the xunit
report") in fstests upstream adds an xmlns attribute to the xunit
<testcase/> tag.  Unfortunately, this breaks the junitparser.py Python
package, since it uses lxml Python package, and by adding an xmlns
specifier, junitxml.py would need to know the schema and map that to
namespace tag.

So edit it out of the xml file using sed, which relies on the fact
that fstests will add the xmlns file on a single line.  The "right"
way would be to use an XSLT processor, but that would bloat the test
appliance significantly.  So we'll just cheat for now while we discuss
with fstests upstream whether adding the xmlns attribute is really
worth the pain and incompatibility that it causes.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 test-appliance/files/root/runtests.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test-appliance/files/root/runtests.sh b/test-appliance/files/root/runtests.sh
index 9b32d287..c4ddb739 100755
--- a/test-appliance/files/root/runtests.sh
+++ b/test-appliance/files/root/runtests.sh
@@ -23,6 +23,7 @@ function copy_xunit_results()
 
     if test -f "$RESULT"
     then
+	sed -i.orig -e 's/xmlns=\".*\"//' "$RESULT"
 	if test -f "$RESULTS"
 	then
 	    merge_xunit "$RESULTS" "$RESULT"
-- 
2.31.0

[PATCH 2/2] test-appliance: support a timestamp specifier which contains a timezone

[Theodore Ts'o]

Commit 545315976e72 ("report: capture the time zone in the test report
timestamp") in the fstests upstream adds a timezone to the timestamp.
This is useful, but it breaks the strptime parsing of the timestamp.
It's not entirely clear that adding a timezone is legal.  According to
this schema[1] that it is a ISO 8061 date/time stamp where the
"Timezone may not be specified".  However in this schema[2], the
timestamp is just an optional string, and it says nothing about the
format of the timestamp.

[1] https://gist.github.com/jclosure/45d7005d120d90ba6430130356e4cd61#file-xunit-xsd-L140
[2] https://github.com/junit-team/junit5/blob/main/platform-tests/src/test/resources/jenkins-junit.xsd#L96

It's not entirely clear which schema is "official", but in the spirit
of the second part of Jon Postel's Robustness principal --- "be
liberal in what you accept" --- fix gen_results_summary.py to accept
an ISO 8061 timestamp with or without the timestamp.

It might be that in the spirit of "be conservative in what you send",
fstests upstream should use the moral equivalent of 'date -u +"%F %T" |
sed -e "s/ /T/"' instead of 'date -Iseconds', but this change will
work either way, as well as with both the older and newer versions of
fstests.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 .../usr/lib/python3/dist-packages/gen_results_summary.py  | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/test-appliance/files/usr/lib/python3/dist-packages/gen_results_summary.py b/test-appliance/files/usr/lib/python3/dist-packages/gen_results_summary.py
index ecf5dc1a..44fb07d2 100644
--- a/test-appliance/files/usr/lib/python3/dist-packages/gen_results_summary.py
+++ b/test-appliance/files/usr/lib/python3/dist-packages/gen_results_summary.py
@@ -61,8 +61,12 @@ def parse_timestamp(timestamp):
     """Parse an ISO-8601-like timestamp as found in an xUnit file."""
     if timestamp == "":
         return 0
-    return time.mktime(datetime.strptime(timestamp,
-                                         '%Y-%m-%dT%H:%M:%S').timetuple())
+    for fmt in ('%Y-%m-%dT%H:%M:%S%z', '%Y-%m-%dT%H:%M:%S'):
+        try:
+            return time.mktime(datetime.strptime(timestamp, fmt).timetuple())
+        except ValueError:
+            pass
+    raise ValueError('no valid timestamp format found')
 
 def failed_tests(testsuite):
     """This iterator the failed tests from the testsuite."""
-- 
2.31.0

[PATCH 0/2] Work around various report.xml compatibility issues

[Theodore Ts'o]

While updating {kvm,gce}-xfstests to work with fstests upstream, we
found that Darrick's changes have led to some incompatibilities.  The
most serious one is that it breaks the ability of the junitxml Python
package from being able to parse the report.xml file.  I've worked
around this in my test appliance in the first patch, via the simple
expedient of simply nuking the xmlns attribute from the testcases tag.
However, this may be a compatibility issue that will impact other
fstests users.

The second issue is one which depending on which xunit/Jenkins schema
you believe, the timestamp attribute may possibly not allow the use of
a timezone.  Again, I've worked around it in my test appliance, but it
might be that this will cause compatibility problem with other xunit
parsers.

I know that the report.xml file is strictly not conformat with the
xunit schema already, as discussed in fstests commit b76a6cdb40b5
("report: derive an xml schema for the xunit report").  However, I
believe that in order to allow fstests users to use xunit parsing
packages without having to write their own custom XML parsers, we
should strive to be compatible with as many xunit tools as possible,
whenever it's reasonably possible.

(It's kind of like HTML; no one actually follows the formal spec, so
HTML authoring tools need to be as compatible as possible as with the
most commmonly used browsers, and browsers need to be as flexible as
possible to support HTML files which are "mostly conformant".  :-)

So while this patch series is against fstests-bld, I'm hoping this can
spark a discussion about whether we should make some changes to
fstests to be more compatible in practice with various xunit file
consumers/parsers which might be out there.

Theodore Ts'o (2):
  test-appliance: edit out xmlns from the result.xml file
  test-appliance: support a timestamp specifier which contains a
    timezone

 test-appliance/files/root/runtests.sh                     | 1 +
 .../usr/lib/python3/dist-packages/gen_results_summary.py  | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.31.0

[PATCH 2/2] test-appliance: support a timestamp specifier which contains a timezone

[Theodore Ts'o]

Commit 545315976e72 ("report: capture the time zone in the test report
timestamp") in the fstests upstream adds a timezone to the timestamp.
This is useful, but it breaks the strptime parsing of the timestamp.
It's not entirely clear that adding a timezone is legal.  According to
this schema[1] that it is a ISO 8061 date/time stamp where the
"Timezone may not be specified".  However in this schema[2], the
timestamp is just an optional string, and it says nothing about the
format of the timestamp.

[1] https://gist.github.com/jclosure/45d7005d120d90ba6430130356e4cd61#file-xunit-xsd-L140
[2] https://github.com/junit-team/junit5/blob/main/platform-tests/src/test/resources/jenkins-junit.xsd#L96

It's not entirely clear which schema is "official", but in the spirit
of the second part of Jon Postel's Robustness principal --- "be
liberal in what you accept" --- fix gen_results_summary.py to accept
an ISO 8061 timestamp with or without the timestamp.

It might be that in the spirit of "be conservative in what you send",
fstests upstream should use the moral equivalent of 'date -u +"%F %T" |
sed -e "s/ /T/"' instead of 'date -Iseconds', but this change will
work either way, as well as with both the older and newer versions of
fstests.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 .../usr/lib/python3/dist-packages/gen_results_summary.py  | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/test-appliance/files/usr/lib/python3/dist-packages/gen_results_summary.py b/test-appliance/files/usr/lib/python3/dist-packages/gen_results_summary.py
index ecf5dc1a..44fb07d2 100644
--- a/test-appliance/files/usr/lib/python3/dist-packages/gen_results_summary.py
+++ b/test-appliance/files/usr/lib/python3/dist-packages/gen_results_summary.py
@@ -61,8 +61,12 @@ def parse_timestamp(timestamp):
     """Parse an ISO-8601-like timestamp as found in an xUnit file."""
     if timestamp == "":
         return 0
-    return time.mktime(datetime.strptime(timestamp,
-                                         '%Y-%m-%dT%H:%M:%S').timetuple())
+    for fmt in ('%Y-%m-%dT%H:%M:%S%z', '%Y-%m-%dT%H:%M:%S'):
+        try:
+            return time.mktime(datetime.strptime(timestamp, fmt).timetuple())
+        except ValueError:
+            pass
+    raise ValueError('no valid timestamp format found')
 
 def failed_tests(testsuite):
     """This iterator the failed tests from the testsuite."""
-- 
2.31.0

Re: [PATCH 2/2] test-appliance: support a timestamp specifier which contains a timezone

[Darrick J. Wong]

On Thu, Apr 20, 2023 at 12:08:37PM -0400, Theodore Ts'o wrote:
> Commit 545315976e72 ("report: capture the time zone in the test report
> timestamp") in the fstests upstream adds a timezone to the timestamp.
> This is useful, but it breaks the strptime parsing of the timestamp.
> It's not entirely clear that adding a timezone is legal.  According to
> this schema[1] that it is a ISO 8061 date/time stamp where the
> "Timezone may not be specified".  However in this schema[2], the
> timestamp is just an optional string, and it says nothing about the
> format of the timestamp.
> 
> [1] https://gist.github.com/jclosure/45d7005d120d90ba6430130356e4cd61#file-xunit-xsd-L140
> [2] https://github.com/junit-team/junit5/blob/main/platform-tests/src/test/resources/jenkins-junit.xsd#L96
> 
> It's not entirely clear which schema is "official", but in the spirit
> of the second part of Jon Postel's Robustness principal --- "be
> liberal in what you accept" --- fix gen_results_summary.py to accept
> an ISO 8061 timestamp with or without the timestamp.

FWIW I added the explicit UTC offset because without it, we have to
assume local time.  That works poorly when your fstests cloud is
scattered around the world, because then the result timestamps go all
over the place.

> It might be that in the spirit of "be conservative in what you send",
> fstests upstream should use the moral equivalent of 'date -u +"%F %T" |
> sed -e "s/ /T/"' instead of 'date -Iseconds', but this change will
> work either way, as well as with both the older and newer versions of
> fstests.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Probably the only sane way to handle this, so...

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  .../usr/lib/python3/dist-packages/gen_results_summary.py  | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/test-appliance/files/usr/lib/python3/dist-packages/gen_results_summary.py b/test-appliance/files/usr/lib/python3/dist-packages/gen_results_summary.py
> index ecf5dc1a..44fb07d2 100644
> --- a/test-appliance/files/usr/lib/python3/dist-packages/gen_results_summary.py
> +++ b/test-appliance/files/usr/lib/python3/dist-packages/gen_results_summary.py
> @@ -61,8 +61,12 @@ def parse_timestamp(timestamp):
>      """Parse an ISO-8601-like timestamp as found in an xUnit file."""
>      if timestamp == "":
>          return 0
> -    return time.mktime(datetime.strptime(timestamp,
> -                                         '%Y-%m-%dT%H:%M:%S').timetuple())
> +    for fmt in ('%Y-%m-%dT%H:%M:%S%z', '%Y-%m-%dT%H:%M:%S'):
> +        try:
> +            return time.mktime(datetime.strptime(timestamp, fmt).timetuple())
> +        except ValueError:
> +            pass
> +    raise ValueError('no valid timestamp format found')
>  
>  def failed_tests(testsuite):
>      """This iterator the failed tests from the testsuite."""
> -- 
> 2.31.0
>