threads / patch / 32074

v2, 6 partscompletion: test consolidations

Subject: [PATCH v2 0/6] completion: test consolidations

## tl;dr

16 messages between Nov 11, 2012 and Nov 17, 2012. Diffs are folded; open one to read it.

replies: 15people: 3as markdown or json

Felipe Contreras· Nov 11, 2012, 14:35 UTC · lore

These started from a discussion with SZEDER, but then I realized there were many improvements possible.

Changes since v1:
 * A lot more cleanups
Felipe Contreras (6):
  completion: add comment for test_completion()
  completion: standardize final space marker in tests
  completion: simplify tests using test_completion_long()
  completion: consolidate test_completion*() tests
  completion: refactor __gitcomp related tests
  completion: simplify __gitcomp() test helper
 t/t9902-completion.sh | 133 +++++++++++++++++++-------------------------------
 1 file changed, 51 insertions(+), 82 deletions(-)
-- 
1.8.0
Felipe Contreras· Nov 11, 2012, 14:35 UTC · re: Felipe Contreras · lore

[PATCH v2 1/6] completion: add comment for test_completion()

So that it's easier to understand what it does.

Also, make sure we pass only the first argument for completion. Shouldn't cause any functional changes because run_completion only checks $1.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to t/t9902-completion.sh +5 −1
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cbd0fb6..5c06709 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -54,10 +54,14 @@ run_completion ()
 	__git_wrap__git_main && print_comp
 }
 
+# Test high-level completion
+# Arguments are:
+# 1: typed text so far (cur)
+# 2: expected completion
 test_completion ()
 {
 	test $# -gt 1 && echo "$2" > expected
-	run_completion "$@" &&
+	run_completion "$1" &&
 	test_cmp expected out
 }
 
-- 
1.8.0
SZEDER Gábor· Nov 16, 2012, 20:54 UTC · re: Felipe Contreras · lore

Re: [PATCH v2 1/6] completion: add comment for test_completion()

On Sun, Nov 11, 2012 at 03:35:53PM +0100, Felipe Contreras wrote:
Show 22 quoted lines
> So that it's easier to understand what it does.
> 
> Also, make sure we pass only the first argument for completion.
> Shouldn't cause any functional changes because run_completion only
> checks $1.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index cbd0fb6..5c06709 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -54,10 +54,14 @@ run_completion ()
>  	__git_wrap__git_main && print_comp
>  }
>  
> +# Test high-level completion
> +# Arguments are:
> +# 1: typed text so far (cur)

Bash manuals calls this the current command line or words in the current command line. I'm not sure what you mean with '(cur)' here. The variable $cur in the completion script (or in bash-completion in general) is something completely different.

Show 11 quoted lines
> +# 2: expected completion
>  test_completion ()
>  {
>  	test $# -gt 1 && echo "$2" > expected
> -	run_completion "$@" &&
> +	run_completion "$1" &&
>  	test_cmp expected out
>  }
>  
> -- 
> 1.8.0
Felipe Contreras· Nov 16, 2012, 21:06 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2 1/6] completion: add comment for test_completion()

On Fri, Nov 16, 2012 at 9:54 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 26 quoted lines
> On Sun, Nov 11, 2012 at 03:35:53PM +0100, Felipe Contreras wrote:
>> So that it's easier to understand what it does.
>>
>> Also, make sure we pass only the first argument for completion.
>> Shouldn't cause any functional changes because run_completion only
>> checks $1.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index cbd0fb6..5c06709 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -54,10 +54,14 @@ run_completion ()
>>       __git_wrap__git_main && print_comp
>>  }
>>
>> +# Test high-level completion
>> +# Arguments are:
>> +# 1: typed text so far (cur)
>
> Bash manuals calls this the current command line or words in the
> current command line.  I'm not sure what you mean with '(cur)' here.
The current _word_ text typed so far.
> The variable $cur in the completion script (or in bash-completion in
> general) is something completely different.

I believe bash's completion, this test, and the whole git completion stuff uses the same definition of 'cur'.

Cheers.
-- 
Felipe Contreras
Felipe Contreras· Nov 16, 2012, 21:09 UTC · re: Felipe Contreras · lore

