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

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

[Fabian Stelzer]

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/

changes since v1:
 - use \012 instead of \n for possible portability reasons
 - fix typo in commit msg

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(+)

Range-diff against v1:
1:  69e77cd854 ! 1:  775c0e5ef0 test-lib: show missing prereq summary
    @@ t/aggregate-results.sh: do
     +then
     +	unique_missing_prereq=$(
     +		echo $missing_prereq |
    -+		tr -s "," "\n" |
    ++		tr -s "," "\012" |
     +		grep -v '^$' |
     +		sort -u |
     +		paste -s -d ',')
2:  12bd18c5ce ! 2:  eb1bbb8d01 test-lib: introduce required prereq for test runs
    @@ Commit message
         test-lib: introduce required prereq for test runs
     
         In certain environments or for specific test scenarios we might expect a
    -    specific prerequisite check to be succeed. Therefore we would like to
    +    specific prerequisite check to 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
-- 
2.31.1

[PATCH v2 1/2] test-lib: show missing prereq summary

[Fabian Stelzer]

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 <fs@gigacodes.de>
---
 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..ce217b4c0e 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 "," "\012" |
+		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

[PATCH v2 2/2] test-lib: introduce required prereq for test runs

[Fabian Stelzer]

In certain environments or for specific test scenarios we might expect a
specific prerequisite check to 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 <fs@gigacodes.de>
---
 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

Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

> +			# Abort if this prereq was marked as required
> +			if test -n $GIT_TEST_REQUIRE_PREREQ

If GIT_TEST_REQUIRE_PREREQ is an empty string, this will ask

	test -n

and "test" will say "yes" (because "-n" is not an empty string).

Let's surround it with a pair of double-quotes.

> +			then
> +				case ",$GIT_TEST_REQUIRE_PREREQ," in
> +				*,$prerequisite,*)
> +					error "required prereq $prerequisite failed"
> +					;;
> +				esac
> +			fi
> +
>  			if test -z "$missing_prereq"
>  			then
>  				missing_prereq=$prerequisite

Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs

[Fabian Stelzer]

On 18.11.2021 15:42, Junio C Hamano wrote:
>Fabian Stelzer <fs@gigacodes.de> writes:
>
>> +			# Abort if this prereq was marked as required
>> +			if test -n $GIT_TEST_REQUIRE_PREREQ
>
>If GIT_TEST_REQUIRE_PREREQ is an empty string, this will ask
>
>	test -n
>
>and "test" will say "yes" (because "-n" is not an empty string).
>
>Let's surround it with a pair of double-quotes.

Will do.

Thanks

>
>> +			then
>> +				case ",$GIT_TEST_REQUIRE_PREREQ," in
>> +				*,$prerequisite,*)
>> +					error "required prereq $prerequisite failed"
>> +					;;
>> +				esac
>> +			fi
>> +
>>  			if test -z "$missing_prereq"
>>  			then
>>  				missing_prereq=$prerequisite

Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 17 2021, Fabian Stelzer wrote:

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

trigger an error but...

> 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.

..here it's "abort the whole test run". If that's what you want use
BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!"
syntax on bad SANITIZE=leak use, 2021-10-14)

> +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
>  ------------

For other things we specify via lists such as GIT_SKIP_TESTS that's
space-separated, but here it's comma-separated, isn't that just a leaky
abstraction in this case? I.e. this is exposing a previously
internal-only implementation detail of the prereq code.

It's less painful in shellscript if anything like this supports
space-separated parameters, as you can interpolate them more easily in
any wrapper script without using "tr" or the like...

Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs

[Fabian Stelzer]

On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote:
>
>On Wed, Nov 17 2021, Fabian Stelzer wrote:
>
>> In certain environments or for specific test scenarios we might expect a
>> specific prerequisite check to succeed. Therefore we would like to
>> trigger an error when running our tests if this is not the case.
>
>trigger an error but...
>
>> 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.
>
>..here it's "abort the whole test run". If that's what you want use
>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!"
>syntax on bad SANITIZE=leak use, 2021-10-14)
>

ok, thanks. BAIL_OUT seems better. i grepped through the tests and
didn't find anything like it, so i used error.

>> +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
>>  ------------
>
>For other things we specify via lists such as GIT_SKIP_TESTS that's
>space-separated, but here it's comma-separated, isn't that just a leaky
>abstraction in this case? I.e. this is exposing a previously
>internal-only implementation detail of the prereq code.
>
>It's less painful in shellscript if anything like this supports
>space-separated parameters, as you can interpolate them more easily in
>any wrapper script without using "tr" or the like...

Ok. easy enough to change. Should the listing of missing prereq at the
end of a test run be space separated as well? (maybe helps with word wrapping)

Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs

[Fabian Stelzer]

