diff mbox series

[4/5] tests: do not require Git to be built when testing an installed Git

Message ID 316e215e54b921330b91c763255eb25f475a64ae.1542030510.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series tests: various improvements to the GIT_TEST_INSTALLED feature | expand

Commit Message

Linus Arver via GitGitGadget Nov. 12, 2018, 1:48 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

We really only need the test helpers in that case, but that is not what
we test for. So let's skip the test for now when we know that we want to
test an installed Git.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano Nov. 14, 2018, 5:01 a.m. UTC | #1 
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> We really only need the test helpers in that case, but that is not what
> we test for. So let's skip the test for now when we know that we want to
> test an installed Git.

True, but...  hopefully we are making sure t/helpers/ has been built
in some other ways, though, right?  I do not offhand see how tests
would work in a pristine checkout with "cd t && sh t0000-*.sh" as
t/test-lib.sh is not running $(MAKE) itself (and the design of the
test-lib.sh, as can be seen in the check this part of it makes, is
to just abort if we cannot test) with this patch (and PATCH 1/5)
under the test-installed mode, though.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/test-lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 832ede5099..1ea20dc2dc 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -51,7 +51,7 @@ export LSAN_OPTIONS
>  
>  ################################################################
>  # It appears that people try to run tests without building...
> -"$GIT_BUILD_DIR/git" >/dev/null
> +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
>  if test $? != 1
>  then
>  	echo >&2 'error: you do not seem to have built git yet.'
Jeff King Nov. 14, 2018, 12:52 p.m. UTC | #2 
On Mon, Nov 12, 2018 at 05:48:37AM -0800, Johannes Schindelin via GitGitGadget wrote:

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 832ede5099..1ea20dc2dc 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -51,7 +51,7 @@ export LSAN_OPTIONS
>  
>  ################################################################
>  # It appears that people try to run tests without building...
> -"$GIT_BUILD_DIR/git" >/dev/null
> +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
>  if test $? != 1
>  then
>  	echo >&2 'error: you do not seem to have built git yet.'

The original runs the command independently and then checks $?. Your
replacement chains the ||. I think it works, because the only case that
is different is if running git returns 0 (in which case we currently
complain, but the new code would quietly accept this).

That should never happen, but if it does we'd probably want to complain.
And it's pretty subtle all around.  Maybe this would be a bit clearer:

  if test -n "$GIT_TEST_INSTALLED"
  then
	: assume installed git is OK
  else
	"$GIT_BUILD_DIR/git" >/dev/null
	if test $? != 1
	then
		... die ...
	fi
  fi

Though arguably we should be checking that there is a git in our path in
the first instance. So maybe:

  if test -n "$GIT_TEST_INSTALLED"
	"$GIT_TEST_INSTALLED/git" >/dev/null
  else
	"$GIT_BUILD_DIR/git" >/dev/null
  fi
  if test $? != 1 ...

-Peff
Johannes Schindelin Nov. 14, 2018, 1:20 p.m. UTC | #3 
Hi Junio,

On Wed, 14 Nov 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > We really only need the test helpers in that case, but that is not what
> > we test for. So let's skip the test for now when we know that we want to
> > test an installed Git.
> 
> True, but...  hopefully we are making sure t/helpers/ has been built
> in some other ways, though, right?

We do it implicitly, in the test cases that use the helpers.

However, t/test-lib.sh does not particularly verify that the test helpers
have been built.

And I think that is a good thing: we do have several test scripts, I would
think, that do not require any test helper to begin with. These scripts
can work pretty well without having to build anything (read: on a machine
where there is not even a toolchain available to build things).

Besides, it is pretty much only t/helper/test-tool these days, and it is
unlikely that anybody has a `test-tool` in their `PATH`. If they do,
they'll find out soon enough iff they test with `GIT_TEST_INSTALLED` and
iff they do not have the test helper(s) in t/helper/ ;-)

Ciao,
Dscho

> I do not offhand see how tests would work in a pristine checkout with
> "cd t && sh t0000-*.sh" as t/test-lib.sh is not running $(MAKE) itself
> (and the design of the test-lib.sh, as can be seen in the check this
> part of it makes, is to just abort if we cannot test) with this patch
> (and PATCH 1/5) under the test-installed mode, though.
> 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/test-lib.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 832ede5099..1ea20dc2dc 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -51,7 +51,7 @@ export LSAN_OPTIONS
> >  
> >  ################################################################
> >  # It appears that people try to run tests without building...
> > -"$GIT_BUILD_DIR/git" >/dev/null
> > +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
> >  if test $? != 1
> >  then
> >  	echo >&2 'error: you do not seem to have built git yet.'
>
Johannes Schindelin Nov. 14, 2018, 1:41 p.m. UTC | #4 
Hi Peff,

On Wed, 14 Nov 2018, Jeff King wrote:

> On Mon, Nov 12, 2018 at 05:48:37AM -0800, Johannes Schindelin via GitGitGadget wrote:
> 
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 832ede5099..1ea20dc2dc 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -51,7 +51,7 @@ export LSAN_OPTIONS
> >  
> >  ################################################################
> >  # It appears that people try to run tests without building...
> > -"$GIT_BUILD_DIR/git" >/dev/null
> > +test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
> >  if test $? != 1
> >  then
> >  	echo >&2 'error: you do not seem to have built git yet.'
> 
> The original runs the command independently and then checks $?. Your
> replacement chains the ||. I think it works, because the only case that
> is different is if running git returns 0 (in which case we currently
> complain, but the new code would quietly accept this).
> 
> That should never happen, but if it does we'd probably want to complain.
> And it's pretty subtle all around.  Maybe this would be a bit clearer:
> 
>   if test -n "$GIT_TEST_INSTALLED"
>   then
> 	: assume installed git is OK
>   else
> 	"$GIT_BUILD_DIR/git" >/dev/null
> 	if test $? != 1
> 	then
> 		... die ...
> 	fi
>   fi
> 
> Though arguably we should be checking that there is a git in our path in
> the first instance. So maybe:
> 
>   if test -n "$GIT_TEST_INSTALLED"
> 	"$GIT_TEST_INSTALLED/git" >/dev/null
>   else
> 	"$GIT_BUILD_DIR/git" >/dev/null
>   fi
>   if test $? != 1 ...

You're right. I did it using `"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git"`
and I also adjust the error message now.

Will be fixed in the next iteration,
Dscho

> 
> -Peff
>
diff mbox series

Patch

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 832ede5099..1ea20dc2dc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -51,7 +51,7 @@  export LSAN_OPTIONS
 
 ################################################################
 # It appears that people try to run tests without building...
-"$GIT_BUILD_DIR/git" >/dev/null
+test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
 if test $? != 1
 then
 	echo >&2 'error: you do not seem to have built git yet.'