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

[Anthony PERARD <anthony.perard@citrix.com>]

On Thu, Jul 13, 2017 at 02:28:11PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[OSSTEST PATCH v12 18/21] TestSupport: Implement target_cmd_subunit a subunit stream parser into substeps"):
> > 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.

The subunit stream contains the timestamps, so it just a matter of
having substep_* taking it as an argument.

> > +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.

Will do.

> > +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.

What about subunit_sanitize_testname?

What kind of whitelist? What should it includes?

> > +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.)

Will do. And yes, this will duplicate most of the output. But it can
help debug osstest, for everything that the parser ignore.

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

OK.

> > +    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 ?

multipart just describes how the following lines are formated, it would
have the 'content-type:' and the size of the output. non-multipart is
just followed by text, and ends with '\n]\n' (both format do).

I don't think the code needs to care about it, just that it may or
may not be there.

> 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.

Looking at a description of the protocol and at the subunit code, does
are single spaces.

What do you mean by "extended" ? Maybe operator like /.+?/, or maybe
/(?<NAME>pattern)/ ?

> > +            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 ?

No reason, I'll print.

> 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.

Without "multipart", there does not seems to be any escaping. With
multipart, the size of the output is in the protocol, I could extend the
parser take it into account. It's just more work.

FYI, part of the protocol about the output (with the beginning of
DETAILS been "\[( multipart)?" in the regex):

DETAILS ::= BRACKETED | MULTIPART
BRACKETED ::= '[' CR UTF8-lines ']' CR
MULTIPART ::= '[ multipart' CR PART* ']' CR
PART ::= PART_TYPE CR NAME CR PART_BYTES CR
PART_TYPE ::= Content-Type: type/sub-type(;parameter=value,parameter=value)
PART_BYTES ::= (DIGITS CR LF BYTE{DIGITS})* '0' CR LF

> > +            }
> > +            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...

"unexpected lines [...] should be forwarded unaltered". That's is in the
readme of python-subunit project.

As for keywords that can exist, there is "tags:", but in the case of
tempest, it describe which worker did a test, when there is several
concurrent worker. There is also "progress:" which is not very usefull
for osstest. There is maybe more keywords which are test result which I
should probably find out what there are, but I've got at least the one
used by Tempest.

Thanks,

-- 
Anthony PERARD

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