On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote:
>
>On Wed, Nov 17 2021, Fabian Stelzer wrote:
>
>> In certain environments or for specific test scenarios we might expect a
>> specific prerequisite check to succeed. Therefore we would like to
>> trigger an error when running our tests if this is not the case.
>
>trigger an error but...
>
>> 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.
>
>..here it's "abort the whole test run". If that's what you want use
>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!"
>syntax on bad SANITIZE=leak use, 2021-10-14)
>

Hm, while testing this change i noticed another problem that i really
have no idea how to fix.
When a test uses test_have_prereq then the error/BAIL_OUT message will only be printed
when run with '-v'. This is not the case when the prereq is specified
in the test header. The test run will abort, but no error will be
printed which can be quite confusing :/
I guess this has something to do with how tests are run in subshells and
their outputs only printed with -v. Maybe there should be some kind of
override for BAIL_OUT at least? Not sure if/how this could be done.

Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 19 2021, Fabian Stelzer wrote:

> On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote:
>>
>>On Wed, Nov 17 2021, Fabian Stelzer wrote:
>>
>>> In certain environments or for specific test scenarios we might expect a
>>> specific prerequisite check to succeed. Therefore we would like to
>>> trigger an error when running our tests if this is not the case.
>>
>>trigger an error but...
>>
>>> 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.
>>
>>..here it's "abort the whole test run". If that's what you want use
>>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!"
>>syntax on bad SANITIZE=leak use, 2021-10-14)
>>
>
> Hm, while testing this change i noticed another problem that i really
> have no idea how to fix.
> When a test uses test_have_prereq then the error/BAIL_OUT message will only be printed
> when run with '-v'. This is not the case when the prereq is specified
> in the test header. The test run will abort, but no error will be
> printed which can be quite confusing :/
> I guess this has something to do with how tests are run in subshells and
> their outputs only printed with -v. Maybe there should be some kind of
> override for BAIL_OUT at least? Not sure if/how this could be done.

It has to do with how we juggle file descriptors around, see test_eval_
in test-lib.sh.

So the "real" stdout is fd 5, not 1 when you're in a prereq.

Just:

    BAIL_OUT "bad" >&5

Will work, maybe it's a good idea to have:

	BAIL_OUT_PREREQ () {
		BAIL_OUT $@ >&5
	}

Sorry, I forgot about that caveat when suggesting it.

Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs

[Fabian Stelzer]

On 19.11.2021 15:26, Ævar Arnfjörð Bjarmason wrote:
>
>On Fri, Nov 19 2021, Fabian Stelzer wrote:
>
>> On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote:
>>>
>>>On Wed, Nov 17 2021, Fabian Stelzer wrote:
>>>
>>>> In certain environments or for specific test scenarios we might expect a
>>>> specific prerequisite check to succeed. Therefore we would like to
>>>> trigger an error when running our tests if this is not the case.
>>>
>>>trigger an error but...
>>>
>>>> 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.
>>>
>>>..here it's "abort the whole test run". If that's what you want use
>>>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!"
>>>syntax on bad SANITIZE=leak use, 2021-10-14)
>>>
>>
>> Hm, while testing this change i noticed another problem that i really
>> have no idea how to fix.
>> When a test uses test_have_prereq then the error/BAIL_OUT message will only be printed
>> when run with '-v'. This is not the case when the prereq is specified
>> in the test header. The test run will abort, but no error will be
>> printed which can be quite confusing :/
>> I guess this has something to do with how tests are run in subshells and
>> their outputs only printed with -v. Maybe there should be some kind of
>> override for BAIL_OUT at least? Not sure if/how this could be done.
>
>It has to do with how we juggle file descriptors around, see test_eval_
>in test-lib.sh.
>
>So the "real" stdout is fd 5, not 1 when you're in a prereq.
>
>Just:
>
>    BAIL_OUT "bad" >&5
>
>Will work, maybe it's a good idea to have:
>
>	BAIL_OUT_PREREQ () {
>		BAIL_OUT $@ >&5
>	}
>
>Sorry, I forgot about that caveat when suggesting it.

Hm. Any reason to not do this in BAIL_OUT itself?  As far as i can see
the setup of the additional fd's would only need to move up a few lines.

Re: [PATCH v2 2/2] test-lib: introduce required prereq for test runs

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 19 2021, Fabian Stelzer wrote:

