[PATCH 0/3] remote-helpers: trivial test fixes

[PATCH 0/3] remote-helpers: trivial test fixes

[Felipe Contreras]

Hi,

Here are a few fixes for the test framework on the remote-helpers.

Felipe Contreras (3):
  remote-helpers: fix the run of all tests
  remote-bzr: remove stale check code for tests
  remote-hg: fix hg-git test-case

 contrib/remote-helpers/Makefile          |  1 +
 contrib/remote-helpers/test-bzr.sh       | 14 --------------
 contrib/remote-helpers/test-hg-hg-git.sh |  1 -
 3 files changed, 1 insertion(+), 15 deletions(-)

-- 
1.8.2

[PATCH 1/3] remote-helpers: fix the run of all tests

[Felipe Contreras]

Since test-lint doesn't seem to work here, lets disable it. It wouldn't
help either way.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/remote-helpers/Makefile b/contrib/remote-helpers/Makefile
index 9a76575..9c18ed8 100644
--- a/contrib/remote-helpers/Makefile
+++ b/contrib/remote-helpers/Makefile
@@ -3,6 +3,7 @@ TESTS := $(wildcard test*.sh)
 export T := $(addprefix $(CURDIR)/,$(TESTS))
 export MAKE := $(MAKE) -e
 export PATH := $(CURDIR):$(PATH)
+export TEST_LINT :=
 
 test:
 	$(MAKE) -C ../../t $@
-- 
1.8.2

Re: [PATCH 1/3] remote-helpers: fix the run of all tests

[Antoine Pelisse]

Hey Felipe,

On Mon, Apr 1, 2013 at 11:14 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> +export TEST_LINT :=

I think "test-lint-executable" still makes sense here.

Re: [PATCH 1/3] remote-helpers: fix the run of all tests

[Jeff King]

On Mon, Apr 01, 2013 at 11:46:00PM +0200, Antoine Pelisse wrote:

> On Mon, Apr 1, 2013 at 11:14 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> > +export TEST_LINT :=
> 
> I think "test-lint-executable" still makes sense here.

Also test-lint-shell-syntax, which finds a problem with the current
code:

  $ cd contrib/remote-helpers
  $ make test TEST_LINT=test-lint-shell-syntax
  make -e -C ../../t test
  make[1]: Entering directory `/home/peff/compile/git/t'
  rm -f -r test-results
  /home/peff/compile/git/contrib/remote-helpers/test-bzr.sh:139: error: echo -n is not portable (please use printf):   echo -n content > expected &&
  make[1]: *** [test-lint-shell-syntax] Error 1
  make[1]: Leaving directory `/home/peff/compile/git/t'
  make: *** [test] Error 2

I think the check for duplicate-numbers is the only one that does not
make sense. Unfortunately it is not as simple as using $(filter-out) to
get rid of it from TEST_LINT, as that variable may not even be set until
we chain to the other Makefile (e.g., if it comes from loading
config.mak).

-Peff

Re: [PATCH 1/3] remote-helpers: fix the run of all tests

[Torsten Bögershausen]

On 02.04.13 01:40, Jeff King wrote:
> On Mon, Apr 01, 2013 at 11:46:00PM +0200, Antoine Pelisse wrote:
> 
>> On Mon, Apr 1, 2013 at 11:14 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>> +export TEST_LINT :=
>>
>> I think "test-lint-executable" still makes sense here.
> 
> Also test-lint-shell-syntax, which finds a problem with the current
> code:
> 
>   $ cd contrib/remote-helpers
>   $ make test TEST_LINT=test-lint-shell-syntax
>   make -e -C ../../t test
>   make[1]: Entering directory `/home/peff/compile/git/t'
>   rm -f -r test-results
>   /home/peff/compile/git/contrib/remote-helpers/test-bzr.sh:139: error: echo -n is not portable (please use printf):   echo -n content > expected &&
>   make[1]: *** [test-lint-shell-syntax] Error 1
>   make[1]: Leaving directory `/home/peff/compile/git/t'
>   make: *** [test] Error 2
> 
> I think the check for duplicate-numbers is the only one that does not
> make sense. 
[]
Not sure about that, I send a suggestion of a patch in a minute.
Highlights:
1) - rename the contrib test cases and assigns real TC numbers
2) - Forward the numbers into the main "test Makefile"

1) Will probably collide with Felipe's changes, so we just can pick up the idea.
2) Is for only review.
  If we agree on the re-numbering of TC's in contrib, we can apply
  a second round of the patch later.

