diff mbox series

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

Message ID 20230414081352.810296-3-rybak.a.v@gmail.com (mailing list archive)
State Superseded
Headers show
Series git config tests for "'git config ignores pairs ..." | expand

Commit Message

Andrei Rybak April 14, 2023, 8:13 a.m. UTC
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.  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(-)

Comments

Andrei Rybak April 14, 2023, 1:28 p.m. UTC | #1
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
>
diff mbox series

Patch

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 &&
 	test_must_be_empty error
 '