tests: various improvements to the GIT_TEST_INSTALLED feature

PATCH Superseded Johannes Schindelin via GitGitGadget 4 participants 27 messages Nov 12, 2018 – Nov 15, 2018 Kbuild

Share & tools

Johannes Schindelin via GitGitGadget

author

2018-11-12 13:48 UTC #0 lore raw

cover tests: various improvements to the GIT_TEST_INSTALLED feature

By setting the GIT_TEST_INSTALLED variable to the path of an installed Git executable, it is possible to run the test suite also on a specific installed version (as opposed to a version built from scratch).

The only thing this needs that is unlikely to be installed is the test helper(s).

However, there have been a few rough edges around that, identified in my (still ongoing) work to support building Git in Visual Studio (where we do not want to run GNU Make, and where we have no canonical way to create, say, hard-linked copies of the built-in commands, and other work to let Git for Windows play better with BusyBox.

Triggered by a comment of AEvar
[https://public-inbox.org/git/[email protected]/], I hereby contribute these assorted fixes for the GIT_TEST_INSTALLED feature.

Johannes Schindelin (5):
tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/
tests: respect GIT_TEST_INSTALLED when initializing repositories t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set
tests: do not require Git to be built when testing an installed Git
tests: explicitly use git.exe on Windows

 Makefile                |  1 +
 t/lib-gettext.sh        |  7 ++++++-
 t/test-lib-functions.sh |  3 ++-
 t/test-lib.sh           | 15 ++++++++++-----
 4 files changed, 19 insertions(+), 7 deletions(-)

base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-73%2Fdscho%2Ftest-git-installed-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-73/dscho/test-git-installed-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/73

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2018-11-12 13:48 UTC #1 lore raw

PATCH 1/5 tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/

From: Johannes Schindelin <[email protected]>

We really need to be able to find the test helpers... Really. This change was forgotten when we moved the test helpers into t/helper/

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/test-lib.sh +1 -1

index 47a99aa0ed..832ede5099 100644
@@ -957,7 +957,7 @@ elif test -n "$GIT_TEST_INSTALLED"
 then
 	GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
 	error "Cannot run git from $GIT_TEST_INSTALLED."
-	PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR:$PATH
+	PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH
 	GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
 else # normal case, use ../bin-wrappers only unless $with_dashes:
 	git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"

Signature · 1 line

gitgitgadget

Junio C Hamano

2018-11-14 04:53 UTC #2 lore raw

Re: [PATCH 1/5] tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 4 lines · writes:

From: Johannes Schindelin <[email protected]>
We really need to be able to find the test helpers... Really. This change was forgotten when we moved the test helpers into t/helper/

Yeah, it is unfortunate that more and more things have internal whitebox test that can only be checked with test-helper binaries, which makes it impossible to fully testing installed git, but that is not a new issue this fix introduces. This at least allows us to keep using the test-helpers from freshly built Git while using the installed version of Git for most of the tests.

Thanks.

Quoted 18 lines

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/test-lib.sh +1 -1

index 47a99aa0ed..832ede5099 100644
@@ -957,7 +957,7 @@ elif test -n "$GIT_TEST_INSTALLED"
 then
 	GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
 	error "Cannot run git from $GIT_TEST_INSTALLED."
-	PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR:$PATH
+	PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH
 	GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
 else # normal case, use ../bin-wrappers only unless $with_dashes:
 	git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"

Johannes Schindelin via GitGitGadget

author

2018-11-12 13:48 UTC #3 lore raw

PATCH 2/5 tests: respect GIT_TEST_INSTALLED when initializing repositories

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

It really makes very, very little sense to use a different git executable than the one the caller indicated via setting the environment variable GIT_TEST_INSTALLED.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/test-lib-functions.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

t/test-lib-functions.sh +2 -1

index 78d8c3783b..801cc9b2ef 100644
@@ -900,7 +900,8 @@ test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled
 	) || exit

Signature · 1 line

gitgitgadget

Junio C Hamano

2018-11-14 04:55 UTC #4 lore raw

Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 5 lines · writes:

From: Johannes Schindelin <[email protected]>
It really makes very, very little sense to use a different git executable than the one the caller indicated via setting the environment variable GIT_TEST_INSTALLED.

Makes perfect sense. Shouldn't we be asking where the template directory of the installed version is and using it instead of the freshly built one, no?

Quoted 20 lines

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/test-lib-functions.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

t/test-lib-functions.sh +2 -1

index 78d8c3783b..801cc9b2ef 100644
@@ -900,7 +900,8 @@ test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled
 	) || exit