> On 19.11.2021 15:26, Ævar Arnfjörð Bjarmason wrote:
>>
>>On Fri, Nov 19 2021, Fabian Stelzer wrote:
>>
>>> On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote:
>>>>
>>>>On Wed, Nov 17 2021, Fabian Stelzer wrote:
>>>>
>>>>> In certain environments or for specific test scenarios we might expect a
>>>>> specific prerequisite check to succeed. Therefore we would like to
>>>>> trigger an error when running our tests if this is not the case.
>>>>
>>>>trigger an error but...
>>>>
>>>>> 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.
>>>>
>>>>..here it's "abort the whole test run". If that's what you want use
>>>>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!"
>>>>syntax on bad SANITIZE=leak use, 2021-10-14)
>>>>
>>>
>>> Hm, while testing this change i noticed another problem that i really
>>> have no idea how to fix.
>>> When a test uses test_have_prereq then the error/BAIL_OUT message will only be printed
>>> when run with '-v'. This is not the case when the prereq is specified
>>> in the test header. The test run will abort, but no error will be
>>> printed which can be quite confusing :/
>>> I guess this has something to do with how tests are run in subshells and
>>> their outputs only printed with -v. Maybe there should be some kind of
>>> override for BAIL_OUT at least? Not sure if/how this could be done.
>>
>>It has to do with how we juggle file descriptors around, see test_eval_
>>in test-lib.sh.
>>
>>So the "real" stdout is fd 5, not 1 when you're in a prereq.
>>
>>Just:
>>
>>    BAIL_OUT "bad" >&5
>>
>>Will work, maybe it's a good idea to have:
>>
>>	BAIL_OUT_PREREQ () {
>>		BAIL_OUT $@ >&5
>>	}
>>
>>Sorry, I forgot about that caveat when suggesting it.
>
> Hm. Any reason to not do this in BAIL_OUT itself?  As far as i can see
> the setup of the additional fd's would only need to move up a few lines.

That does look like a better solution, I've tried it just now locally &
it works for me. Perhaps there's some subtlety I'm missing, but that
should Just Work.

This is by far not the first time I've poked at something in test-lib.sh
only to discover that its pattern of doing setup A, setup C, setup B
etc. caused a problem solved by moving B & C around :(

It could really do with a change to move everything it's now doing to
functions, which we'd then call, so what setup we do in what order would
fit on a single screen, but that's a much larger change...

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

[Fabian Stelzer]

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/

changes sinve v2:
 - use a space separated list for GIT_TES_REQUIRED_PREREQ like we do for
   GIT_SKIP_TESTS
 - use BAIL_OUT() insted of just error()
 - make BAIL_OUT() print errors even when used within prereq context

changes since v1:
 - use \012 instead of \n for possible portability reasons
 - fix typo in commit msg

Fabian Stelzer (3):
  test-lib: show missing prereq summary
  test-lib: introduce required prereq for test runs
  test-lib: make BAIL_OUT() work in tests and prereq

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

Range-diff against v2:
1:  69e77cd854 ! 1:  35c92671e5 test-lib: show missing prereq summary
    @@ t/aggregate-results.sh: do
     +		tr -s "," "\n" |
     +		grep -v '^$' |
     +		sort -u |
    -+		paste -s -d ',')
    -+	if test -n $unique_missing_prereq
    ++		paste -s -d ' ')
    ++	if test -n "$unique_missing_prereq"
     +	then
     +		printf "\nmissing prereq: $unique_missing_prereq\n\n"
     +	fi
2:  12bd18c5ce ! 2:  d6a53f0980 test-lib: introduce required prereq for test runs
    @@ Commit message
         test-lib: introduce required prereq for test runs
     
         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.
    +    specific prerequisite check to succeed. Therefore we would like to abort
    +    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
    +    which can be set to a space separated list of prereqs. If one of these
         prereq tests fail then the whole test run will abort.
     
         Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
    @@ t/README: 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
    ++GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a space 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
    @@ t/test-lib-functions.sh: test_have_prereq () {
      			prerequisite=${negative_prereq:+!}$prerequisite
     +
     +			# Abort if this prereq was marked as required
    -+			if test -n $GIT_TEST_REQUIRE_PREREQ
    ++			if test -n "$GIT_TEST_REQUIRE_PREREQ"
     +			then
    -+				case ",$GIT_TEST_REQUIRE_PREREQ," in
    -+				*,$prerequisite,*)
    -+					error "required prereq $prerequisite failed"
    ++				case " $GIT_TEST_REQUIRE_PREREQ " in
    ++				*" $prerequisite "*)
    ++					BAIL_OUT "required prereq $prerequisite failed"
     +					;;
     +				esac
     +			fi
-:  ---------- > 3:  de21c484d6 test-lib: make BAIL_OUT() work in tests and prereq

base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
-- 
2.31.1

[PATCH v3 1/3] test-lib: show missing prereq summary

[Fabian Stelzer]

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 <fs@gigacodes.de>
---
 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..7f2b83bdc8 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

[PATCH v3 2/3] test-lib: introduce required prereq for test runs

[Fabian Stelzer]

