Re: [PATCH/RFC] Add test_repo_expect_success for running tests in a new repository

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

This could be convenient when tests are independent from the rest in the
same file. Normally we would do this

test_expect_success '...' '
	git init foo &&
	(
		cd foo &&
		<script>
	)
'

Now we can write a shorter version

test_repo_expect_success '...' '
	<script>
'

The other function, test_subdir_expect_success, expands the script to
"( cd <repo> && <script> )", which can be useful for grouping a series of
tests that operate on the same repository in a subdir, e.g.

test_expect_success 'create repo abc' 'test_create_repo abc'
test_subdir_expect_success abc '...' <script>
test_subdir_expect_success abc '...' <another-script>

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Lately I start to add more and more tests in this style. So this
 looks like a good change to me.

Implementations of these helper functions are not all that
interesting for reviewers (except for finding unacceptable bits,
that is), I would think.

More interesting are how much cleaner the existing code would become.
I know we have tons of them that do "create a new, do this and that
in the repository".

Need a good doc here for a function that takes variable number of
parameters in a funny way.

+test_subdir_expect_success () {
+	local subdir="$1"

This bash-ism will never be applied to my tree.

+	shift
+	case "$#" in
+		2) test_expect_success "$1" "( cd $subdir && $2 )";;

Mental note: Two-arg form is for title and script.

By the way, unquoted subdir makes it too sloppy to live.

+		3) test_expect_success "$1" "$2" "( cd $subdir && $3 )";;

Mental note: Three-arg form is for title, prereq and script.

+		4) test_expect_success "$1" "$2" "$4 && ( cd $subdir && $3 )";;

What the heck is this one?  That is why I said "implementations are
not interesting, the way they help existing ones more readable is".
It is totally unclear where the $4 comes from and has to be given in
a funny order to the helper (and I am sure it will make perfect
sense when it is explained).

+		*) error "bug in the test script: not 3-5 parameters to test-subdir-expect-success";;

Too deep an indent level here.

+	esac
+}
+
+test_repo_expect_success () {
+	local repo=repo-$(($test_count+1))
+	case "$#" in
+		2) test_subdir_expect_success "$repo" '' "$1" "$2" "test_create_repo $repo";;
+		3) test_subdir_expect_success "$repo" "$1" "$2" "$3" "test_create_repo $repo";;
+		*) error "bug in the test script: not 2 or 3 parameters to test-repo-expect-success";;
+	esac

I do not like a few things in here (besides the same kind of
unacceptable stuff as the other one).

Often we need a new repository for testing a handful steps, and we
would want to be able to do this sequence:

    - create a new repo
    - do one thing in that repo
    - do another thing in that repo
    - do yet another thing in that repo

That would force tests to say "test_repo_expect_success" to do the
first thing (creating and running the first command) and then
subsequently do "test_subdir_expect_success $there" to run the
remainder.  

I think we would only want to have test_expect_success_there (I am
avoiding "subdir" because the word has connotations that you do not
want in your use case.  When we say "subdir" we typically do not
mean a separate repository or submodule) without the "auto creation
of a repository with unknown name" bit.

	test_expect_success 'set up a new repository for testing' '
		test_create_repo myrepo
	'

        test_expect_success_there mytest 'do one thing there' '
		>empty &&
                git add empty
                git commit -m "add empty"
	'

        test_expect_success_there mytest 'do another thing there' '
		git ls-files >actual &&
                echo empty >expect &&
                test_cmp expect actual
	'