diff mbox series

[10/10] test-lib: return 1 from test_expect_{success,failure}

Message ID 20210228195414.21372-11-avarab@gmail.com (mailing list archive)
State New
Headers show
Series describe: dont abort too early when searching tags | expand

Commit Message

Ævar Arnfjörð Bjarmason Feb. 28, 2021, 7:54 p.m. UTC
An earlier commit fixed an issue in "check_describe" with
"test_expect_success" being called within another
"test_expect_success", causing the test to succeed even if it should
fail.

Let's try to guard against this in the test library by returning 1
from these two functions. This change would have caught the issue I've
now fixed in the "check_describe" function.

I could equivalently add this "return 1" to the "test_finish_"
function itself, but I think doing it here is more readable.

Because of this change any tests which ran under "set -e" needed to be
refactored not to use "set -e". Luckily there were only two such
tests, earlier commits did that refactoring.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 2 ++
 1 file changed, 2 insertions(+)

Comments

Junio C Hamano March 1, 2021, 9:43 p.m. UTC | #1
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index c6cdabf53e..3dd68091bb 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -636,6 +636,7 @@ test_expect_failure () {
>  		fi
>  	fi
>  	test_finish_
> +	return 1
>  }
>  
>  test_expect_success () {
> @@ -656,6 +657,7 @@ test_expect_success () {
>  		fi
>  	fi
>  	test_finish_
> +	return 1
>  }
>  
>  # test_external runs external test scripts that provide continuous

Hmph.

This does not catch if the outer expect_success does not catch a
failure in the inner expect_success and signal a failure.

When I asked if this kind of breakage is an easy mistake to catch by
the test lint, I had something along this in mind:

	test_expect_success () {
		if test -n "$GIT_IN_TEST_EXPECT"
		then
			BUG caling "$GIT_IN_TEST_EXPECT" inside test_expect_success
		fi
		GIT_IN_TEST_EXPECT=test_expect_success

		... do the 'eval the given test body' thing ..

		GIT_IN_TEST_EXPECT=
	}

After all, the error is in the outer expect_success in that it
called another one, so it feels more natural that the called inner
expect_success to notice the situation and barf.

Thanks.
diff mbox series

Patch

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index c6cdabf53e..3dd68091bb 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -636,6 +636,7 @@  test_expect_failure () {
 		fi
 	fi
 	test_finish_
+	return 1
 }
 
 test_expect_success () {
@@ -656,6 +657,7 @@  test_expect_success () {
 		fi
 	fi
 	test_finish_
+	return 1
 }
 
 # test_external runs external test scripts that provide continuous