In certain environments or for specific test scenarios we might expect a
specific prerequisite check to succeed. Therefore we would like to abort
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 space separated list of prereqs. If one of these
prereq tests fail then the whole test run will abort.

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

diff --git a/t/README b/t/README
index 29f72354bf..2353a4c5e1 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 space 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..389153e591 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 "*)
+					BAIL_OUT "required prereq $prerequisite failed"
+					;;
+				esac
+			fi
+
 			if test -z "$missing_prereq"
 			then
 				missing_prereq=$prerequisite
-- 
2.31.1

[PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq

[Fabian Stelzer]

BAIL_OUT() is meant to abort the whole test run and print a message with
a standard prefix that can be parsed to stdout. Since for every test the
normal fd`s are redirected in test_eval_ this output would not be seen
when used within the context of a test or prereq like we do in
test_have_prereq(). To make this function work in these contexts we move
the setup of the fd aliases a few lines up before the first use of
BAIL_OUT() and then have this function always print to the alias.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 t/test-lib.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index f61da562f6..96a09a26a1 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -589,6 +589,11 @@ USER_TERM="$TERM"
 TERM=dumb
 export TERM USER_TERM
 
+# Set up additional fds so we can control single test i/o
+exec 5>&1
+exec 6<&0
+exec 7>&2
+
 _error_exit () {
 	finalize_junit_xml
 	GIT_EXIT_OK=t
@@ -612,7 +617,7 @@ BAIL_OUT () {
 	local bail_out="Bail out! "
 	local message="$1"
 
-	say_color error $bail_out "$message"
+	say_color error $bail_out "$message" >&5
 	_error_exit
 }
 
@@ -637,9 +642,6 @@ then
 	exit 0
 fi
 
-exec 5>&1
-exec 6<&0
-exec 7>&2
 if test "$verbose_log" = "t"
 then
 	exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
-- 
2.31.1

Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq

[Ævar Arnfjörð Bjarmason]

On Sat, Nov 20 2021, Fabian Stelzer wrote:

> BAIL_OUT() is meant to abort the whole test run and print a message with
> a standard prefix that can be parsed to stdout. Since for every test the
> normal fd`s are redirected in test_eval_ this output would not be seen
> when used within the context of a test or prereq like we do in
> test_have_prereq(). To make this function work in these contexts we move
> the setup of the fd aliases a few lines up before the first use of
> BAIL_OUT() and then have this function always print to the alias.
>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
> ---
>  t/test-lib.sh | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index f61da562f6..96a09a26a1 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -589,6 +589,11 @@ USER_TERM="$TERM"
>  TERM=dumb
>  export TERM USER_TERM
>  
> +# Set up additional fds so we can control single test i/o
> +exec 5>&1
> +exec 6<&0
> +exec 7>&2
> +
>  _error_exit () {
>  	finalize_junit_xml
>  	GIT_EXIT_OK=t
> @@ -612,7 +617,7 @@ BAIL_OUT () {
>  	local bail_out="Bail out! "
>  	local message="$1"
>  
> -	say_color error $bail_out "$message"
> +	say_color error $bail_out "$message" >&5
>  	_error_exit
>  }
>  
> @@ -637,9 +642,6 @@ then
>  	exit 0
>  fi
>  
> -exec 5>&1
> -exec 6<&0
> -exec 7>&2

This doesn't break (I think) with your change here because you only
manipulate >&5, but I think the post-image would be lot clearer if...

>  if test "$verbose_log" = "t"
>  then
>  	exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3

...this bit of code were moved up along with the "exec". They're
currently intentionally snuggled together as we conditionally set the
3rd and 4th fd depending on verbose/tee settings right after the setup
of 5/6/7, keeping them grouped in the post-image makes more sense than
splitting them up here.

Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> -exec 5>&1
>> -exec 6<&0
>> -exec 7>&2
>
> This doesn't break (I think) with your change here because you only
> manipulate >&5, but I think the post-image would be lot clearer if...
>
>>  if test "$verbose_log" = "t"
>>  then
>>  	exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
>
> ...this bit of code were moved up along with the "exec". They're
> currently intentionally snuggled together as we conditionally set the
> 3rd and 4th fd depending on verbose/tee settings right after the setup
> of 5/6/7, keeping them grouped in the post-image makes more sense than
> splitting them up here.

I actually have to wonder if the handling of 3 and 4 should be moved
down, not up like you suggest, so that they are grouped together
with the maybe_teardown_verbose and the maybe_setup_verbose helper
functions.  The lines we see here are the file descriptors that are
always redirected, which is a bit different.  Raising all of them up
to group them together is also fine.

In any case, the comment in front of the block of exec wants to
become a bit more detailed than just "# Set up additional fds", with
an explanation about which FD is used for what.

And any change that involves handling of FD #4 probably wants to
further include the BASH_XTRACEFD setting.

Thanks.

Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq

[Fabian Stelzer]

On 22.11.2021 09:05, Junio C Hamano wrote:
>Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> -exec 5>&1
>>> -exec 6<&0
>>> -exec 7>&2
>>
>> This doesn't break (I think) with your change here because you only
>> manipulate >&5, but I think the post-image would be lot clearer if...
>>
>>>  if test "$verbose_log" = "t"
>>>  then
>>>  	exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
>>
>> ...this bit of code were moved up along with the "exec". They're
>> currently intentionally snuggled together as we conditionally set the
>> 3rd and 4th fd depending on verbose/tee settings right after the setup
>> of 5/6/7, keeping them grouped in the post-image makes more sense than
>> splitting them up here.
>
>I actually have to wonder if the handling of 3 and 4 should be moved
>down, not up like you suggest, so that they are grouped together
>with the maybe_teardown_verbose and the maybe_setup_verbose helper
>functions.  The lines we see here are the file descriptors that are
>always redirected, which is a bit different.  Raising all of them up
>to group them together is also fine.

Since there is lots of other things happening in between i'd rather
leave 3 & 4 where they are for now. I'm having a hard time grasping what
test-lib.sh does when / where since there is lots of intermingled
function definition / executed code. It could probably benefit from a
new include to move functions to (and wrap some code into new ones). At
the moment i'm a bit swamped with other work though :/

>
>In any case, the comment in front of the block of exec wants to
>become a bit more detailed than just "# Set up additional fds", with
>an explanation about which FD is used for what.
>

How about:

# Set up additional fds to allow i/o with the surrounding test
# harness when redirecting individual test i/o in test_eval_
# fd 5 -> stdout
# fd 6 <- stdin
# fd 7 -> stderr

exec 5>&1
exec 6<&0
exec 7>&2

Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

>>In any case, the comment in front of the block of exec wants to
>>become a bit more detailed than just "# Set up additional fds", with
>>an explanation about which FD is used for what.
>>
>
> How about:
>
> # Set up additional fds to allow i/o with the surrounding test
> # harness when redirecting individual test i/o in test_eval_

This does not quite say how "setting up additional fds" helpss the
test harness, though.  And this ...

> # fd 5 -> stdout
> # fd 6 <- stdin
> # fd 7 -> stderr

... is literal translation of what is written below, without adding
any new information.

> exec 5>&1
> exec 6<&0
> exec 7>&2

I was expecting something along the lines of ...

# What is written by tests to their FD #1 and #2 are sent to
# different places depending on the test mode (e.g. /dev/null in
# non-verbose mode, piped to tee with --tee option, etc.)  Original
# FD #1 and #2 are saved away to #5 and #7, so that test framework
# can use them to send the output to these low FDs before the
# mode-specific redirection.

... but this only talks about the output side.  The final version
needs to mention the input side, too.

Thanks.

Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq

[Fabian Stelzer]

On 26.11.2021 13:02, Junio C Hamano wrote:
>Fabian Stelzer <fs@gigacodes.de> writes:
>
>>>In any case, the comment in front of the block of exec wants to
>>>become a bit more detailed than just "# Set up additional fds", with
>>>an explanation about which FD is used for what.
>>>
>>
>> How about:
>>
>> # Set up additional fds to allow i/o with the surrounding test
>> # harness when redirecting individual test i/o in test_eval_
>
>This does not quite say how "setting up additional fds" helpss the
>test harness, though.  And this ...
>
>> # fd 5 -> stdout
>> # fd 6 <- stdin
>> # fd 7 -> stderr
>
>... is literal translation of what is written below, without adding
>any new information.
>
>> exec 5>&1
>> exec 6<&0
>> exec 7>&2
>
>I was expecting something along the lines of ...
>
># What is written by tests to their FD #1 and #2 are sent to
># different places depending on the test mode (e.g. /dev/null in
># non-verbose mode, piped to tee with --tee option, etc.)  Original
># FD #1 and #2 are saved away to #5 and #7, so that test framework
># can use them to send the output to these low FDs before the
># mode-specific redirection.
>
>... but this only talks about the output side.  The final version
>needs to mention the input side, too.
>

I like to use the term stdin/err/out since that is what i would grep for
when trying to find out more about the test i/o behaviour.

I understand that you would like to give exaxples what the fd aliases
are used for and I think thats a good idea.  But I'm having a bit of a
hard time understanding the use of the stdin alias. I did not find a
single use in the test suite - but my grep might not have been complete.
Or if it's just there for the sake of completeness.

As far as i understand the test framework runs the individual tests in
test_eval_ redirecting its stdout/err to fd #3 & #4 (with some xtracefd
logic for -x) and passing /dev/null as stdin to the test.  What would FD
#6 actually be used for then? A test lib function being called from
within a test that expects user input?

Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

>>I was expecting something along the lines of ...
>>
>># What is written by tests to their FD #1 and #2 are sent to
>># different places depending on the test mode (e.g. /dev/null in
>># non-verbose mode, piped to tee with --tee option, etc.)  Original
>># FD #1 and #2 are saved away to #5 and #7, so that test framework
>># can use them to send the output to these low FDs before the
>># mode-specific redirection.
>>
>>... but this only talks about the output side.  The final version
>>needs to mention the input side, too.
>>
>
> I like to use the term stdin/err/out since that is what i would grep for
> when trying to find out more about the test i/o behaviour.

I do not mind phrasing "original FD #1" as "original standard
output" at all.  I just wanted to make sure it is clear to readers
whose FD #1 and FD #5 we are talking about. In other words, the
readers should get a clear understanding of where they are writing
to, when the code they write in test_expect_success block outputs to
FD #1, and what the code needs to do if it wants to always show
something to the original standard output stream.

Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq

[Fabian Stelzer]

On 28.11.2021 15:38, Junio C Hamano wrote:
>Fabian Stelzer <fs@gigacodes.de> writes:
>
>>>I was expecting something along the lines of ...
>>>
>>># What is written by tests to their FD #1 and #2 are sent to
>>># different places depending on the test mode (e.g. /dev/null in
>>># non-verbose mode, piped to tee with --tee option, etc.)  Original
>>># FD #1 and #2 are saved away to #5 and #7, so that test framework
>>># can use them to send the output to these low FDs before the
>>># mode-specific redirection.
>>>
>>>... but this only talks about the output side.  The final version
>>>needs to mention the input side, too.
>>>
>>
>> I like to use the term stdin/err/out since that is what i would grep for
>> when trying to find out more about the test i/o behaviour.
>
>I do not mind phrasing "original FD #1" as "original standard
>output" at all.  I just wanted to make sure it is clear to readers
>whose FD #1 and FD #5 we are talking about. In other words, the
>readers should get a clear understanding of where they are writing
>to, when the code they write in test_expect_success block outputs to
>FD #1, and what the code needs to do if it wants to always show
>something to the original standard output stream.

The current version in my branch is now:

What is written by tests to stdout and stderr is sent so different places
depending on the test mode (e.g. /dev/null in non-verbose mode, piped to tee
with --tee option, etc.). We save the original stdin to FD #6 and stdout and
stderr to #5 and #7, so that the test framework can use them (e.g. for
printing errors within the test framework) independently of the test mode.

