[PATCH 0/2] test-lib: improve missing prereq handling

The ssh signing feature was breaking tests when the broken openssh-8.7
was used. We have now fixed that by checking for this exact case in the
GPGSSH prereq and I will improve that check further in a future patch.
However we are now in a situation where a broken openssh in the future
will result in successfull tests but not a working git build afterwards
(either not compiling in the expected feature or like in the ssh case
runtime failures) resulting in a false sense of security in the tests.
This patches try to improve this situation by showing which prereqs
failed in the test summary and by adding an environment variable to
enforce certain prereqs to succeed or abort the test otherwise.

See also:
https://public-inbox.org/git/xmqqv916wh7t.fsf@gitster.g/

Fabian Stelzer (2):
  test-lib: show missing prereq summary
  test-lib: introduce required prereq for test runs

 t/README                |  6 ++++++
 t/aggregate-results.sh  | 17 +++++++++++++++++
 t/test-lib-functions.sh | 11 +++++++++++
 t/test-lib.sh           | 11 +++++++++++
 4 files changed, 45 insertions(+)


base-commit: 5a73c6bdc717127c2da99f57bc630c4efd8aed02
-- 
2.31.1

When running the full test suite many tests can be skipped because of
missing prerequisites. It not easy right now to get an overview of which
ones are missing.
When switching to a new machine or environment some libraries and tools
might be missing or maybe a dependency broke completely. In this case
the tests would indicate nothing since all dependant tests are simply
skipped. This could hide broken behaviour or missing features in the
build. Therefore this patch summarizes the missing prereqs at the end of
the test run making it easier to spot such cases.

 - Add failed prereqs to the test results.
 - Aggregate and then show them with the totals.

Signed-off-by: Fabian Stelzer <redacted>
---
 t/aggregate-results.sh | 17 +++++++++++++++++
 t/test-lib.sh          | 11 +++++++++++
 2 files changed, 28 insertions(+)

diff --git a/t/aggregate-results.sh b/t/aggregate-results.sh
index 7913e206ed..87c16fcee1 100755
--- a/t/aggregate-results.sh
+++ b/t/aggregate-results.sh

@@ -6,6 +6,7 @@ success=0
 failed=0
 broken=0
 total=0
+missing_prereq=
 
 while read file
 do

@@ -30,10 +31,26 @@ do
 			broken=$(($broken + $value)) ;;
 		total)
 			total=$(($total + $value)) ;;
+		missing_prereq)
+			missing_prereq="$missing_prereq,$value" ;;
 		esac
 	done <"$file"
 done
 
+if test -n "$missing_prereq"
+then
+	unique_missing_prereq=$(
+		echo $missing_prereq |
+		tr -s "," "\n" |
+		grep -v '^$' |
+		sort -u |
+		paste -s -d ',')
+	if test -n $unique_missing_prereq
+	then
+		printf "\nmissing prereq: $unique_missing_prereq\n\n"
+	fi
+fi
+
 if test -n "$failed_tests"
 then
 	printf "\nfailed test(s):$failed_tests\n\n"

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 2679a7596a..f61da562f6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -669,6 +669,8 @@ test_fixed=0
 test_broken=0
 test_success=0
 
