Hi guys,
On Wed, Nov 28, 2018 at 11:17:22AM +0100, Frederic Lecaille wrote:
> Perhaps we should "chmod +x" this script.
Good point, done here.
However I'm now seeing this when starting it :
########################## Starting varnishtest ##########################
Testing with haproxy version: 1.8-dev0-3b0a6d-66
Assert error in start_test(), vtc_main.c line 283:
Condition((mkdir(tmpdir, 0711)) == 0) not true.
errno = 13 (Permission denied)
./scripts/run-regtests.sh: line 299: 21484 Aborted
$VARNISHTEST_PROGRAM $varnishtestparams $verbose $jobcount -l -k -t 10 $testlist
########################## Gathering failed results ##########################
find: `/proc/tty/driver': Permission denied
find: `/proc/1/task/1/fd': Permission denied
find: `/proc/1/task/1/fdinfo': Permission denied
find: `/proc/1/task/1/ns': Permission denied
find: `/proc/1/fd': Permission denied
find: `/proc/1/map_files': Permission denied
(...)
Then I stopped it using Ctrl-C.
I'm seeing a few minor issues we must still address :
- haproxy is started from the path, which means that on all systems
where it's installed, it will be the one provided by the system and
not the just built one which will be tested (as happened above),
which is confusing. I think we shouldn't search for haproxy in the
path but use $PWD/haproxy or ./haproxy or something like this.
- it seems there are some issues in the way TMPDIR/TESTDIR are created,
as it ended up with an empty TESTDIR. In my case, TMPDIR is not set,
so TESTDIR was set to /tmp/varnishtest_haproxy. mkdir worked (I now
have this directory), but a test is missing on this mkdir.
- the way the sub-directory is created is problematic on shared
development machines, as only the first user will own the directory
and will thus prevent other users from creating their own overthere,
thus it's probably preferable not to create an intermediary directory
in the end.
- in my case it's mktemp which failed :
++ date +%Y-%m-%d_%H-%M-%S
+ TESTRUNDATETIME=2018-11-29_05-03-43
+ TESTDIR=/tmp/varnishtest_haproxy
+ mkdir -p /tmp/varnishtest_haproxy
++ mktemp -d /tmp/varnishtest_haproxy/2018-11-29_05-03-43.XXXX
mktemp: cannot make temp dir
/tmp/varnishtest_haproxy/2018-11-29_05-03-43.XXXX: Invalid argument
+ TESTDIR=
I haven't checked why yet, but we definitely need to test the status
code for success as well.
- in my opinion the script is still missing a number of quotes when using
string variables, especially in the directory names. If someone is crazy
enough to have TMPDIR="/tmp/Temp Dir", then he'll have some fun.
- I'm seeing a linux-specific "rm -d" at the end, it's be better to
replace it with rmdir.
- there's "/usr/bin/env sh" at the top of the shell script. /bin/sh is
the portable one, I've spent lots of time in the past editing scripts
where env was forced to /usr/bin while it was placed in /bin on some
systems, so I'm pretty certain that this one is not portable at all :-)
However I'm well aware that it's easier to work on improvements once the
script is merged, so what I've done now is to merge it and create a
temporary "reg-tests2" target in the makefile to use it without losing
the existing one. This way everyone can work in parallel, and once the
few issues seem reliably addressed, we can definitely replace the make
target.
Thanks!
Willy
