| Message ID | 1479346630-122644-6-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On Thu, Nov 17, 2016 at 12:21:58PM +0000, Ian Jackson wrote: > 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. Thank you! Let me answer only to some as I am fixing the rest per your comments: > > 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 If the invocations have failed that could mean: - We don't have livepatching enabled (somebody swapped the hypervisor?) - The livepatching did not work right and we have code in the hypervisor that patched something else. Undefined results. - It may be that the test-case failed to load due to dependencies issues - and while that means the system can recover - the test should fail immediately. > 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. At this stage I believe all of these test-cases should work. If they don't we got big problems. .. > > + {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: But this is not libxl. The error values are very much defined by xen-livepatch. > * Run the command with LC_MESSAGES=C Aye. > * Fail the test if the exit status is zero But some of the test-cases are suppose to return 0 - as the program executed correctly. > * Collect its stderr output (by appending 2>&1) > * Match the stderr output against a regexp in the perl program Right.
Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH v1 5/7] ts-livepatch: Initial test-cases."):
> Let me answer only to some as I am fixing the rest per your comments:
>
> On Thu, Nov 17, 2016 at 12:21:58PM +0000, Ian Jackson wrote:
> > 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
>
> If the invocations have failed that could mean:
> - We don't have livepatching enabled (somebody swapped the hypervisor?)
> - The livepatching did not work right and we have code in the
> hypervisor that patched something else. Undefined results.
> - It may be that the test-case failed to load due to dependencies
> issues - and while that means the system can recover - the test
> should fail immediately.
Right.
> > 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.
>
> At this stage I believe all of these test-cases should work. If they
> don't we got big problems.
OK. So I think in that case your script should probably fail
completely, and not run any of the further test cases, if any test
fails.
> > > + {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:
>
> But this is not libxl. The error values are very much defined
> by xen-livepatch.
No, they clearly aren't. I just looked at the code and main()
returns like this:
return !!ret;
So that means that the exit status is either 0 meaning "all is good"
or 1 meaning "something, which might be anything at all, went wrong".
Tests should be arranged to figure out whether the test fails in the
expected way. With programs whose exit status does not discriminate
between causes of failure, this has to be done by grepping error
messages :-/.
> > * Run the command with LC_MESSAGES=C
>
> Aye.
>
> > * Fail the test if the exit status is zero
>
> But some of the test-cases are suppose to return 0 - as the program
> executed correctly.
I meant, if you expect the test to fail, you should insist that it
fails.
Ian.
diff --git a/ts-livepatch b/ts-livepatch new file mode 100755 index 0000000..a996ee9 --- /dev/null +++ b/ts-livepatch @@ -0,0 +1,102 @@ +#!/usr/bin/perl -w +# This is part of "osstest", an automated testing framework for Xen. +# Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +use strict qw(vars); +use Osstest; +use POSIX; +use Osstest::TestSupport; + +tsreadconfig(); + +my @livepatch_files = ("xen_hello_world.livepatch", "xen_replace_world.livepatch", + "xen_bye_world.livepatch", "xen_nop.livepatch"); + +my @livepatch_tests = ( + {cmd => "xen-livepatch list", rc => 0}, + {cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256}, + {cmd => "xen-livepatch revert xen_hello_world", rc => 256}, + {cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 0}, + {cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 256}, + {cmd => "xen-livepatch list | grep -q xen_hello_world", rc => 0}, + {cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 0}, + {cmd => "xen-livepatch revert xen_hello_world", rc => 0}, + {cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256}, + {cmd => "xen-livepatch unload xen_hello_world", rc => 0}, + {cmd => "xen-livepatch unload xen_hello_world", rc => 256}, + {cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256}, + {cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 0}, + {cmd => "xen-livepatch load xen_bye_world.livepatch", rc => 0}, + {cmd => "xl info | grep xen_extra | grep -q \"Bye World\"", rc => 0}, + {cmd => "xen-livepatch upload xen_replace xen_replace_world.livepatch", rc => 0}, + {cmd => "xen-livepatch replace xen_replace", rc => 0}, + {cmd => "xen-livepatch apply xen_hello_world", rc => 256}, + {cmd => "xen-livepatch apply xen_bye_world", rc => 256}, + {cmd => "xl info | grep xen_extra | grep -q \"Hello Again Wor\"", rc => 0}, + {cmd => "xen-livepatch apply xen_replace", rc => 0}, + {cmd => "xen-livepatch revert xen_replace", rc => 0}, + {cmd => "xen-livepatch unload xen_replace", rc => 0}, + {cmd => "xen-livepatch unload xen_hello_world", rc => 0}, + {cmd => "xen-livepatch unload xen_bye_world", rc => 0}, + {cmd => "xen-livepatch list | grep -q xen", rc => 256}, + {cmd => "xen-livepatch load xen_nop.livepatch", rc => 0}, + {cmd => "xen-livepatch revert xen_nop", rc => 0}, + {cmd => "xen-livepatch apply xen_nop", rc => 0}, + {cmd => "[ `xl info| grep \"xen_m\" | grep or | sed s/.*:// | uniq | wc -l` == 1 ]", rc => 0}, + {cmd => "xen-livepatch unload xen_nop", rc => 256}, + {cmd => "xen-livepatch revert xen_nop", rc => 0}, + {cmd => "xen-livepatch unload xen_nop", rc => 0}, + ); + +our $ho = selecthost('host'); + +sub livepatch_test () +{ + logm "Have $#livepatch_tests test-cases\n"; + my $rc=0; + for my $i ( 0 .. $#livepatch_tests ) { + my $expected_rc = $livepatch_tests[$i]{rc}; + my $cmd = "(cd /usr/lib/debug;$livepatch_tests[$i]{cmd})"; + logm "Executing: '$cmd:' .."; + my $rc=target_cmd_root_rc($ho, $cmd); + if ( $rc != $expected_rc ) { + logm "FAILED (got $rc, expected: $expected_rc): \n"; + return $rc; + } + logm ".. OK!\n"; + } + return $rc +} + +sub livepatch_check () +{ + foreach my $file (@livepatch_files) + { + if (!target_file_exists($ho, "/usr/lib/debug/$$file")) { + die "$file is missing!\n"; + } + } + return 0 +} + + +our $livepatch_result = livepatch_check(); +exit $livepatch_result if $livepatch_result; +$livepatch_result = livepatch_test(); + +logm("Livepatch test returned $livepatch_result"); + +exit $livepatch_result;
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. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- ts-livepatch | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100755 ts-livepatch