Libguestfs - Oct 2014 - Re: [PATCH v5 1/7] tests: Introduce test harness for running tests.

On Sunday 05 October 2014 14:08:35 Richard W.M. Jones
wrote:
> +# Install the test harness.
> +localtestsdir = $(alltestsdir)
> +localtests_SCRIPTS = ../test-harness
Shouldn't it rather go to libexec?
> +../test-harness:
> +       rm -f $@ $@-t
> +       echo 'echo Warning: Install OCaml compiler in order to rebuild
the test-harness.' > $@-t
> +       chmod +x $@-t
> +       mv $@-t $@
> +
>  endif
I guess this needs a configure check to disable the test suite when
OCaml is not present.
> +let is_executable path > +  try (stat path).st_perm land 0o111 <>
0
> +  with Unix_error _ -> false
I guess Unix.access might be better here.
> +let relative_path_to_absolute path > +  let cmd = sprintf "unset
CDPATH; cd %s && pwd" (Filename.quote path) in
> +  let chan = open_process_in cmd in
> +  let path = input_line chan in
> +  (match close_process_in chan with
> +  | WEXITED 0 -> ()
> +  | WEXITED _
> +  | WSIGNALED _
> +  | WSTOPPED _ ->
> +    failwith (sprintf "failed to convert relative path to absolute
path: %s"
> +                cmd)
> +  );
> +  path
Interesting, it seems OCaml has nothing in the core libraries to
resolve paths, nor realpath implemented in the Unix module...
Beside that, I'd just invoke realpath, available from GNU coreutils.
> +  let argspec = Arg.align [
> +    "--debug",        Arg.Set debug,      " Run tests with
debugging enabled";
> +    "--direct",       Arg.Set direct,     " Run tests using
the direct backend";
> +    "--fast",         Arg.Set fast,       " Run only tests
which do not need the appliance";
> +    "--local-guests", Arg.Set local_guests, " Run tests
that use locally installed guests r/o";
> +    "--make-phony-guests-only", Arg.Set make_phony_guests_only,
" Generate the phony guests used for testing";
> +    "--slow",         Arg.Set slow,       " Run only
long-running tests";
> +    "--uml",          Arg.Set uml,        " Run tests using
UML backend";
> +    "--upstream-libvirt", Arg.Set upstream_libvirt, " Run
tests using upstream libvirt";
> +    "--upstream-qemu", Arg.Set upstream_qemu, " Run tests
using upstream qemu";
> +    "--valgrind",     Arg.Set valgrind,   " Run tests under
valgrind";
> +    "--with-uml",     Arg.Set_string uml_binary, "vmlinux
Select UML binary";
> +    "--with-upstream-libvirt", Arg.Set_string libvirtdir,
"dir Select libvirt directory";
> +    "--with-upstream-qemu", Arg.Set_string qemu_binary,
"qemu Select qemu binary";
> +  ] in
While I understand that the priority is the libvirt backend, I'd make
the backend choice generic; something like:
 --backend NAME (direct, libvirt, uml, etc)
 --backend-option NAME=VAL (e.g. binary=/path/to/qemu for qemu, etc;
                            can be specified multiple times)
