On Thu, Mar 17, 2016 at 02:31:49PM +0100, Michal Privoznik wrote:
> Currently we spawn couple of binaries in our test suite.
> Moreover, we provide some spoofed versions of system binaries
> hoping that those will be executed instead of the system ones.
> For instance, for testing SSH socket we have written our own ssh
> binary for producing predictable results. We certainly don't want
> to execute the system ssh binary.
> However, in order to prefer our binaries over system ones, we
> need to set PATH environment variable. But this is done only at
> the Makefile level. So if anybody runs a test by hand that
> expects our spoofed binary, the test ends up executing real
> system binaries. This is not good. In fact, it's terribly wrong.
What a tragedy!
> The fix lies in a small trick - putting our build directory at
> the beginning of the PATH environment variable in each test.
> Hopefully, since every test has this VIRT_TEST_MAIN* wrapper, we
> can fix this at a single place.
> Moreover, while this removes setting PATH for our tests written
> in bash, it's safe as we are not calling anything ours that would
> require PATH change there.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> tests/Makefile.am | 3 ---
> tests/testutils.c | 21 ++++++++++++++++++++-
> 2 files changed, 20 insertions(+), 4 deletions(-)
ACK
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 90981dc..74f7f5a 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -462,8 +462,6 @@ TESTS = $(test_programs) \
> # intermediate shell variable, but must do all the expansion in make
>
> lv_abs_top_builddir=$(shell cd '$(top_builddir)' && pwd)
> -path_add = $(subst :,$(PATH_SEPARATOR),\
> - $(subst !,$(lv_abs_top_builddir)/,!daemon:!tools:!tests))
>
> VIR_TEST_EXPENSIVE ?= $(VIR_TEST_EXPENSIVE_DEFAULT)
> TESTS_ENVIRONMENT = \
> @@ -472,7 +470,6 @@ TESTS_ENVIRONMENT = \
> abs_builddir=$(abs_builddir) \
> abs_srcdir=$(abs_srcdir) \
> CONFIG_HEADER="$(lv_abs_top_builddir)/config.h" \
> - PATH="$(path_add)$(PATH_SEPARATOR)$$PATH" \
> SHELL="$(SHELL)" \
> LIBVIRT_DRIVER_DIR="$(lv_abs_top_builddir)/src/.libs" \
> LIBVIRT_AUTOSTART=0 \
> diff --git a/tests/testutils.c b/tests/testutils.c
> index 88c4d4b..9211b94 100644
> --- a/tests/testutils.c
> +++ b/tests/testutils.c
> @@ -806,6 +806,24 @@ virTestGetRegenerate(void)
> return testRegenerate;
> }
>
> +static int
> +virTestSetEnvPath(void)
> +{
> + int ret = -1;
> + const char *path = getenv("PATH");
> + char *new_path = NULL;
> +
> + if (strstr(path, abs_builddir) != path &&
Is the strstr call really necessary?
I would rather prepend it unconditionally.
> + (virAsprintf(&new_path, "%s:%s", abs_builddir, path) < 0 ||
> + setenv("PATH", new_path, 1) < 0))
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(new_path);
> + return ret;
> +}
> +
> int virtTestMain(int argc,
> char **argv,
> int (*func)(void))
> @@ -818,7 +836,8 @@ int virtTestMain(int argc,
>
> virFileActivateDirOverride(argv[0]);
>
> - if (!virFileExists(abs_srcdir))
> + if (virTestSetEnvPath() < 0 ||
These are not related, both deserve their separate ifs.
Jan
> + !virFileExists(abs_srcdir))
> return EXIT_AM_HARDFAIL;
>
> progname = last_component(argv[0]);
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list