[PATCHv1] git-p4: test harness directory handling tidyup

Luke Diamand· May 12, 2011, 05:14 UTC · lore

Junio pointed out that the git-p4 test harness (t9800-git-p4.sh) has tests that 'cd' into a directory, do stuff, and then cd back out. If the tests fail partway through then the process is left in the wrong directory for the next test.

This patch changes it to use the "(cd dir && do_test)" pattern instead.

Luke Diamand (1):
  git-p4: test harness directory changing tidyup

 t/t9800-git-p4.sh |   37 ++++++++++++++++++++++---------------
 1 files changed, 22 insertions(+), 15 deletions(-)

Luke Diamand· May 12, 2011, 05:14 UTC · re: Luke Diamand · lore

The git-p4 test harness relied a lot on cd'ing to the target directory
and then cd'ing back explicitly. That caused problems if the test failed
partway through. i.e.
  cd $git && stuff && cd "$TRASH_DIRECTORY"

Instead, use:
  (cd $git && stuff)

Signed-off-by: Luke Diamand <luke@diamand.org>
Suggested-by: Junio C Hamano <gitster@pobox.com>
---
 t/t9800-git-p4.sh |   37 ++++++++++++++++++++++---------------
 1 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 888ad54..122e91d 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -28,6 +28,8 @@ test_expect_success setup '
 '
 
 test_expect_success 'add p4 files' '
+	export P4CLIENT=client &&
+	(
 	cd "$cli" &&
 	p4 client -i <<-EOF &&
 	Client: client
@@ -35,14 +37,13 @@ test_expect_success 'add p4 files' '
 	Root: $cli
 	View: //depot/... //client/...
 	EOF
-	export P4CLIENT=client &&
 	echo file1 >file1 &&
 	p4 add file1 &&
 	p4 submit -d "file1" &&
 	echo file2 >file2 &&
 	p4 add file2 &&
-	p4 submit -d "file2" &&
-	cd "$TRASH_DIRECTORY"
+	p4 submit -d "file2"
+	)
 '
 
 test_expect_success 'basic git-p4 clone' '
@@ -100,34 +101,37 @@ test_expect_success 'exit when p4 fails to produce marshaled output' '
 '
 
 test_expect_success 'add p4 files with wildcards in the names' '
+	(
 	cd "$cli" &&
 	echo file-wild-hash >file-wild#hash &&
 	echo file-wild-star >file-wild\*star &&
 	echo file-wild-at >file-wild@at &&
 	echo file-wild-percent >file-wild%percent &&
 	p4 add -f file-wild* &&
-	p4 submit -d "file wildcards" &&
-	cd "$TRASH_DIRECTORY"
+	p4 submit -d "file wildcards"
+	)
 '
 
 test_expect_success 'wildcard files git-p4 clone' '
 	"$GITP4" clone --dest="$git" //depot &&
+	(
 	cd "$git" &&
 	test -f file-wild#hash &&
 	test -f file-wild\*star &&
 	test -f file-wild@at &&
-	test -f file-wild%percent &&
-	cd "$TRASH_DIRECTORY" &&
+	test -f file-wild%percent
+	)
 	rm -rf "$git" && mkdir "$git"
 '
 
 test_expect_success 'clone bare' '
 	"$GITP4" clone --dest="$git" --bare //depot &&
+	(
 	cd "$git" &&
 	test ! -d .git &&
 	bare=`git config --get core.bare` &&
-	test "$bare" = true &&
-	cd "$TRASH_DIRECTORY" &&
+	test "$bare" = true
+	)
 	rm -rf "$git" && mkdir "$git"
 '
 
@@ -175,6 +179,7 @@ test_expect_success 'preserve users' '
 	p4_add_user bob Bob &&
 	p4_grant_admin alice &&
 	"$GITP4" clone --dest="$git" //depot &&
+	(
 	cd "$git" &&
 	echo "username: a change by alice" >> file1 &&
 	echo "username: a change by bob" >> file2 &&
@@ -183,8 +188,8 @@ test_expect_success 'preserve users' '
 	git config git-p4.skipSubmitEditCheck true &&
 	P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
 	p4_check_commit_author file1 alice &&
-	p4_check_commit_author file2 bob &&
-	cd "$TRASH_DIRECTORY" &&
+	p4_check_commit_author file2 bob
+	)
 	rm -rf "$git" && mkdir "$git"
 '
 
@@ -192,18 +197,20 @@ test_expect_success 'preserve users' '
 # not submit change to p4 (git diff should show deltas).
 test_expect_success 'refuse to preserve users without perms' '
 	"$GITP4" clone --dest="$git" //depot &&
+	(
 	cd "$git" &&
 	echo "username-noperms: a change by alice" >> file1 &&
 	git commit --author "Alice <alice@localhost>" -m "perms: a change by alice" file1 &&
 	! P4EDITOR=touch P4USER=bob P4PASSWD=secret "$GITP4" commit --preserve-user &&
-	! git diff --exit-code HEAD..p4/master > /dev/null &&
-	cd "$TRASH_DIRECTORY" &&
+	! git diff --exit-code HEAD..p4/master > /dev/null
+	)
 	rm -rf "$git" && mkdir "$git"
 '
 
 # What happens with unknown author? Without allowMissingP4Users it should fail.
 test_expect_success 'preserve user where author is unknown to p4' '
 	"$GITP4" clone --dest="$git" //depot &&
+	(
 	cd "$git" &&
 	git config git-p4.skipSubmitEditCheck true
 	echo "username-bob: a change by bob" >> file1 &&
@@ -217,8 +224,8 @@ test_expect_success 'preserve user where author is unknown to p4' '
 	git config git-p4.preserveUser true &&
 	P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit &&
 	git diff --exit-code HEAD..p4/master > /dev/null &&
-	p4_check_commit_author file1 alice &&
-	cd "$TRASH_DIRECTORY" &&
+	p4_check_commit_author file1 alice
+	)
 	rm -rf "$git" && mkdir "$git"
 '

-- 
1.7.1

Pete Wyckoff· May 12, 2011, 11:29 UTC · re: Luke Diamand · lore

Re: [PATCHv1] git-p4: test harness directory handling tidyup

luke@diamand.org wrote on Thu, 12 May 2011 06:14 +0100:

Show 10 quoted lines

> The git-p4 test harness relied a lot on cd'ing to the target directory
> and then cd'ing back explicitly. That caused problems if the test failed
> partway through. i.e.
>   cd $git && stuff && cd "$TRASH_DIRECTORY"
> 
> Instead, use:
>   (cd $git && stuff)
> 
> Signed-off-by: Luke Diamand <luke@diamand.org>
> Suggested-by: Junio C Hamano <gitster@pobox.com>

I'm glad that Junio noticed and that you took the time to fix this. It was indeed a problem.

Acked-by: Pete Wyckoff <pw@padd.com>