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

Commit Message

Duy Nguyen Sept. 29, 2018, 3:30 p.m. UTC

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 <[e-mail]>
---
 t/t1300-config.sh       | 79 ++++++++++-------------------------------
 t/test-lib-functions.sh | 24 +++++++++++++
 2 files changed, 43 insertions(+), 60 deletions(-)

Comments

Eric Sunshine Sept. 30, 2018, 4:05 a.m. UTC | #1

On Sat, Sep 29, 2018 at 11:30 AM Nguyễn Thái Ngọc Duy <[e-mail]> wrote:
> 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).
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy <[e-mail]>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -747,6 +747,30 @@ test_cmp() {
> +# similar to test_cmp but $2 is a config key instead of actual value
> +# it can also accept -C to read from a different repo, e.g.

Minor: maybe say that "-C <dir>" changes to <dir> for the git-config invocation.

> +#     test_cmp_config -C xyz foo core.bar
> +#
> +# is sort of equivalent of
> +#
> +#     test "foo" = "$(git -C xyz core.bar)"

Should be: $(git -C xyz config core.bar)

> +test_cmp_config() {
> +       if [ "$1" = "-C" ]
> +       then

Style:

    if test "$1" = "-C"
    then
        ...

> +               shift &&
> +               GD="-C $1" &&
> +               shift
> +       else
> +               GD=
> +       fi &&
> +       echo "$1" >expected &&

If $1 starts with a hyphen, 'echo' might try interpreting it as an
option. Use printf instead:

    printf "%s\n" "$1" &&

> +       shift &&
> +       git $GD config "$@" >actual &&
> +       test_cmp expected actual

Please choose names other than "actual" and "expected" since those are
likely to clobber files of the same name which the test has set up
itself. (Or, at the very least, document that this function clobbers
those files -- but using better filenames is preferable.)

> +}

SZEDER Gábor Sept. 30, 2018, 12:31 p.m. UTC | #2

On Sat, Sep 29, 2018 at 05:30:04PM +0200, Nguyễn Thái Ngọc Duy wrote:
> 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 <[e-mail]>
> ---
>  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

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

> +# 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.

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

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

Patch

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
 test_expect_success 'non-match result' 'test_cmp expect .git/config'
 
 test_expect_success 'find mixed-case key by canonical name' '
-	echo Second >expect &&
-	git config cores.whatever >actual &&
-	test_cmp expect actual
+	test_cmp_config Second cores.whatever
 '
 
 test_expect_success 'find mixed-case key by non-canonical name' '
-	echo Second >expect &&
-	git config CoReS.WhAtEvEr >actual &&
-	test_cmp expect actual
+	test_cmp_config Second CoReS.WhAtEvEr
 '
 
 test_expect_success 'subsections are not canonicalized by git-config' '
@@ -94,12 +90,8 @@  test_expect_success 'subsections are not canonicalized by git-config' '
 	[section "SubSection"]
 	key = two
 	EOF
-	echo one >expect &&
-	git config section.subsection.key >actual &&
-	test_cmp expect actual &&
-	echo two >expect &&
-	git config section.SubSection.key >actual &&
-	test_cmp expect actual
+	test_cmp_config one section.subsection.key &&
+	test_cmp_config two section.SubSection.key
 '
 
 cat > .git/config <<\EOF
@@ -212,9 +204,7 @@  test_expect_success 'really really mean test' '
 '
 
 test_expect_success 'get value' '
-	echo alpha >expect &&
-	git config beta.haha >actual &&
-	test_cmp expect actual
+	test_cmp_config alpha beta.haha
 '
 
 cat > expect << EOF
@@ -251,15 +241,11 @@  test_expect_success 'non-match' '
 '
 
 test_expect_success 'non-match value' '
-	echo wow >expect &&
-	git config --get nextsection.nonewline !for >actual &&
-	test_cmp expect actual
+	test_cmp_config wow --get nextsection.nonewline !for
 '
 
 test_expect_success 'multi-valued get returns final one' '
-	echo "wow2 for me" >expect &&
-	git config --get nextsection.nonewline >actual &&
-	test_cmp expect actual
+	test_cmp_config "wow2 for me" --get nextsection.nonewline
 '
 
 test_expect_success 'multi-valued get-all returns all' '
@@ -520,21 +506,11 @@  test_expect_success 'editing stdin is an error' '
 
 test_expect_success 'refer config from subdirectory' '
 	mkdir x &&
-	(
-		cd x &&
-		echo strasse >expect &&
-		git config --get --file ../other-config ein.bahn >actual &&
-		test_cmp expect actual
-	)
-
+	test_cmp_config -C x strasse --get --file ../other-config ein.bahn
 '
 
 test_expect_success 'refer config from subdirectory via --file' '
-	(
-		cd x &&
-		git config --file=../other-config --get ein.bahn >actual &&
-		test_cmp expect actual
-	)
+	test_cmp_config -C x strasse --file=../other-config --get ein.bahn
 '
 
 cat > expect << EOF
@@ -688,16 +664,13 @@  test_expect_success numbers '
 
 test_expect_success '--int is at least 64 bits' '
 	git config giga.watts 121g &&
-	echo 129922760704 >expect &&
-	git config --int --get giga.watts >actual &&
-	test_cmp expect actual
+	echo  >expect &&
+	test_cmp_config 129922760704 --int --get giga.watts
 '
 
 test_expect_success 'invalid unit' '
 	git config aninvalid.unit "1auto" &&
-	echo 1auto >expect &&
-	git config aninvalid.unit >actual &&
-	test_cmp expect actual &&
+	test_cmp_config 1auto aninvalid.unit &&
 	test_must_fail git config --int --get aninvalid.unit 2>actual &&
 	test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual
 '
@@ -1039,9 +1012,7 @@  test_expect_success '--null --get-regexp' '
 
 test_expect_success 'inner whitespace kept verbatim' '
 	git config section.val "foo 	  bar" &&
-	echo "foo 	  bar" >expect &&
-	git config section.val >actual &&
-	test_cmp expect actual
+	test_cmp_config "foo 	  bar" section.val
 '
 
 test_expect_success SYMLINKS 'symlinked configuration' '
@@ -1808,21 +1779,15 @@  big = 1M
 EOF
 
 test_expect_success 'identical modern --type specifiers are allowed' '
-	git config --type=int --type=int core.big >actual &&
-	echo 1048576 >expect &&
-	test_cmp expect actual
+	test_cmp_config 1048576 --type=int --type=int core.big
 '
 
 test_expect_success 'identical legacy --type specifiers are allowed' '
-	git config --int --int core.big >actual &&
-	echo 1048576 >expect &&
-	test_cmp expect actual
+	test_cmp_config 1048576 --int --int core.big
 '
 
 test_expect_success 'identical mixed --type specifiers are allowed' '
-	git config --int --type=int core.big >actual &&
-	echo 1048576 >expect &&
-	test_cmp expect actual
+	test_cmp_config 1048576 --int --type=int core.big
 '
 
 test_expect_success 'non-identical modern --type specifiers are not allowed' '
@@ -1841,21 +1806,15 @@  test_expect_success 'non-identical mixed --type specifiers are not allowed' '
 '
 
 test_expect_success '--type allows valid type specifiers' '
-	echo "true" >expect &&
-	git config --type=bool core.foo >actual &&
-	test_cmp expect actual
+	test_cmp_config true  --type=bool core.foo
 '
 
 test_expect_success '--no-type unsets type specifiers' '
-	echo "10" >expect &&
-	git config --type=bool --no-type core.number >actual &&
-	test_cmp expect actual
+	test_cmp_config 10 --type=bool --no-type core.number
 '
 
 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
+# 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
+#
+#     test "foo" = "$(git -C xyz core.bar)"
+
+test_cmp_config() {
+	if [ "$1" = "-C" ]
+	then
+		shift &&
+		GD="-C $1" &&
+		shift
+	else
+		GD=
+	fi &&
+	echo "$1" >expected &&
+	shift &&
+	git $GD config "$@" >actual &&
+	test_cmp expected actual
+}
+
 # test_cmp_bin - helper to compare binary files
 
 test_cmp_bin() {