# [PATCH] tests: clean after SANITY tests 2 messages from 2018-06-15 to 2018-06-15. Participants: Junio C Hamano, Eric Sunshine. Thread: https://gitlist.dev/t/48717 ## Junio C Hamano, 2018-06-15 18:30 Subject: [PATCH] tests: clean after SANITY tests Message-ID: URL: https://gitlist.dev/e/xmqqa7rv297r.fsf%40gitster-ct.c.googlers.com ``` Some of our tests try to make sure Git behaves sensibly in a read-only directory, by dropping 'w' permission bit before doing a test and then restoring it after it is done. The latter is needed for the test framework to clean after itself without leaving a leftover directory that cannot be removed. Ancient parts of tests however arrange the above with chmod a-w . && ... do the test ... status=$? chmod 775 . (exit $status) which obviously would not work if the test somehow dies before it has the chance to do "chmod 775". Rewrite them by following a more robust pattern recently written tests use, which is test_when_finished "chmod 775 ." && chmod a-w . && ... do the test ... Signed-off-by: Junio C Hamano --- * I happened to hit ^C at the right moment and saw "make distclean" fail miserably. I guess you can still kill the process in such a way that when-finished hander would not fire, so in that sense this is more about consistency and uniformity than the actual safety, but anyway... t/t0001-init.sh | 1 + t/t0070-fundamental.sh | 2 +- t/t1004-read-tree-m-u-wf.sh | 6 ++---- t/t5537-fetch-shallow.sh | 2 +- t/t7508-status.sh | 4 +--- 5 files changed, 6 insertions(+), 9 deletions(-) diff --git a/t/t0001-init.sh b/t/t0001-init.sh index c413bff9cf..4c865051e7 100755 --- a/t/t0001-init.sh +++ b/t/t0001-init.sh @@ -287,6 +287,7 @@ test_expect_success 'init notices EEXIST (2)' ' ' test_expect_success POSIXPERM,SANITY 'init notices EPERM' ' + test_when_finished "chmod +w newdir" && rm -fr newdir && mkdir newdir && chmod -w newdir && diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 991ed2a48d..60e3de7b7b 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -19,8 +19,8 @@ test_expect_success 'mktemp to nonexistent directory prints filename' ' test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints filename' ' mkdir cannotwrite && - chmod -w cannotwrite && test_when_finished "chmod +w cannotwrite" && + chmod -w cannotwrite && test_must_fail test-mktemp cannotwrite/testXXXXXX 2>err && grep "cannotwrite/test" err ' diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh index c7ce5d8bb5..826cd32e23 100755 --- a/t/t1004-read-tree-m-u-wf.sh +++ b/t/t1004-read-tree-m-u-wf.sh @@ -179,6 +179,8 @@ test_expect_success 'funny symlink in work tree' ' test_expect_success SANITY 'funny symlink in work tree, un-unlink-able' ' + test_when_finished "chmod a+w a 2>/dev/null; rm -fr a b" && + rm -fr a b && git reset --hard && @@ -188,10 +190,6 @@ test_expect_success SANITY 'funny symlink in work tree, un-unlink-able' ' ' -# clean-up from the above test -chmod a+w a 2>/dev/null -rm -fr a b - test_expect_success 'D/F setup' ' git reset --hard && diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index df8d2f095a..943231af96 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -175,8 +175,8 @@ EOF test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' ' cp -R .git read-only.git && - find read-only.git -print | xargs chmod -w && test_when_finished "find read-only.git -type d -print | xargs chmod +w" && + find read-only.git -print | xargs chmod -w && git clone --no-local --depth=2 read-only.git from-read-only && git --git-dir=from-read-only/.git log --format=%s >actual && cat >expect < bar && git add bar && git commit -q -m 'Add bar') && git add sm -- 2.18.0-rc2 ``` ## Eric Sunshine, 2018-06-15 19:57 Subject: Re: [PATCH] tests: clean after SANITY tests Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQQEKjM43OqX9bT2%3D_yFjkiUMqjbZag5TWpsh6d_Hwn_g%40mail.gmail.com In-Reply-To: ``` On Fri, Jun 15, 2018 at 2:30 PM Junio C Hamano wrote: > Some of our tests try to make sure Git behaves sensibly in a > read-only directory, by dropping 'w' permission bit before doing a > test and then restoring it after it is done. The latter is needed > for the test framework to clean after itself without leaving a > leftover directory that cannot be removed. > [...] > Signed-off-by: Junio C Hamano > --- > diff --git a/t/t0001-init.sh b/t/t0001-init.sh > @@ -287,6 +287,7 @@ test_expect_success 'init notices EEXIST (2)' ' > test_expect_success POSIXPERM,SANITY 'init notices EPERM' ' > + test_when_finished "chmod +w newdir" && > rm -fr newdir && > mkdir newdir && > chmod -w newdir && When reading this, I was wondering if there was a "rm -fr newdir" at the end of the test which could be removed now that it uses test_when_finished(). Checking beyond the shown context, I see that it doesn't bother with removal since the next test removes the directory as a preparatory step. Even before the addition of test_when_finished() to restore write permission, the subsequent test's removal of the directory worked because this is test is only run when POSIXPERM is met, and POSIX allows for such an operation. Okay, looks good. ```