> +  (* If we are running from automake, then automake will pass $srcdir
> +   * to us, and if it's not "." then we have to adjust our
path to the
> +   * top source directory accordingly.
> +   *)
> +  let srcdir = try Sys.getenv "srcdir" with Not_found ->
"." in
> +
> +  (* Are we running from the build directory or from installed tests? *)
> +  let running_in_builddir = is_file (srcdir // "Makefile.am") in
The logic above would fool the runner if $srcdir is not set, and
there's a file called Makefile.am in the current directory.
Maybe something like:
  let srcdir, running_in_builddir     try
      let srcdir = Sys.getenv "srcdir" in
      srcdir, is_file (srcdir // "Makefile.am")
    with Not_found ->
      ".", false in

Or, maybe it would be even clearer/better, just have a --uninstalled
parameter to explicitly turn the runner into that mode, without
implicit logic.
> +  (* If installed, then we cannot write to the phony guests directory. *)
> +  let phonydir > +    if running_in_builddir then (
> +      match start_dir with
> +      | TopDir ->
> +        relative_path_to_absolute "tests/guests"
> +      | PhonyGuestsDir ->
> +        relative_path_to_absolute "."
> +      | Dir (dir, _) ->
> +        let top_builddir = ref ".." in
> +        for i = 0 to String.length dir-1 do
> +          if dir.[i] = '/' then top_builddir := !top_builddir //
".."
> +        done;
> +        let top_builddir = !top_builddir in
> +        relative_path_to_absolute (top_builddir //
"tests/guests")
> +    )
> +    else (
> +      (try mkdir (home // ".cache") 0o755
> +       with Unix_error _ -> ());
> +      (try mkdir (home // ".cache/libguestfs-tests") 0o755
> +       with Unix_error _ -> ());
> +      (try mkdir (home //
".cache/libguestfs-tests/phony-guests") 0o755
> +       with Unix_error _ -> ());
> +      home // ".cache/libguestfs-tests/phony-guests"
> +    ) in
Would it be possible to use use $XDG_CACHE_HOME if available, falling
back to $HOME/.cache if not?
> +  (* Run a single test. *)
> +  and run_one_test dir test args > +    let skip_env = try Sys.getenv
(skip_name test) with Not_found -> "" in
> +    if skip_env = "1" then (
> +      printf "SKIP: %s\n%!" test;
> +      (1, 0, 0, 0, 1)
> +    )
> +    else (
> +      set_up_environment ();
> +
> +      (* If it's a C program, and we're valgrinding, then we run
the
> +       * C program under valgrind directly.
> +       *)
> +      let is_program > +        not (Filename.check_suffix test
".pl") &&
> +        not (Filename.check_suffix test ".sh") in
I think this should be extended to .lua files as well.
> +  (* Run the tests. *)
> +  let total, successful, failed, timedout, skipped > +    match
start_dir with
> +    | TopDir -> run_all_tests ()
> +    | Dir (dir, tests) -> run_dir_tests dir tests
> +    | PhonyGuestsDir -> (0, 0, 0, 0, 0) (* do nothing in this directory
*) in
Might use null_results, I guess.
> diff --git a/inspector/test-xmllint.sh.in b/inspector/test-xmllint.sh.in
> index aef5ebc..f1195fd 100755
> --- a/inspector/test-xmllint.sh.in
> +++ b/inspector/test-xmllint.sh.in
> @@ -19,6 +19,11 @@
>  export LANG=C
>  set -e
>  
> +if ! "@XMLLINT@" --version >/dev/null 2>&1; then
> +    echo "$0: test skipped before xmllint is not installed"
> +    exit 77
> +fi
> +
>  for f in $srcdir/example-*.xml; do
>      @XMLLINT@ --noout --relaxng $srcdir/virt-inspector.rng $f
>  done
This bit could go in even now, I guess.
> diff --git a/tests/guests/guest-aux/make-debian-img.sh
b/tests/guests/guest-aux/make-debian-img.sh
> index 95228ab..af251b4 100755
> --- a/tests/guests/guest-aux/make-debian-img.sh
> +++ b/tests/guests/guest-aux/make-debian-img.sh
> @@ -82,11 +82,11 @@ upload fstab.tmp.$$ /etc/fstab
>  write /etc/debian_version "5.0.1"
>  write /etc/hostname "debian.invalid"
>  
> -upload $SRCDIR/guest-aux/debian-packages /var/lib/dpkg/status
> +upload $srcdir/guest-aux/debian-packages /var/lib/dpkg/status
>  
> -upload $SRCDIR/../data/bin-x86_64-dynamic /bin/ls
> +upload $datadir/../data/bin-x86_64-dynamic /bin/ls
>  
> -upload $SRCDIR/guest-aux/debian-syslog /var/log/syslog
> +upload $srcdir/guest-aux/debian-syslog /var/log/syslog
Replacing $SRCDIR with $srcdir is something doable even right now, I guess.
I will send a patch for this.

-- 
Pino Toscano

On Tue, Oct 21, 2014 at 05:56:14PM +0200, Pino Toscano
wrote:
> On Sunday 05 October 2014 14:08:35 Richard W.M. Jones wrote:
> > +# Install the test harness.
> > +localtestsdir = $(alltestsdir)
> > +localtests_SCRIPTS = ../test-harness
> 
> Shouldn't it rather go to libexec?
No, it's meant to be run by the end user.
> > +../test-harness:
> > +       rm -f $@ $@-t
> > +       echo 'echo Warning: Install OCaml compiler in order to
rebuild the test-harness.' > $@-t
> > +       chmod +x $@-t
> > +       mv $@-t $@
> > +
> >  endif
> 
> I guess this needs a configure check to disable the test suite when
> OCaml is not present.
Probably, although that would add quite a lot of complexity ...
> While I understand that the priority is the libvirt backend, I'd make
> the backend choice generic; something like:
>  --backend NAME (direct, libvirt, uml, etc)
>  --backend-option NAME=VAL (e.g. binary=/path/to/qemu for qemu, etc;
>                             can be specified multiple times)
I suspect that encoding all this as options is going to be tedious,
considering people can just set the associated environment variables
instead.  Could drop the --direct option for the same reason.
> > +  (* Run a single test. *)
> > +  and run_one_test dir test args > > +    let skip_env = try
Sys.getenv (skip_name test) with Not_found -> "" in
> > +    if skip_env = "1" then (
> > +      printf "SKIP: %s\n%!" test;
> > +      (1, 0, 0, 0, 1)
> > +    )
> > +    else (
> > +      set_up_environment ();
> > +
> > +      (* If it's a C program, and we're valgrinding, then we
run the
> > +       * C program under valgrind directly.
> > +       *)
> > +      let is_program > > +        not (Filename.check_suffix
test ".pl") &&
> > +        not (Filename.check_suffix test ".sh") in
> 
> I think this should be extended to .lua files as well.
Yeah, and more.  I've not implemented the other bindings tests yet.

I'll fix the other stuff in the next patch.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Fri, Oct 24, 2014 at 10:42:38AM +0100, Richard W.M. Jones
wrote:
> > I guess this needs a configure check to disable the test suite when
> > OCaml is not present.
> 
> Probably, although that would add quite a lot of complexity ...
Oh I see, a configure test.  I don't even know if that is possible ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Tue, Oct 21, 2014 at 05:56:14PM +0200, Pino Toscano
wrote:
> > +  (* If we are running from automake, then automake will pass $srcdir
> > +   * to us, and if it's not "." then we have to adjust
our path to the
> > +   * top source directory accordingly.
> > +   *)
> > +  let srcdir = try Sys.getenv "srcdir" with Not_found ->
"." in
> > +
> > +  (* Are we running from the build directory or from installed tests?
*)
> > +  let running_in_builddir = is_file (srcdir //
"Makefile.am") in
> 
> The logic above would fool the runner if $srcdir is not set, and
> there's a file called Makefile.am in the current directory.
> Maybe something like:
>   let srcdir, running_in_builddir >     try
>       let srcdir = Sys.getenv "srcdir" in
>       srcdir, is_file (srcdir // "Makefile.am")
>     with Not_found ->
>       ".", false in
Actually I think the original is correct, since it lets you run
'./test-harness' from the build directory.
> Or, maybe it would be even clearer/better, just have a --uninstalled
> parameter to explicitly turn the runner into that mode, without
> implicit logic.
Then developers would need to use an explicit flag for this case.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top