On 18.03.2016 10:42, Ján Tomko wrote:
> 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.
Well, it's not necessary now, but it will be later. Thing is, if a
program would re-exec itself, the path would be there twice.
>
>> + (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.
>
Okay.
Pushed. Thanks.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list