Johannes Schindelin

2018-11-14 13:16 UTC #5 lore raw

Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

Hi Junio,

Quoted 5 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 2 lines · writes:
From: Johannes Schindelin <[email protected]>

Quoted 7 lines

Quoted 3 lines
It really makes very, very little sense to use a different git executable than the one the caller indicated via setting the environment variable GIT_TEST_INSTALLED.
Makes perfect sense. Shouldn't we be asking where the template directory of the installed version is and using it instead of the freshly built one, no?

It would make sense, but we don't know how to get that information, do we?

$ git grep DEFAULT_GIT_TEMPLATE_DIR
Makefile:       -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR
builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
builtin/init-db.c:              template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \

In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the only user in the code is init-db.c which uses it in copy_templates().

And changing the code *now* to let us query Git where it thinks its templates should be won't work, as this patch is about using the installed Git (at whatever pre-compiled version that might be).

Ciao,
Dscho

Quoted 21 lines

Quoted 20 lines

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/test-lib-functions.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

t/test-lib-functions.sh +2 -1

index 78d8c3783b..801cc9b2ef 100644
@@ -900,7 +900,8 @@ test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled
 	) || exit

Junio C Hamano

2018-11-14 14:59 UTC #6 lore raw

Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

Quoted 5 lines · Johannes Schindelin

It would make sense, but we don't know how to get that information, do we?
...
And changing the code *now* to let us query Git where it thinks its templates should be won't work, as this patch is about using the installed Git (at whatever pre-compiled version that might be).

It won't work, but we can add something like "git var templatedir" to help those who want to further improve the test-installed mode next year, preparing for better future by sowing seeds now.

In the meantime, using the same temlate dir as before is probably the best we can do. Two and a half tangential thoughts are:

- But then, we need to make sure $GIT_BUILD_DIR/templates/blt/ is populated, if we do rely on them (i.e. we probably want to make sure we have built).

- Yet, once installed, the contents of the templatedir can be arbitrarily munged by the end user, so anything that depends on what is in the template won't work as a reliable test piece.

- Among what's in templates/blt/, we explicitly disable hooks at the beginning of the test repository creation to ensure no hooks interfere what we test by default, but we will get affected by what is in info/excludes. The contents of freshly-built one is empty, so it is unlikely that this will cause problems, but if we use installed templates, we cannot control what's in there, letting the tests get affected to random things the end-user happens to have.

So after all, if we were to change anything, it might make better sense not to install anything from any templatedir.

But of course, that is orthogonal to the test-install mode. If we want to make the test more robust by emptying the templates, we should do that also for the test-freshly-baked mode, too.

Jeff King

2018-11-14 21:38 UTC #7 lore raw

Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

In reply to Johannes Schindelin

Quoted 13 lines · Johannes Schindelin

Quoted 3 lines
Makes perfect sense. Shouldn't we be asking where the template directory of the installed version is and using it instead of the freshly built one, no?
It would make sense, but we don't know how to get that information, do we?
$ git grep DEFAULT_GIT_TEMPLATE_DIR
Makefile:       -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR
builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
builtin/init-db.c:              template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \

Quoted 6 lines

In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the only user in the code is init-db.c which uses it in copy_templates().
And changing the code *now* to let us query Git where it thinks its templates should be won't work, as this patch is about using the installed Git (at whatever pre-compiled version that might be).

Do we actually care where the templates are? I thought the point was to override for the built Git to use the built templates instead of the installed one. For an installed Git, shouldn't we not be overriding the templates at all? I.e.:

if test -n "$GIT_TEST_INSTALLED"
then
"$GIT_TEST_INSTALLED/git" init
else

	"$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt"

fi >&3 2>&4