/Torsten

Re: [PATCH 1/3] remote-helpers: fix the run of all tests

[Jeff King]

On Tue, Apr 02, 2013 at 06:53:28PM +0200, Torsten Bögershausen wrote:

> > I think the check for duplicate-numbers is the only one that does not
> > make sense. 
> []
> Not sure about that, I send a suggestion of a patch in a minute.
> Highlights:
> 1) - rename the contrib test cases and assigns real TC numbers
> 2) - Forward the numbers into the main "test Makefile"

I'm not sure if this is a good idea or not. It puts the
contrib/remote-helpers into the same "number namespace" as the rest of
the test scripts, and enforces uniqueness with test-lint-duplicates,
when "make test" is run from contrib/remote-helpers. But people working
on the main test scripts would not get any such check, and would happily
break contrib/remote-helpers by adding duplicate test numbers.

It makes sense to me to either:

  1. Have the contrib/remote-helpers test live in their own test
     namespace completely, with their own numbers and test-results, and
     pull in relevant bits from the main test harness. We do this
     already with contrib/subtree.  I suggested this when the tests
     first appeared, but there was some argument, and I don't remember
     the details.

  2. Just integrate contrib test scripts into the main repository, but
     leave them off by default. For example, add:

       if test -z "$GIT_TEST_REMOTE_HELPERS"; then
              skip_all="Remote helper tests disabled (define GIT_TEST_REMOTE_HELPERS)"
              test_done
       fi

     to the top of the scripts, and then set GIT_TEST_REMOTE_HELPERS
     in contrib/remote-helpers/Makefile before chaining to the test
     Makefile.

-Peff

Re: [PATCH 1/3] remote-helpers: fix the run of all tests

[Torsten Bögershausen]

On 02.04.13 19:36, Jeff King wrote:
> On Tue, Apr 02, 2013 at 06:53:28PM +0200, Torsten Bögershausen wrote:
> 
>>> I think the check for duplicate-numbers is the only one that does not
>>> make sense. 
>> []
>> Not sure about that, I send a suggestion of a patch in a minute.
>> Highlights:
>> 1) - rename the contrib test cases and assigns real TC numbers
>> 2) - Forward the numbers into the main "test Makefile"
> 
> I'm not sure if this is a good idea or not. It puts the
> contrib/remote-helpers into the same "number namespace" as the rest of
> the test scripts, and enforces uniqueness with test-lint-duplicates,
> when "make test" is run from contrib/remote-helpers. But people working
> on the main test scripts would not get any such check, and would happily
> break contrib/remote-helpers by adding duplicate test numbers.
> 
> It makes sense to me to either:
> 
>   1. Have the contrib/remote-helpers test live in their own test
>      namespace completely, with their own numbers and test-results, and
>      pull in relevant bits from the main test harness. We do this
>      already with contrib/subtree.  I suggested this when the tests
>      first appeared, but there was some argument, and I don't remember
>      the details.
> 
>   2. Just integrate contrib test scripts into the main repository, but
>      leave them off by default. For example, add:
> 
>        if test -z "$GIT_TEST_REMOTE_HELPERS"; then
>               skip_all="Remote helper tests disabled (define GIT_TEST_REMOTE_HELPERS)"
>               test_done
>        fi
> 
>      to the top of the scripts, and then set GIT_TEST_REMOTE_HELPERS
>      in contrib/remote-helpers/Makefile before chaining to the test
>      Makefile.
> 
> -Peff

I think I do my homework, and start with 3), just check duplicates on numbered
test cases.

http://comments.gmane.org/gmane.comp.version-control.git/214194
I send a patch in a minute (thanks Eric for reading)

Re: [PATCH 1/3] remote-helpers: fix the run of all tests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Apr 02, 2013 at 06:53:28PM +0200, Torsten Bögershausen wrote:
>
>> > I think the check for duplicate-numbers is the only one that does not
>> > make sense. 
>> []
>> Not sure about that, I send a suggestion of a patch in a minute.
>> Highlights:
>> 1) - rename the contrib test cases and assigns real TC numbers
>> 2) - Forward the numbers into the main "test Makefile"
>
> I'm not sure if this is a good idea or not.

If that's a polite way to say that this is not a good idea, I'd
agree for all the reasons you mentioned.

