Re: [PATCH v2 1/2] t1300: extract and use test_cmp_config()

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Sep 30, 2018, 12:31 UTC
Message-ID: <20180930123116.GI23446@localhost>
In-Reply-To: <20180929153005.10599-2-pclouds@gmail.com>

On Sat, Sep 29, 2018 at 05:30:04PM +0200, Nguyễn Thái Ngọc Duy wrote:

Show 20 quoted lines

> In many config-related tests it's common to check if a config variable
> has expected value and we want to print the differences when the test
> fails. Doing it the normal way is three lines of shell code. Let's add
> a function do to all this (and a little more).
> 
> This function has uses outside t1300 as well but I'm not going to
> convert them all. And it will be used in the next commit where
> per-worktree config feature is introduced.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  t/t1300-config.sh       | 79 ++++++++++-------------------------------
>  t/test-lib-functions.sh | 24 +++++++++++++
>  2 files changed, 43 insertions(+), 60 deletions(-)
> 
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index cdf1fed5d1..00c2b0f0eb 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -76,15 +76,11 @@ EOF

Show 17 quoted lines

>  test_expect_success 'unset type specifiers may be reset to conflicting ones' '
> -	echo 1048576 >expect &&
> -	git config --type=bool --no-type --type=int core.big >actual &&
> -	test_cmp expect actual
> +	test_cmp_config 1048576 --type=bool --no-type --type=int core.big
>  '
>  
>  test_expect_success '--type rejects unknown specifiers' '
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index d82fac9d79..4cd7fb8fdf 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -747,6 +747,30 @@ test_cmp() {
>  	$GIT_TEST_CMP "$@"
>  }
>  
> +# similar to test_cmp but $2 is a config key instead of actual value

This doesn't describe very well the function's parameters: $1 specifies the expected value (as opposed to the file containing the expected value), and the rest can be all kinds of 'git config' options, not just a second argument with the config key; e.g. see call in the above hunk.

Perheps only a brief description and a usage string like this would sufficiently cover everything?

  # Check that the given config key has the expected value.
  #
  #   test_cmp_config [-C <dir>] <expected-value>
  #                   [<git-config-options>...] <config-key>

Show 5 quoted lines

> +# it can also accept -C to read from a different repo, e.g.
> +#
> +#     test_cmp_config -C xyz foo core.bar
> +#
> +# is sort of equivalent of

I think this comment should outright say that "is a better alternative to", because it provides useful output on failure, and because it doesn't hide 'git config's exit code.

Note that this second point does make a bit of a difference:

  test_cmp_config "" nonexisting.variable

would fail, because

  $ git config nonexisting.variable ; echo $?
  1

and the &&-chain.  However,

  test "" = "$(git config nonexisting.variable)"

would still succeed, because the non-zero exit code is ignored.

I consider this a benefit, as it will protect us from a typo in the name of a set but empty variable, even though we won't get any error message.

> +#
> +#     test "foo" = "$(git -C xyz core.bar)"
> +

Nit: unnecessary empty line.

Show 9 quoted lines

> +test_cmp_config() {
> +	if [ "$1" = "-C" ]
> +	then
> +		shift &&
> +		GD="-C $1" &&
> +		shift
> +	else
> +		GD=
> +	fi &&

I think you could now safely declare GD as local. The test balloon 01d3a526ad (t0000: check whether the shell supports the "local" keyword, 2017-10-26) has been out there for 4 releases / almost a year, and we haven't heard about any issues, and the upcoming hash translation test helpers use 'local' as well.

Show 12 quoted lines

> +	echo "$1" >expected &&
> +	shift &&
> +	git $GD config "$@" >actual &&
> +	test_cmp expected actual
> +}
> +
>  # test_cmp_bin - helper to compare binary files
>  
>  test_cmp_bin() {
> -- 
> 2.19.0.341.g3acb95d729
>