(That's all leaving aside the question of whether we ought to be using a clean template dir instead of this).

-Peff

Johannes Schindelin

2018-11-15 12:30 UTC #8 lore raw

Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

Hi Junio,

Quoted 31 lines · Jeff King

Quoted 13 lines · Johannes Schindelin
Quoted 3 lines
Makes perfect sense. Shouldn't we be asking where the template directory of the installed version is and using it instead of the freshly built one, no?
It would make sense, but we don't know how to get that information, do we?
$ git grep DEFAULT_GIT_TEMPLATE_DIR
Makefile:       -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR
builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
builtin/init-db.c:              template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \
Quoted 6 lines
In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the only user in the code is init-db.c which uses it in copy_templates().
And changing the code *now* to let us query Git where it thinks its templates should be won't work, as this patch is about using the installed Git (at whatever pre-compiled version that might be).
Do we actually care where the templates are? I thought the point was to override for the built Git to use the built templates instead of the installed one. For an installed Git, shouldn't we not be overriding the templates at all? I.e.:
if test -n "$GIT_TEST_INSTALLED"
then
"$GIT_TEST_INSTALLED/git" init
else

Quoted 5 lines

	"$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt"
fi >&3 2>&4
(That's all leaving aside the question of whether we ought to be using a clean template dir instead of this).

I fear that that might buy us a ton of trouble. Just like we override the system config, we should override the templates.

Ciao,
Dscho

Jeff King

2018-11-15 12:41 UTC #9 lore raw

Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories

Quoted 12 lines · Johannes Schindelin

Quoted 12 lines
Do we actually care where the templates are? I thought the point was to override for the built Git to use the built templates instead of the installed one. For an installed Git, shouldn't we not be overriding the templates at all? I.e.:
if test -n "$GIT_TEST_INSTALLED"
then
"$GIT_TEST_INSTALLED/git" init
else
	"$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt"
fi >&3 2>&4

Quoted 5 lines

Quoted 2 lines
(That's all leaving aside the question of whether we ought to be using a clean template dir instead of this).
I fear that that might buy us a ton of trouble. Just like we override the system config, we should override the templates.

Yes, it might. I guess it just seems plausible to me that somebody would expect GIT_TEST_INSTALLED to be as close to the installed experience as possible. I dunno. I do not use it myself.

At any rate, my point was that for GIT_TEST_INSTALLED, either:

1. We can use a known-clean set of templates (either our local templates/blt, or an even-cleaner empty set).

2. We do not need to specify any template, and it will just use whatever it came installed with.

And in either case, we do not have to worry about asking it "where are your templates?".

-Peff

Johannes Schindelin via GitGitGadget

author

2018-11-12 13:48 UTC #10 lore raw

PATCH 3/5 t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

It makes very, very little sense to test the built git-sh-i18n when the user asked specifically to test another one.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/lib-gettext.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

t/lib-gettext.sh +6 -1

index eec757f104..9eb160c997 100644
@@ -10,7 +10,12 @@ GIT_TEXTDOMAINDIR="$GIT_BUILD_DIR/po/build/locale"
 GIT_PO_PATH="$GIT_BUILD_DIR/po"
 export GIT_TEXTDOMAINDIR GIT_PO_PATH
 
-. "$GIT_BUILD_DIR"/git-sh-i18n
+if test -n "$GIT_TEST_INSTALLED"
+then
+	. "$(git --exec-path)"/git-sh-i18n
+else
+	. "$GIT_BUILD_DIR"/git-sh-i18n
+fi
 
 if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON
 then

Signature · 1 line

gitgitgadget

Junio C Hamano

2018-11-14 04:56 UTC #11 lore raw

Re: [PATCH 3/5] t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 7 lines · writes:

From: Johannes Schindelin <[email protected]>
It makes very, very little sense to test the built git-sh-i18n when the user asked specifically to test another one.
Signed-off-by Johannes Schindelin <[email protected]>
---

Yup. Makes sense.

Quoted 21 lines

 t/lib-gettext.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

t/lib-gettext.sh +6 -1

index eec757f104..9eb160c997 100644
@@ -10,7 +10,12 @@ GIT_TEXTDOMAINDIR="$GIT_BUILD_DIR/po/build/locale"
 GIT_PO_PATH="$GIT_BUILD_DIR/po"
 export GIT_TEXTDOMAINDIR GIT_PO_PATH
 
-. "$GIT_BUILD_DIR"/git-sh-i18n
+if test -n "$GIT_TEST_INSTALLED"
+then
+	. "$(git --exec-path)"/git-sh-i18n
+else
+	. "$GIT_BUILD_DIR"/git-sh-i18n
+fi
 
 if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON
 then

Johannes Schindelin via GitGitGadget

author

2018-11-12 13:48 UTC #12 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/test-lib.sh +1 -1

index 832ede5099..1ea20dc2dc 100644
@@ -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.'

Signature · 1 line

gitgitgadget

Junio C Hamano

2018-11-14 05:01 UTC #13 lore raw

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

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 5 lines · writes:

From: Johannes Schindelin <[email protected]>
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.

Quoted 18 lines

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/test-lib.sh +1 -1

index 832ede5099..1ea20dc2dc 100644
@@ -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

2018-11-14 13:20 UTC #14 lore raw

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

Hi Junio,

Quoted 11 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Johannes Schindelin <[email protected]>
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

Quoted 25 lines

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.
Quoted 18 lines
Signed-off-by Johannes Schindelin <[email protected]>
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
t/test-lib.sh +1 -1
index 832ede5099..1ea20dc2dc 100644
@@ -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

2018-11-14 12:52 UTC #15 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

t/test-lib.sh export LSAN_OPTIONS · line 57

Quoted 13 lines · Johannes Schindelin via GitGitGadget

t/test-lib.sh +1 -1

index 832ede5099..1ea20dc2dc 100644
@@ -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

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

2018-11-14 13:41 UTC #16 lore raw

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

Hi Peff,

Quoted 38 lines · Jeff King

t/test-lib.sh export LSAN_OPTIONS · line 57
Quoted 13 lines · Johannes Schindelin via GitGitGadget
t/test-lib.sh +1 -1
index 832ede5099..1ea20dc2dc 100644
@@ -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:

Quoted 6 lines

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

Quoted 3 lines

-Peff

Johannes Schindelin via GitGitGadget

author

2018-11-12 13:48 UTC #17 lore raw

PATCH 5/5 tests: explicitly use `git.exe` on Windows

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

In the bin-wrappers/* scripts, we already take pains to use git.exe rather than git, as this could pick up the wrong thing on Windows (i.e. if there exists a git file or directory in the build directory).

Now we do the same in the tests' start-up code.

This also helps when testing an installed Git, as there might be even more likely some stray file or directory in the way.

Note: the only way we can record whether the .exe suffix is by writing it to the GIT-BUILD-OPTIONS file and sourcing it at the beginning of t/test-lib.sh. This is not a requirement introduced by this patch, but we move the call to be able to use the $X variable that holds the file extension, if any.

Note also: the many, many calls to git this and git that are unaffected, as the regular PATH search will find the .exe files on Windows (and not be confused by a directory of the name git that is in one of the directories listed in the PATH variable), while /path/to/git would not, per se, know that it is looking for an executable and happily prefer such a directory.

Signed-off-by Johannes Schindelin <[email protected]>

---
 Makefile                |  1 +
 t/test-lib-functions.sh |  2 +-
 t/test-lib.sh           | 13 +++++++++----
 3 files changed, 11 insertions(+), 5 deletions(-)

Makefile +1 -0

index bbfbb4292d..5df0118ce9 100644
@@ -2590,6 +2590,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
+	@echo X=\'$(X)\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
 	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif

t/test-lib-functions.sh +1 -1

index 801cc9b2ef..c167b2e1af 100644
@@ -900,7 +900,7 @@ test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" init \
 			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled

t/test-lib.sh +9 -4

index 1ea20dc2dc..3e2a9ce76d 100644
@@ -49,18 +49,23 @@ export ASAN_OPTIONS
 : ${LSAN_OPTIONS=abort_on_error=1}
 export LSAN_OPTIONS
 
+if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+then
+	echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
+	exit 1
+fi
+. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+export PERL_PATH SHELL_PATH
+
 ################################################################
 # It appears that people try to run tests without building...
-test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
+test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git$X" >/dev/null ||
 if test $? != 1
 then
 	echo >&2 'error: you do not seem to have built git yet.'
 	exit 1
 fi
 
-. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-export PERL_PATH SHELL_PATH
-
 # if --tee was passed, write the output not only to the terminal, but
 # additionally to the file test-results/$BASENAME.out, too.
 case "$GIT_TEST_TEE_STARTED, $* " in

Signature · 1 line

gitgitgadget

Junio C Hamano

2018-11-14 05:14 UTC #18 lore raw

Re: [PATCH 5/5] tests: explicitly use `git.exe` on Windows

"Johannes Schindelin via GitGitGadget" <[email protected]>

Makefile GIT-BUILD-OPTIONS: FORCE · line 2593

Quoted 9 lines · writes:

Makefile +1 -0

index bbfbb4292d..5df0118ce9 100644
@@ -2590,6 +2590,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
+	@echo X=\'$(X)\' >>$@+

Made me wonder if a single letter $(X) is a bit too cute to expose to the outside world; as a narrowly confined local convention in this single Makefile, it was perfectly fine. But it should do for now. Its terseness is attractive, and our eyes (I do not speak for those new to our codebase and build structure) are already used to seeing $X suffix. Somebody may later come and complain but I am OK to rename it to something like $EXE at that time, not now.

t/test-lib-functions.sh test_create_repo () { · line 903

Quoted 13 lines

Makefile +0 -0

 ifdef TEST_OUTPUT_DIRECTORY
 	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif

t/test-lib-functions.sh +1 -1

index 801cc9b2ef..c167b2e1af 100644
@@ -900,7 +900,7 @@ test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" init \

Good.

Quoted 8 lines

t/test-lib-functions.sh +0 -0

 			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled

t/test-lib.sh +0 -0

index 1ea20dc2dc..3e2a9ce76d 100644

t/test-lib.sh export ASAN_OPTIONS · line 56

Quoted 8 lines

t/test-lib.sh +5 -0

@@ -49,18 +49,23 @@ export ASAN_OPTIONS
 : ${LSAN_OPTIONS=abort_on_error=1}
 export LSAN_OPTIONS
 
+if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+then
+	echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
+	exit 1
+fi

OK, this tells us that we at least attempted to build git once, even under test-installed mode, and hopefully people won't run $(MAKE) and immediately ^C it only to fool us by leaving this file while keeping git binary and t/helpers/ binary unbuilt.

t/test-lib.sh export ASAN_OPTIONS · line 62

Quoted 7 lines

t/test-lib.sh +4 -1

+. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+export PERL_PATH SHELL_PATH
+
 ################################################################
 # It appears that people try to run tests without building...
-test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
+test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git$X" >/dev/null ||

The latter half of this change is a good one. Given what the proposed log message of this patch says

which I read as "path-prefixed invocation, i.e. some/path/to/git, is bad, it must be spelled some/path/to/git.exe", I am surprised that tests ever worked on Windows without these five patches, as this line used to read like this:

	"$GIT_BUILD_DIR/git" >/dev/null
	if test $? != 1
	then
		...

Wouldn't "$GIT_BUILD_DIR/git" have failed (and "executable not found" hopefully won't produce exit code 1) and stopped the test suite from running even after you built git and not under the test-installed-git mode?

Quoted 12 lines

t/test-lib.sh +0 -3

 if test $? != 1
 then
 	echo >&2 'error: you do not seem to have built git yet.'
 	exit 1
 fi
 
-. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-export PERL_PATH SHELL_PATH
-
 # if --tee was passed, write the output not only to the terminal, but
 # additionally to the file test-results/$BASENAME.out, too.
 case "$GIT_TEST_TEE_STARTED, $* " in

Johannes Schindelin

2018-11-14 13:24 UTC #19 lore raw

Re: [PATCH 5/5] tests: explicitly use `git.exe` on Windows

Hi Junio,

Quoted 87 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>
Makefile GIT-BUILD-OPTIONS: FORCE · line 2593
Quoted 9 lines · writes:
Makefile +1 -0
index bbfbb4292d..5df0118ce9 100644
@@ -2590,6 +2590,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
+	@echo X=\'$(X)\' >>$@+
Made me wonder if a single letter $(X) is a bit too cute to expose to the outside world; as a narrowly confined local convention in this single Makefile, it was perfectly fine. But it should do for now. Its terseness is attractive, and our eyes (I do not speak for those new to our codebase and build structure) are already used to seeing $X suffix. Somebody may later come and complain but I am OK to rename it to something like $EXE at that time, not now.
t/test-lib-functions.sh test_create_repo () { · line 903
Quoted 13 lines
Makefile +0 -0
 ifdef TEST_OUTPUT_DIRECTORY
 	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif
t/test-lib-functions.sh +1 -1
index 801cc9b2ef..c167b2e1af 100644
@@ -900,7 +900,7 @@ test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" init \
Good.
Quoted 8 lines
t/test-lib-functions.sh +0 -0
 			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled
t/test-lib.sh +0 -0
index 1ea20dc2dc..3e2a9ce76d 100644
t/test-lib.sh export ASAN_OPTIONS · line 56
Quoted 8 lines
t/test-lib.sh +5 -0
@@ -49,18 +49,23 @@ export ASAN_OPTIONS
 : ${LSAN_OPTIONS=abort_on_error=1}
 export LSAN_OPTIONS
 
+if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+then
+	echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
+	exit 1
+fi
OK, this tells us that we at least attempted to build git once, even under test-installed mode, and hopefully people won't run $(MAKE) and immediately ^C it only to fool us by leaving this file while keeping git binary and t/helpers/ binary unbuilt.
t/test-lib.sh export ASAN_OPTIONS · line 62
Quoted 7 lines
t/test-lib.sh +4 -1
+. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+export PERL_PATH SHELL_PATH
+
 ################################################################
 # It appears that people try to run tests without building...
-test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
+test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git$X" >/dev/null ||
The latter half of this change is a good one. Given what the proposed log message of this patch says
Note also: the many, many calls to git this and git that are unaffected, as the regular PATH search will find the .exe files on Windows (and not be confused by a directory of the name git that is in one of the directories listed in the PATH variable), while /path/to/git would not, per se, know that it is looking for an executable and happily prefer such a directory.
which I read as "path-prefixed invocation, i.e. some/path/to/git, is bad, it must be spelled some/path/to/git.exe", I am surprised that tests ever worked on Windows without these five patches, as this line used to read like this:
	"$GIT_BUILD_DIR/git" >/dev/null
	if test $? != 1
	then
		...

Quoted 4 lines

Wouldn't "$GIT_BUILD_DIR/git" have failed (and "executable not found" hopefully won't produce exit code 1) and stopped the test suite from running even after you built git and not under the test-installed-git mode?

"$GIT_BUILD_DIR/git" did not fail until I regularly built with Visual Studio (and my Visual Studio project generator generates a directory named "git" to live alongside "git.exe").

And when it failed, I patched Git for Windows. Fast-forward, years later I managed to contribute the patch we are discussing right now ;-)

So yes, it is primarily a concern when testing Git in specific setups where a "git" directory can live next to the "git.exe" that we want to test. Not necessarily a big deal for most developers on Windows.

Ciao,
Dscho

Quoted 14 lines

Quoted 13 lines

t/test-lib.sh +0 -3

 if test $? != 1
 then
 	echo >&2 'error: you do not seem to have built git yet.'
 	exit 1
 fi
 
-. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-export PERL_PATH SHELL_PATH
-
 # if --tee was passed, write the output not only to the terminal, but
 # additionally to the file test-results/$BASENAME.out, too.
 case "$GIT_TEST_TEE_STARTED, $* " in

Junio C Hamano

2018-11-14 14:47 UTC #20 lore raw

Re: [PATCH 5/5] tests: explicitly use `git.exe` on Windows

Quoted 25 lines · Johannes Schindelin

Quoted 20 lines
The latter half of this change is a good one. Given what the proposed log message of this patch says
Note also: the many, many calls to git this and git that are unaffected, as the regular PATH search will find the .exe files on Windows (and not be confused by a directory of the name git that is in one of the directories listed in the PATH variable), while /path/to/git would not, per se, know that it is looking for an executable and happily prefer such a directory.
which I read as "path-prefixed invocation, i.e. some/path/to/git, is bad, it must be spelled some/path/to/git.exe", I am surprised that tests ever worked on Windows without these five patches, as this line used to read like this:
	"$GIT_BUILD_DIR/git" >/dev/null
	if test $? != 1
	then
		...
Quoted 4 lines
Wouldn't "$GIT_BUILD_DIR/git" have failed (and "executable not found" hopefully won't produce exit code 1) and stopped the test suite from running even after you built git and not under the test-installed-git mode?

Quoted 6 lines

"$GIT_BUILD_DIR/git" did not fail until I regularly built with Visual Studio (and my Visual Studio project generator generates a directory named "git" to live alongside "git.exe").
And when it failed, I patched Git for Windows. Fast-forward, years later I managed to contribute the patch we are discussing right now ;-)

OK, I still cannot read it out of what you wrote in the proposed log message without aid, but in essense the crux of the problem is that invoking "some/path/to/git" finds "some/path/to/git.exe" only when there is no "some/path/to/git" directory at the same time, and if that directory exists, tries and fails to execute that directory, and the change in this patch protects us from that problem.

Did I get it right? I'd really prefer to see it more clearly written in the log message so the next person who reads "git log" do not have to ask the same question to you.

Assuming that I read it correctly, I think this patch as a whole makes tons of sense as a change to make the invocation more robust.

Thanks.

Johannes Schindelin via GitGitGadget

author

2018-11-14 16:32 UTC #21 lore raw

v2 cover tests: various improvements to the GIT_TEST_INSTALLED feature

In reply to Johannes Schindelin via GitGitGadget

The only thing this needs that is unlikely to be installed is the test helper(s).

Triggered by a comment of AEvar
[https://public-inbox.org/git/[email protected]/], I hereby contribute these assorted fixes for the GIT_TEST_INSTALLED feature.

Changes since v1:

* Now we verify in test-lib.sh also in the GIT_TEST_INSTALLED case whether the Git executable is working (thanks, Peff!).
* The commit message of 5/5 was touched up.

 Makefile                |  1 +
 t/lib-gettext.sh        |  7 ++++++-
 t/test-lib-functions.sh |  3 ++-
 t/test-lib.sh           | 22 ++++++++++++++++------
 4 files changed, 25 insertions(+), 8 deletions(-)

base-commit: d166e6afe5f257217836ef24a73764eba390c58d
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-73%2Fdscho%2Ftest-git-installed-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-73/dscho/test-git-installed-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/73

Range-diff vs v1:

 1:  2b04f9f086 = 1:  3b68e0fe8a tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/
 2:  948b3dc146 = 2:  80d50d5932 tests: respect GIT_TEST_INSTALLED when initializing repositories
 3:  eddea552e4 = 3:  49e408677a t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set
 4:  316e215e54 < -:  ---------- tests: do not require Git to be built when testing an installed Git
 -:  ---------- > 4:  b801dc8027 tests: do not require Git to be built when testing an installed Git
 5:  cd314e1384 ! 5:  fbdb659de6 tests: explicitly use `git.exe` on Windows

@@ -2,6 +2,17 @@

tests: explicitly use git.exe on Windows

+ On Windows, when we refer to /an/absolute/path/to/git, it magically
+ resolves git.exe at that location. Except if something of the name
+ git exists next to that git.exe. So if we call $BUILD_DIR/git, it
+ will find $BUILD_DIR/git.exe *only* if there is not, say, a directory
+ called $BUILD_DIR/git.
+
+ Such a directory, however, exists in Git for Windows when building with
+ Visual Studio (our Visual Studio project generator defaults to putting
+ the build files into a directory whose name is the base name of the
+ corresponding .exe).
+
In the bin-wrappers/* scripts, we already take pains to use git.exe rather than git, as this could pick up the wrong thing on Windows (i.e. if there exists a git file or directory in the build directory).
@@ -68,11 +79,12 @@
+
################################################################
# It appears that people try to run tests without building...
--test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null || -+test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git$X" >/dev/null || +-"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git" >/dev/null ++"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
if test $? != 1
then
- echo >&2 'error: you do not seem to have built git yet.'
+ if test -n "$GIT_TEST_INSTALLED"
+@@
exit 1
fi

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2018-11-14 16:32 UTC #22 lore raw

PATCH v2 1/5 tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/

From: Johannes Schindelin <[email protected]>

We really need to be able to find the test helpers... Really. This change was forgotten when we moved the test helpers into t/helper/

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/test-lib.sh +1 -1

index aba66cafa2..93883580a8 100644
@@ -957,7 +957,7 @@ elif test -n "$GIT_TEST_INSTALLED"
 then
 	GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path)  ||
 	error "Cannot run git from $GIT_TEST_INSTALLED."
-	PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR:$PATH
+	PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH
 	GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
 else # normal case, use ../bin-wrappers only unless $with_dashes:
 	git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2018-11-14 16:32 UTC #23 lore raw

PATCH v2 2/5 tests: respect GIT_TEST_INSTALLED when initializing repositories

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

It really makes very, very little sense to use a different git executable than the one the caller indicated via setting the environment variable GIT_TEST_INSTALLED.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/test-lib-functions.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

t/test-lib-functions.sh +2 -1

index d158c8d0bf..3472716651 100644
@@ -923,7 +923,8 @@ test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"$GIT_EXEC_PATH/git-init" "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled
 	) || exit

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2018-11-14 16:32 UTC #24 lore raw

PATCH v2 3/5 t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

It makes very, very little sense to test the built git-sh-i18n when the user asked specifically to test another one.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/lib-gettext.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

t/lib-gettext.sh +6 -1

index eec757f104..9eb160c997 100644
@@ -10,7 +10,12 @@ GIT_TEXTDOMAINDIR="$GIT_BUILD_DIR/po/build/locale"
 GIT_PO_PATH="$GIT_BUILD_DIR/po"
 export GIT_TEXTDOMAINDIR GIT_PO_PATH
 
-. "$GIT_BUILD_DIR"/git-sh-i18n
+if test -n "$GIT_TEST_INSTALLED"
+then
+	. "$(git --exec-path)"/git-sh-i18n
+else
+	. "$GIT_BUILD_DIR"/git-sh-i18n
+fi
 
 if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON
 then

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2018-11-14 16:32 UTC #25 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

We really only need the test helpers to be built in the worktree in that case, but that is not what we test for.

On the other hand it is a perfect opportunity to verify that GIT_TEST_INSTALLED points to a working Git.

So let's test the appropriate Git executable. While at it, also adjust the error message in the GIT_TEST_INSTALLED case.

This patch is best viewed with -w --patience.

Helped-by Jeff King <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/test-lib.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

t/test-lib.sh +7 -2

index 93883580a8..3d3a65ed0e 100644
@@ -51,10 +51,15 @@ export LSAN_OPTIONS
 
 ################################################################
 # It appears that people try to run tests without building...
-"$GIT_BUILD_DIR/git" >/dev/null
+"${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.'
+	if test -n "$GIT_TEST_INSTALLED"
+	then
+		echo >&2 "error: there is no working Git at '$GIT_TEST_INSTALLED'"
+	else
+		echo >&2 'error: you do not seem to have built git yet.'
+	fi
 	exit 1
 fi

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2018-11-14 16:32 UTC #26 lore raw

PATCH v2 5/5 tests: explicitly use `git.exe` on Windows

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

On Windows, when we refer to /an/absolute/path/to/git, it magically resolves git.exe at that location. Except if something of the name git exists next to that git.exe. So if we call $BUILD_DIR/git, it will find $BUILD_DIR/git.exe *only* if there is not, say, a directory called $BUILD_DIR/git.

Such a directory, however, exists in Git for Windows when building with Visual Studio (our Visual Studio project generator defaults to putting the build files into a directory whose name is the base name of the corresponding .exe).

Now we do the same in the tests' start-up code.

This also helps when testing an installed Git, as there might be even more likely some stray file or directory in the way.

Signed-off-by Johannes Schindelin <[email protected]>

---
 Makefile                |  1 +
 t/test-lib-functions.sh |  2 +-
 t/test-lib.sh           | 13 +++++++++----
 3 files changed, 11 insertions(+), 5 deletions(-)

Makefile +1 -0

index 016fdcdb81..21b3978744 100644
@@ -2591,6 +2591,7 @@ GIT-BUILD-OPTIONS: FORCE
 	@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
+	@echo X=\'$(X)\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
 	@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif

t/test-lib-functions.sh +1 -1

index 3472716651..274cbc2d6e 100644
@@ -923,7 +923,7 @@ test_create_repo () {
 	mkdir -p "$repo"
 	(
 		cd "$repo" || error "Cannot setup test environment"
-		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+		"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" init \
 			"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
 		error "cannot run git init -- have you built things yet?"
 		mv .git/hooks .git/hooks-disabled

t/test-lib.sh +9 -4

index 3d3a65ed0e..e12addc324 100644
@@ -49,9 +49,17 @@ export ASAN_OPTIONS
 : ${LSAN_OPTIONS=abort_on_error=1}
 export LSAN_OPTIONS
 
+if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+then
+	echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
+	exit 1
+fi
+. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+export PERL_PATH SHELL_PATH
+
 ################################################################
 # It appears that people try to run tests without building...
-"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git" >/dev/null
+"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
 if test $? != 1
 then
 	if test -n "$GIT_TEST_INSTALLED"

@@ -63,9 +71,6 @@ then
 	exit 1
 fi
 
-. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-export PERL_PATH SHELL_PATH
-
 # if --tee was passed, write the output not only to the terminal, but
 # additionally to the file test-results/$BASENAME.out, too.
 case "$GIT_TEST_TEE_STARTED, $* " in

Signature · 1 line

gitgitgadget