threads / patch / 40427

patcht5561: get rid of racy appending to logfile

Subject: [PATCH] t5561: get rid of racy appending to logfile

## tl;dr

2 messages between Sep 24, 2015 and Sep 25, 2015. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Stephan Beyer· Sep 24, 2015, 23:31 UTC · lore

The definition of log_div() appended information to the web server's logfile to make the test more readable. However, log_div() was called right after a request is served (which is done by git-http-backend); the web server waits for the git-http-backend process to exit before it writes to the log file. When the duration between serving a request and exiting was long, the log_div() output was written before the last request's log, and the test failed. (This duration could become especially long for PROFILE=GEN builds.)

To get rid of this behavior, we should not change the logfile at all. This commit removes log_div() and its calls. The additional information is kept in the test (for readability reasons) but filtered out before comparing it to the actual logfile.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
Acked-by: Jeff King <peff@peff.net>
---
 SubmittingPatches says that when there is consensus the patch has to
 be resent to Junio and cc'ed to the list. Here it is (although I
 don't know if there is consensus, but, hey, it's a rather trivial patch,
 so it should be okay).
 See http://git.661346.n2.nabble.com/t5561-failing-after-make-PROFILE-GEN-td7640150.html
 for the original thread.
 Compared to the last version, I only added the Acked-by line.
 t/t5560-http-backend-noserver.sh |  4 ----
 t/t5561-http-backend.sh          |  8 +-------
 t/t556x_common                   | 12 ------------
 3 files changed, 1 insertion(+), 23 deletions(-)
Show changes to 3 files +1 −23

t/t5560-http-backend-noserver.sh, t/t5561-http-backend.sh, t/t556x_common

diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index aa73eea..9fafcf1 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -44,10 +44,6 @@ POST() {
 	test_cmp exp act
 }
 
-log_div() {
-	return 0
-}
-
 . "$TEST_DIRECTORY"/t556x_common
 
 expect_aliased() {
diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index 19afe96..73dcb29 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -29,15 +29,9 @@ POST() {
 	test_cmp exp act
 }
 
-log_div() {
-	echo >>"$HTTPD_ROOT_PATH"/access.log
-	echo "###  $1" >>"$HTTPD_ROOT_PATH"/access.log
-	echo "###" >>"$HTTPD_ROOT_PATH"/access.log
-}
-
 . "$TEST_DIRECTORY"/t556x_common
 
-cat >exp <<EOF
+grep '^[^#]' >exp <<EOF
 
 ###  refs/heads/master
 ###
diff --git a/t/t556x_common b/t/t556x_common
index 82926cf..359fcfe 100755
--- a/t/t556x_common
+++ b/t/t556x_common
@@ -52,21 +52,17 @@ get_static_files() {
 SMART=smart
 GIT_HTTP_EXPORT_ALL=1 && export GIT_HTTP_EXPORT_ALL
 test_expect_success 'direct refs/heads/master not found' '
-	log_div "refs/heads/master" &&
 	GET refs/heads/master "404 Not Found"
 '
 test_expect_success 'static file is ok' '
-	log_div "getanyfile default" &&
 	get_static_files "200 OK"
 '
 SMART=smart_noexport
 unset GIT_HTTP_EXPORT_ALL
 test_expect_success 'no export by default' '
-	log_div "no git-daemon-export-ok" &&
 	get_static_files "404 Not Found"
 '
 test_expect_success 'export if git-daemon-export-ok' '
-	log_div "git-daemon-export-ok" &&
         (cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
 	 touch git-daemon-export-ok
 	) &&
@@ -75,47 +71,39 @@ test_expect_success 'export if git-daemon-export-ok' '
 SMART=smart
 GIT_HTTP_EXPORT_ALL=1 && export GIT_HTTP_EXPORT_ALL
 test_expect_success 'static file if http.getanyfile true is ok' '
-	log_div "getanyfile true" &&
 	config http.getanyfile true &&
 	get_static_files "200 OK"
 '
 test_expect_success 'static file if http.getanyfile false fails' '
-	log_div "getanyfile false" &&
 	config http.getanyfile false &&
 	get_static_files "403 Forbidden"
 '
 
 test_expect_success 'http.uploadpack default enabled' '
-	log_div "uploadpack default" &&
 	GET info/refs?service=git-upload-pack "200 OK"  &&
 	POST git-upload-pack 0000 "200 OK"
 '
 test_expect_success 'http.uploadpack true' '
-	log_div "uploadpack true" &&
 	config http.uploadpack true &&
 	GET info/refs?service=git-upload-pack "200 OK" &&
 	POST git-upload-pack 0000 "200 OK"
 '
 test_expect_success 'http.uploadpack false' '
-	log_div "uploadpack false" &&
 	config http.uploadpack false &&
 	GET info/refs?service=git-upload-pack "403 Forbidden" &&
 	POST git-upload-pack 0000 "403 Forbidden"
 '
 
 test_expect_success 'http.receivepack default disabled' '
-	log_div "receivepack default" &&
 	GET info/refs?service=git-receive-pack "403 Forbidden"  &&
 	POST git-receive-pack 0000 "403 Forbidden"
 '
 test_expect_success 'http.receivepack true' '
-	log_div "receivepack true" &&
 	config http.receivepack true &&
 	GET info/refs?service=git-receive-pack "200 OK" &&
 	POST git-receive-pack 0000 "200 OK"
 '
 test_expect_success 'http.receivepack false' '
-	log_div "receivepack false" &&
 	config http.receivepack false &&
 	GET info/refs?service=git-receive-pack "403 Forbidden" &&
 	POST git-receive-pack 0000 "403 Forbidden"
-- 
2.6.0.rc3.dirty
Junio C Hamano· Sep 25, 2015, 17:44 UTC · re: Stephan Beyer · lore

Re: [PATCH] t5561: get rid of racy appending to logfile

Stephan Beyer <s-beyer@gmx.net> writes:
>  SubmittingPatches says that when there is consensus the patch has to
>  be resent to Junio and cc'ed to the list. Here it is (although I
>  don't know if there is consensus, but, hey, it's a rather trivial patch,
>  so it should be okay).
Yup.

The patch text matches exactly what I already queued with Reviewed-by from Peff earlier, which is good.

Thanks.

← back to recent threads