git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 17/25] t0020: use modern test_* helpers

From
SZEDER Gábor <szeder@ira.uka.de>
Date
Mar 25, 2015, 00:23 UTC
Message-ID
<20150325012323.Horde.zCWvV1mF8OBE1PxYPuuEFg8@webmail.informatik.kit.edu>
In-Reply-To
<20150320101308.GQ12543@peff.net>
Quoting Jeff King <peff@peff.net>:
Show 28 quoted lines
> 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 <peff@peff.net>
> ---
>   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.

Show 253 quoted lines
>   	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 and
> three 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
Previous: Jeff KingNext: Jeff King
Message 23 of 68 in “detecting &&-chain breakage”
  1. 0/25 detecting &&-chain breakageJeff King, Mar 20, 2015
  2. 01/25 t/test-lib: introduce --chain-lint optionJeff King, Mar 20, 2015
  3. SZEDER GáborMar 25, 2015
  4. Jeff KingMar 25, 2015
  5. 02/25 t: fix severe &&-chain breakageJeff King, Mar 20, 2015
  6. 03/25 t: fix moderate &&-chain breakageJeff King, Mar 20, 2015
  7. 04/25 t: fix trivial &&-chain breakageJeff King, Mar 20, 2015
  8. 05/25 t: assume test_cmp produces verbose outputJeff King, Mar 20, 2015
  9. 06/25 t: use verbose instead of hand-rolled errorsJeff King, Mar 20, 2015
  10. 07/25 t: use test_must_fail instead of hand-rolled blocksJeff King, Mar 20, 2015
  11. 08/25 t: fix &&-chaining issues around setup which might failJeff King, Mar 20, 2015
  12. 09/25 t: use test_might_fail for diff and grepJeff King, Mar 20, 2015
  13. 10/25 t: use test_expect_code instead of hand-rolled comparisonJeff King, Mar 20, 2015
  14. 11/25 t: wrap complicated expect_code users in a blockJeff King, Mar 20, 2015
  15. 12/25 t: avoid using ":" for commentsJeff King, Mar 20, 2015
  16. 13/25 t3600: fix &&-chain breakage for setup commandsJeff King, Mar 20, 2015
  17. 14/25 t7201: fix &&-chain breakageJeff King, Mar 20, 2015
  18. 15/25 t9502: fix &&-chain breakageJeff King, Mar 20, 2015
  19. Johannes SixtMar 20, 2015
  20. Jeff KingMar 20, 2015
  21. 16/25 t6030: use modern test_* helpersJeff King, Mar 20, 2015
  22. 17/25 t0020: use modern test_* helpersJeff King, Mar 20, 2015
  23. SZEDER GáborMar 25, 2015
  24. Jeff KingMar 25, 2015
  25. 0/8 more &&-chaining test fixupsJeff King, Mar 25, 2015
  26. 1/8 perf-lib: fix ignored exit code inside loopJeff King, Mar 25, 2015
  27. 2/8 t0020: fix ignored exit code inside loopsJeff King, Mar 25, 2015
  28. 3/8 t3305: fix ignored exit code inside loopJeff King, Mar 25, 2015
  29. Johan HerlandMar 25, 2015
  30. 4/8 t7701: fix ignored exit code inside loopJeff King, Mar 25, 2015
  31. 5/8 t: fix some trivial cases of ignored exit codes in loopsJeff King, Mar 25, 2015
  32. 6/8 t: simplify loop exit-code status variablesJeff King, Mar 25, 2015
  33. Junio C HamanoMar 25, 2015
  34. Jeff KingMar 25, 2015
  35. 7/8 t0020: use test_* helpers instead of hand-rolled messagesJeff King, Mar 25, 2015
  36. 8/8 t9001: drop save_confirm helperJeff King, Mar 25, 2015
  37. Junio C HamanoMar 25, 2015
  38. 18/25 t1301: use modern test_* helpersJeff King, Mar 20, 2015
  39. SZEDER GáborMar 24, 2015
  40. Jeff KingMar 25, 2015
  41. 19/25 t6034: use modern test_* helpersJeff King, Mar 20, 2015
  42. SZEDER GáborMar 24, 2015
  43. 20/25 t4117: use modern test_* helpersJeff King, Mar 20, 2015
  44. 21/25 t9001: use test_when_finishedJeff King, Mar 20, 2015
  45. SZEDER GáborMar 25, 2015
  46. Jeff KingMar 25, 2015
  47. 22/25 t0050: appease --chain-lintJeff King, Mar 20, 2015
  48. 23/25 t7004: fix embedded single-quotesJeff King, Mar 20, 2015
  49. 24/25 t0005: fix broken &&-chainsJeff King, Mar 20, 2015
  50. 25/25 t4104: drop hand-rolled error reportingJeff King, Mar 20, 2015
  51. Jeff KingMar 20, 2015
  52. Michael J GruberMar 20, 2015
  53. 26/27 t/*svn*: fix moderate &&-chain breakageMichael J Gruber, Mar 20, 2015
  54. Junio C HamanoMar 20, 2015
  55. Junio C HamanoMar 20, 2015
  56. Jeff KingMar 20, 2015
  57. Jeff KingMar 20, 2015
  58. Michael J GruberMar 23, 2015
  59. 27/27 t9104: fix test for following larger parentsMichael J Gruber, Mar 20, 2015
  60. Jeff KingMar 20, 2015
  61. Junio C HamanoMar 20, 2015
  62. Junio C HamanoMar 20, 2015
  63. Jeff KingMar 20, 2015
  64. Junio C HamanoMar 20, 2015
  65. Eric SunshineMar 20, 2015
  66. Jeff KingMar 21, 2015
  67. Junio C HamanoMar 21, 2015
  68. Jeff KingMar 21, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.