Re: [PATCH 05/11] tests: use 'test_atexit' to stop httpd

From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Jeff King <peff@peff.net>, Luke Diamand <luke@diamand.org>,
	Lars Schneider <larsxschneider@gmail.com>
Subject: Re: [PATCH 05/11] tests: use 'test_atexit' to stop httpd
Date: Thu, 14 Mar 2019 16:19:37 +0100	[thread overview]
Message-ID: <20190314151937.GG28939@szeder.dev> (raw)
In-Reply-To: <xmqqd0mub0d8.fsf@gitster-ct.c.googlers.com>

On Thu, Mar 14, 2019 at 12:28:35PM +0900, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:

> I see most of these changes are removal of stop_httpd because it is
> done as part of start_httpd() to arrange it to be called at exit.
> 
> But ...
> 
> > @@ -176,7 +175,7 @@ prepare_httpd() {
> >  start_httpd() {
> >  	prepare_httpd >&3 2>&4
> >  
> > -	trap 'code=$?; stop_httpd; (exit $code); die' EXIT
> > +	test_atexit stop_httpd
> >  
> >  	"$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
> >  		-f "$TEST_PATH/apache.conf" $HTTPD_PARA \
> > @@ -184,15 +183,12 @@ start_httpd() {
> >  		>&3 2>&4
> >  	if test $? -ne 0
> >  	then
> > -		trap 'die' EXIT
> >  		cat "$HTTPD_ROOT_PATH"/error.log >&4 2>/dev/null
> >  		test_skip_or_die $GIT_TEST_HTTPD "web server setup failed"
> >  	fi
> >  }
> >  
> >  stop_httpd() {
> > -	trap 'die' EXIT
> > -
> >  	"$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
> >  		-f "$TEST_PATH/apache.conf" $HTTPD_PARA -k stop
> >  }
> 
> ... I see we lost many "trap 'die' EXIT" in the orignal.  Is that
> something we want to lose as part of this commit?  It does not make
> sense, at least to me, to add a "test_atexit die" and I am mostly
> wondering what these traps were trying to do in the original.

It restored our test framework's default EXIT trap, because without it
'stop_httpd' would have been invoked after 'test_done' as well.  While
invoking it twice is probably not that big of a deal (though arguably
not the cleanest solution), invoking it after 'test_done' is
definitely bad, because at that point the trash directory and thus
$HTTPD_ROOT_PATH has already been removed, resulting in an ugly error:

  $ ./t5561-http-backend.sh
  <....>
  ok 14 - server request log matches test results
  # passed all 14 test(s)
  1..14
  apache2: Syntax error on line 11 of /home/szeder/src/git/t/lib-httpd/apache.conf: Cannot load modules/mod_alias.so into server: /home/szeder/src/git/t/trash directory.t5561-http-backend/httpd/modules/mod_alias.so: cannot open shared object file: No such file or directory

'test_atexit' doesn't overwrite the test frameowork's default EXIT
trap, so we don't have to restore anything.