which I think should make this sufficiently clear.
I'm wondering now though if we should write to #7 instead of #5 in 
BAIL_OUT(). The current use in test-lib/test-lib-functions seems a bit 
inconsistent.

For example:
error >&7 "bug in the test script: $*"
echo >&7 "test_must_fail: only 'git' is allowed: $*"

but:
echo >&5 "FATAL: Cannot prepare test area"
echo >&5 "FATAL: Unexpected exit with code $code"

Sometimes these errors result in immediate exit 1, but not always.

I'm not sure if the TAP framework that BAIL_OUT() references expects the 
bail out error on a specific fd.

Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 30 2021, Fabian Stelzer wrote:

> On 28.11.2021 15:38, Junio C Hamano wrote:
>>Fabian Stelzer <fs@gigacodes.de> writes:
>>
>>>>I was expecting something along the lines of ...
>>>>
>>>># What is written by tests to their FD #1 and #2 are sent to
>>>># different places depending on the test mode (e.g. /dev/null in
>>>># non-verbose mode, piped to tee with --tee option, etc.)  Original
>>>># FD #1 and #2 are saved away to #5 and #7, so that test framework
>>>># can use them to send the output to these low FDs before the
>>>># mode-specific redirection.
>>>>
>>>>... but this only talks about the output side.  The final version
>>>>needs to mention the input side, too.
>>>>
>>>
>>> I like to use the term stdin/err/out since that is what i would grep for
>>> when trying to find out more about the test i/o behaviour.
>>
>>I do not mind phrasing "original FD #1" as "original standard
>>output" at all.  I just wanted to make sure it is clear to readers
>>whose FD #1 and FD #5 we are talking about. In other words, the
>>readers should get a clear understanding of where they are writing
>>to, when the code they write in test_expect_success block outputs to
>>FD #1, and what the code needs to do if it wants to always show
>>something to the original standard output stream.
>
> The current version in my branch is now:
>
> What is written by tests to stdout and stderr is sent so different places
> depending on the test mode (e.g. /dev/null in non-verbose mode, piped to tee
> with --tee option, etc.). We save the original stdin to FD #6 and stdout and
> stderr to #5 and #7, so that the test framework can use them (e.g. for
> printing errors within the test framework) independently of the test mode.
>
> which I think should make this sufficiently clear.
> I'm wondering now though if we should write to #7 instead of #5 in
> BAIL_OUT(). The current use in test-lib/test-lib-functions seems a bit 
> inconsistent.
>
> For example:
> error >&7 "bug in the test script: $*"
> echo >&7 "test_must_fail: only 'git' is allowed: $*"
>
> but:
> echo >&5 "FATAL: Cannot prepare test area"
> echo >&5 "FATAL: Unexpected exit with code $code"
>
> Sometimes these errors result in immediate exit 1, but not always.
>
> I'm not sure if the TAP framework that BAIL_OUT() references expects
> the bail out error on a specific fd.

