From: Eric Sunshine <sunshine@sunshineco.com>
Date: Wed, 02 Sep 2020 16:38:00 GMT
Subject: Re: [PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config
Message-ID: <CAPig+cSdAp=gaJ01MSxhxc+j=+OFkiPK7e6c960RZwUU22sKhQ@mail.gmail.com>
In-Reply-To: <CAHd-oW5gk16=Tmhi_e43vVCufM9=zd8jtZJ_EFJVLmC4dJ06=w@mail.gmail.com>

On Wed, Sep 2, 2020 at 12:16 PM Matheus Tavares Bernardino
<matheus.bernardino@usp.br> wrote:
> With that said, I'm wondering now whether we should change the
> function's signature from:
>
> `check_config [expect_code <code>] <cmd> <key> <expected_value>`
>
> to:
>
> `check_config <cmd> <key> <expected_value>`
> `check_config expect_not_found <cmd> <key> <value>`
>
> The second form would then automatically expect exit code 1 and check
> stdout for the message 'Value not found for "<value>"'. With this we
> can avoid wrong uses of check_config to check an arbitrary error code
> without also checking stderr.

Yes, that seems more straightforward. In fact, at this point, you
could just have two distinct functions and eliminate the ugly
complexity of the existing check_config() implementation. Perhaps
something like this (typed in email):

    check_config () {
        test_tool config "$1" "$2" >actual &&
        shift && shift &&
        printf "%s\n" "$@" >expect &&
        test_cmp expect actual
    }

    check_not_found () {
        test_expect_code 1 test_tool config "$1" "$2" >actual &&
        echo "Value not found for \"$2\"" >expect &&
        test_cmp expect actual
    }