threads / patch / 31893

patch, 2 partscompletion: test simplifications

Subject: [PATCH 0/2] completion: test simplifications

## tl;dr

7 messages between Oct 22, 2012 and Oct 30, 2012. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Felipe Contreras· Oct 22, 2012, 01:38 UTC · lore

As the subject says. These came up in a discussion with SZEDER.

Felipe Contreras (2):
  completion: refactor __gitcomp related tests
  completion: simplify __gitcomp test helper

 t/t9902-completion.sh | 71 +++++++++++++++------------------------------------
 1 file changed, 21 insertions(+), 50 deletions(-)

-- 
1.8.0

Felipe Contreras· Oct 22, 2012, 01:39 UTC · re: Felipe Contreras · lore

[PATCH 1/2] completion: refactor __gitcomp related tests

Lots of duplicated code!

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 72 ++++++++++++++++-----------------------------------
 1 file changed, 23 insertions(+), 49 deletions(-)

Show changes to t/t9902-completion.sh +23 −49

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cbd0fb6..1c6952a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -72,87 +72,61 @@ test_completion_long ()
 
 newline=$'\n'
 
-test_expect_success '__gitcomp - trailing space - options' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
-	--reuse-message=Z
-	--reedit-message=Z
-	--reset-author Z
-	EOF
+test_gitcomp ()
+{
+	sed -e 's/Z$//' > expected &&
 	(
 		local -a COMPREPLY &&
-		cur="--re" &&
-		__gitcomp "--dry-run --reuse-message= --reedit-message=
-				--reset-author" &&
+		cur="$1" &&
+		shift &&
+		__gitcomp "$@" &&
 		IFS="$newline" &&
 		echo "${COMPREPLY[*]}" > out
 	) &&
 	test_cmp expected out
+}
+
+test_expect_success '__gitcomp - trailing space - options' '
+	test_gitcomp "--re" "--dry-run --reuse-message= --reedit-message=
+		--reset-author" <<-EOF
+	--reuse-message=Z
+	--reedit-message=Z
+	--reset-author Z
+	EOF
 '
 
 test_expect_success '__gitcomp - trailing space - config keys' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_gitcomp "br" "branch. branch.autosetupmerge
+		branch.autosetuprebase browser." <<-\EOF
 	branch.Z
 	branch.autosetupmerge Z
 	branch.autosetuprebase Z
 	browser.Z
 	EOF
-	(
-		local -a COMPREPLY &&
-		cur="br" &&
-		__gitcomp "branch. branch.autosetupmerge
-				branch.autosetuprebase browser." &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
-	) &&
-	test_cmp expected out
 '
 
 test_expect_success '__gitcomp - option parameter' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_gitcomp "--strategy=re" "octopus ours recursive resolve subtree" \
+		"" "re" <<-\EOF
 	recursive Z
 	resolve Z
 	EOF
-	(
-		local -a COMPREPLY &&
-		cur="--strategy=re" &&
-		__gitcomp "octopus ours recursive resolve subtree
-			" "" "re" &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
-	) &&
-	test_cmp expected out
 '
 
 test_expect_success '__gitcomp - prefix' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_gitcomp "branch.me" "remote merge mergeoptions rebase" \
+		"branch.maint." "me" <<-\EOF
 	branch.maint.merge Z
 	branch.maint.mergeoptions Z
 	EOF
-	(
-		local -a COMPREPLY &&
-		cur="branch.me" &&
-		__gitcomp "remote merge mergeoptions rebase
-			" "branch.maint." "me" &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
-	) &&
-	test_cmp expected out
 '
 
 test_expect_success '__gitcomp - suffix' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_gitcomp "branch.me" "master maint next pu" "branch." \
+		"ma" "." <<-\EOF
 	branch.master.Z
 	branch.maint.Z
 	EOF
-	(
-		local -a COMPREPLY &&
-		cur="branch.me" &&
-		__gitcomp "master maint next pu
-			" "branch." "ma" "." &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
-	) &&
-	test_cmp expected out
 '
 
 test_expect_success 'basic' '

-- 
1.8.0

SZEDER Gábor· Oct 30, 2012, 22:45 UTC · re: Felipe Contreras · lore

Re: [PATCH 1/2] completion: refactor __gitcomp related tests

On Mon, Oct 22, 2012 at 03:39:00AM +0200, Felipe Contreras wrote:

> Lots of duplicated code!
> 
> No functional changes.

I'm not sure. I'm all for removing duplicated application code, but I'm usually more conservative when it comes to test code. The more logic, the more possibility for bugs in tests. So tests should be dead simple, even if that means some duplicated test code or the lack of convenience functions. While this might be considered just a matter of personal preference, ...