All TAP must be emitted to stdout. You can test that with e.g.:
    
    $ cat tap.sh
    #!/bin/sh 
    echo "ok 1 one"
    echo "ok 2 two" >&2
    echo "1..1"
    $ prove --exec /bin/sh tap.sh
    tap.sh .. 1/? ok 2 two
    tap.sh .. ok   
    All tests successful.
    Files=1, Tests=1,  0 wallclock secs ( 0.01 usr +  0.00 sys =  0.01 CPU)
    Result: PASS

Note how the "ok 2 two" is emitted to STDERR, and doesn't count towards
the number of tests. If it's changed to:
    
    $ cat tap.sh
    #!/bin/sh
    echo "ok 1 one"
    echo "ok 2 two"
    echo "1..2"
    $ prove --exec /bin/sh tap.sh
    tap.sh .. ok   
    All tests successful.
    Files=1, Tests=2,  0 wallclock secs ( 0.00 usr +  0.01 sys =  0.01 CPU)
    Result: PASS

You can see it runs two tests.

The reason the existing cases are inconsistent are because of various
reasons, probably none good at this point.

Some are just because the error handling pre-dates the TAP support in
the test suite, I think at this point we should just be moving to making
it first-class in terms of TAP support. I.e. it's clearly the most
commonly used test mode (and we use it in CI etc.). So we should emit
all directives on STDOUT.

