From df4c0d1a7927e17e7944ec24fa94468eee979f6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?SZEDER=20G=C3=A1bor?= Date: Thu, 20 Apr 2017 18:52:30 +0200 Subject: [PATCH 1/3] test-lib: abort when can't remove trash directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We had two similar bugs in the tests sporadically triggering error messages during the removal of the trash directory, see commits bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01) and ef09036cf (t6500: wait for detached auto gc at the end of the test script, 2017-04-13). The test script succeeded nonetheless, because these errors are ignored during housekeeping in 'test_done'. However, such an error is a sign that something is fishy in the test script. Print an error message and abort the test script when the trash directory can't be removed successfully or is already removed, because that's unexpected and we would prefer somebody notice and figure out why. Signed-off-by: SZEDER Gábor Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- t/test-lib.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index cde7fc7fcf..cb0766b9ee 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -760,9 +760,12 @@ test_done () { say "1..$test_count$skip_all" fi - test -d "$remove_trash" && + test -d "$remove_trash" || + error "Tests passed but trash directory already removed before test cleanup; aborting" + cd "$(dirname "$remove_trash")" && - rm -rf "$(basename "$remove_trash")" + rm -rf "$(basename "$remove_trash")" || + error "Tests passed but test cleanup failed; aborting" test_at_end_hook_ From 4d0912a206a32e9763424363617e8425f049f344 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 24 Apr 2017 23:39:47 -0700 Subject: [PATCH 2/3] test-lib.sh: do not barf under --debug at the end of the test The original did "does $remove_trash exist? Then go one level above and remove it". There was no problem under "--debug", where the variable is left empty, as the first "test -d $remove_trash" would have said "No, it doesn't". With the check implemented in the previous step, we'd always get an error under "--debug". Noticed-by: Jeff King Signed-off-by: Junio C Hamano --- t/test-lib.sh | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index cb0766b9ee..d2f9ad5ae5 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -760,13 +760,16 @@ test_done () { say "1..$test_count$skip_all" fi - test -d "$remove_trash" || - error "Tests passed but trash directory already removed before test cleanup; aborting" + if test -n "$remove_trash" + then + test -d "$remove_trash" || + error "Tests passed but trash directory already removed before test cleanup; aborting" - cd "$(dirname "$remove_trash")" && - rm -rf "$(basename "$remove_trash")" || - error "Tests passed but test cleanup failed; aborting" + cd "$(dirname "$remove_trash")" && + rm -rf "$(basename "$remove_trash")" || + error "Tests passed but test cleanup failed; aborting" + fi test_at_end_hook_ exit 0 ;; From 06478dab4c9fb57dc6b7299eafd18df1bea9ca22 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 23 Apr 2017 17:15:09 -0700 Subject: [PATCH 3/3] test-lib: retire $remove_trash variable The convention "$remove_trash is set to the trash directory that is used during the test, so that it will be removed at the end, but under --debug option we set the varilable to empty string to preserve the directory" made sense back when it was introduced, as there was no $TRASH_DIRECTORY variable. These days, since no tests looks at the variable, it is obscure and even risks that by mistake the variable gets used for something else (e.g. remove_trash=yes) and cause us misbehave. Worse yet, remove_trash was not initialized to an empty string at the beginning, so a stray environment variable the user has could have affected the logic when "--debug" is in use. Rewrite the clean-up sequence in test_done helper to explicitly check the $debug condition and remove the trash directory using the $TRASH_DIRECTORY variable. Note that "go to the directory one level above the trash and then remove it" is kept and this is deliverate; test_at_end_hook_ will keep running from the expected location, and also some platforms may not like a directory that is serving as the $cwd of a still-active process removed. Signed-off-by: Junio C Hamano --- t/test-lib.sh | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index d2f9ad5ae5..51b59c69cb 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -760,15 +760,14 @@ test_done () { say "1..$test_count$skip_all" fi - if test -n "$remove_trash" + if test -z "$debug" then - test -d "$remove_trash" || + test -d "$TRASH_DIRECTORY" || error "Tests passed but trash directory already removed before test cleanup; aborting" - cd "$(dirname "$remove_trash")" && - rm -rf "$(basename "$remove_trash")" || + cd "$TRASH_DIRECTORY/.." && + rm -fr "$TRASH_DIRECTORY" || error "Tests passed but test cleanup failed; aborting" - fi test_at_end_hook_ @@ -924,7 +923,6 @@ case "$TRASH_DIRECTORY" in /*) ;; # absolute path is good *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;; esac -test ! -z "$debug" || remove_trash=$TRASH_DIRECTORY rm -fr "$TRASH_DIRECTORY" || { GIT_EXIT_OK=t echo >&5 "FATAL: Cannot prepare test area"