Show 30 quoted lines

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 72 ++++++++++++++++-----------------------------------
>  1 file changed, 23 insertions(+), 49 deletions(-)
> 
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index cbd0fb6..1c6952a 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -72,87 +72,61 @@ test_completion_long ()
>  
>  newline=$'\n'
>  
> -test_expect_success '__gitcomp - trailing space - options' '
> -	sed -e "s/Z$//" >expected <<-\EOF &&
> -	--reuse-message=Z
> -	--reedit-message=Z
> -	--reset-author Z
> -	EOF
> +test_gitcomp ()
> +{
> +	sed -e 's/Z$//' > expected &&
>  	(
>  		local -a COMPREPLY &&
> -		cur="--re" &&
> -		__gitcomp "--dry-run --reuse-message= --reedit-message=
> -				--reset-author" &&
> +		cur="$1" &&
> +		shift &&
> +		__gitcomp "$@" &&

... I was really puzzled by how __gitcomp() gets its arguments here, and had to think for a while to figure out why it's not broken.

Felipe Contreras· Oct 30, 2012, 23:02 UTC · re: SZEDER Gábor · lore

Re: [PATCH 1/2] completion: refactor __gitcomp related tests

On Tue, Oct 30, 2012 at 11:45 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

Show 12 quoted lines

> On Mon, Oct 22, 2012 at 03:39:00AM +0200, Felipe Contreras wrote:
>> Lots of duplicated code!
>>
>> No functional changes.
>
> I'm not sure.
> I'm all for removing duplicated application code, but I'm usually more
> conservative when it comes to test code.  The more logic, the more
> possibility for bugs in tests.  So tests should be dead simple, even
> if that means some duplicated test code or the lack of convenience
> functions.
> While this might be considered just a matter of personal preference, ...

Fair enough, but my preference is different: test code should be the same as normal code; easy to maintain.

Show 14 quoted lines

>> +test_gitcomp ()
>> +{
>> +     sed -e 's/Z$//' > expected &&
>>       (
>>               local -a COMPREPLY &&
>> -             cur="--re" &&
>> -             __gitcomp "--dry-run --reuse-message= --reedit-message=
>> -                             --reset-author" &&
>> +             cur="$1" &&
>> +             shift &&
>> +             __gitcomp "$@" &&
>
> ... I was really puzzled by how __gitcomp() gets its arguments here,
> and had to think for a while to figure out why it's not broken.

You mean because "$@" is treated in a special way? Without that behavior I'm not sure many things would be possible :)

Anyway, I don't think we should make our life more difficult while maintaining test code... that's my opinion.

Cheers.

-- 
Felipe Contreras

Felipe Contreras· Oct 22, 2012, 01:39 UTC · re: Felipe Contreras · lore

[PATCH 2/2] completion: simplify __gitcomp test helper

By using print_comp as suggested by SZEDER Gábor.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Show changes to t/t9902-completion.sh +5 −8

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 1c6952a..2e7fc06 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -74,15 +74,12 @@ newline=$'\n'
 
 test_gitcomp ()
 {
+	local -a COMPREPLY &&
 	sed -e 's/Z$//' > expected &&
-	(
-		local -a COMPREPLY &&
-		cur="$1" &&
-		shift &&
-		__gitcomp "$@" &&
-		IFS="$newline" &&
-		echo "${COMPREPLY[*]}" > out
-	) &&
+	cur="$1" &&
+	shift &&
+	__gitcomp "$@" &&
+	print_comp &&
 	test_cmp expected out
 }

-- 
1.8.0

SZEDER Gábor· Oct 30, 2012, 21:27 UTC · re: Felipe Contreras · lore

Re: [PATCH 2/2] completion: simplify __gitcomp test helper

On Mon, Oct 22, 2012 at 03:39:01AM +0200, Felipe Contreras wrote:

Show 12 quoted lines

> By using print_comp as suggested by SZEDER Gábor.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 1c6952a..2e7fc06 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -74,15 +74,12 @@ newline=$'\n'

This $newline variable was only used to set IFS to a newline inside SQ blocks. AFAICS after this change there are no such places left, because print_comp() takes care of IFS, so $newline is not necessary anymore.

Show 22 quoted lines

>  test_gitcomp ()
>  {
> +	local -a COMPREPLY &&
>  	sed -e 's/Z$//' > expected &&
> -	(
> -		local -a COMPREPLY &&
> -		cur="$1" &&
> -		shift &&
> -		__gitcomp "$@" &&
> -		IFS="$newline" &&
> -		echo "${COMPREPLY[*]}" > out
> -	) &&
> +	cur="$1" &&
> +	shift &&
> +	__gitcomp "$@" &&
> +	print_comp &&
>  	test_cmp expected out
>  }
>  
> -- 
> 1.8.0
>

Felipe Contreras· Oct 30, 2012, 21:43 UTC · re: SZEDER Gábor · lore

Re: [PATCH 2/2] completion: simplify __gitcomp test helper

On Tue, Oct 30, 2012 at 10:27 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

Show 18 quoted lines

> On Mon, Oct 22, 2012 at 03:39:01AM +0200, Felipe Contreras wrote:
>> By using print_comp as suggested by SZEDER Gábor.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh | 13 +++++--------
>>  1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 1c6952a..2e7fc06 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -74,15 +74,12 @@ newline=$'\n'
>
> This $newline variable was only used to set IFS to a newline inside SQ
> blocks.  AFAICS after this change there are no such places left,
> because print_comp() takes care of IFS, so $newline is not necessary
> anymore.

Right, I thought I did that =/

-- 
Felipe Contreras