And some are probably just copy/pasting, or error handling that didn't
consider TAP at the time of writing.

Note that not all of these should be "Bail out!". We should really
reserve that for wanting to stall the entire test run, but e.g. not for
"cannot prep test area", which might only be a permission error with one
trash directory.

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

[Fabian Stelzer]

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/

changes since v3:
 - reword comment about test framework fd setup

changes sinve v2:
 - use a space separated list for GIT_TES_REQUIRED_PREREQ like we do for
   GIT_SKIP_TESTS
 - use BAIL_OUT() insted of just error()
 - make BAIL_OUT() print errors even when used within prereq context

changes since v1:
 - use \012 instead of \n for possible portability reasons
 - fix typo in commit msg

Fabian Stelzer (3):
  test-lib: show missing prereq summary
  test-lib: introduce required prereq for test runs
  test-lib: make BAIL_OUT() work in tests and prereq

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

Range-diff against v3:
1:  35c92671e5 = 1:  9617d336c7 test-lib: show missing prereq summary
2:  d6a53f0980 = 2:  409694823a test-lib: introduce required prereq for test runs
3:  de21c484d6 ! 3:  3757e4e238 test-lib: make BAIL_OUT() work in tests and prereq
    @@ t/test-lib.sh: USER_TERM="$TERM"
      TERM=dumb
      export TERM USER_TERM
      
    -+# Set up additional fds so we can control single test i/o
    ++# What is written by tests to stdout and stderr is sent so different places
    ++# depending on the test mode (e.g. /dev/null in non-verbose mode, piped to tee
    ++# with --tee option, etc.). We save the original stdin to FD #6 and stdout and
    ++# stderr to #5 and #7, so that the test framework can use them (e.g. for
    ++# printing errors within the test framework) independently of the test mode.
     +exec 5>&1
     +exec 6<&0
     +exec 7>&2

base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa
-- 
2.31.1

[PATCH v4 1/3] test-lib: show missing prereq summary

[Fabian Stelzer]

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 <fs@gigacodes.de>
---
 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..7f2b83bdc8 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 57efcc5e97..9090ce1225 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

[PATCH v4 2/3] test-lib: introduce required prereq for test runs

[Fabian Stelzer]

In certain environments or for specific test scenarios we might expect a
specific prerequisite check to succeed. Therefore we would like to abort
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 space separated list of prereqs. If one of these
prereq tests fail then the whole test run will abort.

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

diff --git a/t/README b/t/README
index 29f72354bf..2353a4c5e1 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 space 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..389153e591 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 "*)
+					BAIL_OUT "required prereq $prerequisite failed"
+					;;
+				esac
+			fi
+
 			if test -z "$missing_prereq"
 			then
 				missing_prereq=$prerequisite
