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

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

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

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

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

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.

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.

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

Ian.

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