| Message ID | f53782f14c5f53da5d5537b369a810a94f9ce184.1599026986.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | grep: honor sparse checkout and add option to ignore it | expand |
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 && > @@ -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?
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.
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 }
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index 3a527e3a84..cff17120dc 100755 --- 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 && test_expect_code $expect_code test-tool config "$op" "$key" >actual && test_cmp expect actual } @@ -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 ' test_expect_success 'find bool value for the entered key' '
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> --- t/t1308-config-set.sh | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)