+test_missing_prereq=
+
 test_external_has_tap=0
 
 die () {

@@ -1069,6 +1071,14 @@ test_skip () {
 			of_prereq=" of $test_prereq"
 		fi
 		skipped_reason="missing $missing_prereq${of_prereq}"
+
+		# Keep a list of all the missing prereq for result aggregation
+		if test -z "$missing_prereq"
+		then
+			test_missing_prereq=$missing_prereq
+		else
+			test_missing_prereq="$test_missing_prereq,$missing_prereq"
+		fi
 	fi
 
 	case "$to_skip" in

@@ -1175,6 +1185,7 @@ test_done () {
 		fixed $test_fixed
 		broken $test_broken
 		failed $test_failure
+		missing_prereq $test_missing_prereq
 
 		EOF
 	fi

-- 
2.31.1

In certain environments or for specific test scenarios we might expect a
specific prerequisite check to be succeed. Therefore we would like to
trigger an error when running our tests if this is not the case.

To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ
which can be set to a comma separated list of prereqs. If one of these
prereq tests fail then the whole test run will abort.

Signed-off-by: Fabian Stelzer <redacted>
---
 t/README                |  6 ++++++
 t/test-lib-functions.sh | 11 +++++++++++
 2 files changed, 17 insertions(+)

diff --git a/t/README b/t/README
index 29f72354bf..18ce75976e 100644
--- a/t/README
+++ b/t/README

@@ -466,6 +466,12 @@ explicitly providing repositories when accessing submodule objects is
 complete or needs to be abandoned for whatever reason (in which case the
 migrated codepaths still retain their performance benefits).
 
+GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a comma speparated list of
+prereqs that are required to succeed. If a prereq in this list is triggered by
+a test and then fails then the whole test run will abort. This can help to make
+sure the expected tests are executed and not silently skipped when their
+dependency breaks or is simply not present in a new environment.
+
 Naming Tests
 ------------
 

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index eef2262a36..2c8abf3420 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

@@ -680,6 +680,17 @@ test_have_prereq () {
 			# Keep a list of missing prerequisites; restore
 			# the negative marker if necessary.
 			prerequisite=${negative_prereq:+!}$prerequisite
+
+			# Abort if this prereq was marked as required
+			if test -n $GIT_TEST_REQUIRE_PREREQ
+			then
+				case ",$GIT_TEST_REQUIRE_PREREQ," in
+				*,$prerequisite,*)
+					error "required prereq $prerequisite failed"
+					;;
+				esac
+			fi
+
 			if test -z "$missing_prereq"
 			then
 				missing_prereq=$prerequisite

-- 
2.31.1

On Mon, Nov 15 2021, Fabian Stelzer wrote:

What is paste? Some out-of-tree debugging utility?

I think you might find a better way to do this shown in my
"ab/generate-command-list" topic, currently in seen. It removed most of
the same sort of tr|grep|sort etc. chain in generate-cmdlist.sh.

Fabian Stelzer [off-list ref] writes:

In certain environments or for specific test scenarios we might expect a
specific prerequisite check to be succeed. Therefore we would like to

"to be succeed" -> "to succeed"?

On 15.11.2021 18:44, Ævar Arnfjörð Bjarmason wrote:

On Mon, Nov 15 2021, Fabian Stelzer wrote:

quoted

+if test -n "$missing_prereq"
+then
+	unique_missing_prereq=$(
+		echo $missing_prereq |
+		tr -s "," "\n" |
+		grep -v '^$' |
+		sort -u |
+		paste -s -d ',')

What is paste? Some out-of-tree debugging utility?

I think you might find a better way to do this shown in my
"ab/generate-command-list" topic, currently in seen. It removed most of
the same sort of tr|grep|sort etc. chain in generate-cmdlist.sh.

I've looked at the generate-command-list code and TBH i still think this
is a better solution. If I read your change correctly you've removed the
sort and unique completely since it was not necessary for the use-case.
In this case i think it is. Since we call tr with `-s` the grep -v might
not be strictly necessary though. Also in this case these commands are
only called once at the end of the test run and not in any kind of loop
like in the cmdlist code so i think this variant is much easier to read
and debug with a negligible performance impact.

I tried writing a sh only variant and this is what i came up with. Not
sure if this could be much more simplified. It looses the sort though.

input="PCRE,JGIT2,JGIT2,,PCRE,JGIT2,PCRE,PCRE2,!PCRE,!WINDOWS,GPG,GPGSSH,PCRE,!GPG,GPG,JGIT2"

unique=
save_IFS=$IFS
IFS=,
for prereq in $input
do
	case "$prereq" in
	'')
		# Skip empty entries
		;;
	*)
		case ",$unique," in
		*,$prereq,*)
			# Skip over duplicates
			;;
		*)
			if test -z "$unique"
			then
				unique="$prereq"
			else
				unique="$unique,$prereq"
			fi
			;;
		esac
	esac
