Re: [PATCH 17/25] t0020: use modern test_* helpers
From: SZEDER Gábor <hidden>
Date: 2016-06-15 23:04:16
Quoting Jeff King [off-list ref]:
quoted hunk ↗ jump to hunk
This test contains a lot of hand-rolled messages to show when the test fails. We can omit most of these by using "verbose" and "test_must_fail". A few of them are for update-index, but we can assume it produces reasonable error messages when it fails. Signed-off-by: Jeff King <redacted> --- t/t0020-crlf.sh | 144 +++++++++++--------------------------------------------- 1 file changed, 28 insertions(+), 116 deletions(-)diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh index d2e51a8..9fa26df 100755 --- a/t/t0020-crlf.sh +++ b/t/t0020-crlf.sh@@ -104,18 +104,12 @@ test_expect_success 'update with autocrlf=input' ' for f in one dir/two do append_cr <$f >tmp && mv -f tmp $f && - git update-index -- $f || { - echo Oops - false - break - } + git update-index -- $f || + break done &&
Ah, these tests are evil, I remember them from the time when I was fiddling with Jonathan's patch. They can fail silently without testing what they were supposed to test. If something in the loop fails, the break will leave the loop but it will do so with zero return value and, consequently, the test will continue as if everything were OK. And unless it was 'git update-index' that failed in a way that left a borked index behind, the 'git diff-index --cached' below will not error out or produce some output that would cause the test to fail. i.e. I tried e.g. append_cr <$f >tmp && mv -f tmp $f && false && in the loop and the test succeeded. I think the best fix would be to unroll the loop: after this patch the loop body consists of only two significant lines and we iterate through the loop only twice, so the test would be even shorter.
quoted hunk ↗ jump to hunk
differs=$(git diff-index --cached HEAD) && - test -z "$differs" || { - echo Oops "$differs" - false - } + verbose test -z "$differs" '@@ -128,18 +122,12 @@ test_expect_success 'update with autocrlf=true' ' for f in one dir/two do append_cr <$f >tmp && mv -f tmp $f && - git update-index -- $f || { - echo "Oops $f" - false - break - } + git update-index -- $f || + break done && differs=$(git diff-index --cached HEAD) && - test -z "$differs" || { - echo Oops "$differs" - false - } + verbose test -z "$differs" '@@ -152,19 +140,13 @@ test_expect_success 'checkout with autocrlf=true' ' for f in one dir/two do remove_cr <"$f" >tmp && mv -f tmp $f && - git update-index -- $f || { - echo "Eh? $f" - false - break - } + verbose git update-index -- $f || + break done && test "$one" = $(git hash-object --stdin <one) && test "$two" = $(git hash-object --stdin <dir/two) && differs=$(git diff-index --cached HEAD) && - test -z "$differs" || { - echo Oops "$differs" - false - } + verbose test -z "$differs" ' test_expect_success 'checkout with autocrlf=input' '@@ -187,10 +169,7 @@ test_expect_success 'checkout with autocrlf=input' ' test "$one" = $(git hash-object --stdin <one) && test "$two" = $(git hash-object --stdin <dir/two) && differs=$(git diff-index --cached HEAD) && - test -z "$differs" || { - echo Oops "$differs" - false - } + verbose test -z "$differs" ' test_expect_success 'apply patch (autocrlf=input)' '@@ -200,10 +179,7 @@ test_expect_success 'apply patch (autocrlf=input)' ' git read-tree --reset -u HEAD && git apply patch.file && - test "$patched" = "$(git hash-object --stdin <one)" || { - echo "Eh? apply without index" - false - } + verbose test "$patched" = "$(git hash-object --stdin <one)" ' test_expect_success 'apply patch --cached (autocrlf=input)' '@@ -213,10 +189,7 @@ test_expect_success 'apply patch --cached(autocrlf=input)' ' git read-tree --reset -u HEAD && git apply --cached patch.file && - test "$patched" = $(git rev-parse :one) || { - echo "Eh? apply with --cached" - false - } + verbose test "$patched" = $(git rev-parse :one) ' test_expect_success 'apply patch --index (autocrlf=input)' '@@ -226,11 +199,8 @@ test_expect_success 'apply patch --index(autocrlf=input)' ' git read-tree --reset -u HEAD && git apply --index patch.file && - test "$patched" = $(git rev-parse :one) && - test "$patched" = $(git hash-object --stdin <one) || { - echo "Eh? apply with --index" - false - } + verbose test "$patched" = $(git rev-parse :one) && + verbose test "$patched" = $(git hash-object --stdin <one) ' test_expect_success 'apply patch (autocrlf=true)' '@@ -240,10 +210,7 @@ test_expect_success 'apply patch (autocrlf=true)' ' git read-tree --reset -u HEAD && git apply patch.file && - test "$patched" = "$(remove_cr <one | git hash-object --stdin)" || { - echo "Eh? apply without index" - false - } + verbose test "$patched" = "$(remove_cr <one | git hash-object --stdin)" ' test_expect_success 'apply patch --cached (autocrlf=true)' '@@ -253,10 +220,7 @@ test_expect_success 'apply patch --cached(autocrlf=true)' ' git read-tree --reset -u HEAD && git apply --cached patch.file && - test "$patched" = $(git rev-parse :one) || { - echo "Eh? apply without index" - false - } + verbose test "$patched" = $(git rev-parse :one) ' test_expect_success 'apply patch --index (autocrlf=true)' '@@ -266,11 +230,8 @@ test_expect_success 'apply patch --index(autocrlf=true)' ' git read-tree --reset -u HEAD && git apply --index patch.file && - test "$patched" = $(git rev-parse :one) && - test "$patched" = "$(remove_cr <one | git hash-object --stdin)" || { - echo "Eh? apply with --index" - false - } + verbose test "$patched" = $(git rev-parse :one) && + verbose test "$patched" = "$(remove_cr <one | git hash-object --stdin)" ' test_expect_success '.gitattributes says two is binary' '@@ -326,21 +287,8 @@ test_expect_success '.gitattributes says two andthree are text' ' echo "t* crlf" >.gitattributes && git read-tree --reset -u HEAD && - if has_cr dir/two - then - : happy - else - echo "Huh?" - false - fi && - - if has_cr three - then - : happy - else - echo "Huh?" - false - fi + verbose has_cr dir/two && + verbose has_cr three ' test_expect_success 'in-tree .gitattributes (1)' '@@ -352,17 +300,8 @@ test_expect_success 'in-tree .gitattributes (1)' ' rm -rf tmp one dir .gitattributes patch.file three && git read-tree --reset -u HEAD && - if has_cr one - then - echo "Eh? one should not have CRLF" - false - else - : happy - fi && - has_cr three || { - echo "Eh? three should still have CRLF" - false - } + test_must_fail has_cr one && + verbose has_cr three ' test_expect_success 'in-tree .gitattributes (2)' '@@ -371,17 +310,8 @@ test_expect_success 'in-tree .gitattributes (2)' ' git read-tree --reset HEAD && git checkout-index -f -q -u -a && - if has_cr one - then - echo "Eh? one should not have CRLF" - false - else - : happy - fi && - has_cr three || { - echo "Eh? three should still have CRLF" - false - } + test_must_fail has_cr one && + verbose has_cr three ' test_expect_success 'in-tree .gitattributes (3)' '@@ -391,17 +321,8 @@ test_expect_success 'in-tree .gitattributes (3)' ' git checkout-index -u .gitattributes && git checkout-index -u one dir/two three && - if has_cr one - then - echo "Eh? one should not have CRLF" - false - else - : happy - fi && - has_cr three || { - echo "Eh? three should still have CRLF" - false - } + test_must_fail has_cr one && + verbose has_cr three ' test_expect_success 'in-tree .gitattributes (4)' '@@ -411,17 +332,8 @@ test_expect_success 'in-tree .gitattributes (4)' ' git checkout-index -u one dir/two three && git checkout-index -u .gitattributes && - if has_cr one - then - echo "Eh? one should not have CRLF" - false - else - : happy - fi && - has_cr three || { - echo "Eh? three should still have CRLF" - false - } + test_must_fail has_cr one && + verbose has_cr three ' test_expect_success 'checkout with existing .gitattributes' ' --2.3.3.520.g3cfbb5d