Re: [PATCH v2 1/6] completion: add comment for test_completion()

On Fri, Nov 16, 2012 at 10:06 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 35 quoted lines
> On Fri, Nov 16, 2012 at 9:54 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> On Sun, Nov 11, 2012 at 03:35:53PM +0100, Felipe Contreras wrote:
>>> So that it's easier to understand what it does.
>>>
>>> Also, make sure we pass only the first argument for completion.
>>> Shouldn't cause any functional changes because run_completion only
>>> checks $1.
>>>
>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>> ---
>>>  t/t9902-completion.sh | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>>> index cbd0fb6..5c06709 100755
>>> --- a/t/t9902-completion.sh
>>> +++ b/t/t9902-completion.sh
>>> @@ -54,10 +54,14 @@ run_completion ()
>>>       __git_wrap__git_main && print_comp
>>>  }
>>>
>>> +# Test high-level completion
>>> +# Arguments are:
>>> +# 1: typed text so far (cur)
>>
>> Bash manuals calls this the current command line or words in the
>> current command line.  I'm not sure what you mean with '(cur)' here.
>
> The current _word_ text typed so far.
>
>> The variable $cur in the completion script (or in bash-completion in
>> general) is something completely different.
>
> I believe bash's completion, this test, and the whole git completion
> stuff uses the same definition of 'cur'.

Oops, actually in this particular helper this is not actually 'cur'. I think 'command line' might be more appropriate.

-- 
Felipe Contreras
Felipe Contreras· Nov 11, 2012, 14:35 UTC · re: Felipe Contreras · lore

[PATCH v2 2/6] completion: standardize final space marker in tests

The rest of the code uses ' Z$'. Lets use that for test_completion_long() as well.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)
Show changes to t/t9902-completion.sh +13 −14
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5c06709..aff7e44 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -66,11 +66,10 @@ test_completion ()
 }
 
 # Like test_completion, but reads expectation from stdin,
-# which is convenient when it is multiline. We also process "_" into
-# spaces to make test vectors more readable.
+# which is convenient when it is multiline.
 test_completion_long ()
 {
-	tr _ " " >expected &&
+	sed -e 's/Z$//' > expected &&
 	test_completion "$1"
 }
 
@@ -252,24 +251,24 @@ test_expect_success 'setup for ref completion' '
 
 test_expect_success 'checkout completes ref names' '
 	test_completion_long "git checkout m" <<-\EOF
-	master_
-	mybranch_
-	mytag_
+	master Z
+	mybranch Z
+	mytag Z
 	EOF
 '
 
 test_expect_success 'show completes all refs' '
 	test_completion_long "git show m" <<-\EOF
-	master_
-	mybranch_
-	mytag_
+	master Z
+	mybranch Z
+	mytag Z
 	EOF
 '
 
 test_expect_success '<ref>: completes paths' '
 	test_completion_long "git show mytag:f" <<-\EOF
-	file1_
-	file2_
+	file1 Z
+	file2 Z
 	EOF
 '
 
@@ -278,7 +277,7 @@ test_expect_success 'complete tree filename with spaces' '
 	git add . &&
 	git commit -m spaces &&
 	test_completion_long "git show HEAD:nam" <<-\EOF
-	name with spaces_
+	name with spaces Z
 	EOF
 '
 
@@ -287,8 +286,8 @@ test_expect_failure 'complete tree filename with metacharacters' '
 	git add . &&
 	git commit -m meta &&
 	test_completion_long "git show HEAD:nam" <<-\EOF
-	name with ${meta}_
-	name with spaces_
+	name with ${meta} Z
+	name with spaces Z
 	EOF
 '
 
-- 
1.8.0
Felipe Contreras· Nov 11, 2012, 14:35 UTC · re: Felipe Contreras · lore

[PATCH v2 3/6] completion: simplify tests using test_completion_long()

No need to duplicate that functionality.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
Show changes to t/t9902-completion.sh +3 −6
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index aff7e44..204c92a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -172,7 +172,7 @@ test_expect_success 'basic' '
 '
 
 test_expect_success 'double dash "git" itself' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_completion_long "git --" <<-\EOF
 	--paginate Z
 	--no-pager Z
 	--git-dir=
