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

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

On Wed, Sep 2, 2020 at 3:57 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares
> <matheus.bernardino@usp.br> wrote:
> > 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 &&

Thanks for catching this. I will add the check.

> > @@ -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
> >  '
>exit
> 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

Will do, thanks.

> 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?

Right. This happens because, for a "value not found" error,
test-config will exit with code 1 and print to stdout. This is the
only case where it exits with a non-zero code and prints to stdout
instead of stderr.

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.

> 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?

That's an interesting idea. However, because some callers may want to
use test_i18ngrep instead of test_cmp, I think the required logic
would become too complex.