> It puts the
> contrib/remote-helpers into the same "number namespace" as the rest of
> the test scripts, and enforces uniqueness with test-lint-duplicates,
> when "make test" is run from contrib/remote-helpers. But people working
> on the main test scripts would not get any such check, and would happily
> break contrib/remote-helpers by adding duplicate test numbers.
>
> It makes sense to me to either:
>
>   1. Have the contrib/remote-helpers test live in their own test
>      namespace completely, with their own numbers and test-results, and
>      pull in relevant bits from the main test harness. We do this
>      already with contrib/subtree.  I suggested this when the tests
>      first appeared, but there was some argument, and I don't remember
>      the details.

This makes more sense than the alternative, given that contrib/
material is "optional" from the main tree's point of view, at least
to me.

Thanks.

>   2. Just integrate contrib test scripts into the main repository, but
>      leave them off by default. For example, add:
>
>        if test -z "$GIT_TEST_REMOTE_HELPERS"; then
>               skip_all="Remote helper tests disabled (define GIT_TEST_REMOTE_HELPERS)"
>               test_done
>        fi
>
>      to the top of the scripts, and then set GIT_TEST_REMOTE_HELPERS
>      in contrib/remote-helpers/Makefile before chaining to the test
>      Makefile.

Re: [PATCH 1/3] remote-helpers: fix the run of all tests

[Felipe Contreras]

Jeff King wrote:
> On Mon, Apr 01, 2013 at 11:46:00PM +0200, Antoine Pelisse wrote:
> 
> > On Mon, Apr 1, 2013 at 11:14 PM, Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> > > +export TEST_LINT :=
> > 
> > I think "test-lint-executable" still makes sense here.
> 
> Also test-lint-shell-syntax, which finds a problem with the current
> code:
> 
>   $ cd contrib/remote-helpers
>   $ make test TEST_LINT=test-lint-shell-syntax
>   make -e -C ../../t test
>   make[1]: Entering directory `/home/peff/compile/git/t'
>   rm -f -r test-results
>   /home/peff/compile/git/contrib/remote-helpers/test-bzr.sh:139: error: echo -n is not portable (please use printf):   echo -n content > expected &&
>   make[1]: *** [test-lint-shell-syntax] Error 1
>   make[1]: Leaving directory `/home/peff/compile/git/t'
>   make: *** [test] Error 2

Indeed. I've fixed he problem, and added the two lint checks to the new version
of the series.

-- 
Felipe Contreras

[PATCH 2/3] remote-bzr: remove stale check code for tests

[Felipe Contreras]

The fastimport plugin was only required in the early stage of
development.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/test-bzr.sh | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/contrib/remote-helpers/test-bzr.sh b/contrib/remote-helpers/test-bzr.sh
index 70aa8a0..5dac171 100755
--- a/contrib/remote-helpers/test-bzr.sh
+++ b/contrib/remote-helpers/test-bzr.sh
@@ -17,20 +17,6 @@ if ! "$PYTHON_PATH" -c 'import bzrlib'; then
 	test_done
 fi
 
