[PATCH] rewrite t1500-rev-parse.sh

git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH] rewrite t1500-rev-parse.sh
@ 2016-04-09 11:19 Michael Rappazzo
  2016-04-09 11:19 ` [PATCH] t1500-rev-parse: rewrite each test to run in isolation Michael Rappazzo
  0 siblings, 1 reply; 7+ messages in thread
From: Michael Rappazzo @ 2016-04-09 11:19 UTC (permalink / raw)
  To: git; +Cc: gitster, sunshine, pclouds, szeder, Michael Rappazzo

I was working on a simple bug fix[1], and I wanted to add a test to t1500.  I put
the new test at the beginning rather than try to decipher what state the test run
would be in at the end. In the review, Eric Sunshine described the test:

> this script is ancient and cd's all around the place with wild abandon and
> leaks environment variables

I decided to rewrite this test in (what I hope is) the modern test format.  In
doing so, I expanded the size of the test file greatly, but I think it is much
clearer to read and understand as you go through it.  This also has the advantage
of allowing a tester to use verbose options when testing.  As noted in the commit
comment, tests which compare text (rather than booleans) were also adjusted to use
the test_cmp function.

This patch is based on 'pu' because of a feature to add `--absolute-git-dir` to
rev-parse[2].

I considered including this patch as the first commit in the aforementioned bug
fix, but I wanted to get it out for review to make sure that it is acceptible.

[1]http://thread.gmane.org/gmane.comp.version-control.git/290669
[2]http://article.gmane.org/gmane.comp.version-control.git/287462

Michael Rappazzo (1):
  t1500-rev-parse: rewrite each test to run in isolation

 t/t1500-rev-parse.sh | 607 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 545 insertions(+), 62 deletions(-)

-- 
2.8.0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] t1500-rev-parse: rewrite each test to run in isolation
  2016-04-09 11:19 [PATCH] rewrite t1500-rev-parse.sh Michael Rappazzo
@ 2016-04-09 11:19 ` Michael Rappazzo
  2016-04-13  2:03   ` Junio C Hamano
  2016-04-13  4:54   ` Eric Sunshine
  0 siblings, 2 replies; 7+ messages in thread
From: Michael Rappazzo @ 2016-04-09 11:19 UTC (permalink / raw)
  To: git; +Cc: gitster, sunshine, pclouds, szeder, Michael Rappazzo

t1500-rev-parse has many tests which change directories and leak
environment variables.  This makes it difficult to add new tests without
minding the environment variables and current directory.

Each test is now setup, executed, and cleaned up without leaving anything
behind.  Tests which have textual expectations have been converted to use
test_cmp (which will show a diff when the test is run with --verbose).

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 t/t1500-rev-parse.sh | 607 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 545 insertions(+), 62 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 617fcd8..dffa9f3 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,88 +3,571 @@
 test_description='test git rev-parse'
 . ./test-lib.sh
 
