Re: [PATCH v2 1/2] t1300: extract and use test_cmp_config()
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
> ---
> 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 ]
# [...]
> +# 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 (t: 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
>
Re: [PATCH v2 1/2] t1300: extract and use test_cmp_config()
On Sat, Sep 29, 2018 at 11:30 AM 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).
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> 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 " changes to 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.)
> +}
[PATCH v2 1/2] t1300: extract and use test_cmp_config()
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 --- 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 '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 +
