* [PATCH] t6002: make test "set -e" clean
2026-03-24 17:13 ` Re* " Junio C Hamano
@ 2026-03-24 18:05 ` Junio C Hamano
2026-03-24 18:13 ` [PATCH] test-lib: catch misspelt 'test_expect_successo' Junio C Hamano
` (3 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2026-03-24 18:05 UTC (permalink / raw)
To: git
In order to catch mistakes like misspelling "test_expect_success",
we would like to eventually be able to run our test suite with the
"-e" option on.
We often use
val=$(expr expression)
only for the computation, and it is good that "expr" exits non-zero
with syntactically invalid expression (it exits with 2) and other
errors (with 3).
"expr" however also exits with "1" if it yields 0 or null X-<.
Make sure we do not fail unnecessarily under "set -e".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* It was fun to figure this one out.
t/t6002-rev-list-bisect.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git i/t/t6002-rev-list-bisect.sh w/t/t6002-rev-list-bisect.sh
index daa009c9a1..1bd720d240 100755
--- i/t/t6002-rev-list-bisect.sh
+++ w/t/t6002-rev-list-bisect.sh
@@ -27,9 +27,9 @@ test_bisection_diff()
# Test if bisection size is close to half of list size within
# tolerance.
#
- _bisect_err=$(expr $_list_size - $_bisection_size \* 2)
+ _bisect_err=$(expr $_list_size - $_bisection_size \* 2) && test $? -le 1
test "$_bisect_err" -lt 0 && _bisect_err=$(expr 0 - $_bisect_err)
- _bisect_err=$(expr $_bisect_err / 2) ; # floor
+ _bisect_err=$(expr $_bisect_err / 2) && test $? -le 1; # floor
test_expect_success \
"bisection diff $_bisect_option $_head $* <= $_max_diff" \
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH] test-lib: catch misspelt 'test_expect_successo'
2026-03-24 17:13 ` Re* " Junio C Hamano
2026-03-24 18:05 ` [PATCH] t6002: make test "set -e" clean Junio C Hamano
@ 2026-03-24 18:13 ` Junio C Hamano
2026-03-24 19:35 ` Jeff King
2026-03-24 18:20 ` [PATCH] t0008: make test "set -e" clean Junio C Hamano
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2026-03-24 18:13 UTC (permalink / raw)
To: git
In order to catch mistakes like misspelling "test_expect_success",
we would like to eventually be able to run our test suite with the
"-e" option on.
All tests dot-source "test-lib.sh" as the first thing to do.
Starting the script with "set -e" immediately reveals one place in
the test framework itself that is not clean.
The test framework runs "$GIT_BINARY" without any argument. We
expect it to exit with status 1 (i.e., "git<RETURN>" that spits out
the list of common commands) as a sign that we have an instance of
Git that we want to test. We cannot quite say
git
if test $? != 1; then you have not built git; fi
as the first invocation that exits non-zero is caught with "set -e".
Work this around by rewriting the construct like so:
status=0; git || status=$?
if test $status != 1; then you have not built git; fi
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* As we look into other breakages, we may discover more breakages
in the test framework that need to be fixed, but this change
alone seems to get thing going for many test scripts.
t/test-lib.sh | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git i/t/test-lib.sh w/t/test-lib.sh
index 70fd3e9baf..a2aa97fba3 100644
--- i/t/test-lib.sh
+++ w/t/test-lib.sh
@@ -17,6 +17,9 @@
# Test the binaries we have just built. The tests are kept in
# t/ subdirectory and are run in 'trash directory' subdirectory.
+
+set -e
+
if test -z "$TEST_DIRECTORY"
then
# ensure that TEST_DIRECTORY is an absolute path so that it
@@ -143,8 +146,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
################################################################
# It appears that people try to run tests without building...
GIT_BINARY="${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X"
-"$GIT_BINARY" >/dev/null
-if test $? != 1
+status=0 && "$GIT_BINARY" >/dev/null || status=$?
+if test $status != 1
then
if test -n "$GIT_TEST_INSTALLED"
then
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH] test-lib: catch misspelt 'test_expect_successo'
2026-03-24 18:13 ` [PATCH] test-lib: catch misspelt 'test_expect_successo' Junio C Hamano
@ 2026-03-24 19:35 ` Jeff King
2026-03-24 19:48 ` Junio C Hamano
0 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2026-03-24 19:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Tue, Mar 24, 2026 at 11:13:26AM -0700, Junio C Hamano wrote:
> In order to catch mistakes like misspelling "test_expect_success",
> we would like to eventually be able to run our test suite with the
> "-e" option on.
Using "-e" makes me very nervous, given all of its quirks. Granted, most
of them are related to it _not_ kicking in when you'd want it to, but I
worry it will create false positive/negative headaches.
In the past I've caught errors outside of the test snippet by noticing
cruft on stderr. This is especially obvious if you use "prove", which
captures stdout and gives a nice display (which the extra stderr then
makes uglier).
I wonder if we could automate / formalize that. If we do this hacky
patch on master:
diff --git a/t/Makefile b/t/Makefile
index ab8a5b54aa..f57180cc7b 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -79,7 +79,7 @@ prove: pre-clean $(TEST_LINT)
$(MAKE) clean-except-prove-cache
$(T):
- @echo "*** $@ ***"; '$(TEST_SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+ echo "*** $@ ***"; '$(TEST_SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS) 2>$@.stderr
$(UNIT_TESTS):
@echo "*** $@ ***"; $@
then:
cd t
make test
for i in *.stderr; do test -s $i && echo $i; done
catches the problem in t4014 and nothing else. Note that it _doesn't_
work with --verbose-log, though, as that redirects stderr to stdout
(which is going to the log). It might be possible to do something
cleaner and more clever within test-lib.sh, though.
-Peff
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] test-lib: catch misspelt 'test_expect_successo'
2026-03-24 19:35 ` Jeff King
@ 2026-03-24 19:48 ` Junio C Hamano
2026-03-25 5:46 ` Jeff King
0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2026-03-24 19:48 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> On Tue, Mar 24, 2026 at 11:13:26AM -0700, Junio C Hamano wrote:
>
>> In order to catch mistakes like misspelling "test_expect_success",
>> we would like to eventually be able to run our test suite with the
>> "-e" option on.
>
> Using "-e" makes me very nervous, given all of its quirks. Granted, most
> of them are related to it _not_ kicking in when you'd want it to, but I
> worry it will create false positive/negative headaches.
After looking at a few scripts, I am not suffering from such
headaches yet; it does not look too bad. I'll stop this effort for
now, but with a handful of patches I already sent, more than 80-90%
of the entire test scripts that I run are now "set -e" clean, I
think. Note that I do not run svn, cvs, or p4 tests ;-)
> In the past I've caught errors outside of the test snippet by noticing
> cruft on stderr. This is especially obvious if you use "prove", which
> captures stdout and gives a nice display (which the extra stderr then
> makes uglier).
> I wonder if we could automate / formalize that.
That's a thought.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] test-lib: catch misspelt 'test_expect_successo'
2026-03-24 19:48 ` Junio C Hamano
@ 2026-03-25 5:46 ` Jeff King
0 siblings, 0 replies; 16+ messages in thread
From: Jeff King @ 2026-03-25 5:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Tue, Mar 24, 2026 at 12:48:24PM -0700, Junio C Hamano wrote:
> > Using "-e" makes me very nervous, given all of its quirks. Granted, most
> > of them are related to it _not_ kicking in when you'd want it to, but I
> > worry it will create false positive/negative headaches.
>
> After looking at a few scripts, I am not suffering from such
> headaches yet; it does not look too bad. I'll stop this effort for
> now, but with a handful of patches I already sent, more than 80-90%
> of the entire test scripts that I run are now "set -e" clean, I
> think. Note that I do not run svn, cvs, or p4 tests ;-)
Clean in the sense that you don't _notice_ any problems. But there may
be lurking ones. For example, given this:
set -e
foo() {
false
echo foo
}
what would you expect the output to be for:
echo before &&
foo &&
echo after
versus:
echo before &&
foo
Whether that "false" triggers "-e" depends on where in the &&-chain the
call to the containing function is. So things that are not problems now
may suddenly become ones when far-away code is changed.
Maybe it's enough that people would notice and debug them when they
happen (if "set -e" is in test-lib.sh), and they wouldn't come up all
that much. I dunno. I just have been bitten enough by "-e" quirks that
I'm wary.
-Peff
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] t0008: make test "set -e" clean
2026-03-24 17:13 ` Re* " Junio C Hamano
2026-03-24 18:05 ` [PATCH] t6002: make test "set -e" clean Junio C Hamano
2026-03-24 18:13 ` [PATCH] test-lib: catch misspelt 'test_expect_successo' Junio C Hamano
@ 2026-03-24 18:20 ` Junio C Hamano
2026-03-24 18:32 ` [PATCH] t7450: " Junio C Hamano
2026-03-25 7:07 ` Re* [PATCH] t4014: fix call to `test_expect_success ()` Patrick Steinhardt
4 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2026-03-24 18:20 UTC (permalink / raw)
To: git
In order to catch mistakes like misspelling "test_expect_success",
we would like to eventually be able to run our test suite with the
"-e" option on.
A piece of script used "grep" to filter out its input purely for its
output, but of course, "grep" reports with its exit value when it
did not see any hits, which didn't mesh quite well with "set -e".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t0008-ignores.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git i/t/t0008-ignores.sh w/t/t0008-ignores.sh
index db8bde280e..8edb08d9c2 100755
--- i/t/t0008-ignores.sh
+++ w/t/t0008-ignores.sh
@@ -122,7 +122,7 @@ test_expect_success_multiple () {
fi
testname="$1" expect_all="$2" code="$3"
- expect_verbose=$( echo "$expect_all" | grep -v '^:: ' )
+ expect_verbose=$( echo "$expect_all" | grep -v '^:: ' ) || :
expect=$( echo "$expect_verbose" | sed -e 's/.* //' )
test_expect_success $prereq "$testname${no_index_opt:+ with $no_index_opt}" '
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH] t7450: make test "set -e" clean
2026-03-24 17:13 ` Re* " Junio C Hamano
` (2 preceding siblings ...)
2026-03-24 18:20 ` [PATCH] t0008: make test "set -e" clean Junio C Hamano
@ 2026-03-24 18:32 ` Junio C Hamano
2026-03-24 18:38 ` Eric Sunshine
2026-03-25 7:07 ` Re* [PATCH] t4014: fix call to `test_expect_success ()` Patrick Steinhardt
4 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2026-03-24 18:32 UTC (permalink / raw)
To: git
In order to catch mistakes like misspelling "test_expect_success",
we would like to eventually be able to run our test suite with the
"-e" option on.
Often we write "A && test_expect_success ..." and want it to mean
"If and only if A holds true, this needs to be tested", but under
"set -e", this will cause failure when A does not hold true. We
need to write "!A || test_expect_success ..." if we want to run the
test conditionally.
Or write it properly with if/then/fi, perhaps like:
if ! A
then
test_expect_success ...
fi
Make sure we do not fail unnecessarily under "set -e".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t7450-bad-git-dotfiles.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git i/t/t7450-bad-git-dotfiles.sh w/t/t7450-bad-git-dotfiles.sh
index f512eed278..047e4085d7 100755
--- i/t/t7450-bad-git-dotfiles.sh
+++ w/t/t7450-bad-git-dotfiles.sh
@@ -220,7 +220,7 @@ check_dotx_symlink () {
)
'
- test -n "$refuse_index" &&
+ test -z "$refuse_index" ||
test_expect_success "refuse to load symlinked $name into index ($type)" '
test_must_fail \
git -C $dir \
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH] t7450: make test "set -e" clean
2026-03-24 18:32 ` [PATCH] t7450: " Junio C Hamano
@ 2026-03-24 18:38 ` Eric Sunshine
2026-03-24 19:03 ` Junio C Hamano
0 siblings, 1 reply; 16+ messages in thread
From: Eric Sunshine @ 2026-03-24 18:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Tue, Mar 24, 2026 at 2:32 PM Junio C Hamano <gitster@pobox.com> wrote:
> In order to catch mistakes like misspelling "test_expect_success",
> we would like to eventually be able to run our test suite with the
> "-e" option on.
>
> Often we write "A && test_expect_success ..." and want it to mean
> "If and only if A holds true, this needs to be tested", but under
> "set -e", this will cause failure when A does not hold true. We
> need to write "!A || test_expect_success ..." if we want to run the
> test conditionally.
>
> Or write it properly with if/then/fi, perhaps like:
>
> if ! A
> then
> test_expect_success ...
> fi
>
> Make sure we do not fail unnecessarily under "set -e".
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git i/t/t7450-bad-git-dotfiles.sh w/t/t7450-bad-git-dotfiles.sh
> @@ -220,7 +220,7 @@ check_dotx_symlink () {
> - test -n "$refuse_index" &&
> + test -z "$refuse_index" ||
> test_expect_success "refuse to load symlinked $name into index ($type)" '
> test_must_fail \
> git -C $dir \
I suppose this is the absolute minimum change to make this work, but
typically we would handle this sort of case by defining a PREREQ,
wouldn't we? Using a PREREQ would also set a better example for those
new to the codebase.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] t7450: make test "set -e" clean
2026-03-24 18:38 ` Eric Sunshine
@ 2026-03-24 19:03 ` Junio C Hamano
0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2026-03-24 19:03 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Tue, Mar 24, 2026 at 2:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>> In order to catch mistakes like misspelling "test_expect_success",
>> we would like to eventually be able to run our test suite with the
>> "-e" option on.
>>
>> Often we write "A && test_expect_success ..." and want it to mean
>> "If and only if A holds true, this needs to be tested", but under
>> "set -e", this will cause failure when A does not hold true. We
>> need to write "!A || test_expect_success ..." if we want to run the
>> test conditionally.
>>
>> Or write it properly with if/then/fi, perhaps like:
>>
>> if ! A
>> then
>> test_expect_success ...
>> fi
>>
>> Make sure we do not fail unnecessarily under "set -e".
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> diff --git i/t/t7450-bad-git-dotfiles.sh w/t/t7450-bad-git-dotfiles.sh
>> @@ -220,7 +220,7 @@ check_dotx_symlink () {
>> - test -n "$refuse_index" &&
>> + test -z "$refuse_index" ||
>> test_expect_success "refuse to load symlinked $name into index ($type)" '
>> test_must_fail \
>> git -C $dir \
>
> I suppose this is the absolute minimum change to make this work, but
> typically we would handle this sort of case by defining a PREREQ,
> wouldn't we? Using a PREREQ would also set a better example for those
> new to the codebase.
In some situations, maybe, but I do not think this one is a good fit
for a prerequisite, whose typical pattern is "let's see what we have
in the executing platform environment just once, and act accordingly".
This is a "the outside helper function is repeatedly called, and the
caller may or may not call it with an option, depending on which
this extra test may or may not make sense to run, so run this one
conditionally".
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re* [PATCH] t4014: fix call to `test_expect_success ()`
2026-03-24 17:13 ` Re* " Junio C Hamano
` (3 preceding siblings ...)
2026-03-24 18:32 ` [PATCH] t7450: " Junio C Hamano
@ 2026-03-25 7:07 ` Patrick Steinhardt
4 siblings, 0 replies; 16+ messages in thread
From: Patrick Steinhardt @ 2026-03-25 7:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Mirko Faina
On Tue, Mar 24, 2026 at 10:13:09AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I was wondering if we can make the test framework better so that a
> > misspelt test_expect_success would cause a louder failure than what
> > we have now, which is something like:
> >
> > ...
> > ok 5 - check hash-object
> >
> > t0002-gitfile.sh: line 46: test_expect_successo: command not found
> > expecting success of 0002.6 'check update-index':
> > test_path_is_missing "$REAL/index" &&
> > ...
> > ok 13 - enter_repo strict mode
> >
> > # passed all 13 test(s)
> > 1..13
> >
> > when I corrupt the 6th test of a random script.
> >
> > diff --git i/t/t0002-gitfile.sh w/t/t0002-gitfile.sh
> > index dfbcdddbcc..d65f664914 100755
> > --- i/t/t0002-gitfile.sh
> > +++ w/t/t0002-gitfile.sh
> > @@ -43,7 +43,7 @@ test_expect_success 'check hash-object' '
> > test_path_is_file "$REAL/objects/$(objpath $SHA)"
> > '
> >
> > -test_expect_success 'check cat-file' '
> > +test_expect_successo 'check cat-file' '
> > git cat-file blob $SHA >actual &&
> > test_cmp bar actual
> > '
> >
> > There is no indication of something bad happened, other than
> > "command not found" and 13 tests passed instead of 14 the script
> > has, which nobody knows.
> >
> > So, no, it hardly is your fault.
> >
> > I wonder if the test framework is safe to run with "set -e".
>
> It turns out that the test framework itself is not so clean. If I
> add "set -e" near the beginning of <t/test-lib.sh>, the first
> roadblock we hit is this one:
>
> # It appears that people try to run tests without building...
> GIT_BINARY="${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X"
> "$GIT_BINARY" >/dev/null
> if test $? != 1
> then
> ... complain that you haven't built and ...
> exit 1
> fi
>
> With "set -e", "$GIT_BINARY" we expect to exit with status 1 (i.e.,
> "git<RETURN>" that spits out the list of common commands) as a sign
> that we have an instance of Git that we want to test is not even
> allowed to do so.
>
> I did this single liner at the end of <t/test-lib.sh>
>
> t/test-lib.sh | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git c/t/test-lib.sh w/t/test-lib.sh
> index 70fd3e9baf..4a80933487 100644
> --- c/t/test-lib.sh
> +++ w/t/test-lib.sh
> @@ -1971,3 +1971,5 @@ test_lazy_prereq FSMONITOR_DAEMON '
> git version --build-options >output &&
> grep "feature: fsmonitor--daemon" output
> '
> +
> +set -e
>
> and started running "make test". I see some failures I haven't yet
> looked into, but it seems promising.
Yeah, I was playing around with the same idea yesterday, but got pulled
into some meetings and thus couldn't finish that work.
> Fixing all may involve finding and fixing little things like the
> attached patch. I am not sure if this would be a good microproject
> canidate for the next year. There are a handful of them that
> multiple students can work on independently, but some of them
> require familiarity with the test framework and shell scripting.
I think it's overall not that bad, and I've got something that's almost
done. It's an easy win for students indeed, but in this case I'd rather
make our test suite a bit more robust sooner rather than later :)
I'll likely have something later today. Thanks!
Patrick
^ permalink raw reply [flat|nested] 16+ messages in thread