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

Re: [PATCH v2 2/2] t1500-rev-parse: rewrite each test to run in isolation

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Apr 17, 2016, 05:59 UTC
Message-ID
<20160417055955.GA13384@flurp.local>
In-Reply-To
<1460823230-45692-3-git-send-email-rappazzo@gmail.com>
On Sat, Apr 16, 2016 at 12:13:50PM -0400, Michael Rappazzo wrote:
Show 6 quoted lines
> 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.  Test comparisons been converted to use test_cmp or test_stdout.

Overall, I'm not enthused about how this patch unrolls the systematic function-driven approach taken by the original code and turns it into a series of highly repetitive individual tests. Not only does it make the patch mind-numbing to review, but it is far too easy for errors to creep into the conversion which simply wouldn't exist if a systematic approach was used (whether via function, table, or for-loops).

The very fact that you missed several test_stdout conversion opportunities and didn't notice a bit of gunk (an unnecessary ">actual") or several bogus and misspelled "test_config care.bar =" invocations, makes a good argument that this patch's approach is undesirable.

The fact that I also missed these problems when reading through the patch hammers the point home. It wasn't until I started actually changing the patch in order to present you with a "here's a diff atop your patch which fixes the issues" as a convenience, that I noticed the more serious problems.

Show 9 quoted lines
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> @@ -3,85 +3,320 @@
> +test_expect_success '.git/objects/: git-dir' '
> +	echo $(pwd)/.git >expect &&
> +	git -C .git/objects rev-parse --git-dir >actual &&
> +	test_cmp expect actual
> +'
You forgot to convert this test_cmp to test_stdout.
Show 5 quoted lines
> +test_expect_success 'subdirectory: prefix' '
> +	mkdir -p sub/dir &&
> +	test_when_finished "rm -rf sub" &&
> +	test sub/dir/ = "$(git -C sub/dir rev-parse --show-prefix)"
> +'
You forgot to convert this 'test' to test_stdout.
Show 6 quoted lines
> -git config core.bare true
> -test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''
> +test_expect_success 'subdirectory: git-dir' '
> +	mkdir -p sub/dir &&
> +	test_when_finished "rm -rf sub" &&
> +	echo $(pwd)/.git >expect &&
Nit: Here and one other place, you could quote this: "$(pwd)/.git"
> +	git -C sub/dir rev-parse --git-dir >actual &&
> +	test_cmp expect actual
> +'
Ditto: test_cmp => test_stdout
> +test_expect_success 'core.bare = true: is-bare-repository' '
> +	test_config core.bare true &&
> +	test_stdout true git rev-parse --is-bare-repository
> +'

Is there a reason you chose to use test_config rather than the more concise '-c foo=bar' as suggested by the review[1]?

[1]: http://article.gmane.org/gmane.comp.version-control.git/291368
> +test_expect_success 'core.bare undefined: is-bare-repository' '
> +	test_config core.bare "" &&

