Re: [PATCH] http-push: add regression tests

From: Mike Hommey <mh@glandium.org>
Date: Feb 26, 2008, 19:32 UTC
Message-ID: <20080226193206.GC16889@glandium.org>
In-Reply-To: <20080225232820.GA18254@localhost>

On Tue, Feb 26, 2008 at 12:28:20AM +0100, Clemens Buchacher wrote:

Show 11 quoted lines

> http-push tests require a web server with WebDAV support.
> 
> This commit introduces a HTTPD test library, which can be configured using
> the following environment variables.
> 
> LIB_HTTPD_PATH		web server path
> LIB_HTTPD_MODULE_PATH	web server modules path
> LIB_HTTPD_PORT		listening port
> LIB_HTTPD_DAV		enable DAV
> LIB_HTTPD_SVN		enable SVN
> LIB_HTTPD_SSL		enable SSL

Overall, this looks fine to me, though I haven't taken a deep look at what the test itself does. I was more interested in the "library".

Just a few nitpicks.

> +HTTPD_ROOT_PATH="$PWD"/httpd
> +HTTPD_DOCUMENT_ROOT_PATH="$PWD"/httpd/www

Why not just use $HTTPD_ROOT_PATH/www and don't use the $HTTPD_DOCUMENT_ROOT_PATH variable ?

> +	mkdir $HTTPD_ROOT_PATH
> +	mkdir $HTTPD_DOCUMENT_ROOT_PATH

This could become mkdir -p $HTTPD_ROOT_PATH/www

> +	if test -n "$LIB_HTTPD_SSL"
> +	if test "$LIB_HTTPD_SSL" != ""

Why use different tests?

Show 7 quoted lines

> +		openssl req \
> +			-config $TEST_PATH/ssl.cnf \
> +			-new -x509 -nodes \
> +			-out $HTTPD_ROOT_PATH/httpd.pem \
> +			-keyout $HTTPD_ROOT_PATH/httpd.pem
> +		export GIT_SSL_NO_VERIFY=t
> +		HTTPD_PARA="$HTTPD_PARA -DSSL"

For future improvement (that could be another patch), it would probably be nice to generate a CAcert and a certificate signed by it, so we can also test without GIT_SSL_NO_VERIFY.

By the way, why not just include the .pem ?

> +stop_httpd() {
> +	"$LIB_HTTPD_PATH" $HTTPD_PARA -k stop
> +}

As Junio said, it might be good to add a trap.

> +RANDFILE                = $ENV::HOME/.rnd

Wouldn't it be better elsewhere ?

Mike