@@ -187,11 +187,10 @@ test_expect_success 'double dash "git" itself' '
 	--no-replace-objects Z
 	--help Z
 	EOF
-	test_completion "git --"
 '
 
 test_expect_success 'double dash "git checkout"' '
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_completion_long "git checkout --" <<-\EOF
 	--quiet Z
 	--ours Z
 	--theirs Z
@@ -202,17 +201,15 @@ test_expect_success 'double dash "git checkout"' '
 	--orphan Z
 	--patch Z
 	EOF
-	test_completion "git checkout --"
 '
 
 test_expect_success 'general options' '
 	test_completion "git --ver" "--version " &&
 	test_completion "git --hel" "--help " &&
-	sed -e "s/Z$//" >expected <<-\EOF &&
+	test_completion_long "git --exe" <<-\EOF &&
 	--exec-path Z
 	--exec-path=
 	EOF
-	test_completion "git --exe" &&
 	test_completion "git --htm" "--html-path " &&
 	test_completion "git --pag" "--paginate " &&
 	test_completion "git --no-p" "--no-pager " &&
-- 
1.8.0
Felipe Contreras· Nov 11, 2012, 14:35 UTC · re: Felipe Contreras · lore

[PATCH v2 4/6] completion: consolidate test_completion*() tests

No need to have two versions; if a second argument is specified, use that, otherwise use stdin.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)
Show changes to t/t9902-completion.sh +13 −17
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 204c92a..59cdbfd 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -60,19 +60,15 @@ run_completion ()
 # 2: expected completion
 test_completion ()
 {
-	test $# -gt 1 && echo "$2" > expected
+	if [ $# -gt 1 ]; then
+		echo "$2" > expected
+	else
+		sed -e 's/Z$//' > expected
+	fi &&
 	run_completion "$1" &&
 	test_cmp expected out
 }
 
-# Like test_completion, but reads expectation from stdin,
-# which is convenient when it is multiline.
-test_completion_long ()
-{
-	sed -e 's/Z$//' > expected &&
-	test_completion "$1"
-}
-
 newline=$'\n'
 
 test_expect_success '__gitcomp - trailing space - options' '
@@ -172,7 +168,7 @@ test_expect_success 'basic' '
 '
 
 test_expect_success 'double dash "git" itself' '
-	test_completion_long "git --" <<-\EOF
+	test_completion "git --" <<-\EOF
 	--paginate Z
 	--no-pager Z
 	--git-dir=
@@ -190,7 +186,7 @@ test_expect_success 'double dash "git" itself' '
 '
 
 test_expect_success 'double dash "git checkout"' '
-	test_completion_long "git checkout --" <<-\EOF
+	test_completion "git checkout --" <<-\EOF
 	--quiet Z
 	--ours Z
 	--theirs Z
@@ -206,7 +202,7 @@ test_expect_success 'double dash "git checkout"' '
 test_expect_success 'general options' '
 	test_completion "git --ver" "--version " &&
 	test_completion "git --hel" "--help " &&
-	test_completion_long "git --exe" <<-\EOF &&
+	test_completion "git --exe" <<-\EOF &&
 	--exec-path Z
 	--exec-path=
 	EOF
@@ -247,7 +243,7 @@ test_expect_success 'setup for ref completion' '
 '
 
 test_expect_success 'checkout completes ref names' '
-	test_completion_long "git checkout m" <<-\EOF
+	test_completion "git checkout m" <<-\EOF
 	master Z
 	mybranch Z
 	mytag Z
@@ -255,7 +251,7 @@ test_expect_success 'checkout completes ref names' '
 '
 
 test_expect_success 'show completes all refs' '
-	test_completion_long "git show m" <<-\EOF
+	test_completion "git show m" <<-\EOF
 	master Z
 	mybranch Z
 	mytag Z
@@ -263,7 +259,7 @@ test_expect_success 'show completes all refs' '
 '
 
 test_expect_success '<ref>: completes paths' '
-	test_completion_long "git show mytag:f" <<-\EOF
+	test_completion "git show mytag:f" <<-\EOF
 	file1 Z
 	file2 Z
 	EOF
@@ -273,7 +269,7 @@ test_expect_success 'complete tree filename with spaces' '
 	echo content >"name with spaces" &&
 	git add . &&
 	git commit -m spaces &&
-	test_completion_long "git show HEAD:nam" <<-\EOF
+	test_completion "git show HEAD:nam" <<-\EOF
 	name with spaces Z
 	EOF
 '
@@ -282,7 +278,7 @@ test_expect_failure 'complete tree filename with metacharacters' '
 	echo content >"name with \${meta}" &&
 	git add . &&
 	git commit -m meta &&
-	test_completion_long "git show HEAD:nam" <<-\EOF
+	test_completion "git show HEAD:nam" <<-\EOF
 	name with ${meta} Z
 	name with spaces Z
 	EOF
-- 
1.8.0
Junio C Hamano· Nov 16, 2012, 23:41 UTC · re: Felipe Contreras · lore

Re: [PATCH v2 4/6] completion: consolidate test_completion*() tests

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 22 quoted lines
> No need to have two versions; if a second argument is specified, use
> that, otherwise use stdin.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 204c92a..59cdbfd 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -60,19 +60,15 @@ run_completion ()
>  # 2: expected completion
>  test_completion ()
>  {
> -	test $# -gt 1 && echo "$2" > expected
> +	if [ $# -gt 1 ]; then
> +		echo "$2" > expected
> +	else
> +		sed -e 's/Z$//' > expected
> +	fi &&

As "$2" could begin with dash, end with \c, etc. that possibly can be misinterpred by echo, I'd rewrite this as

		printf '%s\n' "$2" >expected
Otherwise looked fine; thanks.
Felipe Contreras· Nov 17, 2012, 20:26 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 4/6] completion: consolidate test_completion*() tests

On Sat, Nov 17, 2012 at 12:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 31 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> No need to have two versions; if a second argument is specified, use
>> that, otherwise use stdin.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh | 30 +++++++++++++-----------------
>>  1 file changed, 13 insertions(+), 17 deletions(-)
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 204c92a..59cdbfd 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -60,19 +60,15 @@ run_completion ()
>>  # 2: expected completion
>>  test_completion ()
>>  {
>> -     test $# -gt 1 && echo "$2" > expected
>> +     if [ $# -gt 1 ]; then
>> +             echo "$2" > expected
>> +     else
>> +             sed -e 's/Z$//' > expected
>> +     fi &&
>
> As "$2" could begin with dash, end with \c, etc. that possibly can
> be misinterpred by echo, I'd rewrite this as
>
>                 printf '%s\n' "$2" >expected
>
> Otherwise looked fine; thanks.
But that was the case before. I would do that in a separate patch.
Cheers.
-- 
Felipe Contreras
Felipe Contreras· Nov 11, 2012, 14:35 UTC · re: Felipe Contreras · lore

[PATCH v2 5/6] 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 | 76 ++++++++++++++++++---------------------------------
 1 file changed, 27 insertions(+), 49 deletions(-)
Show changes to t/t9902-completion.sh +27 −49
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 59cdbfd..66c7af6 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -71,87 +71,65 @@ test_completion ()
 
 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.
+# Arguments are:
+# 1: typed text so far (cur)
+# *: arguments to pass to __gitcomp
+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
Junio C Hamano· Nov 16, 2012, 19:13 UTC · re: Felipe Contreras · lore

Re: [PATCH v2 5/6] completion: refactor __gitcomp related tests

Not asking for a re-roll but am asking for clarification so that I can locally update before queuing.

Felipe Contreras <felipe.contreras@gmail.com> writes:
> Lots of duplicated code!
... removed, you mean?
Show 25 quoted lines
> No functional changes.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 76 ++++++++++++++++++---------------------------------
>  1 file changed, 27 insertions(+), 49 deletions(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 59cdbfd..66c7af6 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -71,87 +71,65 @@ test_completion ()
>  
>  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.
> +# Arguments are:
> +# 1: typed text so far (cur)
> +# *: arguments to pass to __gitcomp

s/\*/remainder/, perhaps? I think you shift $1 out and do not pass it to __gitcomp.

And expected output is from the standard input just like test_completion?

Show 25 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 "$@" &&
>  		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
>  '
Nice shrinkage.
Felipe Contreras· Nov 16, 2012, 19:53 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 5/6] completion: refactor __gitcomp related tests

On Fri, Nov 16, 2012 at 8:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> Not asking for a re-roll but am asking for clarification so that I
> can locally update before queuing.
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Lots of duplicated code!
>
> ... removed, you mean?
Yes.
Show 28 quoted lines
>> No functional changes.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh | 76 ++++++++++++++++++---------------------------------
>>  1 file changed, 27 insertions(+), 49 deletions(-)
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 59cdbfd..66c7af6 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -71,87 +71,65 @@ test_completion ()
>>
>>  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.
>> +# Arguments are:
>> +# 1: typed text so far (cur)
>> +# *: arguments to pass to __gitcomp
>
> s/\*/remainder/, perhaps?  I think you shift $1 out and do not pass
> it to __gitcomp.
Right, by * I meant the rest.
> And expected output is from the standard input just like
> test_completion?
Correct.
Show 27 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 "$@" &&
>>               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
>>  '
>
> Nice shrinkage.
That's a comment about the whole patch series I hope :)
Cheers.
-- 
Felipe Contreras
SZEDER Gábor· Nov 16, 2012, 21:02 UTC · re: Felipe Contreras · lore

Re: [PATCH v2 5/6] completion: refactor __gitcomp related tests

On Sun, Nov 11, 2012 at 03:35:57PM +0100, Felipe Contreras wrote:
Show 8 quoted lines
> Lots of duplicated code!
> 
> No functional changes.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 76 ++++++++++++++++++---------------------------------
>  1 file changed, 27 insertions(+), 49 deletions(-)

Despite the impressive numbers, these tests are more useful without this cleanup.

Show 17 quoted lines
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 59cdbfd..66c7af6 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -71,87 +71,65 @@ test_completion ()
>  
>  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.
> +# Arguments are:
> +# 1: typed text so far (cur)

The first argument is not the typed text so far, but the word currently containing the cursor position.

Show 105 quoted lines
> +# *: arguments to pass to __gitcomp
> +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
> 
Junio C Hamano· Nov 16, 2012, 23:40 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2 5/6] completion: refactor __gitcomp related tests

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 12 quoted lines
> On Sun, Nov 11, 2012 at 03:35:57PM +0100, Felipe Contreras wrote:
>> Lots of duplicated code!
>> 
>> No functional changes.
>> 
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh | 76 ++++++++++++++++++---------------------------------
>>  1 file changed, 27 insertions(+), 49 deletions(-)
>
> Despite the impressive numbers, these tests are more useful without
> this cleanup.

Is this because consolidation of the duplicated part of the tests into a single helper makes it harder to instrument one test you are interested in (or developing) for debugging?

It indeed is a problem, and cutting and pasting the same code to multiple tests is one way to solve the problem (you can easily instrument the copy in the test you are interested in while leaving others intact), but I do not think that is a good solution. A bugfix or enhancement to the shared (or duplicated) part can be done by touching one place only after this change, while with the current code you have to repeat the same fix to all places, no?

Show 6 quoted lines
>> +# Test __gitcomp.
>> +# Arguments are:
>> +# 1: typed text so far (cur)
>
> The first argument is not the typed text so far, but the word
> currently containing the cursor position.

Care to update this with a follow-up patch, so that I do not have to keep track of minute details ;-)?

Felipe Contreras· Nov 11, 2012, 14:35 UTC · re: Felipe Contreras · lore

[PATCH v2 6/6] 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 | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
Show changes to t/t9902-completion.sh +5 −10
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 66c7af6..9b38b69 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -69,23 +69,18 @@ test_completion ()
 	test_cmp expected out
 }
 
-newline=$'\n'
-
 # Test __gitcomp.
 # Arguments are:
 # 1: typed text so far (cur)
 # *: arguments to pass to __gitcomp
 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

← back to recent threads