-cmd='
-import bzrlib
-bzrlib.initialize()
-import bzrlib.plugin
-bzrlib.plugin.load_plugins()
-import bzrlib.plugins.fastimport
-'
-
-if ! "$PYTHON_PATH" -c "$cmd"; then
-	echo "consider setting BZR_PLUGIN_PATH=$HOME/.bazaar/plugins" 1>&2
-	skip_all='skipping remote-bzr tests; bzr-fastimport not available'
-	test_done
-fi
-
 check () {
 	(cd $1 &&
 	git log --format='%s' -1 &&
-- 
1.8.2

Re: [PATCH 2/3] remote-bzr: remove stale check code for tests

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> The fastimport plugin was only required in the early stage of
> development.

OK, thanks, will queue.

>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/remote-helpers/test-bzr.sh | 14 --------------
>  1 file changed, 14 deletions(-)
>
> diff --git a/contrib/remote-helpers/test-bzr.sh b/contrib/remote-helpers/test-bzr.sh
> index 70aa8a0..5dac171 100755
> --- a/contrib/remote-helpers/test-bzr.sh
> +++ b/contrib/remote-helpers/test-bzr.sh
> @@ -17,20 +17,6 @@ if ! "$PYTHON_PATH" -c 'import bzrlib'; then
>  	test_done
>  fi
>  
> -cmd='
> -import bzrlib
> -bzrlib.initialize()
> -import bzrlib.plugin
> -bzrlib.plugin.load_plugins()
> -import bzrlib.plugins.fastimport
> -'
> -
> -if ! "$PYTHON_PATH" -c "$cmd"; then
> -	echo "consider setting BZR_PLUGIN_PATH=$HOME/.bazaar/plugins" 1>&2
> -	skip_all='skipping remote-bzr tests; bzr-fastimport not available'
> -	test_done
> -fi
> -
>  check () {
>  	(cd $1 &&
>  	git log --format='%s' -1 &&

[PATCH 3/3] remote-hg: fix hg-git test-case

[Felipe Contreras]

There was some lingering code that shouldn't have been there in the
first place.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/test-hg-hg-git.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/contrib/remote-helpers/test-hg-hg-git.sh b/contrib/remote-helpers/test-hg-hg-git.sh
index 7e3967f..3f253b7 100755
--- a/contrib/remote-helpers/test-hg-hg-git.sh
+++ b/contrib/remote-helpers/test-hg-hg-git.sh
@@ -140,7 +140,6 @@ test_expect_success 'executable bit' '
 		git_clone_$x hgrepo-$x gitrepo2-$x &&
 		git_log gitrepo2-$x > log-$x
 	done &&
-	cp -r log-* output-* /tmp/foo/ &&
 
 	test_cmp output-hg output-git &&
 	test_cmp log-hg log-git
-- 
1.8.2

Re: [PATCH 0/3] remote-helpers: trivial test fixes

[Antoine Pelisse]

I'm having an error in "contrib/remote-helpers/test-hg.sh" and it
reproduces on every version I tested since you implemented it. Is it
something in my environment that would make it wrong ?

Cheers,
Antoine

---
expecting success:
  test_when_finished "rm -rf gitrepo*" &&

  (
  cd hgrepo &&
  hg update -r 0
  ) &&

  git clone "hg::$PWD/hgrepo" gitrepo &&
  check gitrepo zero master

1 files updated, 0 files merged, 0 files removed, 0 files unresolved
Cloning into 'gitrepo'...
--- expected 2013-04-01 21:46:32.086796549 +0000
+++ actual 2013-04-01 21:46:32.086796549 +0000
@@ -1,2 +1,2 @@
-zero
-refs/heads/master
+feature-a
+refs/heads/feature-a
not ok 4 - cloning with detached head

On Mon, Apr 1, 2013 at 11:14 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Hi,
>
> Here are a few fixes for the test framework on the remote-helpers.
>
> Felipe Contreras (3):
>   remote-helpers: fix the run of all tests
>   remote-bzr: remove stale check code for tests
>   remote-hg: fix hg-git test-case
>
>  contrib/remote-helpers/Makefile          |  1 +
>  contrib/remote-helpers/test-bzr.sh       | 14 --------------
>  contrib/remote-helpers/test-hg-hg-git.sh |  1 -
>  3 files changed, 1 insertion(+), 15 deletions(-)
>
> --
> 1.8.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] remote-helpers: trivial test fixes

[Felipe Contreras]

Hi,

On Mon, Apr 1, 2013 at 3:48 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
> I'm having an error in "contrib/remote-helpers/test-hg.sh" and it
> reproduces on every version I tested since you implemented it. Is it
> something in my environment that would make it wrong ?
>
> Cheers,
> Antoine
>
> ---
> expecting success:
>   test_when_finished "rm -rf gitrepo*" &&
>
>   (
>   cd hgrepo &&
>   hg update -r 0
>   ) &&
>
>   git clone "hg::$PWD/hgrepo" gitrepo &&
>   check gitrepo zero master
>
> 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> Cloning into 'gitrepo'...
> --- expected 2013-04-01 21:46:32.086796549 +0000
> +++ actual 2013-04-01 21:46:32.086796549 +0000
> @@ -1,2 +1,2 @@
> -zero
> -refs/heads/master
> +feature-a
> +refs/heads/feature-a
> not ok 4 - cloning with detached head

You probably have a very old version of mercurial that has a bug, this
should workaround the issue:
http://thread.gmane.org/gmane.comp.version-control.git/209491

But it shouldn't be a big deal, it's just the default branch (HEAD).

--
Felipe Contreras

Re: [PATCH 0/3] remote-helpers: trivial test fixes

[Felipe Contreras]

On Mon, Apr 1, 2013 at 3:56 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:

> You probably have a very old version of mercurial that has a bug, this
> should workaround the issue:
> http://thread.gmane.org/gmane.comp.version-control.git/209491

I meant:
http://article.gmane.org/gmane.comp.version-control.git/209581


--
Felipe Contreras