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

[Pino Toscano]

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