# [PATCH 0/2] completion: test simplifications 7 messages from 2012-10-22 to 2012-10-30. Participants: Felipe Contreras, SZEDER Gábor. Thread: https://gitlist.dev/t/31893 ## Felipe Contreras, 2012-10-22 01:38 Subject: [PATCH 0/2] completion: test simplifications Message-ID: <1350869941-22485-1-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1350869941-22485-1-git-send-email-felipe.contreras%40gmail.com ``` 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, 2012-10-22 01:39 Subject: [PATCH 1/2] completion: refactor __gitcomp related tests Message-ID: <1350869941-22485-2-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1350869941-22485-2-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1350869941-22485-1-git-send-email-felipe.contreras@gmail.com> ``` Lots of duplicated code! No functional changes. Signed-off-by: Felipe Contreras --- 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 ``` ## Felipe Contreras, 2012-10-22 01:39 Subject: [PATCH 2/2] completion: simplify __gitcomp test helper Message-ID: <1350869941-22485-3-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1350869941-22485-3-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1350869941-22485-1-git-send-email-felipe.contreras@gmail.com> ``` By using print_comp as suggested by SZEDER Gábor. Signed-off-by: Felipe Contreras --- 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 ``` ## SZEDER Gábor, 2012-10-30 21:27 Subject: Re: [PATCH 2/2] completion: simplify __gitcomp test helper Message-ID: <20121030212725.GA15709@goldbirke> URL: https://gitlist.dev/e/20121030212725.GA15709%40goldbirke In-Reply-To: <1350869941-22485-3-git-send-email-felipe.contreras@gmail.com> ``` 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 > --- > 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 > ``` ## Felipe Contreras, 2012-10-30 21:43 Subject: Re: [PATCH 2/2] completion: simplify __gitcomp test helper Message-ID: URL: https://gitlist.dev/e/CAMP44s34yBSurkiCb7bvJFt82_F%3DhdkFLDyd0o%3D9Yrsn-TRWqg%40mail.gmail.com In-Reply-To: <20121030212725.GA15709@goldbirke> ``` On Tue, Oct 30, 2012 at 10:27 PM, SZEDER Gábor 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 >> --- >> 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 ``` ## SZEDER Gábor, 2012-10-30 22:45 Subject: Re: [PATCH 1/2] completion: refactor __gitcomp related tests Message-ID: <20121030224558.GN12052@goldbirke> URL: https://gitlist.dev/e/20121030224558.GN12052%40goldbirke In-Reply-To: <1350869941-22485-2-git-send-email-felipe.contreras@gmail.com> ``` 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 > --- > 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, 2012-10-30 23:02 Subject: Re: [PATCH 1/2] completion: refactor __gitcomp related tests Message-ID: URL: https://gitlist.dev/e/CAMP44s0aN_FU%2B51huzdtEKOMaw2yhVy4d%3DjV6YHMeoCKDeqU8Q%40mail.gmail.com In-Reply-To: <20121030224558.GN12052@goldbirke> ``` On Tue, Oct 30, 2012 at 11:45 PM, SZEDER Gábor 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 ```