Re: [Xen-devel] [PATCH v1 5/7] ts-livepatch: Initial test-cases.

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Konrad Rzeszutek Wilk writes ("[PATCH v1 5/7] ts-livepatch: Initial 
test-cases."):
> There are 32 test-cases in there. Before we run
> any of them we verify that the payload files are present
> in /usr/lib/debug.
> 
> If so we go through all of the test-cases.

> +my @livepatch_files = ("xen_hello_world.livepatch", 
> "xen_replace_world.livepatch",
> +                     "xen_bye_world.livepatch", "xen_nop.livepatch");

Maybe use qw() ?

> +my @livepatch_tests = (
> +     {cmd => "xen-livepatch list", rc => 0},

I think the formatting and ease-of-editing of this list should be
improved.

You could make rc => 0 the default.  Then you wouldn't need to say it.
(But as I say below, you may not want to test rc anyway.)  Also please
use "status" rather than "rc" throughout.

In osstest we use StudlyCaps for hash keys of this kind.  In this case
I suggest "C" instead of "cmd" because the scope is so narrow.  Also,
we put spaces inside the { }, and indent by 4.  In this case you might
want to indent by less to make it fit better.

You might want to consider whether having an entry that's
just a string, rather than a hashref, ought to mean { C => $string }.
But that depends on whether you are going to want to give each test an
ID:

Is it ever possible to continue with the rest of the livepatch tests
after one of these command invocations has failed, or does it leave
the system in an undefined state ?  If it _is_ possible then it would
be possible to provide per-step results to osstest, but that's only
valuable if this would tell us something meaningfully interesting in
terms of regressions - eg if we might tolerate a regression of one
step but still care about the others.

> +     {cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 
> 256},

I'm afraid I think it is rather silly to do this greppery in shell,
when you are writing a Perl script.  Perl is much better at string
matching than shell.  Also your shell rune has bad error handling (for
example, the test will pass if "xl info" coredumps), which would be
tiresome to fix.

I suggest you provide a facility where each test case can provide a
subref to be called on the output from the command.  Then you would
use target_cmd_output_root_status (which you would have to create).

Something like

 { C => "xl info",
   OutputCheck => sub { m/xen_extra/ && !m/Hello World/ } },

where your actual driver code would call OutputCheck with $_ set,
and fail if the result was falseish.

The OutputCheck key would have to be optional so you wouldn't specify
it on each command.

> +     {cmd => "xen-livepatch revert xen_hello_world", rc => 256},

libxl exit statuses are not very good and should not be relied on,
really.  Instead, you should arrange to:
  * Run the command with LC_MESSAGES=C
  * Fail the test if the exit status is zero
  * Collect its stderr output (by appending 2>&1)
  * Match the stderr output against a regexp in the perl program

Something like:

 { C => "xen-livepatch revert xen_hello_world",
   Fails => qr{xl: cannot revert xen_hello_world which is not installed} },

or whatever the message actually is.  Your actual driver code would
see Fails in the hashref and DTRT.

> +     {cmd => "[ `xl info| grep \"xen_m\" | grep or | sed s/.*:// | uniq | wc 
> -l` == 1 ]", rc => 0},

You really really wanted to write this in shell, didn't you ? :-)
But getting the shell error handling right will be a pain...

> +sub livepatch_test ()
> +{

Style: { on same line as ().

> +    logm "Have $#livepatch_tests test-cases\n";

This is a lie because $#livepatch_tests is the highest populated array
index.  You mean  "Have ".(scalar @livepatch_tests)."..."
or sprintf "...%d...", (scalar @livepatch_tests)
or something.

> +    my $rc=0;
> +    for my $i ( 0 .. $#livepatch_tests ) {

Style: no spaces inside ( ).  But: please use

   foreach my $test (@livepatch_tests) {

(since I don't think you need the array index.  And the local variable
$test being effectively $livepatch_tests[$i] makes the rest of the
code much easier to read.)

> +        my $expected_rc = $livepatch_tests[$i]{rc};
> +     my $cmd = "(cd /usr/lib/debug;$livepatch_tests[$i]{cmd})";

YM "set -e; cd ...; $test->{C}".

The ( ) are redundant and you need the set -e in case the cd fails.

> +     logm "Executing: '$cmd:' ..";

target_cmd_* already log.  You can probably drop that.

> +     my $rc=target_cmd_root_rc($ho, $cmd);
> +     if ( $rc != $expected_rc ) {
> +             logm "FAILED (got $rc, expected: $expected_rc): \n";
> +             return $rc;

This bubbling up of the failure code from in the middle here seems
unnecessary.  Why not die, here ?

This bit of code will become more sophisticated if you take my
suggestions about OutputCheck and Fails, above.

> +sub livepatch_check ()
> +{
> +    foreach my $file (@livepatch_files)
> +    {

Style.

> +        if (!target_file_exists($ho, "/usr/lib/debug/$$file")) {
> +         die "$file is missing!\n";

I'm not sure why you actually need to check this separately.  Won't
the later tests spot this ?  In which case this simply generates log
clutter.  But I don't object to the separate check.

> +our $livepatch_result = livepatch_check();
> +exit $livepatch_result if $livepatch_result;

Your livepatch_check already dies or returns 0.  I suggest you delete
the return 0 from it, and don't check separately.  In perl it is best
practice to convert fatal errors to exceptions (by calling die) ASAP.
That avoids a lot of error handling code in the rest of the call
stack.  Subroutines which are called only to check things, or only for
their side effects, don't need to explicitly return anything (and
their callers don't need to test the return value).

Thanks,
Ian.

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