Re: [PATCH v1 2/2] t1300: check stderr for "ignores pairs" tests

[Andrei Rybak <rybak.a.v@gmail.com>]

On Fri, 14 Apr 2023 at 10:13, Andrei Rybak <rybak.a.v@gmail.com> wrote:
>
> Tests "git config ignores pairs ..." in t1300-config.sh validate that
> "git config" ignores with various kinds of supplied pairs of environment
> variables GIT_CONFIG_KEY_* GIT_CONFIG_VALUE_* that should be ingored.
> By "ignores" here we mean that "git config" doesn't complain about them
> to standard error.

After thinking about this some more, I've realized that this is an
incorrect interpretation
of the titles of these tests. The correct interpretation is more
obvious from another test:

    test_expect_success 'git config ignores pairs exceeding count' '
           GIT_CONFIG_COUNT=1 \
                  GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
                  GIT_CONFIG_KEY_1="pair.two" GIT_CONFIG_VALUE_1="value" \
                  git config --get-regexp "pair.*" >actual &&
           cat >expect <<-EOF &&
           pair.one value
           EOF
           test_cmp expect actual
    '

Key-value pair "pair.two=value" is ignored because it's outside of the
range of the
supplied value of GIT_CONFIG_COUNT.  That is, these tests validate that reading
of these environment variables reads GIT_CONFIG_COUNT first and only loads
GIT_CONFIG_KEY_<n> and GIT_CONFIG_VALUE_<n> that fit in the range.

> This is validated by redirecting the standard error
> to a file called "error" and asserting that it is empty.  However, two
> of these tests incorrectly redirect to standard output while calling the
> file "error", and test 'git config ignores pairs exceeding count'
> doesn't validate standard error at all.
>
> Fix it by redirecting standard error to file "error" and asserting its
> emptiness.
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  t/t1300-config.sh | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 696dca17c6..20a15ede5c 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1462,24 +1462,25 @@ test_expect_success 'git config ignores pairs exceeding count' '
>         GIT_CONFIG_COUNT=1 \
>                 GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
>                 GIT_CONFIG_KEY_1="pair.two" GIT_CONFIG_VALUE_1="value" \
> -               git config --get-regexp "pair.*" >actual &&
> +               git config --get-regexp "pair.*" >actual 2>error &&
>         cat >expect <<-EOF &&
>         pair.one value
>         EOF
> -       test_cmp expect actual
> +       test_cmp expect actual &&
> +       test_must_be_empty error
>  '
>
>  test_expect_success 'git config ignores pairs with zero count' '
>         test_must_fail env \
>                 GIT_CONFIG_COUNT=0 GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
> -               git config pair.one >error &&
> +               git config pair.one 2>error &&
>         test_must_be_empty error
>  '
>
>  test_expect_success 'git config ignores pairs with empty count' '
>         test_must_fail env \
>                 GIT_CONFIG_COUNT= GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \
> -               git config pair.one >error &&
> +               git config pair.one 2>error &&


Same question as in Ævar's
https://lore.kernel.org/git/230406.86pm8htnfk.gmgdl@evledraar.gmail.com/
and my reply https://lore.kernel.org/git/c43e6b71-075a-e39a-7351-8595e145dacf@gmail.com/
applies here, though.  In tests 'git config ignores pairs with zero count' and
 'git config ignores pairs with empty count' test_must_fail already asserts that
"git config" couldn't get the value.  Should we be also inspecting
both stdout and
stderr, as the test  'git config ignores pairs exceeding count' does
(after this patch)?

>         test_must_be_empty error
>  '

>
> --
> 2.40.0
>