[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [OSSTEST PATCH v12 18/21] TestSupport: Implement target_cmd_subunit a subunit stream parser into substeps
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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |