On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares
<matheus.bernardino@xxxxxx> 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@xxxxxx>
> ---
> 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 &&
> @@ -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?
