[PATCH 0/2] completion: test simplifications

[PATCH 0/2] completion: test simplifications

[Felipe Contreras]

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

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

[Felipe Contreras]

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(-)

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

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

[SZEDER Gábor]

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, ...

> 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.

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

[Felipe Contreras]

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

>> +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

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

[Felipe Contreras]

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'
 
 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

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

[SZEDER Gábor]

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.

>  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
> 

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

[Felipe Contreras]

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