Re: [PATCH 2/2] remote-helpers: move tests out of contrib

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Apr 21, 2014, 18:53 UTC
Message-ID: <5355690fbe8ad_32c4849310d1@nysa.notmuch>
In-Reply-To: <xmqq7g6ia5rr.fsf@gitster.dls.corp.google.com>

Junio C Hamano wrote:

Show 62 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > They should be tested by default.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  contrib/remote-helpers/Makefile                            | 14 --------------
> >  t/Makefile                                                 |  8 +++++++-
> >  .../remote-helpers/test-bzr.sh => t/remote-helpers/bzr.t   |  2 +-
> >  .../test-hg-bidi.sh => t/remote-helpers/hg-bidi.t          |  2 +-
> >  .../test-hg-hg-git.sh => t/remote-helpers/hg-hg-git.t      |  2 +-
> >  contrib/remote-helpers/test-hg.sh => t/remote-helpers/hg.t |  2 +-
> >  6 files changed, 11 insertions(+), 19 deletions(-)
> >  delete mode 100644 contrib/remote-helpers/Makefile
> >  rename contrib/remote-helpers/test-bzr.sh => t/remote-helpers/bzr.t (99%)
> >  rename contrib/remote-helpers/test-hg-bidi.sh => t/remote-helpers/hg-bidi.t (98%)
> >  rename contrib/remote-helpers/test-hg-hg-git.sh => t/remote-helpers/hg-hg-git.t (99%)
> >  rename contrib/remote-helpers/test-hg.sh => t/remote-helpers/hg.t (99%)
> >
> > diff --git a/contrib/remote-helpers/Makefile b/contrib/remote-helpers/Makefile
> > deleted file mode 100644
> > index 239161d..0000000
> > --- a/contrib/remote-helpers/Makefile
> > +++ /dev/null
> > @@ -1,14 +0,0 @@
> > -TESTS := $(wildcard test*.sh)
> > -
> > -export T := $(addprefix $(CURDIR)/,$(TESTS))
> > -export MAKE := $(MAKE) -e
> > -export PATH := $(CURDIR):$(PATH)
> > -export TEST_LINT := test-lint-executable test-lint-shell-syntax
> > -
> > -test:
> > -	$(MAKE) -C ../../t $@
> > -
> > -$(TESTS):
> > -	$(MAKE) -C ../../t $(CURDIR)/$@
> > -
> > -.PHONY: $(TESTS)
> > diff --git a/t/Makefile b/t/Makefile
> > index 8fd1a72..818f4ed 100644
> > --- a/t/Makefile
> > +++ b/t/Makefile
> > @@ -14,6 +14,7 @@ RM ?= rm -f
> >  PROVE ?= prove
> >  DEFAULT_TEST_TARGET ?= test
> >  TEST_LINT ?= test-lint-duplicates test-lint-executable
> > +export TEST_DIRECTORY = $(CURDIR)
> >  
> >  ifdef TEST_OUTPUT_DIRECTORY
> >  TEST_RESULTS_DIRECTORY = $(TEST_OUTPUT_DIRECTORY)/test-results
> > @@ -29,6 +30,9 @@ TEST_RESULTS_DIRECTORY_SQ = $(subst ','\'',$(TEST_RESULTS_DIRECTORY))
> >  T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
> >  TSVN = $(sort $(wildcard t91[0-9][0-9]-*.sh))
> >  TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
> > +TREMOTE = $(wildcard remote-helpers/*.t)
> 
> This step needs a bit more work, I am afraid, to at least have these
> three test scripts follow the same numbering scheme.  Especially given
> that there were recent discussions on allowing a range of tests to
> be run (or omitted) via notations like "5000,5020,9800-9812", not
> doing so now will make it harder to implement such an enhancement.

I don't see how such an enhancement would be beneficial to these remote helpers. For starters there aren't any number rages left for them, which is the reason why I created a new directory and handled them in a special way.

There might be other practical reasons to use this numbering scheme (although I'm sure the scripts could be fixed to not assume those), so I'm OK with having numbers _for now_, but the range namespace should be different from t/t*.sh; it should start from 0, since this is a different directory. That would probably require updating the scripts anyway, so I'm not sure what would be the benefit of this numbering scheme.

Show 22 quoted lines

> Also, I noticed that, unlike say t9801 that shows this:
> 
>     $ cd t && make T=t9801-*sh prove
>     rm -f -r 'test-results'
>     *** prove ***
>     t9801-git-p4-branch.sh .. skipped: skipping git p4 tests; no p4 or p4d
>     Files=1, Tests=0,  0 wallclock secs ( 0.03 usr  0.00 sys +  0.00 ...
>     Result: NOTESTS
> 
> these are unnecessarily noisy when refusing to run:
> 
>     $ cd t && make T=remote-helpers/bzr.t prove
>     *** prove ***
>     remote-helpers/bzr.t .. Traceback (most recent call last):
>       File "<string>", line 1, in <module>
>       File "/usr/local/buildtools/current/sitecustomize/sitecu...
>         return real_import(name, globals, locals, fromlist, level)
>     ImportError: No module named bzrlib
>     remote-helpers/bzr.t .. skipped: skipping remote-bzr tests; bzr not available
>     Files=1, Tests=0,  0 wallclock secs ( 0.03 usr  0.00 sys +  0.02 ...
> 
> Can we squelch these expected import error messages?

Sure:

if ! "$PYTHON_PATH" -c 'import bzrlib' > /dev/null 2>&1

-- 
Felipe Contreras