| Message ID | 1f6d9a80ad45fd9f51c8cffe9ce3721fce9b80c0.1574884302.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: test cleanup stemming from experimentally enabling pipefail | expand |
Hi Denton, On 2019-11-27 11:54:04-0800, Denton Liu <liu.denton@gmail.com> wrote: > - test 0 = $(git verify-pack -v -- .git/objects/pack/*.idx | > - grep "^$coid " | sort | uniq | wc -l) && > + git verify-pack -v -- .git/objects/pack/*.idx >packlist && > + ! grep "^$coid " packlist && I think we want to use test_must_fail instead of ! > echo >.git/objects/info/alternates && > test_must_fail git show $coid > ' > @@ -151,8 +150,8 @@ test_expect_success 'local packed unreachable obs that exist in alternate ODB ar > --unpack-unreachable </dev/null pack && > rm -f .git/objects/pack/* && > mv pack-* .git/objects/pack/ && > - test 0 = $(git verify-pack -v -- .git/objects/pack/*.idx | > - grep "^$coid " | sort | uniq | wc -l) && > + git verify-pack -v -- .git/objects/pack/*.idx >packlist && > + ! grep "^$coid " && ditto
On Sat, Nov 30, 2019 at 5:48 AM Danh Doan <congdanhqx@gmail.com> wrote: > On 2019-11-27 11:54:04-0800, Denton Liu <liu.denton@gmail.com> wrote: > > + ! grep "^$coid " packlist && > > I think we want to use test_must_fail instead of ! test_must_fail() is intended only for use with 'git' commands; "!" should be used otherwise. Quoting from t/README: Don't use '! git cmd' when you want to make sure the git command exits with failure in a controlled way by calling "die()". Instead, use 'test_must_fail git cmd'. This will signal a failure if git dies in an unexpected way (e.g. segfault). On the other hand, don't use test_must_fail for running regular platform commands; just use '! cmd'. We are not in the business of verifying that the world given to us sanely works. So, Denton's use of "!" here is correct.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Sat, Nov 30, 2019 at 5:48 AM Danh Doan <congdanhqx@gmail.com> wrote: >> On 2019-11-27 11:54:04-0800, Denton Liu <liu.denton@gmail.com> wrote: >> > + ! grep "^$coid " packlist && >> >> I think we want to use test_must_fail instead of ! > > test_must_fail() is intended only for use with 'git' commands; "!" > should be used otherwise. Quoting from t/README: > > Don't use '! git cmd' when you want to make sure the git command > exits with failure in a controlled way by calling "die()". Instead, > use 'test_must_fail git cmd'. This will signal a failure if git > dies in an unexpected way (e.g. segfault). > > On the other hand, don't use test_must_fail for running regular > platform commands; just use '! cmd'. We are not in the business > of verifying that the world given to us sanely works. > > So, Denton's use of "!" here is correct. I wonder we can make the framework a bit more self-documenting to avoid having to waste time on discovering potential issues and explaining why it is not an issue, like this exchange. Some ideas: * Perhaps test_must_fail is not descriptive enough that it should apply only to git command invocation. Would it make it more obvious to rename it to say git_must_fail? That would also make it unnecessary to give this rather unfortunate comment in test-lib-functions.sh: # This is not among top-level (test_expect_success | test_expect_failure) # but is a prefix that can be used in the test script, like: # # test_expect_success 'complain and die' ' # do something && # do something else && # test_must_fail git checkout ../outerspace # ' # * If it is too much trouble to rename it, perhaps test_must_fail can be documented better up there? diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index b299ecc326..052d88c5da 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -817,6 +817,13 @@ list_contains () { # Multiple signals can be specified as a comma separated list. # Currently recognized signal names are: sigpipe, success. # (Don't use 'success', use 'test_might_fail' instead.) +# +# Do not use this to run anything but "git". We are not in the business +# of vetting system supplied commands---IOW this is wrong: +# +# test_must_fail grep pattern output +# +# Just use '!' instead. test_must_fail () { case "$1" in * Or perhaps we can detect its use on anything that is not "git" automatically? This is merely to illustrate the idea (the exemption of "env" shown here is too broad for production use) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index b299ecc326..7ab113cd50 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -828,6 +828,10 @@ test_must_fail () { _test_ok= ;; esac + case "$1" in + git|test-tool|env) ;; + *) echo >&7 "warning: test_must_fail $*???" ;; + esac "$@" 2>&7 exit_code=$? if test $exit_code -eq 0 && ! list_contains "$_test_ok" success Hmm?
Hi all, On Sat, Nov 30, 2019 at 09:00:08AM -0800, Junio C Hamano wrote: > * Or perhaps we can detect its use on anything that is not "git" > automatically? This is merely to illustrate the idea (the > exemption of "env" shown here is too broad for production use) > > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > index b299ecc326..7ab113cd50 100644 > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -828,6 +828,10 @@ test_must_fail () { > _test_ok= > ;; > esac > + case "$1" in > + git|test-tool|env) ;; > + *) echo >&7 "warning: test_must_fail $*???" ;; > + esac > "$@" 2>&7 > exit_code=$? > if test $exit_code -eq 0 && ! list_contains "$_test_ok" success I've been cooking a series that gets rid of inappropriate uses test_must_fail() for a while now. As a finishing touch, I implemented the idea Junio suggested above and it seems to be working well. It's a pretty hefty series, weighing in at 46 patches. After the dust settles on 'dl/test-cleanup' (once it gets merged to master), I'll probably start sending out this test_must_fail() series around 10 patches at a time. An advanced preview can be found here[1]. Or, if you'd like me to privately mail you the series, I can do that too. Early comments would be very appreciated. Thanks, Denton [1]: https://github.com/Denton-L/git/commits/ready/cleanup-test-must-fail
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 1edb21bf93..d5cce7c06f 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -48,14 +48,13 @@ test_expect_success 'objects in packs marked .keep are not repacked' ' git commit -m initial_commit && # Create two packs # The first pack will contain all of the objects except one - git rev-list --objects --all | grep -v file2 | - git pack-objects pack && + git rev-list --objects --all >objs && + grep -v file2 objs | git pack-objects pack && # The second pack will contain the excluded object - packid=$(git rev-list --objects --all | grep file2 | - git pack-objects pack) && + packid=$(grep file2 objs | git pack-objects pack) && >pack-$packid.keep && - oid=$(git verify-pack -v pack-$packid.idx | head -n 1 | - sed -e "s/^\($OID_REGEX\).*/\1/") && + git verify-pack -v pack-$packid.idx >packlist && + oid=$(head -n 1 packlist | sed -e "s/^\($OID_REGEX\).*/\1/") && mv pack-* .git/objects/pack/ && git repack -A -d -l && git prune-packed && @@ -134,8 +133,8 @@ test_expect_success 'packed unreachable obs in alternate ODB are not loosened' ' --unpack-unreachable </dev/null pack && rm -f .git/objects/pack/* && mv pack-* .git/objects/pack/ && - test 0 = $(git verify-pack -v -- .git/objects/pack/*.idx | - grep "^$coid " | sort | uniq | wc -l) && + git verify-pack -v -- .git/objects/pack/*.idx >packlist && + ! grep "^$coid " packlist && echo >.git/objects/info/alternates && test_must_fail git show $coid ' @@ -151,8 +150,8 @@ test_expect_success 'local packed unreachable obs that exist in alternate ODB ar --unpack-unreachable </dev/null pack && rm -f .git/objects/pack/* && mv pack-* .git/objects/pack/ && - test 0 = $(git verify-pack -v -- .git/objects/pack/*.idx | - grep "^$coid " | sort | uniq | wc -l) && + git verify-pack -v -- .git/objects/pack/*.idx >packlist && + ! grep "^$coid " && echo >.git/objects/info/alternates && test_must_fail git show $coid '
In a pipe, only the return code of the last command is used. Thus, all other commands will have their return codes masked. Rewrite pipes so that there are no git commands upstream so that we will know if a command fails. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t7700-repack.sh | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)