From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 25 Mar 2026 07:07:04 GMT
Subject: Re: Re* [PATCH] t4014: fix call to `test_expect_success ()`
Message-ID: <acOJmBluqb5SvjpW@pks.im>
In-Reply-To: <xmqqcy0t178a.fsf_-_@gitster.g>

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