[lvm-devel] RFC: testing framework

[Jim Meyering]

Alasdair G Kergon <agk at redhat.com> wrote:

> On Mon, Sep 17, 2007 at 01:56:04PM +0200, Jim Meyering wrote:
>> +++ b/test/Makefile.in
>> @@ -0,0 +1,75 @@
>> +#TEST_OPTS=--verbose --debug
>> +SHELL_PATH ?= $(SHELL)
>> +TAR ?= $(TAR)
>> +RM ?= rm -f
>> +
>> +subdir := $(shell pwd|sed 's,.*/,,')
>> +
>> +srcdir = .
>> +top_srcdir = ..
>> +top_builddir = ..
>
> I'd prefer to see this handled consistently across all our Makefiles.

Well, that's what automake does.
Why bother, if no other rules use them?
IMHO, it's bad enough to add them in one place, but
adding such duplication across all 24 Makefile.in files
is bad for maintenance: it's just asking for some of them
to get out of sync.

> Elsewhere we have srcdir = @srcdir@, for example.

Good point.  Adjusted.
Oh!  I see that I can use all of those @...dir@ names.
Now, I have the following (this is all normally generated by
automake, so people tend to use the $(*dir) form):

srcdir = @srcdir@
top_srcdir = @top_srcdir@
top_builddir = @top_builddir@
abs_srcdir = @abs_srcdir@
abs_top_builddir = @abs_top_builddir@
abs_top_srcdir = @abs_top_srcdir@

>> +dmdir = $(abs_top_srcdir)/../device-mapper
>> +so_name = $(dmdir)/lib/ioctl/libdevmapper.so.1.02
>
> Please avoid the 1.02 hard-coding:-)
> (Include a symlink within the dm tree if you want at build time - maybe
> from lib/libdevmapper.so -> ioctl/libdevmapper.so.*)

Ok.

>> +clean:
>> +	rm -rf init.sh lvm-wrapper bin .bin-dir-stamp
>> +
>> +all: $(T)
>> +.PHONY: $(T) clean
>
> Last time I read the docs, they said .PHONY had to appear before
> 'clean:', not after it.

Nope.  Order doesn't matter, just existence of the dependency.
Here's a tiny demo to show that:

    $ printf 'x:;\n.PHONY: x'|make -t -f - x
    make: Nothing to be done for `x'.

>> +++ b/test/mkdtemp
>
> Can these ugly infrastructure scripts be grouped together in a subdir?

You mean test-lib.sh and mkdtemp?
Why bother just for two files?

>> @@ -0,0 +1,107 @@
>> +#!/bin/sh
>> +# Create a temporary directory, sort of like mktemp -d does.
>> +# Usage: mkdtemp /tmp phoey.XXXXXXXXXX
>
> Where has this come from and has it been audited?  I get worried when I
> see scripts like this that don't include any comments indicating that
> security was the prime consideration, and doing our own audit would
> be an unnecessary distraction.

IMHO, it's not risky, here.
The usage here is far less risky than in parted,
since here it's used only to create a directory in the build
directory, which we can presume is not world writable.

I wrote it when I had to run tests in GNU Parted as root,
and couldn't depend on having mktemp installed, and didn't want to
subject other root invokers of "make check" to the slightest risk of
race-condition exploits.  Besides, even when mktemp is installed, there
are multiple versions of it, with different options...  mkdtemp is also
being used in coreutils, now, and will probably soon migrate to gnulib.

> I'd prefer not to have to ship (i.e. take responsibility for) a script
> like this ourselves if we can avoid that.

No need for you to take responsibility for it.  I own it in coreutils.
If there's any problem with it, it'll get plenty of exposure there.
People are using it through Parted, too.

>> +++ b/test/t0000-basic.sh
>> +lvm >/dev/null 2>&1
>
> (How about using $LVM set to include the full path?)

I deliberately rely on the $PATH setting there.
That's part of the test.  If you use the full name of the binary,
then you have to be careful with quoting (i.e., always use "$LVM"),
in case there are shell meta-characters in the user's working
directory name.  That's risky and ugly, and besides, tends
to pollute diagnostics if a program prints raw argv[0].

>> +# Protect ourselves from common misconfiguration to export
>> +# CDPATH into the environment
>> +unset CDPATH
>
> I'd feel safer if this approach was inverted: clear all environment
> variables except the few required for correct reproducible operation of
> the tests.

How about clearing just those few that LVM is known to use?

> The test preparation also needs to take steps to insulate itself
> as far as possible from any 'real' lvm2 installation on the system.
> This will also aid reproducibility.
> E.g. I suggest it uses private directories instead of /etc/lvm and
> /dev.

Good idea.  Do you mean by creating $PWD/lvm.conf and setting
LVM_SYSTEM_DIR to that string?  Can you suggest actual contents
to use for that file?

How about if I commit the series of patches I have now,
and add that in a separate patch, tomorrow?

BTW, with changes derived from your suggestions, I now have four
patch sets.  I prefer to keep them separate in order to better
attribute the authorship/origin.  BTW, that's another reason I
prefer git:  there's an official way to credit the author when s/he
is not the same as the committer.