Re: [PATCH v9 2/2] test-config: add tests for the config_set API

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 15, 2014, 15:57 UTC
Message-ID: <xmqqk37ewr5r.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1405434571-25459-3-git-send-email-tanayabh@gmail.com>

Tanay Abhra <tanayabh@gmail.com> writes:

Show 58 quoted lines

> Expose the `config_set` C API as a set of simple commands in order to
> facilitate testing. Add tests for the `config_set` API as well as for
> `git_config_get_*()` family for the usual config files.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  .gitignore            |   1 +
>  Makefile              |   1 +
>  t/t1308-config-set.sh | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  test-config.c         | 140 +++++++++++++++++++++++++++++++++
>  4 files changed, 354 insertions(+)
>  create mode 100755 t/t1308-config-set.sh
>  create mode 100644 test-config.c
>
> diff --git a/.gitignore b/.gitignore
> index 42294e5..7677533 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -177,6 +177,7 @@
>  /gitweb/static/gitweb.min.*
>  /test-chmtime
>  /test-ctype
> +/test-config
>  /test-date
>  /test-delta
>  /test-dump-cache-tree
> diff --git a/Makefile b/Makefile
> index b92418d..e070eb8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -549,6 +549,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
>  
>  TEST_PROGRAMS_NEED_X += test-chmtime
>  TEST_PROGRAMS_NEED_X += test-ctype
> +TEST_PROGRAMS_NEED_X += test-config
>  TEST_PROGRAMS_NEED_X += test-date
>  TEST_PROGRAMS_NEED_X += test-delta
>  TEST_PROGRAMS_NEED_X += test-dump-cache-tree
> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
> new file mode 100755
> index 0000000..94085eb
> --- /dev/null
> +++ b/t/t1308-config-set.sh
> @@ -0,0 +1,212 @@
> +#!/bin/sh
> +
> +test_description='Test git config-set API in different settings'
> +
> +. ./test-lib.sh
> +
> +# 'check_config get_* section.key value' verifies that the entry for
> +# section.key is 'value'
> +check_config () {
> +	case "$1" in
> +	expect_code)
> +		if [ "$#" -lt 5 ];
> +		then

Spell out "test" and drop unnecessary semicolon, i.e.

	if test $# -lt 5
        then

> +			>expect
> +		else
> +			printf "%s\n" "$5" >expect

The other "expecting success" side of this function allows to expect more than one line of output, but this one only allows you to expect at most one line? Why?

Show 12 quoted lines

> +		fi &&
> +		test_expect_code "$2" test-config "$3" "$4" >actual
> +		;;
> +	*)
> +		op=$1 key=$2 &&
> +		shift &&
> +		shift &&
> +		printf "%s\n" "$@" >expect &&
> +		test-config "$op" "$key" >actual
> +		;;
> +	esac &&
> +	test_cmp expect actual

Perhaps you meant to say something like this instead?

        if test "$1" = expect_code
        then
        	expect_code="$2" && shift && shift
	else
        	expect_code=0
	fi &&
	op=$1 key=$2 && shift && shift
        if test $# != 0
        then
        	printf "%s\n" "$@"
	fi >expect &&
        test_expect_code $expet_code test-config "$op" "$key" >actual &&
	test_cmp expect actual

I dunno.

> +test_expect_success 'setup default config' '
> +	cat >.git/config <<\EOF

So the default .git/config that was prepared by "git init" is discarded and replaced with this? Shouldn't it be

	cat >>.git/config <<\EOF

instead?

Show 9 quoted lines

> +test_expect_success 'find multiple values' '
> +	cat >expect <<-\EOF &&
> +	sam
> +	bat
> +	hask
> +	EOF
> +	test-config get_value_multi "case.baz">actual &&
> +	test_cmp expect actual
> +'

Hmmm, wasn't the whole point of the helper to allow us to make things like the above into a one-liner, perhaps like this?

      check_config get_value_multi case.baz sam bat hask

I suspect the same applies to most if not all uses of test-config in the remainder of this patch.