Re: Re* [PATCH] t4014: fix call to `test_expect_success ()`
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 25, 2026, 07:07 UTC
- 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:
Show 78 quoted lines
> 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.
Show 5 quoted lines
> 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