Re* [PATCH] t4014: fix call to `test_expect_success ()`

[Junio C Hamano <gitster@pobox.com>]

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.

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'll stop at marking this #leftoverbits but it probably is not for
microproject.


---- >8 ----
Subject: [PATCH] t4032: make test "set -e" clean

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 few shell construct used in this test were not ready.  Make them
so.

 * "git config --unset VAR" can fail when VAR is not defined.

 * The author of "test -f X && run test that uses X" written here
   really wanted to say "if file X is there, then run the test", not
   "file X must exist and the test using it must succeed".  The
   proper way to express it is to say "test ! -f X || use X".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4032-diff-inter-hunk-context.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git c/t/t4032-diff-inter-hunk-context.sh w/t/t4032-diff-inter-hunk-context.sh
index bada0cbd32..efcd863126 100755
--- c/t/t4032-diff-inter-hunk-context.sh
+++ w/t/t4032-diff-inter-hunk-context.sh
@@ -17,7 +17,7 @@ f() {
 
 t() {
 	use_config=
-	git config --unset diff.interHunkContext
+	git config --unset diff.interHunkContext || :
 
 	case $# in
 	4) hunks=$4; cmd="diff -U$3";;
@@ -40,7 +40,7 @@ t() {
 		test $(git $cmd $file | grep '^@@ ' | wc -l) = $hunks
 	"
 
-	test -f $expected &&
+	test ! -f $expected ||
 	test_expect_success "$label: check output" "
 		git $cmd $file | grep -v '^index ' >actual &&
 		test_cmp $expected actual