Is setting core.bare to "" really the same as undefining it, or is the effect the same only as an accident of implementation? (Genuine question; I haven't checked.)

Show 6 quoted lines
> +	test_stdout 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" &&
> +	test_config -C "$(pwd)"/.git core.bare false &&
Drop the unnecessary "$(pwd)"/ here and elsewhere.
Show 8 quoted lines
> +	GIT_DIR=../.git test_stdout false git -C work rev-parse --is-bare-repository
> +'
> +
> +test_expect_success 'GIT_DIR=../.git, core.bare = false: prefix' '
> +	mkdir work &&
> +	test_when_finished "rm -rf work" &&
> +	test_config -C "$(pwd)"/.git core.bare false &&
> +	GIT_DIR=../.git test_stdout "" git -C work rev-parse --show-prefix >actual
Drop the unnecessary '>actual' redirection.
Show 6 quoted lines
> +'
> +
> +test_expect_success 'GIT_DIR=../.git, core.bare undefined: is-bare-repository' '
> +	mkdir work &&
> +	test_when_finished "rm -rf work" &&
> +	test_config -C "$(pwd)"/.git core.bar = &&
What is "core.bar =" (here and elsewhere)?
Show 8 quoted lines
> +	GIT_DIR=../.git test_stdout false git -C work rev-parse --is-bare-repository
> +'
> +
> +test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' '
> +	mkdir work &&
> +	test_when_finished "rm -rf work" &&
> +	cp -r .git repo.git &&
> +	test_when_finished "rm -r repo.git" &&
You could coalesce these two test_when_finished invocations:
    test_when_finished "rm -rf work repo.git" &&

Windows folk might appreciate it since process spawning is expensive and slow there.

> +	test_config -C "$(pwd)"/repo.git core.bare false &&
> +	GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-bare-repository
> +'

For convenience, here's a diff atop your patch which addresses the above issues (except the question about core.bare set to "" versus being undefined). However, as noted above, I think this patch's approach is going in the wrong direction.

--- 8< ---
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index e2c2a06..beaf6e3 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -60,9 +60,7 @@ test_expect_success '.git/objects/: prefix' '
 '
 
 test_expect_success '.git/objects/: git-dir' '
-	echo $(pwd)/.git >expect &&
-	git -C .git/objects rev-parse --git-dir >actual &&
-	test_cmp expect actual
+	test_stdout "$(pwd)/.git" git -C .git/objects rev-parse --git-dir
 '
 
 test_expect_success 'subdirectory: is-bare-repository' '
@@ -86,237 +84,193 @@ test_expect_success 'subdirectory: is-inside-work-tree' '
 test_expect_success 'subdirectory: prefix' '
 	mkdir -p sub/dir &&
 	test_when_finished "rm -rf sub" &&
-	test sub/dir/ = "$(git -C sub/dir rev-parse --show-prefix)"
+	test_stdout sub/dir/ git -C sub/dir rev-parse --show-prefix
 '
 
 test_expect_success 'subdirectory: git-dir' '
 	mkdir -p sub/dir &&
 	test_when_finished "rm -rf sub" &&
-	echo $(pwd)/.git >expect &&
-	git -C sub/dir rev-parse --git-dir >actual &&
-	test_cmp expect actual
+	test_stdout "$(pwd)/.git" git -C sub/dir rev-parse --git-dir
 '
 
 test_expect_success 'core.bare = true: is-bare-repository' '
-	test_config core.bare true &&
-	test_stdout true git rev-parse --is-bare-repository
+	test_stdout true git -c core.bare=true rev-parse --is-bare-repository
 '
 
 test_expect_success 'core.bare = true: is-inside-git-dir' '
-	test_config core.bare true &&
-	test_stdout false git rev-parse --is-inside-git-dir
+	test_stdout false git -c core.bare=true rev-parse --is-inside-git-dir
 '
 
 test_expect_success 'core.bare = true: is-inside-work-tree' '
-	test_config core.bare true &&
-	test_stdout false git rev-parse --is-inside-work-tree
+	test_stdout false git -c core.bare=true rev-parse --is-inside-work-tree
 '
 
 test_expect_success 'core.bare undefined: is-bare-repository' '
-	test_config core.bare "" &&
-	test_stdout false git rev-parse --is-bare-repository
+	test_stdout false git -c core.bare= rev-parse --is-bare-repository
 '
 
 test_expect_success 'core.bare undefined: is-inside-git-dir' '
-	test_config core.bare "" &&
-	test_stdout false git rev-parse --is-inside-git-dir
+	test_stdout false git -c core.bare= rev-parse --is-inside-git-dir
 '
 
 test_expect_success 'core.bare undefined: is-inside-work-tree' '
-	test_config core.bare "" &&
-	test_stdout true git rev-parse --is-inside-work-tree
+	test_stdout true git -c core.bare= 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" &&
-	test_config -C "$(pwd)"/.git core.bare false &&
-	GIT_DIR=../.git test_stdout false git -C work rev-parse --is-bare-repository
+	mkdir work &&
+	GIT_DIR=../.git test_stdout false git -C work -c core.bare=false 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" &&
-	test_config -C "$(pwd)"/.git core.bare false &&
-	GIT_DIR=../.git test_stdout false git -C work rev-parse --is-inside-git-dir
+	mkdir work &&
+	GIT_DIR=../.git test_stdout false git -C work -c core.bare=false 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" &&
-	test_config -C "$(pwd)"/.git core.bare false &&
-	GIT_DIR=../.git test_stdout true git -C work rev-parse --is-inside-work-tree
+	mkdir work &&
+	GIT_DIR=../.git test_stdout true git -C work -c core.bare=false rev-parse --is-inside-work-tree
 '
 
 test_expect_success 'GIT_DIR=../.git, core.bare = false: prefix' '
-	mkdir work &&
 	test_when_finished "rm -rf work" &&
-	test_config -C "$(pwd)"/.git core.bare false &&
-	GIT_DIR=../.git test_stdout "" git -C work rev-parse --show-prefix >actual
+	mkdir work &&
+	GIT_DIR=../.git test_stdout "" git -C work -c core.bare=false rev-parse --show-prefix
 '
 
 test_expect_success 'GIT_DIR=../.git, core.bare = true: is-bare-repository' '
-	mkdir work &&
 	test_when_finished "rm -rf work" &&
-	test_config -C "$(pwd)"/.git core.bare true &&
-	GIT_DIR=../.git test_stdout true git -C work rev-parse --is-bare-repository
+	mkdir work &&
+	GIT_DIR=../.git test_stdout true git -C work -c core.bare=true 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" &&
-	test_config -C "$(pwd)"/.git core.bare true &&
-	GIT_DIR=../.git test_stdout false git -C work rev-parse --is-inside-git-dir
+	mkdir work &&
+	GIT_DIR=../.git test_stdout false git -C work -c core.bare=true 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" &&
-	test_config -C "$(pwd)"/.git core.bare true &&
-	GIT_DIR=../.git test_stdout false git -C work rev-parse --is-inside-work-tree
+	mkdir work &&
+	GIT_DIR=../.git test_stdout false git -C work -c core.bare=true rev-parse --is-inside-work-tree
 '
 
 test_expect_success 'GIT_DIR=../.git, core.bare = true: prefix' '
-	mkdir work &&
 	test_when_finished "rm -rf work" &&
-	test_config -C "$(pwd)"/.git core.bare true &&
-	GIT_DIR=../.git test_stdout "" git -C work rev-parse --show-prefix
+	mkdir work &&
+	GIT_DIR=../.git test_stdout "" git -C work -c core.bare=true rev-parse --show-prefix
 '
 
 test_expect_success 'GIT_DIR=../.git, core.bare undefined: is-bare-repository' '
-	mkdir work &&
 	test_when_finished "rm -rf work" &&
-	test_config -C "$(pwd)"/.git core.bar = &&
-	GIT_DIR=../.git test_stdout false git -C work rev-parse --is-bare-repository
+	mkdir work &&
+	GIT_DIR=../.git test_stdout false git -C work -c core.bare= 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" &&
-	test_config -C "$(pwd)"/.git core.bar = &&
-	GIT_DIR=../.git test_stdout false git -C work rev-parse --is-inside-git-dir
+	mkdir work &&
+	GIT_DIR=../.git test_stdout false git -C work -c core.bare= 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" &&
-	test_config -C "$(pwd)"/.git core.bar = &&
-	GIT_DIR=../.git test_stdout true git -C work rev-parse --is-inside-work-tree
+	mkdir work &&
+	GIT_DIR=../.git test_stdout true git -C work -c core.bare= rev-parse --is-inside-work-tree
 '
 
 test_expect_success 'GIT_DIR=../.git, core.bare undefined: prefix' '
-	mkdir work &&
 	test_when_finished "rm -rf work" &&
-	test_config -C "$(pwd)"/.git core.bar = &&
-	GIT_DIR=../.git test_stdout "" git -C work rev-parse --show-prefix
+	mkdir work &&
+	GIT_DIR=../.git test_stdout "" git -C work -c core.bare= rev-parse --show-prefix
 '
 
 test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-bare-repository' '
+	test_when_finished "rm -rf work repo.git" &&
 	mkdir work &&
-	test_when_finished "rm -rf work" &&
 	cp -r .git repo.git &&
-	test_when_finished "rm -r repo.git" &&
-	test_config -C "$(pwd)"/repo.git core.bare false &&
-	GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-bare-repository
+	GIT_DIR=../repo.git test_stdout false git -C work -c core.bare=false rev-parse --is-bare-repository
 '
 
 test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-inside-git-dir' '
+	test_when_finished "rm -rf work repo.git" &&
 	mkdir work &&
-	test_when_finished "rm -rf work" &&
 	cp -r .git repo.git &&
-	test_when_finished "rm -r repo.git" &&
-	test_config -C "$(pwd)"/repo.git core.bare false &&
-	GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-inside-git-dir
+	GIT_DIR=../repo.git test_stdout false git -C work -c core.bare=false rev-parse --is-inside-git-dir
 '
 
 test_expect_success 'GIT_DIR=../repo.git, core.bare = false: is-inside-work-tree' '
+	test_when_finished "rm -rf work repo.git" &&
 	mkdir work &&
-	test_when_finished "rm -rf work" &&
 	cp -r .git repo.git &&
-	test_when_finished "rm -r repo.git" &&
-	test_config -C "$(pwd)"/repo.git core.bare false &&
-	GIT_DIR=../repo.git test_stdout true git -C work rev-parse --is-inside-work-tree
+	GIT_DIR=../repo.git test_stdout true git -C work -c core.bare=false rev-parse --is-inside-work-tree
 '
 
 test_expect_success 'GIT_DIR=../repo.git, core.bare = false: prefix' '
+	test_when_finished "rm -rf work repo.git" &&
 	mkdir work &&
-	test_when_finished "rm -rf work" &&
 	cp -r .git repo.git &&
-	test_when_finished "rm -r repo.git" &&
-	test_config -C "$(pwd)"/repo.git core.bare false &&
-	GIT_DIR=../repo.git test_stdout "" git -C work rev-parse --show-prefix
+	GIT_DIR=../repo.git test_stdout "" git -C work -c core.bare=false rev-parse --show-prefix
 '
 
 test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-bare-repository' '
+	test_when_finished "rm -rf work repo.git" &&
 	mkdir work &&
-	test_when_finished "rm -rf work" &&
 	cp -r .git repo.git &&
-	test_when_finished "rm -r repo.git" &&
-	test_config -C "$(pwd)"/repo.git core.bare true &&
-	GIT_DIR=../repo.git test_stdout true git -C work rev-parse --is-bare-repository
+	GIT_DIR=../repo.git test_stdout true git -C work -c core.bare=true rev-parse --is-bare-repository
 '
 
 test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-inside-git-dir' '
+	test_when_finished "rm -rf work repo.git" &&
 	mkdir work &&
-	test_when_finished "rm -rf work" &&
 	cp -r .git repo.git &&
-	test_when_finished "rm -r repo.git" &&
-	test_config -C "$(pwd)"/repo.git core.bare true &&
-	GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-inside-git-dir
+	GIT_DIR=../repo.git test_stdout false git -C work -c core.bare=true rev-parse --is-inside-git-dir
 '
 
 test_expect_success 'GIT_DIR=../repo.git, core.bare = true: is-inside-work-tree' '
+	test_when_finished "rm -rf work repo.git" &&
 	mkdir work &&
-	test_when_finished "rm -rf work" &&
 	cp -r .git repo.git &&
-	test_when_finished "rm -r repo.git" &&
-	test_config -C "$(pwd)"/repo.git core.bare true &&
-	GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-inside-work-tree
+	GIT_DIR=../repo.git test_stdout false git -C work -c core.bare=true rev-parse --is-inside-work-tree
 '
 
 test_expect_success 'GIT_DIR=../repo.git, core.bare = true: prefix' '
+	test_when_finished "rm -rf work repo.git" &&
 	mkdir work &&
-	test_when_finished "rm -rf work" &&
 	cp -r .git repo.git &&
-	test_when_finished "rm -r repo.git" &&
-	test_config -C "$(pwd)"/repo.git core.bare true &&
-	GIT_DIR=../repo.git test_stdout "" git -C work rev-parse --show-prefix
+	GIT_DIR=../repo.git test_stdout "" git -C work -c core.bare=true rev-parse --show-prefix
 '
 
 test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-bare-repository' '
+	test_when_finished "rm -rf work repo.git" &&
 	mkdir work &&
-	test_when_finished "rm -rf work" &&
 	cp -r .git repo.git &&
-	test_when_finished "rm -r repo.git" &&
-	test_config -C "$(pwd)"/repo.git core.bare "" &&
-	GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-bare-repository
+	GIT_DIR=../repo.git test_stdout false git -C work -c core.bare= rev-parse --is-bare-repository
 '
 
 test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-inside-git-dir' '
+	test_when_finished "rm -rf work repo.git" &&
 	mkdir work &&
-	test_when_finished "rm -rf work" &&
 	cp -r .git repo.git &&
-	test_when_finished "rm -r repo.git" &&
-	test_config -C "$(pwd)"/repo.git core.bare "" &&
-	GIT_DIR=../repo.git test_stdout false git -C work rev-parse --is-inside-git-dir
+	GIT_DIR=../repo.git test_stdout false git -C work -c core.bare= rev-parse --is-inside-git-dir
 '
 
 test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: is-inside-work-tree' '
+	test_when_finished "rm -rf work repo.git" &&
 	mkdir work &&
-	test_when_finished "rm -rf work" &&
 	cp -r .git repo.git &&
-	test_when_finished "rm -r repo.git" &&
-	test_config -C "$(pwd)"/repo.git core.bare "" &&
-	GIT_DIR=../repo.git test_stdout true git -C work rev-parse --is-inside-work-tree
+	GIT_DIR=../repo.git test_stdout true git -C work -c core.bare= rev-parse --is-inside-work-tree
 '
 
 test_expect_success 'GIT_DIR=../repo.git, core.bare undefined: prefix' '
+	test_when_finished "rm -rf work repo.git" &&
 	mkdir work &&
-	test_when_finished "rm -rf work" &&
 	cp -r .git repo.git &&
-	test_when_finished "rm -r repo.git" &&
-	test_config -C "$(pwd)"/repo.git core.bare "" &&
-	GIT_DIR=../repo.git test_stdout "" git -C work rev-parse --show-prefix
+	GIT_DIR=../repo.git test_stdout "" git -C work -c core.bare= rev-parse --show-prefix
 '
 
 test_done

-- 
2.8.1.217.gcab2cda
Previous: Michael RappazzoNext: Johannes Sixt
Message 10 of 13 in “t1500-rev-parse: re-write t1500”
  1. 0/2 t1500-rev-parse: re-write t1500Michael Rappazzo, Apr 16, 2016
  2. 1/2 test-lib: add a function to compare an expection with stdout from a commandMichael Rappazzo, Apr 16, 2016
  3. Eric SunshineApr 17, 2016
  4. Jeff KingApr 17, 2016
  5. Eric SunshineApr 17, 2016
  6. Jeff KingApr 17, 2016
  7. Johannes SixtApr 17, 2016
  8. Eric SunshineApr 17, 2016
  9. 2/2 t1500-rev-parse: rewrite each test to run in isolationMichael Rappazzo, Apr 16, 2016
  10. Eric SunshineApr 17, 2016
  11. Johannes SixtApr 17, 2016
  12. SZEDER GáborApr 17, 2016
  13. Eric SunshineApr 17, 2016

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.