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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 21, 2015, 17:38 UTC
Message-ID: <xmqq4minbrks.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1442712302-7912-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 29 quoted lines

> 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 <pclouds@gmail.com>
> ---
>  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".

Show 7 quoted lines

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index e8d3c0f..45d7423 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -394,6 +394,26 @@ test_expect_success () {
>  	test_finish_
>  }

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.

Show 10 quoted lines

> +	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
	'