Re: [OSSTEST PATCH RFC 11/14] Introduce ts-xtf-run

[Wei Liu <wei.liu2@citrix.com>]

On Thu, Aug 04, 2016 at 01:19:15PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[OSSTEST PATCH RFC 11/14] Introduce ts-xtf-run"):
> > This is the main script for running XTF.  It will first perform
> > selftest, and then run each XTF test case as a substep.
> > 
> > It does the following things:
> > 
> > 1. Run self tests for individual environment and record the result.
> > 2. Collect tests according to available environments.
> > 3. Run the collected tests one by one.
> > 
> > The script may exit early if it detects the test host is down or
> > xtf-runner returns non-recognisable exit code.
> ...
> > +    foreach my $e (split('\n', $output)) {
> > +        push @all_environments, $e
> > +    }
> 
> Why not
>    push @all_environments, split /\n/, $output;
> ?
> 
> (NB split takes a pattern - a regexp - not a string.  Your code
> provides a literal string which is then used as a regexp.)
> 
> (Another occurrence of this pattern, later.)
> 

Ack.

> > +# Call the runner on one test case, generate a substep for it in final test
> > +# report. Return runner exit code to caller. May exit the script early if
> > +# things go really bad.
> > +sub do_one_test ($) {
> > +    my ($name) = @_;
> > +    my $tid = "xtf/$name";
> > +    my $cmd = "xtf-runner $name";
> > +
> > +    substep_start($tid, $cmd);
> > +    my $output = target_cmd_output_root($ho, <<END, 600);
> > +        $runner $name 1>&2; echo \$\?
> > +END
>                                       ^ this second \ is superfluous
> > +    my ($ret) = $output =~ /(\d+)/;
> 
> die unless the pattern matches.

Ack.

> 
> This code seems to be lacking the error handling we discussed.  In
> particular, if target_cmd_output_root fails (because the dom0
> crashed), target_cmd_output_root will die.  The script will exit
> nonzero and the step will be left `running'.
> 
> > +    # If the host doesn't respond after a test case, always make this substep
> > +    # fail and exit the script early with 0
> > +    my $msg = target_ping_check_up($ho);
> 
> I think it would be better to use
>    target_cmd_output($ho, 'echo ok.');
> than target_ping_check_up.
> 
> There are some kinds of failure which leave the host responding to
> ping but not actually usefully alive.
> 
> > +    # If xtf result can't be recognised, always make this substep fail and exit
> > +    # the script early with status 0.
> > +    if (!xtf_result_defined($ret)) {
> > +        substep_finish($tid, "fail");
> > +        exit 0;
> 
> The lack of use of `eval' (and appropriate use of `die') has resulted
> in a lot of explicit repetition of this error path.
> 
> Please arrange that all problems which ought to cause "record step as
> `fail' and run no more tests" are handled by:
>    
>     - having the code which detects the problem calling die
>     - that die being caught by a single eval instance
>     - the code after the eval handling all exceptions that way
> 

I will see what I can do regarding this. I'm not very familiar with
perl, will need to read some docs first.

> Also there is a latent bug here.  You have xtf_result_defined accept
> exit statuses 1 and 2, but those are actually not defined exit
> statuses for the xtf runner.

FAOD, 1 and 2 are defined xtf exit statuses -- reserved for anything
python interpreter related. But I think this is going to be moot because
they are going to be mapped to fail anyway.

> 
> I think that this would be best fixed by using something like this:
> 
>    +sub xtf_result_to_osstest_result ($) {
>    +    my ($xret) = @_;
>    +
>    +    return "pass" if $xret == 0;
>    +    return "skip" if $xret == 3;
>    +    return "fail" if $xret == 4;
>    +    return "fail" if $xret == 5;
>    +    die "xtf runner gave unexpected exit status $xret";
>    +}
> 
> (And, obviously, calling it within the eval.)
> 
> Then you can abolish xtf_result_defined.
> 
> And another thing: AFAICT there is nothing that prints the XTF exit
> status.  You need at least to report the numerical exit status;
> ideally you would print some human-readable interpretation of it.
> 
> The person reading the logs may not be familiar with osstest or xtf.
> They ought to be told the xtf name for the exit status as well as the
> osstest mapping of it.
> 

The XTF exit status is already available in the log, as is the
osstest result (in substep status line).

I guess you want them on the same line? One logm would do.

> > +# Run selftest for each environment, record the ones that are
> > +# funtional to get maximum coverage.
>         ^c
> 
> > +sub get_tests_list () {
> > +    foreach my $e (sort @environments) {
> > +        my $output = target_cmd_output_root($ho, <<END);
> > +            $runner --list $e --all --host
> > +END
> > +        foreach my $t (split('\n', $output)) {
> > +            push @tests, $t
> > +        }
> 
> It might be worth recording the environment for each test, for the
> log, unless the xtf runner prints that.
> 

It is encoded in test case name.

Wei.

> Ian.

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