-test_rev_parse() {
-	name=$1
-	shift
+test_expect_success 'toplevel: is-bare-repository' '
+	test false = "$(git rev-parse --is-bare-repository)"
+'
 
-	test_expect_success "$name: is-bare-repository" \
-	"test '$1' = \"\$(git rev-parse --is-bare-repository)\""
-	shift
-	[ $# -eq 0 ] && return
+test_expect_success 'toplevel: is-inside-git-dir' '
+	test false = "$(git rev-parse --is-inside-git-dir)"
+'
 
-	test_expect_success "$name: is-inside-git-dir" \
-	"test '$1' = \"\$(git rev-parse --is-inside-git-dir)\""
-	shift
-	[ $# -eq 0 ] && return
+test_expect_success 'toplevel: is-inside-work-tree' '
+	test true = "$(git rev-parse --is-inside-work-tree)"
+'
 
-	test_expect_success "$name: is-inside-work-tree" \
-	"test '$1' = \"\$(git rev-parse --is-inside-work-tree)\""
-	shift
-	[ $# -eq 0 ] && return
+test_expect_success 'toplevel: prefix' '
+	echo >expected &&
+	git rev-parse --show-prefix >actual &&
+	test_cmp expected actual
+'
 
-	test_expect_success "$name: prefix" \
-	"test '$1' = \"\$(git rev-parse --show-prefix)\""
-	shift
-	[ $# -eq 0 ] && return
+test_expect_success 'toplevel: git-dir' '
+	echo .git >expected &&
+	git rev-parse --git-dir >actual &&
+	test_cmp expected actual
+'
 
-	test_expect_success "$name: git-dir" \
-	"test '$1' = \"\$(git rev-parse --git-dir)\""
-	shift
-	[ $# -eq 0 ] && return
+test_expect_success 'toplevel: absolute-git-dir' '
+	echo "$(pwd)/.git" >expected &&
+	git rev-parse --absolute-git-dir >actual &&
+	test_cmp expected actual
+'
 
-	test_expect_success "$name: absolute-git-dir" \
-	"verbose test '$1' = \"\$(git rev-parse --absolute-git-dir)\""
-}
+test_expect_success '.git/: is-bare-repository' '
+	(cd .git && test false = "$(git rev-parse --is-bare-repository)")
+'
 
-# label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
+test_expect_success '.git/: is-inside-git-dir' '
+	(cd .git && test true = "$(git rev-parse --is-inside-git-dir)")
+'
 
-ROOT=$(pwd)
+test_expect_success '.git/: is-inside-work-tree' '
+	(cd .git && test false = "$(git rev-parse --is-inside-work-tree)")
+'
 
-test_rev_parse toplevel false false true '' .git "$ROOT/.git"
+test_expect_success '.git/: prefix' '
+	(
+		cd .git &&
+		echo >expected &&
+		git rev-parse --show-prefix >actual &&
+		test_cmp expected actual
+	)
+'
 
-cd .git || exit 1
-test_rev_parse .git/ false true false '' . "$ROOT/.git"
-cd objects || exit 1
-test_rev_parse .git/objects/ false true false '' "$ROOT/.git" "$ROOT/.git"
-cd ../.. || exit 1
+test_expect_success '.git/: git-dir' '
+	(
+		cd .git &&
+		echo . >expected &&
+		git rev-parse --git-dir >actual &&
+		test_cmp expected actual
+	)
+'
 
-mkdir -p sub/dir || exit 1
-cd sub/dir || exit 1
-test_rev_parse subdirectory false false true sub/dir/ "$ROOT/.git" "$ROOT/.git"
-cd ../.. || exit 1
+test_expect_success '.git/: absolute-git-dir' '
+	(
+		ROOT=$(pwd) &&
+		cd .git &&
+		echo "$ROOT/.git" >expected &&
+		git rev-parse --absolute-git-dir >actual &&
+		test_cmp expected actual
+	)
+'
 
-git config core.bare true
-test_rev_parse 'core.bare = true' true false false
+test_expect_success '.git/objects/: is-bare-repository' '
+	(cd .git/objects && test false = "$(git rev-parse --is-bare-repository)")
+'
 
-git config --unset core.bare
-test_rev_parse 'core.bare undefined' false false true
+test_expect_success '.git/objects/: is-inside-git-dir' '
+	(cd .git/objects && test true = "$(git rev-parse --is-inside-git-dir)")
+'
 
-mkdir work || exit 1
-cd work || exit 1
-GIT_DIR=../.git
-GIT_CONFIG="$(pwd)"/../.git/config
-export GIT_DIR GIT_CONFIG
+test_expect_success '.git/objects/: is-inside-work-tree' '
+	(cd .git/objects && test false = "$(git rev-parse --is-inside-work-tree)")
+'
 
-git config core.bare false
-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true '' "../.git" "$ROOT/.git"
+test_expect_success '.git/objects/: prefix' '
+	(
+		cd .git/objects &&
+		echo >expected &&
+		git rev-parse --show-prefix >actual &&
+		test_cmp expected actual
+	)
+'
 
-git config core.bare true
-test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''
+test_expect_success '.git/objects/: git-dir' '
+	(
+		ROOT=$(pwd) &&
+		cd .git/objects &&
+		echo $ROOT/.git >expected &&
+		git rev-parse --git-dir >actual &&
+		test_cmp expected actual
+	)
+'
 
-git config --unset core.bare
-test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
+test_expect_success '.git/objects/: absolute-git-dir' '
+	(
+		ROOT=$(pwd) &&
+		cd .git/objects &&
+		echo "$ROOT/.git" >expected &&
+		git rev-parse --absolute-git-dir >actual &&
+		test_cmp expected actual
+	)
+'
 
-mv ../.git ../repo.git || exit 1
-GIT_DIR=../repo.git
-GIT_CONFIG="$(pwd)"/../repo.git/config
+test_expect_success 'subdirectory: is-bare-repository' '
+	mkdir -p sub/dir &&
+	test_when_finished "rm -rf sub" &&
+	(cd sub/dir && test false = "$(git rev-parse --is-bare-repository)")
+'
 
-git config core.bare false
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true '' "../repo.git" "$ROOT/repo.git"
+test_expect_success 'subdirectory: is-inside-git-dir' '
+	mkdir -p sub/dir &&
+	test_when_finished "rm -rf sub" &&
+	(cd sub/dir && test false = "$(git rev-parse --is-inside-git-dir)")
+'
 
-git config core.bare true
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''
+test_expect_success 'subdirectory: is-inside-work-tree' '
+	mkdir -p sub/dir &&
+	test_when_finished "rm -rf sub" &&
+	(cd sub/dir && test true = "$(git rev-parse --is-inside-work-tree)")
+'
 
-git config --unset core.bare
-test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_expect_success 'subdirectory: prefix' '
+	mkdir -p sub/dir &&
+	test_when_finished "rm -rf sub" &&
+	(cd sub/dir && test sub/dir/ = "$(git rev-parse --show-prefix)")
+'
+
+test_expect_success 'subdirectory: git-dir' '
+	mkdir -p sub/dir &&
+	test_when_finished "rm -rf sub" &&
+	(
+		ROOT=$(pwd) &&
+		cd sub/dir &&
+		echo $ROOT/.git >expected &&
+		git rev-parse --git-dir >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'subdirectory: absolute-git-dir' '
+	mkdir -p sub/dir &&
+	test_when_finished "rm -rf sub" &&
+	(
+		ROOT=$(pwd) &&
+		cd sub/dir &&
+		echo $ROOT/.git >expected &&
+		git rev-parse --absolute-git-dir >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'core.bare = true: is-bare-repository' '
+	git config core.bare true &&
+	test_when_finished "git config core.bare false" &&
+	test true = "$(git rev-parse --is-bare-repository)"
+'
+
+test_expect_success 'core.bare = true: is-inside-git-dir' '
+	git config core.bare true &&
+	test_when_finished "git config core.bare false" &&
+	test false = "$(git rev-parse --is-inside-git-dir)"
+'
+
+test_expect_success 'core.bare = true: is-inside-work-tree' '
+	git config core.bare true &&
+	test_when_finished "git config core.bare false" &&
+	test false = "$(git rev-parse --is-inside-work-tree)"
+'
+
+test_expect_success 'core.bare undefined: is-bare-repository' '
+	git config --unset core.bare &&
+	test_when_finished "git config core.bare false" &&
+	test false = "$(git rev-parse --is-bare-repository)"
+'
+
+test_expect_success 'core.bare undefined: is-inside-git-dir' '
+	git config --unset core.bare &&
+	test_when_finished "git config core.bare false" &&
+	test false = "$(git rev-parse --is-inside-git-dir)"
+'
+
+test_expect_success 'core.bare undefined: is-inside-work-tree' '
+	git config --unset core.bare &&
+	test_when_finished "git config core.bare false" &&
+	test true = "$(git rev-parse --is-inside-work-tree)"
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = false: is-bare-repository' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config
+		git config core.bare false &&
+		test false = "$(git rev-parse --is-bare-repository)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = false: is-inside-git-dir' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config core.bare false &&
+		test false = "$(git rev-parse --is-inside-git-dir)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = false: is-inside-work-tree' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config core.bare false &&
+		test true = "$(git rev-parse --is-inside-work-tree)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = false: prefix' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config core.bare false &&
+		echo >expected &&
+		git rev-parse --show-prefix >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = false: git-dir' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config core.bare false &&
+		echo ../.git >expected &&
+		git rev-parse --git-dir >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = false: absolute-git-dir' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		ROOT=$(pwd) &&
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config core.bare false &&
+		echo $ROOT/.git >expected &&
+		git rev-parse --absolute-git-dir >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = true: is-bare-repository' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config core.bare true &&
+		test true = "$(git rev-parse --is-bare-repository)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = true: is-inside-git-dir' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config core.bare true &&
+		test false = "$(git rev-parse --is-inside-git-dir)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = true: is-inside-work-tree' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config core.bare true &&
+		test false = "$(git rev-parse --is-inside-work-tree)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare = true: prefix' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config core.bare true &&
+		echo >expected &&
+		git rev-parse --show-prefix >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare undefined: is-bare-repository' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config --unset core.bare &&
+		test false = "$(git rev-parse --is-bare-repository)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare undefined: is-inside-git-dir' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config --unset core.bare &&
+		test false = "$(git rev-parse --is-inside-git-dir)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare undefined: is-inside-work-tree' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config --unset core.bare &&
+		test true = "$(git rev-parse --is-inside-work-tree)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../.git, core.bare undefined: prefix' '
+	mkdir work &&
+	test_when_finished "rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../.git &&
+		export GIT_CONFIG="$(pwd)"/../.git/config &&
+		git config --unset core.bare &&
+		echo >expected &&
+		git rev-parse --show-prefix >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config
+		git config core.bare false &&
+		test false = "$(git rev-parse --is-bare-repository)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-inside-git-dir' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config core.bare false &&
+		test false = "$(git rev-parse --is-inside-git-dir)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-inside-work-tree' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config core.bare false &&
+		test true = "$(git rev-parse --is-inside-work-tree)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: prefix' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config core.bare false &&
+		echo >expected &&
+		git rev-parse --show-prefix >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: git-dir' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config core.bare false &&
+		echo ../repo.git >expected &&
+		git rev-parse --git-dir >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = false: absolute-git-dir' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		ROOT=$(pwd) &&
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config core.bare false &&
+		echo $ROOT/repo.git >expected &&
+		git rev-parse --absolute-git-dir >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-bare-repository' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config core.bare true &&
+		test true = "$(git rev-parse --is-bare-repository)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-inside-git-dir' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config core.bare true &&
+		test false = "$(git rev-parse --is-inside-git-dir)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-inside-work-tree' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config core.bare true &&
+		test false = "$(git rev-parse --is-inside-work-tree)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare = true: prefix' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config core.bare true &&
+		echo >expected &&
+		git rev-parse --show-prefix >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-bare-repository' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config --unset core.bare &&
+		test false = "$(git rev-parse --is-bare-repository)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-inside-git-dir' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config --unset core.bare &&
+		test false = "$(git rev-parse --is-inside-git-dir)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-inside-work-tree' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config --unset core.bare &&
+		test true = "$(git rev-parse --is-inside-work-tree)"
+	)
+'
+
+test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: prefix' '
+	mkdir work &&
+	cp -r .git repo.git &&
+	test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
+	(
+		cd work &&
+		export GIT_DIR=../repo.git &&
+		export GIT_CONFIG="$(pwd)"/../repo.git/config &&
+		git config --unset core.bare &&
+		echo >expected &&
+		git rev-parse --show-prefix >actual &&
+		test_cmp expected actual
+	)
+'
 
 test_done
-- 
2.8.0

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] t1500-rev-parse: rewrite each test to run in isolation
  2016-04-09 11:19 ` [PATCH] t1500-rev-parse: rewrite each test to run in isolation Michael Rappazzo
@ 2016-04-13  2:03   ` Junio C Hamano
  2016-04-16 10:23     ` SZEDER Gábor
  2016-04-13  4:54   ` Eric Sunshine
  1 sibling, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2016-04-13  2:03 UTC (permalink / raw)
  To: Michael Rappazzo; +Cc: git, sunshine, pclouds, szeder

Michael Rappazzo <rappazzo@gmail.com> writes:

> t1500-rev-parse has many tests which change directories and leak
> environment variables.  This makes it difficult to add new tests without
> minding the environment variables and current directory.
>
> Each test is now setup, executed, and cleaned up without leaving anything
> behind.  Tests which have textual expectations have been converted to use
> test_cmp (which will show a diff when the test is run with --verbose).
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---

Applying this patch on top of sg/completion-updates topic makes the
tests much more readable.  Given that sg/completion-updates topic is
planned to be rerolled ($gmane/287839), I think it would be better
to do this as a preparatory clean-up patch before it makes the tests
uglier by doing "add --absolute-git-dir" patch in the middle.

Gábor, what do you think?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] t1500-rev-parse: rewrite each test to run in isolation
  2016-04-13  2:03   ` Junio C Hamano
@ 2016-04-16 10:23     ` SZEDER Gábor
  0 siblings, 0 replies; 7+ messages in thread
From: SZEDER Gábor @ 2016-04-16 10:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael Rappazzo, git, sunshine, pclouds

Quoting Junio C Hamano <gitster@pobox.com>:

> Applying this patch on top of sg/completion-updates topic makes the
> tests much more readable.  Given that sg/completion-updates topic is
> planned to be rerolled ($gmane/287839), I think it would be better
> to do this as a preparatory clean-up patch before it makes the tests
> uglier by doing "add --absolute-git-dir" patch in the middle.
>
> Gábor, what do you think?

Sure, go ahead.  A standalone test cleanup patch could graduate to master
faster than a 20+ patch series, so it shouldn't be held hostage.

The reroll of sg/completion-updates is almost ready.  "Almost", because the
refs completion speedup series (another 10+ patches...) is almost ready as
well, but needs some final polishing and I want to trickle down some of the
test updates into sg/completion-updates.  Unfortunately, I don't expect to
have time to finish it up in the next few days...  but fortunately that'll
leave you time to agree on how to rewrite t1500 and Michael to do the
reroll ;)

Anyway, if Michael rerolls based on master and you have troubles you don't
want to deal with with the conflicts between the reroll and my series, I
would say just discard sg/completion-updates for the time being.

Gábor

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] t1500-rev-parse: rewrite each test to run in isolation
  2016-04-09 11:19 ` [PATCH] t1500-rev-parse: rewrite each test to run in isolation Michael Rappazzo
  2016-04-13  2:03   ` Junio C Hamano
@ 2016-04-13  4:54   ` Eric Sunshine
  2016-04-13 12:21     ` Mike Rappazzo
  2016-04-13 18:29     ` Jeff King
  1 sibling, 2 replies; 7+ messages in thread
From: Eric Sunshine @ 2016-04-13  4:54 UTC (permalink / raw)
  To: Michael Rappazzo
  Cc: Git List, Junio C Hamano, Nguyễn Thái Ngọc Duy,
	SZEDER Gábor

On Sat, Apr 9, 2016 at 7:19 AM, Michael Rappazzo <rappazzo@gmail.com> wrote:
> t1500-rev-parse has many tests which change directories and leak
> environment variables.  This makes it difficult to add new tests without
> minding the environment variables and current directory.
>
> Each test is now setup, executed, and cleaned up without leaving anything
> behind.  Tests which have textual expectations have been converted to use
> test_cmp (which will show a diff when the test is run with --verbose).

It might be easier to review this if broken into several cleanup and
modernization patches, however, some comments below...

> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> @@ -3,88 +3,571 @@
> +test_expect_success '.git/: is-bare-repository' '
> +       (cd .git && test false = "$(git rev-parse --is-bare-repository)")
> +'
>
> -# label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
> +test_expect_success '.git/: is-inside-git-dir' '
> +       (cd .git && test true = "$(git rev-parse --is-inside-git-dir)")

Simpler:

    test true = "$(git -C .git rev-parse --is-inside-git-dir)"

> +'
>
> -ROOT=$(pwd)
> +test_expect_success '.git/: is-inside-work-tree' '
> +       (cd .git && test false = "$(git rev-parse --is-inside-work-tree)")

Ditto.

> +'
>
> -test_rev_parse toplevel false false true '' .git "$ROOT/.git"
> +test_expect_success '.git/: prefix' '
> +       (
> +               cd .git &&
> +               echo >expected &&
> +               git rev-parse --show-prefix >actual &&
> +               test_cmp expected actual
> +       )

Likewise, you could drop the entire subshell:

    echo >expected &&
    git -C .git rev-parse --show-prefix >actual &&
    test_cmp expected actual

> +'
>
> +test_expect_success '.git/: git-dir' '
> +       (
> +               cd .git &&
> +               echo . >expected &&
> +               git rev-parse --git-dir >actual &&
> +               test_cmp expected actual
> +       )

Same here and for many subsequent tests (which I won't quote).

> +'
> +test_expect_success 'core.bare = true: is-bare-repository' '
> +       git config core.bare true &&
> +       test_when_finished "git config core.bare false" &&
> +       test true = "$(git rev-parse --is-bare-repository)"

Simpler:

    test_config core.bare true

and then you can drop 'test_when_finished' altogether. However, even simpler:

    test true = "$(git -c core.bare=false rev-parse --is-bare-repository)"

which allows you to drop 'test_config', as well.

Ditto for subsequent tests (which I won't quote).

> +'
> +test_expect_success 'core.bare undefined: is-bare-repository' '
> +       git config --unset core.bare &&

    test_unconfig core.bare

> +       test_when_finished "git config core.bare false" &&

Why does this need to re-instate core.bare?

Same comments for subsequent tests.

> +       test false = "$(git rev-parse --is-bare-repository)"
> +'
> +test_expect_success 'GIT_DIR=../.git, core.bare = false: is-bare-repository' '
> +       mkdir work &&
> +       test_when_finished "rm -rf work && git config core.bare false" &&
> +       (
> +               cd work &&
> +               export GIT_DIR=../.git &&
> +               export GIT_CONFIG="$(pwd)"/../.git/config
> +               git config core.bare false &&
> +               test false = "$(git rev-parse --is-bare-repository)"
> +       )
> +'

Same comments about -C to avoid the subshell and -c for configuration.

Also, you can do one-shot environment variable setting for the command
invocation, so the subshell goes away, and everything inside the
subshell collapses to:

    test false = "$(GIT_DIR=... GIT_CONFIG=...
        git -C work -c core.bare=false rev-parse ...)"

Additionally, I'm confused about why this test "reverts" the
core.bare=false by setting core.bare=false in 'test_when_finished'.

Ditto for subsequent tests.

> +test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' '
> +       mkdir work &&
> +       cp -r .git repo.git &&
> +       test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&

If 'cp' fails, then 'test_when_finished' will never be invoked, which
means that the cleanup will never happen; thus 'test_when_finished'
needs to be called earlier. Ditto for subsequent tests.

> +       (
> +               cd work &&
> +               export GIT_DIR=../repo.git &&
> +               export GIT_CONFIG="$(pwd)"/../repo.git/config
> +               git config core.bare false &&
> +               test false = "$(git rev-parse --is-bare-repository)"
> +       )
> +'

Closing comments:

By using -C, -c, and one-shot environment variables, you can ditch the
subshells, and most of these tests should collapse to one or two
lines.

There seems to be a lot of repetition here. To reduce the repetition,
have you considered encoding the state which varies between tests into
a table and making the tests table-driven. Or, by encoding the varying
state in some nested for-loops? The nice thing about driving the tests
from a table or for-loops is that it is easier to see at a glance if
all cases are covered.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] t1500-rev-parse: rewrite each test to run in isolation
  2016-04-13  4:54   ` Eric Sunshine
@ 2016-04-13 12:21     ` Mike Rappazzo
  2016-04-13 18:29     ` Jeff King
  1 sibling, 0 replies; 7+ messages in thread
From: Mike Rappazzo @ 2016-04-13 12:21 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Git List, Junio C Hamano, Nguyễn Thái Ngọc Duy,
	SZEDER Gábor

On Wed, Apr 13, 2016 at 12:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Apr 9, 2016 at 7:19 AM, Michael Rappazzo <rappazzo@gmail.com> wrote:
>> t1500-rev-parse has many tests which change directories and leak
>> environment variables.  This makes it difficult to add new tests without
>> minding the environment variables and current directory.
>>
>> Each test is now setup, executed, and cleaned up without leaving anything
>> behind.  Tests which have textual expectations have been converted to use
>> test_cmp (which will show a diff when the test is run with --verbose).
>
> It might be easier to review this if broken into several cleanup and
> modernization patches, however, some comments below...

I felt that the changes are repetitive enough that it did not
necessitate separate patches.  Perhaps after simplifying based on your
suggestions, it will be even smaller.

>
>> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
>> ---
>> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
>> @@ -3,88 +3,571 @@
>> +test_expect_success '.git/: is-bare-repository' '
>> +       (cd .git && test false = "$(git rev-parse --is-bare-repository)")
>> +'
>>
>> -# label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
>> +test_expect_success '.git/: is-inside-git-dir' '
>> +       (cd .git && test true = "$(git rev-parse --is-inside-git-dir)")
>
> Simpler:
>
>     test true = "$(git -C .git rev-parse --is-inside-git-dir)"
>
>> +'
>>
>> -ROOT=$(pwd)
>> +test_expect_success '.git/: is-inside-work-tree' '
>> +       (cd .git && test false = "$(git rev-parse --is-inside-work-tree)")
>
> Ditto.
>
>> +'
>>
>> -test_rev_parse toplevel false false true '' .git "$ROOT/.git"
>> +test_expect_success '.git/: prefix' '
>> +       (
>> +               cd .git &&
>> +               echo >expected &&
>> +               git rev-parse --show-prefix >actual &&
>> +               test_cmp expected actual
>> +       )
>
> Likewise, you could drop the entire subshell:
>
>     echo >expected &&
>     git -C .git rev-parse --show-prefix >actual &&
>     test_cmp expected actual
>
>> +'
>>
>> +test_expect_success '.git/: git-dir' '
>> +       (
>> +               cd .git &&
>> +               echo . >expected &&
>> +               git rev-parse --git-dir >actual &&
>> +               test_cmp expected actual
>> +       )
>
> Same here and for many subsequent tests (which I won't quote).
>
>> +'
>> +test_expect_success 'core.bare = true: is-bare-repository' '
>> +       git config core.bare true &&
>> +       test_when_finished "git config core.bare false" &&
>> +       test true = "$(git rev-parse --is-bare-repository)"
>
> Simpler:
>
>     test_config core.bare true
>
> and then you can drop 'test_when_finished' altogether. However, even simpler:
>
>     test true = "$(git -c core.bare=false rev-parse --is-bare-repository)"
>
> which allows you to drop 'test_config', as well.
>
> Ditto for subsequent tests (which I won't quote).
>
>> +'
>> +test_expect_success 'core.bare undefined: is-bare-repository' '
>> +       git config --unset core.bare &&
>
>     test_unconfig core.bare
>
>> +       test_when_finished "git config core.bare false" &&
>
> Why does this need to re-instate core.bare?
>
> Same comments for subsequent tests.
>
>> +       test false = "$(git rev-parse --is-bare-repository)"
>> +'
>> +test_expect_success 'GIT_DIR=../.git, core.bare = false: is-bare-repository' '
>> +       mkdir work &&
>> +       test_when_finished "rm -rf work && git config core.bare false" &&
>> +       (
>> +               cd work &&
>> +               export GIT_DIR=../.git &&
>> +               export GIT_CONFIG="$(pwd)"/../.git/config
>> +               git config core.bare false &&
>> +               test false = "$(git rev-parse --is-bare-repository)"
>> +       )
>> +'
>
> Same comments about -C to avoid the subshell and -c for configuration.
>
> Also, you can do one-shot environment variable setting for the command
> invocation, so the subshell goes away, and everything inside the
> subshell collapses to:
>
>     test false = "$(GIT_DIR=... GIT_CONFIG=...
>         git -C work -c core.bare=false rev-parse ...)"
>
> Additionally, I'm confused about why this test "reverts" the
> core.bare=false by setting core.bare=false in 'test_when_finished'.
>
> Ditto for subsequent tests.
>
>> +test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' '
>> +       mkdir work &&
>> +       cp -r .git repo.git &&
>> +       test_when_finished "rm -r repo.git && rm -rf work && git config core.bare false" &&
>
> If 'cp' fails, then 'test_when_finished' will never be invoked, which
> means that the cleanup will never happen; thus 'test_when_finished'
> needs to be called earlier. Ditto for subsequent tests.
>
>> +       (
>> +               cd work &&
>> +               export GIT_DIR=../repo.git &&
>> +               export GIT_CONFIG="$(pwd)"/../repo.git/config
>> +               git config core.bare false &&
>> +               test false = "$(git rev-parse --is-bare-repository)"
>> +       )
>> +'
>
> Closing comments:
>
> By using -C, -c, and one-shot environment variables, you can ditch the
> subshells, and most of these tests should collapse to one or two
> lines.

I follow, and I can make that adjustment.  Thanks for the review.

>
> There seems to be a lot of repetition here. To reduce the repetition,
> have you considered encoding the state which varies between tests into
> a table and making the tests table-driven. Or, by encoding the varying
> state in some nested for-loops? The nice thing about driving the tests
> from a table or for-loops is that it is easier to see at a glance if
> all cases are covered.

I was aiming for readability and the ability to test an individual
case.  The previous version was driven by a function, which did reduce
the repetition, but it made it not simple to run an individual test in
the case of a failure.  Would your table implementation be much
different from the function, in terms of readability?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] t1500-rev-parse: rewrite each test to run in isolation
  2016-04-13  4:54   ` Eric Sunshine
  2016-04-13 12:21     ` Mike Rappazzo
@ 2016-04-13 18:29     ` Jeff King
  1 sibling, 0 replies; 7+ messages in thread
From: Jeff King @ 2016-04-13 18:29 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Michael Rappazzo, Git List, Junio C Hamano,
	Nguyễn Thái Ngọc Duy, SZEDER Gábor

On Wed, Apr 13, 2016 at 12:54:16AM -0400, Eric Sunshine wrote:

> > -# label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
> > +test_expect_success '.git/: is-inside-git-dir' '
> > +       (cd .git && test true = "$(git rev-parse --is-inside-git-dir)")
> 
> Simpler:
> 
>     test true = "$(git -C .git rev-parse --is-inside-git-dir)"

While we are modernizing, I wonder if it is a good idea to drop the use
of "test" for string comparisons here. We usually save output to a file
and use test_cmp, for two reasons:

  1. It tests the exit code of the command substitution.

  2. It's easier to debug when it goes wrong (the test produces useful
     output, and the state is left in the filesystem).

It is more verbose to do so, but we could easily wrap that in:

  test_stdout "true" git -C .git rev-parse --is-inside-git-dir

or something. The other problem is that it uses an extra process to do
the test_cmp, which might make the tests slower (especially on Windows).
We could do something like:

  test_stdout () {
          expect=$1; shift
	  if ! actual=$("$@")
	  then
		echo >&2 "test_stdout: command failed: $*"
		return 1
	  fi
	  if test "$expect" != "$actual"
	  then
	          echo >&2 "test_stdout: unexpected output for $*"
		  echo >&2 "test_stdout: $expect != $actual"
		  return 1
	  fi
	  return 0
  }

which is more thorough without incurring extra processes (we lose the
nice diff output of test_cmp, but since this interface is only useful
for short outputs anyway, I don't think that's a big deal.

-Peff

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-04-16 10:23 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-09 11:19 [PATCH] rewrite t1500-rev-parse.sh Michael Rappazzo
2016-04-09 11:19 ` [PATCH] t1500-rev-parse: rewrite each test to run in isolation Michael Rappazzo
2016-04-13  2:03   ` Junio C Hamano
2016-04-16 10:23     ` SZEDER Gábor
2016-04-13  4:54   ` Eric Sunshine
2016-04-13 12:21     ` Mike Rappazzo
2016-04-13 18:29     ` Jeff King

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).