diff mbox series

[v3,12/35] userdiff tests: change setup loop to individual test setup

Message ID 20210224195129.4004-13-avarab@gmail.com (mailing list archive)
State New
Headers show
Series userdiff: refactor + test + doc + misc improvements | expand

Commit Message

Ævar Arnfjörð Bjarmason Feb. 24, 2021, 7:51 p.m. UTC
Change the recently amended setup loop in "setup hunk header tests" to
instead set up the test data as we test each individual hunk header
test.

This means we can get rid of the "|| return 1" case and the previous
for-loop, and won't spend time on setting up all the data only to
e.g. fail on the 1st test when running under "-i".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4018-diff-funcname.sh | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

Comments

Junio C Hamano Feb. 25, 2021, 2:52 a.m. UTC | #1
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +# check each individual file
> +for i in $(git -C t4018 ls-files)
> +do
> +	test_expect_success "setup hunk header: $i" "
> +		grep -v '^t4018' \"t4018/$i\" >\"t4018/$i.content\" &&
> +		sed -n -e 's/^t4018 header: //p' <\"t4018/$i\" >\"t4018/$i.header\" &&
> +		cp \"t4018/$i.content\" \"$i\" &&
>  
>  		# add test file to the index
> -		git add "$i" &&
> +		git add \"$i\" &&
>  		# place modified file in the worktree
> -		sed -e "s/ChangeMe/IWasChanged/" <"t4018/$i.content" >"$i" || return 1
> -	done
> -'
> +		sed -e 's/ChangeMe/IWasChanged/' <\"t4018/$i.content\" >\"$i\"
> +	"

Please use '' around the second argument (i.e. test body) of the
test_expect_success, and use "" inside it.  "$i" that is used in the
loop is visible perfectly fine inside the test body when it is
eval'ed, and we won't have to count ugly backslashes that way.

>  
> -# check each individual file
> -for i in $(git ls-files)
> -do
>  	test_expect_success "hunk header: $i" "
>  		git diff -U1 $i >diff &&
>  		sed -n -e 's/^.*@@$//p' -e 's/^.*@@ //p' <diff >ctx &&
Johannes Sixt Feb. 25, 2021, 6:28 p.m. UTC | #2
Am 25.02.21 um 03:52 schrieb Junio C Hamano:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
>> +# check each individual file
>> +for i in $(git -C t4018 ls-files)
>> +do
>> +	test_expect_success "setup hunk header: $i" "
>> +		grep -v '^t4018' \"t4018/$i\" >\"t4018/$i.content\" &&
>> +		sed -n -e 's/^t4018 header: //p' <\"t4018/$i\" >\"t4018/$i.header\" &&
>> +		cp \"t4018/$i.content\" \"$i\" &&
>>  
>>  		# add test file to the index
>> -		git add "$i" &&
>> +		git add \"$i\" &&
>>  		# place modified file in the worktree
>> -		sed -e "s/ChangeMe/IWasChanged/" <"t4018/$i.content" >"$i" || return 1
>> -	done
>> -'
>> +		sed -e 's/ChangeMe/IWasChanged/' <\"t4018/$i.content\" >\"$i\"
>> +	"
> 
> Please use '' around the second argument (i.e. test body) of the
> test_expect_success, and use "" inside it.  "$i" that is used in the
> loop is visible perfectly fine inside the test body when it is
> eval'ed, and we won't have to count ugly backslashes that way.

If we do that, then we better be sure that the implementation of
test_expect_success does not clobber $i. Looks like we are OK at this time.

-- Hannes
Junio C Hamano Feb. 25, 2021, 7:06 p.m. UTC | #3
Johannes Sixt <j6t@kdbg.org> writes:

> Am 25.02.21 um 03:52 schrieb Junio C Hamano:
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>> 
>>> +# check each individual file
>>> +for i in $(git -C t4018 ls-files)
>>> +do
>>> +	test_expect_success "setup hunk header: $i" "
>>> +		grep -v '^t4018' \"t4018/$i\" >\"t4018/$i.content\" &&
>>> +		sed -n -e 's/^t4018 header: //p' <\"t4018/$i\" >\"t4018/$i.header\" &&
>>> +		cp \"t4018/$i.content\" \"$i\" &&
>>>  
>>>  		# add test file to the index
>>> -		git add "$i" &&
>>> +		git add \"$i\" &&
>>>  		# place modified file in the worktree
>>> -		sed -e "s/ChangeMe/IWasChanged/" <"t4018/$i.content" >"$i" || return 1
>>> -	done
>>> -'
>>> +		sed -e 's/ChangeMe/IWasChanged/' <\"t4018/$i.content\" >\"$i\"
>>> +	"
>> 
>> Please use '' around the second argument (i.e. test body) of the
>> test_expect_success, and use "" inside it.  "$i" that is used in the
>> loop is visible perfectly fine inside the test body when it is
>> eval'ed, and we won't have to count ugly backslashes that way.
>
> If we do that, then we better be sure that the implementation of
> test_expect_success does not clobber $i. Looks like we are OK at this time.

That's a good point.  Often we use more specific variable name than
$i to take advantage of the 'eval'-ed nature of the test body, and
it may probably be wise to do so here, too.
diff mbox series

Patch

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 15dcbe735ca..2365f0e361e 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -72,24 +72,23 @@  test_expect_success 'setup hunk header tests' '
 
 	cp -R "$TEST_DIRECTORY"/t4018 . &&
 	git init t4018 &&
-	git -C t4018 add . &&
+	git -C t4018 add .
+'
 
-	for i in $(git -C t4018 ls-files)
-	do
-		grep -v "^t4018" "t4018/$i" >"t4018/$i.content" &&
-		sed -n -e "s/^t4018 header: //p" <"t4018/$i" >"t4018/$i.header" &&
-		cp "t4018/$i.content" "$i" &&
+# check each individual file
+for i in $(git -C t4018 ls-files)
+do
+	test_expect_success "setup hunk header: $i" "
+		grep -v '^t4018' \"t4018/$i\" >\"t4018/$i.content\" &&
+		sed -n -e 's/^t4018 header: //p' <\"t4018/$i\" >\"t4018/$i.header\" &&
+		cp \"t4018/$i.content\" \"$i\" &&
 
 		# add test file to the index
-		git add "$i" &&
+		git add \"$i\" &&
 		# place modified file in the worktree
-		sed -e "s/ChangeMe/IWasChanged/" <"t4018/$i.content" >"$i" || return 1
-	done
-'
+		sed -e 's/ChangeMe/IWasChanged/' <\"t4018/$i.content\" >\"$i\"
+	"
 
-# check each individual file
-for i in $(git ls-files)
-do
 	test_expect_success "hunk header: $i" "
 		git diff -U1 $i >diff &&
 		sed -n -e 's/^.*@@$//p' -e 's/^.*@@ //p' <diff >ctx &&