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

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

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.

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

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

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

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

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

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.

> +                                } else {
> +                                    # Unexpected output
> +                                    chomp;
> +                                    logm("*** $_");

I guess the error recovery is to continue until you see "]"
and hope.  Fair enough.

Thanks,
Ian.

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