git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 3/3] t/lib-httpd: avoid occasional failures when checking access.log

From
SZEDER Gábor <szeder.dev@gmail.com>
Date
Jul 11, 2018, 12:56 UTC
Message-ID
<20180711125647.16512-4-szeder.dev@gmail.com>
In-Reply-To
<20180711125647.16512-1-szeder.dev@gmail.com>

The last test of 't5561-http-backend.sh', 'server request log matches test results' may fail occasionally, because the order of entries in Apache's access log doesn't match the order of requests sent in the previous tests, although all the right requests are there. I saw it fail on Travis CI five times in the span of about half a year, when the order of two subsequent requests was flipped, and could trigger the failure with a modified Git. However, I was unable to trigger it with stock Git on my machine. Three tests in 't5541-http-push-smart.sh' and 't5551-http-fetch-smart.sh' check requests in the log the same way, so they might be prone to a similar occasional failure as well.

When a test sends a HTTP request, it can continue execution after 'git-http-backend' fulfilled that request, but Apache writes the corresponding access log entry only after 'git-http-backend' exited. Some time inevitably passes between fulfilling the request and writing the log entry, and, under unfavourable circumstances, enough time might pass for the subsequent request to be sent and fulfilled by a different Apache thread or process, and then Apache writes access log entries racily.

This effect can be exacerbated by adding a bit of variable delay after the request is fulfilled but before 'git-http-backend' exits, e.g. like this:

  diff --git a/http-backend.c b/http-backend.c
  index f3dc218b2..bbf4c125b 100644
  --- a/http-backend.c
  +++ b/http-backend.c
  @@ -709,5 +709,7 @@ int cmd_main(int argc, const char **argv)
   					   max_request_buffer);
   	cmd->imp(&hdr, cmd_arg);
  +	if (getpid() % 2)
  +		sleep(1);
   	return 0;
   }

This delay considerably increases the chances of log entries being written out of order, and in turn makes t5561's last test fail almost every time. Alas, it doesn't seem to be enough to trigger a similar failure in t5541 and t5551.

So, since we can't just rely on the order of access log entries always corresponding the order of requests, make checking the access log more deterministic by sorting (simply lexicographically) both the stripped access log entries and the expected entries before the comparison with 'test_cmp'. This way the order of log entries won't matter and occasional out-of-order entries won't trigger a test failure, but the comparison will still notice any unexpected or missing log entries.

OTOH, this sorting will make it harder to identify from which test an unexpected log entry came from or which test's request went missing. Therefore, in case of an error include the comparison of the unsorted log enries in the test output as well.

And since all this should be performed in four tests in three test scripts, put this into a new helper function 'check_access_log' in 't/lib-httpd.sh'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/lib-httpd.sh              | 12 ++++++++++++
 t/t5541-http-push-smart.sh  | 13 +++++--------
 t/t5551-http-fetch-smart.sh |  3 +--
 t/t5561-http-backend.sh     |  3 +--
 4 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index b6788fea57..7f060aebd0 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -296,3 +296,15 @@ strip_access_log() {
 		s/^GET /GET  /
 	" "$HTTPD_ROOT_PATH"/access.log
 }
+
+# Requires one argument: the name of a file containing the expected stripped
+# access log entries.
+check_access_log() {
+	sort "$1" >"$1".sorted &&
+	strip_access_log >access.log.stripped &&
+	sort access.log.stripped >access.log.sorted &&
+	if ! test_cmp "$1".sorted access.log.sorted
+	then
+		test_cmp "$1" access.log.stripped
+	fi
+}
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 6cd986797d..a481e3989a 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -43,15 +43,13 @@ test_expect_success 'no empty path components' '
 	cd "$ROOT_PATH" &&
 	git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
 
-	strip_access_log >act &&
+	check_access_log exp
+'
 
+test_expect_success 'clear access log' '
 	# Clear the log, so that it does not affect the "used receive-pack
 	# service" test which reads the log too.
-	#
-	# We do this before the actual comparison to ensure the log is cleared.
-	>"$HTTPD_ROOT_PATH"/access.log &&
-
-	test_cmp exp act
+	>"$HTTPD_ROOT_PATH"/access.log
 '
 
 test_expect_success 'clone remote repository' '
@@ -132,8 +130,7 @@ GET  /smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
 POST /smart/test_repo.git/git-receive-pack HTTP/1.1 200
 EOF
 test_expect_success 'used receive-pack service' '
-	strip_access_log >act &&
-	test_cmp exp act
+	check_access_log exp
 '
 
 test_http_push_nonff "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index c8b6ec493b..3aab44bdcb 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -103,8 +103,7 @@ GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
 POST /smart/repo.git/git-upload-pack HTTP/1.1 200
 EOF
 test_expect_success 'used upload-pack service' '
-	strip_access_log >act &&
-	test_cmp exp act
+	check_access_log exp
 '
 
 test_expect_success 'follow redirects (301)' '
diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index 4248b5a06d..1c49054595 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -129,8 +129,7 @@ GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -
 POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
 EOF
 test_expect_success 'server request log matches test results' '
-	strip_access_log >act &&
-	test_cmp exp act
+	check_access_log exp
 '
 
 stop_httpd
-- 
2.18.0.273.g57f1ecce9c
Previous: SZEDER GáborNext: Jeff King
Message 17 of 26 in “Fix occasional test failures in http tests”
  1. 0/4 Fix occasional test failures in http testsSZEDER Gábor, Jun 14, 2018
  2. 1/4 t5541: avoid empty line when truncating access logSZEDER Gábor, Jun 14, 2018
  3. 2/4 t/lib-httpd: add the strip_access_log() helper functionSZEDER Gábor, Jun 14, 2018
  4. 3/4 t/lib-httpd: add minor and patch version to $HTTPD_VERSIONSZEDER Gábor, Jun 14, 2018
  5. 4/4 t/lib-httpd: sort log based on timestamp to avoid occasional failureSZEDER Gábor, Jun 14, 2018
  6. SZEDER GáborJun 14, 2018
  7. Jeff KingJun 14, 2018
  8. Junio C HamanoJun 14, 2018
  9. Jeff KingJun 15, 2018
  10. SZEDER GáborJul 11, 2018
  11. Jeff KingJul 11, 2018
  12. Junio C HamanoJun 14, 2018
  13. Jeff KingJun 14, 2018
  14. 0/3 Fix occasional test failures in http testsSZEDER Gábor, Jul 11, 2018
  15. 1/3 t5541: avoid empty line when truncating access logSZEDER Gábor, Jul 11, 2018
  16. 2/3 t/lib-httpd: add the strip_access_log() helper functionSZEDER Gábor, Jul 11, 2018
  17. 3/3 t/lib-httpd: avoid occasional failures when checking access.logSZEDER Gábor, Jul 11, 2018
  18. Jeff KingJul 11, 2018
  19. SZEDER GáborJul 11, 2018
  20. Jeff KingJul 11, 2018
  21. Jeff KingJul 11, 2018
  22. 0/3 Fix occasional test failures in http testsSZEDER Gábor, Jul 12, 2018
  23. 1/3 t5541: clean up truncating access logSZEDER Gábor, Jul 12, 2018
  24. Jeff KingJul 12, 2018
  25. 2/3 t/lib-httpd: add the strip_access_log() helper functionSZEDER Gábor, Jul 12, 2018
  26. 3/3 t/lib-httpd: avoid occasional failures when checking access.logSZEDER Gábor, Jul 12, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.