diff mbox series

[04/19] t4202: clarify intent by creating expected content less cleverly

Message ID 20211209051115.52629-5-sunshine@sunshineco.com (mailing list archive)
State Accepted
Commit 88511d271b503e7e4681d7d8ae01cd3ee3294e35
Headers show
Series tests: fix broken &&-chains & abort loops on error | expand

Commit Message

Eric Sunshine Dec. 9, 2021, 5:11 a.m. UTC 
Several tests assign the output of `$(...)` command substitution to an
"expect" variable, taking advantage of the fact that `$(...)` folds out
the final line terminator while leaving internal line terminators
intact. They do this because the "actual" string with which "expect"
will be compared is shaped the same way. However, this intent (having
internal line terminators, but no final line terminator) is not
necessarily obvious at first glance and may confuse casual readers. The
intent can be made more obvious by using `printf` instead, with which
line termination is stated clearly:

    printf "sixth\nthird"

In fact, many other tests in this script already use `printf` for
precisely this purpose, thus it is an established pattern. Therefore,
convert these tests to employ `printf`, as well.

While at it, modernize the tests to use test_cmp() to compare the
expected and actual output rather than using the semi-deprecated
`verbose test "$x" = "$y"`.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t4202-log.sh | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

Comments

Jeff King Dec. 10, 2021, 9:09 a.m. UTC | #1 
On Thu, Dec 09, 2021 at 12:11:00AM -0500, Eric Sunshine wrote:

> Several tests assign the output of `$(...)` command substitution to an
> "expect" variable, taking advantage of the fact that `$(...)` folds out
> the final line terminator while leaving internal line terminators
> intact. They do this because the "actual" string with which "expect"
> will be compared is shaped the same way. However, this intent (having
> internal line terminators, but no final line terminator) is not
> necessarily obvious at first glance and may confuse casual readers. The
> intent can be made more obvious by using `printf` instead, with which
> line termination is stated clearly:
> 
>     printf "sixth\nthird"
> 
> In fact, many other tests in this script already use `printf` for
> precisely this purpose, thus it is an established pattern. Therefore,
> convert these tests to employ `printf`, as well.

Seems reasonable. I don't think these tests actually care about the lack
of trailing newline, so another option is to use tformat. Or its shorter
cousin, --format. E.g.:

> -	actual=$(git log --pretty="format:%s" --diff-filter=M HEAD) &&
> -	expect=$(echo second) &&
> -	verbose test "$actual" = "$expect"
> +	git log --pretty="format:%s" --diff-filter=M HEAD >actual &&
> +	printf "second" >expect &&
> +	test_cmp expect actual

becomes:

  git log --format=%s --diff-filter=M HEAD >actual &&
  echo second >expect &&
  test_cmp expect actual

which is even less magical. But if the existing pattern is there in
nearby tests, I don't have any problem with following it.

> While at it, modernize the tests to use test_cmp() to compare the
> expected and actual output rather than using the semi-deprecated
> `verbose test "$x" = "$y"`.

Yay. Happy to see more "verbose" calls cleaned up.

-Peff
diff mbox series

Patch

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 7884e3d46b..c2cfbc69f7 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -120,48 +120,48 @@  test_expect_success 'diff-filter=A' '
 
 test_expect_success 'diff-filter=M' '
 
-	actual=$(git log --pretty="format:%s" --diff-filter=M HEAD) &&
-	expect=$(echo second) &&
-	verbose test "$actual" = "$expect"
+	git log --pretty="format:%s" --diff-filter=M HEAD >actual &&
+	printf "second" >expect &&
+	test_cmp expect actual
 
 '
 
 test_expect_success 'diff-filter=D' '
 
-	actual=$(git log --no-renames --pretty="format:%s" --diff-filter=D HEAD) &&
-	expect=$(echo sixth ; echo third) &&
-	verbose test "$actual" = "$expect"
+	git log --no-renames --pretty="format:%s" --diff-filter=D HEAD >actual &&
+	printf "sixth\nthird" >expect &&
+	test_cmp expect actual
 
 '
 
 test_expect_success 'diff-filter=R' '
 
-	actual=$(git log -M --pretty="format:%s" --diff-filter=R HEAD) &&
-	expect=$(echo third) &&
-	verbose test "$actual" = "$expect"
+	git log -M --pretty="format:%s" --diff-filter=R HEAD >actual &&
+	printf "third" >expect &&
+	test_cmp expect actual
 
 '
 
 test_expect_success 'diff-filter=C' '
 
-	actual=$(git log -C -C --pretty="format:%s" --diff-filter=C HEAD) &&
-	expect=$(echo fourth) &&
-	verbose test "$actual" = "$expect"
+	git log -C -C --pretty="format:%s" --diff-filter=C HEAD >actual &&
+	printf "fourth" >expect &&
+	test_cmp expect actual
 
 '
 
 test_expect_success 'git log --follow' '
 
-	actual=$(git log --follow --pretty="format:%s" ichi) &&
-	expect=$(echo third ; echo second ; echo initial) &&
-	verbose test "$actual" = "$expect"
+	git log --follow --pretty="format:%s" ichi >actual &&
+	printf "third\nsecond\ninitial" >expect &&
+	test_cmp expect actual
 '
 
 test_expect_success 'git config log.follow works like --follow' '
 	test_config log.follow true &&
-	actual=$(git log --pretty="format:%s" ichi) &&
-	expect=$(echo third ; echo second ; echo initial) &&
-	verbose test "$actual" = "$expect"
+	git log --pretty="format:%s" ichi >actual &&
+	printf "third\nsecond\ninitial" >expect &&
+	test_cmp expect actual
 '
 
 test_expect_success 'git config log.follow does not die with multiple paths' '
@@ -176,9 +176,9 @@  test_expect_success 'git config log.follow does not die with no paths' '
 
 test_expect_success 'git config log.follow is overridden by --no-follow' '
 	test_config log.follow true &&
-	actual=$(git log --no-follow --pretty="format:%s" ichi) &&
-	expect="third" &&
-	verbose test "$actual" = "$expect"
+	git log --no-follow --pretty="format:%s" ichi >actual &&
+	printf "third" >expect &&
+	test_cmp expect actual
 '
 
 # Note that these commits are intentionally listed out of order.