Re: [OSSTEST PATCH v12 18/21] TestSupport: Implement target_cmd_subunit a subunit stream parser into substeps

[Ian Jackson <ian.jackson@eu.citrix.com>]

Anthony PERARD writes ("[OSSTEST PATCH v12 18/21] TestSupport: Implement target_cmd_subunit a subunit stream parser into substeps"):
> target_cmd_subunit can be used like target_cmd, but the command would
> needs to output a subunit v1 stream, which will be parsed and turned
> into osstest substeps. The command can be `| subunit-2to1` in order to
> turn a subunit v2 stream into v1.

Thanks.

> Currently, time is not taken into account, and all substeps will have
> bogus timestamp as the output of the command is parsed after it has
> runned.

I think this is not a critical problem, but fixing it would be nice at
some point.

> +sub subunit_result_to_osstest_result ($) {
> +    my ($ret) = @_;
> +    return "pass" if $ret eq "success" or $ret eq "successful";
> +    return "fail" if $ret eq "failure";
> +    return "skip" if $ret eq "skip";
> +    return "fail" if $ret eq "error";
> +}

I think this needs to die at the end, if the input is not recognised.

> +sub subunit_sanitize ($) {
> +    my ($testname) = @_;
> +    $testname =~ s/ /_/g;
> +    return $testname;
> +}

This function should have a more specific name.  Also it needs to be a
whitelist.

> +sub target_cmd_subunit ($$;$$) {
> +    my $stdout = IO::File::new_tmpfile();
> +    my $rc = tcmd(undef,$stdout,0, 'osstest', @_);

It would be better to staxh the original subunit output.  And I would
prefer to avoid direct use of tcmd here.  So can you introduce
   target_cmd_stashed
which calls open_unique_stashfile and tcmd, and then use that in your
subunit subroutine?  (And yes this might duplicte output I think.)

I'm not sure target_cmd_subunit is quite the right name.  Maybe
target_subunit_cmd ?

> +    while (<$stdout>) {
> +        if (/^time: (\d+)-(\d+)-(\d+) (\d+):(\d+):(\d+)(\.\d+)?Z$/) {
> +            # This is the timestamp for the next events
> +        } elsif (/^test: (.+)\n/) {
> +            $logfilename = subunit_sanitize($1) . '.log';
> +            $fh = open_unique_stashfile(\$logfilename);
> +            substep_start(subunit_sanitize($1), $logfilename);
> +        } elsif (/^(success(ful)?|failure|skip|error): (.+?)( \[( multipart)?)?$/) {

Please assign your $n to local variables, rather than leaving them in
$3 etc. to be used much later.  (And don't capture things if you don't
intend to, so in that case use ?:).  What does the multpart mean ?
Does this code need to care ?  Does the subunit protocol insist that
the spaces are single spaces ?  If not you need to use \s+.  You may
want to use the extended regexp syntax.

> +            if ($4) {
> +                my $test_output = "";
> +                while (<$stdout>) {
> +                    last if (/^\]$/);
> +                    $test_output .= $_;
> +                }
> +                print $fh $test_output or die $!;

Why do you bother accumulating this in $test_output rather than just
printing it ?

Does the subunit protocol not have any escaping ?  (Ie, what happens
if a thing run as part of a subunit test actually generates a line of
log output "]" ?)  If it does havve some escaping you need to
de-escape it.

> +            }
> +            close $fh or die $!;
> +            substep_finish(subunit_sanitize($3), subunit_result_to_osstest_result($1));
> +        }

What are subunit v1 consumers supposed to do with 1. unknown keywords
2. syntax errors ?

I doubt that the answer to (2) is to ignore them as you do here...

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel