Re: [PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 2, 2020, 06:57 UTC
Message-ID: <CAPig+cSLjMabeLgkg9N7+ZJ1jotbpJx0FAnjkpNSt0Lf+Q0wNQ@mail.gmail.com>
In-Reply-To: <f53782f14c5f53da5d5537b369a810a94f9ce184.1599026986.git.matheus.bernardino@usp.br>

On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares <matheus.bernardino@usp.br> wrote:

Show 26 quoted lines

> One test in t1308 expects test-config to fail with exit code 128 due to
> a parsing error in the config machinery. But test-config might also exit
> with 128 for any other reason that leads it to call die(). Therefore the
> test can potentially succeed for the wrong reason. To avoid false
> positives, let's check test-config's output, in addition to the exit
> code, and make sure that the cause of the error is the one we expect in
> this test.
>
> Moreover, the test was using the auxiliary function check_config which
> optionally takes a string to compare the test-config stdout against.
> Because this string is optional, there is a risk that future callers may
> also check only the exit code and not the output. To avoid that, make
> the string parameter of this function mandatory.
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
> @@ -14,10 +14,7 @@ check_config () {
>                 expect_code=0
>         fi &&
>         op=$1 key=$2 && shift && shift &&
> -       if test $# != 0
> -       then
> -               printf "%s\n" "$@"
> -       fi >expect &&
> +       printf "%s\n" "$@" >expect &&

This change in behavior is quite subtle. With the original code, "expect" will be entirely empty if no argument is provided, whereas with the revised code, "expect" will contain a single newline. This could be improved by making the argument genuinely mandatory as stated in the commit message. Perhaps something like this:

    if test $# -eq 0
    then
        BUG "check_config 'value' argument missing"
    fi &&
    printf "%s\n" "$@" >expect &&

Show 6 quoted lines

> @@ -130,7 +127,8 @@ test_expect_success 'check line error when NULL string is queried' '
>  test_expect_success 'find integer if value is non parse-able' '
> -       check_config expect_code 128 get_int lamb.head
> +       test_expect_code 128 test-tool config get_int lamb.head 2>result &&
> +       test_i18ngrep "fatal: bad numeric config value '\'none\'' for '\'lamb.head\''" result
>  '

The complex '\'quoting\'' magic leaves and re-enters the single-quote context of the test body and makes it difficult to reason about. Since this is a pattern argument to grep, a simpler alternative would be:

    test_i18ngrep "fatal: bad numeric config value .none. for
.lamb.head." result

Aside from that, do I understand correctly that all other callers which expect a non-zero exit code will find the error message on stdout, but this case will find it on stderr? That makes one wonder if, rather than dropping use of check_config() here, instead check_config() should be enhanced to accept an additional option, such as 'stderr' which causes it to check stderr rather than stdout (similar to how 'expect_code' allows the caller to override the expected exit code). But perhaps that would be overengineered if this case is not expected to come up again as more callers are added in the future?