Re: [Xen-devel] [OSSTEST PATCH v13 19/24] TestSupport: Implement target_subunit_cmd a subunit stream parser into substeps

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Jul 25, 2017 at 07:00:47PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[OSSTEST PATCH v13 19/24] TestSupport: Implement 
> target_subunit_cmd a subunit stream parser into substeps"):
> > target_subunit_cmd 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.
> > 
> > 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.
> > 
> > This is a description of the subunit v1 protocol, taken from
> > python-subunit README, or https://pypi.python.org/pypi/python-subunit
> 
> What a lot of code!
> 
> > +    while (<$stdout>) {
> > +        if (/^time: (\d+)-(\d+)-(\d+) (\d+):(\d+):(\d+)(\.\d+)?Z$/) {
> > +            # This is the timestamp for the next events
> 
> I'm not sure what your ( ) are doing here.

No real reason. I'll remove them.

> > +        } elsif (/^test(?:ing)?:? (.+)\n/) {
> > +            # Start of a new test.
> > +            $logfilename = subunit_sanitize_testname($1) . '.log';
> > +            $fh = open_unique_stashfile(\$logfilename);
> 
> This name might clash with existing logfile names, which might be
> generated later.  Can you put "subunit-" on the front maybe ?

Will do.

> > +            substep_start(subunit_sanitize_testname($1), $logfilename);
> 
> And here, I think you should start the parameter you pass to
> substep_start with '/' so that it gets appended to the testid for the
> whole script, for a similar reason.

OK.

> I think it would be better to call subunit_sanitize_testname only
> once.

OK.

> > +        } elsif (/^(success(?:ful)?|failure|skip|error|xfail|uxsuccess):
> > +                   \ (.+?)(\ \[(\ multipart)?)?$/x) {
> > +            # Result of a test, with its output.
> > +            my $result = $1;
> > +            my $testname = $2;
> > +            my $have_details = $3;
> > +            my $is_multipart = $4;
> 
> I would normally write this:
>                my ($result, $testname, $have_... ) = ($1,$2,$3,$4,$5)
> although I don't really mind much that you have written it as you
> have.
> 
> > +            if ($have_details) {
> > +                if ($is_multipart) {
> > +                    # Test output
> > +                    while (<$stdout>) {
> > +                        # part content-type
> > +                        # from 
> > https://tools.ietf.org/html/rfc6838#section-4.2
> > +                        my $restricted_name = 
> > qr'[a-zA-Z0-9][a-zA-Z0-9!#$&^_.+-]*';
> > +                        if (m{ ^Content-Type:\s+
> > +                                $restricted_name/$restricted_name # 
> > type/sub-type
> > +                                # parameters
> > +                                (?:\s*;\s*$restricted_name=[^,]+
> > +                                  (?:,\s*$restricted_name=[^,]+)*)
> > +                                \s*$
> > +                            }xi) {
> 
> I don't understand why you are trying to match this Content-Type so
> precisely.  AFAICT from the grammar, all you need to do is see whether
> there is something vaguely like a c-t header.

I think I start by looking at what kind of characters could be part of
type and sub-type, and just start writing a more complicated regex.

So is the following would be enough for you?
m{^Content-Type: [^/ ]+/[^/ ]+(?:;.+)?$}


> > +                            print $fh $_ or die $!;
> > +
> > +                            # part name
> > +                            my $line = <$stdout>;
> > +                            print $fh $line or die $!;
> > +
> > +                            # Read chunks of a part
> > +                            while (<$stdout>) {
> > +                                if (/^([0-9A-F]+)\r$/i) {
> > +                                    my $chunk_size = hex($1);
> 
> What makes you think the digits are in hex ?

I tried with [0-9] (because DIGITS), but that was not enought. Then I've
check the subunit implementation, there are using "%X" which is hex.

> Since you have to go to the effort of separating out all of this
> stuff, it might be worth printing these multipart objects with one
> object per logfile.  Although I won't insist on that because I suspect
> that multipart results are rare.

There are usually 3 part per tests, with those names:
  pythonlogging:''
  stdout
  stderr
And sometime, there is also one name 'traceback'.
I think stdout and stderr are usually empty.

I think having one file per part will make it more complicated to
read logs of a failed test.

> > +                                } else {
> > +                                    # Unexpected output
> > +                                    chomp;
> > +                                    logm("*** $_");
> 
> I guess the error recovery is to continue until you see "]"
> and hope.  Fair enough.

That one of the reason for the subunit-v2, with a binary protocol,
better recovery.

-- 
Anthony PERARD

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