-- 
2.31.1

[PATCH v4 3/3] test-lib: make BAIL_OUT() work in tests and prereq

[Fabian Stelzer]

BAIL_OUT() is meant to abort the whole test run and print a message with
a standard prefix that can be parsed to stdout. Since for every test the
normal fd`s are redirected in test_eval_ this output would not be seen
when used within the context of a test or prereq like we do in
test_have_prereq(). To make this function work in these contexts we move
the setup of the fd aliases a few lines up before the first use of
BAIL_OUT() and then have this function always print to the alias.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 t/test-lib.sh | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9090ce1225..14a7aeae0f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -589,6 +589,15 @@ USER_TERM="$TERM"
 TERM=dumb
 export TERM USER_TERM
 
+# What is written by tests to stdout and stderr is sent so different places
+# depending on the test mode (e.g. /dev/null in non-verbose mode, piped to tee
+# with --tee option, etc.). We save the original stdin to FD #6 and stdout and
+# stderr to #5 and #7, so that the test framework can use them (e.g. for
+# printing errors within the test framework) independently of the test mode.
+exec 5>&1
+exec 6<&0
+exec 7>&2
+
 _error_exit () {
 	finalize_junit_xml
 	GIT_EXIT_OK=t
@@ -612,7 +621,7 @@ BAIL_OUT () {
 	local bail_out="Bail out! "
 	local message="$1"
 
-	say_color error $bail_out "$message"
+	say_color error $bail_out "$message" >&5
 	_error_exit
 }
 
@@ -637,9 +646,6 @@ then
 	exit 0
 fi
 
-exec 5>&1
-exec 6<&0
-exec 7>&2
 if test "$verbose_log" = "t"
 then
 	exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
-- 
2.31.1

Re: [PATCH v4 0/3] test-lib: improve missing prereq handling

[Adam Dinwoodie]

On Wed, 1 Dec 2021 at 08:53, Fabian Stelzer <fs@gigacodes.de> wrote:
>
> 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

Nit, purely because I just spotted it: "successfull" should be "successful".

> (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.

I've not managed to keep up with the ongoing development of this
function, but I've just tested a recent version (specifically, from
Junio's tree, 1ade7d2334 (test-lib: make BAIL_OUT() work in tests and
prereq, 2021-11-20)). This looks like it would have been fantastically
useful when I was first taking over the maintainership of Git for
Cygwin, and I'm looking forward to having the extra confidence of my
builds and tests after I can add GIT_TEST_REQUIRE_PREREQ to my build
scripts.

Thank you!

Re: [PATCH v4 3/3] test-lib: make BAIL_OUT() work in tests and prereq

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

> BAIL_OUT() is meant to abort the whole test run and print a message with
> a standard prefix that can be parsed to stdout. Since for every test the
> normal fd`s are redirected in test_eval_ this output would not be seen
> when used within the context of a test or prereq like we do in
> test_have_prereq(). To make this function work in these contexts we move
> the setup of the fd aliases a few lines up before the first use of
> BAIL_OUT() and then have this function always print to the alias.
>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
> ---
>  t/test-lib.sh | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 9090ce1225..14a7aeae0f 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -589,6 +589,15 @@ USER_TERM="$TERM"
>  TERM=dumb
>  export TERM USER_TERM
>  
> +# What is written by tests to stdout and stderr is sent so different places

"sent so" -> "sent to".  I'll tweak locally, so no need to resend.

> +# depending on the test mode (e.g. /dev/null in non-verbose mode, piped to tee
> +# with --tee option, etc.). We save the original stdin to FD #6 and stdout and
> +# stderr to #5 and #7, so that the test framework can use them (e.g. for
> +# printing errors within the test framework) independently of the test mode.
> +exec 5>&1
> +exec 6<&0
> +exec 7>&2
> +
>  _error_exit () {
>  	finalize_junit_xml
>  	GIT_EXIT_OK=t
> @@ -612,7 +621,7 @@ BAIL_OUT () {
>  	local bail_out="Bail out! "
>  	local message="$1"
>  
> -	say_color error $bail_out "$message"
> +	say_color error $bail_out "$message" >&5

This is merely a style thing, but as commands get longer, it becomes
easier to spot redirection if it is written immediately after the
verb, i.e.

	say_color >&5 error $bail_out "$message"

>  	_error_exit
>  }
>  
> @@ -637,9 +646,6 @@ then
>  	exit 0
>  fi
>  
> -exec 5>&1
> -exec 6<&0
> -exec 7>&2
>  if test "$verbose_log" = "t"
>  then
>  	exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3

Looks good.  Thanks.