done
IFS=$save_IFS
echo $unique

Fabian Stelzer [off-list ref] writes:

On 15.11.2021 18:44, Ævar Arnfjörð Bjarmason wrote:

quoted

On Mon, Nov 15 2021, Fabian Stelzer wrote:

quoted

+if test -n "$missing_prereq"
+then
+	unique_missing_prereq=$(
+		echo $missing_prereq |
+		tr -s "," "\n" |
+		grep -v '^$' |
+		sort -u |
+		paste -s -d ',')

What is paste? Some out-of-tree debugging utility?

I think you might find a better way to do this shown in my
"ab/generate-command-list" topic, currently in seen. It removed most of
the same sort of tr|grep|sort etc. chain in generate-cmdlist.sh.

I've looked at the generate-command-list code and TBH i still think this
is a better solution. If I read your change correctly you've removed the
sort and unique completely since it was not necessary for the use-case.
In this case i think it is. Since we call tr with `-s` the grep -v might
not be strictly necessary though. Also in this case these commands are
only called once at the end of the test run and not in any kind of loop
like in the cmdlist code so i think this variant is much easier to read
and debug with a negligible performance impact.

I tend to agree with you.  The snippet we see above is quite
straight-forward not over-engineered.

I tried writing a sh only variant and this is what i came up with. Not
sure if this could be much more simplified. It looses the sort though.

Fun, but I'd rather not go there, unless this is a performance
critical bit, which it is not.

Thanks.

input="PCRE,JGIT2,JGIT2,,PCRE,JGIT2,PCRE,PCRE2,!PCRE,!WINDOWS,GPG,GPGSSH,PCRE,!GPG,GPG,JGIT2"

unique=
save_IFS=$IFS
IFS=,
for prereq in $input
do
	case "$prereq" in
	'')
		# Skip empty entries
		;;
	*)
		case ",$unique," in
		*,$prereq,*)
			# Skip over duplicates
			;;
		*)
			if test -z "$unique"
			then
				unique="$prereq"
			else
				unique="$unique,$prereq"
			fi
			;;
		esac
	esac
done
IFS=$save_IFS
echo $unique

On 17.11.2021 00:19, Junio C Hamano wrote:

Fabian Stelzer [off-list ref] writes:

quoted

On 15.11.2021 18:44, Ævar Arnfjörð Bjarmason wrote:

quoted

On Mon, Nov 15 2021, Fabian Stelzer wrote:

quoted

+if test -n "$missing_prereq"
+then
+	unique_missing_prereq=$(
+		echo $missing_prereq |
+		tr -s "," "\n" |
+		grep -v '^$' |
+		sort -u |
+		paste -s -d ',')

What is paste? Some out-of-tree debugging utility?

I think you might find a better way to do this shown in my
"ab/generate-command-list" topic, currently in seen. It removed most of
the same sort of tr|grep|sort etc. chain in generate-cmdlist.sh.

I've looked at the generate-command-list code and TBH i still think this
is a better solution. If I read your change correctly you've removed the
sort and unique completely since it was not necessary for the use-case.
In this case i think it is. Since we call tr with `-s` the grep -v might
not be strictly necessary though. Also in this case these commands are
only called once at the end of the test run and not in any kind of loop
like in the cmdlist code so i think this variant is much easier to read
and debug with a negligible performance impact.

I tend to agree with you.  The snippet we see above is quite
straight-forward not over-engineered.

quoted

I tried writing a sh only variant and this is what i came up with. Not
sure if this could be much more simplified. It looses the sort though.

Fun, but I'd rather not go there, unless this is a performance
critical bit, which it is not.

Thanks.

Ok, i've sent an update fixing the commit msg typo and with \012 instead
of \n. Even if might not be an issue it won't hurt.

Thanks