Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory

[Junio C Hamano <gitster@pobox.com>]

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

> What I had in mind was more a long the lines of this change. I only
> did the first several just for illustration (I added an 'exit' to
> mark where I stopped), but I think you get the idea.  The point is
> that by doing things in subprocess inside test_expect_success you
> can avoid disrupting the main test process even when a test fails as
> much as possible, and this illustrates how you would deal with the
> "cd" and "export".  What I didn't handle was the updates to
> .git/config whose effects by earlier tests are relied on later tests
> in this illustration.
>
> As to "table driven" vs "explicitly spelling out scripts, accepting
> some code repetition", I tend to favor the latter slightly, unless
> the table driven approach is done in such a way that each of its
> tests are truly independent from one another, i.e. if a row of the
> table represents one test, the tests would not be disrupted by
> reordering the rows, inserting a new row in the middle, or removing
> an existing row.  From my experience, "table-driven" sets of tests
> whose rows have inter-dependency has been much harder to debug when
> I had to hunt down a bug that manifests itself only in one test in
> the middle.
>
> Hope this helps clarify what I meant.


If you want to go forward in this direction, three are a few
additional things to note.

> +	test_expect_success "$name: is-bare-repository" "
> +	(
> +		$prep &&
> +		test '$1' = \"\$(git rev-parse --is-bare-repository)\"
> +	)"

This is not a new problem, but quoting the test body with dq instead
of sq pair make it ugly to read.  We might want to do something like

	expect=$1
	test_expect_success "$name: is-bare-repository" '
	(
		$prep &&
		test "$outcome" = "$(git rev-parse --is-bare-repository)"
	)'

>  	shift
>  	[ $# -eq 0 ] && return

Also, let's avoid []; i.e.

	test $# = 0 && return

> -# label is-bare is-inside-git is-inside-work prefix git-dir
> +# label prep is-bare is-inside-git is-inside-work prefix git-dir
>  
>  ROOT=$(pwd)
> +mkdir -p sub/dir work

Remember this place in the script, I may refer to it later.

> -git config core.bare true
> -test_rev_parse 'core.bare = true' true false false
> +test_rev_parse subdirectory 'cd sub/dir' \
> +	false false true sub/dir/ "$ROOT/.git"
>  
> -git config --unset core.bare
> -test_rev_parse 'core.bare undefined' false false true
> +test_rev_parse 'core.bare = true' 'git config core.bare true' \
> +	true false false

I didn't use "test_config" and "test_unconfig" here, because you
cannot use them in a subprocess.  If we truly want to make these
tests independent from each other, we'd need to come up with a way
for the tests to revert the configuration state back to where it was
before they started.
>  
> -mkdir work || exit 1
> -cd work || exit 1
> -GIT_DIR=../.git
> -GIT_CONFIG="$(pwd)"/../.git/config
> -export GIT_DIR GIT_CONFIG
>  
> -git config core.bare false
> -test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
> +test_rev_parse 'GIT_DIR=../.git, core.bare = false' '
> +	cd work && 
> +	GIT_DIR=../.git &&
> +	GIT_CONFIG="$(pwd)"/../.git/config &&
> +	export GIT_DIR GIT_CONFIG &&
> +	git config core.bare false' \
> +	false false true ''

The "prep" step for this test is long and I'd assume that the later
tests would need to do something similar themselves.  To avoid
repetition, create a helper shell function to reduce boilerplate
before the series of these tests start (i.e. the place above) and
call it from the prep parts with different arguments, e.g. making
the above test into

test_rev_parse 'GIT_DIR=../.git, core.bare = false' '
	cd work && 
	set_common_env ../.git false' \
	false false true ''

by having something like

set_dir_and_core_bare () {
	GIT_DIR=$1 &&
	GIT_CONFIG="$(pwd)/$GIT_DIR/config" &&
	export GIT_DIR GIT_CONFIG &&
	git config core.bare "$2"
}

> +exit
>  
>  git config core.bare true
>  test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''