Fabian Stelzer <fs@xxxxxxxxxxxx> writes:
> As for the general prereq issue i ran into that as well during
> development. When you depend on other patches / a specific version of
> ssh-keygen for git I always have to remember to set the path correctly
> or the tests might silently be ignored by the missing prereq. Usually
> not a problem for single test runs, but when i run the full suite before
> sending something.
This will become a handy tool for everybody, not just for those on
minority and/or exotic platforms. When someone prepares a plain
vanilla fresh box and build Git from the source for the first time
on the box, it is fairly easy to end up with a castrated version of
Git, without knowing what is missing. This is especially so when
autoconf is used, but even without using autoconf, if you do not
have libsvn Perl modules, svn binary, or cvs binary installed, our
tests treat it as a signal that you are uninterested in SVN or CVS
interop tests, rather than flagging it as an error. Being able to
see what is automatically skipped would be a good way to sanity
check what you actually have vs what you thought you had. For
example, I just found out that I am still running CVS interop tests
in my installation.
> Subject: [RFC PATCH 1/2] test-lib: show failed prereq summary
>
> Add failed prereqs to the test results.
> Aggregate and then show them with the totals.
>
> Signed-off-by: Fabian Stelzer <fs@xxxxxxxxxxxx>
> ---
> t/aggregate-results.sh | 12 ++++++++++++
> t/test-lib.sh | 4 ++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/t/aggregate-results.sh b/t/aggregate-results.sh
> index 7913e206ed..ad531cc75d 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,21 @@ do
> broken=$(($broken + $value)) ;;
> total)
> total=$(($total + $value)) ;;
> + missing_prereq)
> + missing_prereq="$missing_prereq $value" ;;
It is unclear yet what shape $value has at this point (because we
haven't seen what is in test-lib.sh), but we accumulate them in the
$missing_prereq variable, separated by a space. Also, I notice that
$missing_prereq will begin with a space when it is not empty, which
probably is not a big deal.
> esac
> done <"$file"
> done
>
> +if test -n "$missing_prereq"
> +then
> + unique_missing_prereq=$(
> + echo $missing_prereq | tr -s "," | \
You do not need backslash there; the line ends with '|' and shell
knows you haven't completed the pipeline yet, so it will go on to
read the next line. The same for the next line; instead of adding
a backslash and breaking the line after it, just have the pipe there
and you can break the line safely without a backslash.
> + sed -e 's/ //g' -e 's/^,//' -e 's/,$//' -e 's/,/\n/g' \
> + | sort | uniq | paste -s -d ',')
I suspect you are making more work than necessary for yourself by
choosing to use SP when accumulating values in $missing_prereq
variable. If you used comma instead, "tr -s ','" here will make a
neat sequence of tokens separated with one comma each, possibly with
one extra comma at the beginning and at the end if some $value were
empty.
Would something like this work better, I wonder?
unique_missing_prereq=$(
echo "$missing_prereq" |
tr -s "," "\012" |
grep -v "^$" |
sort -u |
paste -s -d ,
)
> + printf "\nmissing prereq: $unique_missing_prereq\n\n"
I think it is possible that a $missing_prereq that is not empty
still yields an empty $unique_missing_prereq. If $value read from
the files all are empty strings, $missing_prereq will have many SP
(or comma if you take my earlier suggestion), but no actual prereq
will remain after the "unique" thing is computed. I think this
printf should be shown only when $unique_missing_prereq is not
empty.
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 2679a7596a..472387afec 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 () {
> @@ -1068,6 +1070,7 @@ test_skip () {
> then
> of_prereq=" of $test_prereq"
> fi
> + test_missing_prereq="$missing_prereq,$test_missing_prereq"
OK. We accumulate in $test_missing_prereq what is in missing_prereq
(assigned in test_have_prereq check). I notice that over there, it
takes pains to make sure it uses only one comma between each token,
without excess leading or trailing comma, but we are not taking the
same care here. It would be OK as we'd run "tr -s ," on the side
that reads the output, but looks somewhat sloppy.
> skipped_reason="missing $missing_prereq${of_prereq}"
> fi
>
> @@ -1175,6 +1178,7 @@ test_done () {
> fixed $test_fixed
> broken $test_broken
> failed $test_failure
> + missing_prereq $test_missing_prereq
>
> EOF
And this part is quite obvious, after having read the consumer side
already.
Nicely done.
> fi
> --
> 2.31.1
>
> From d13d4c8ccbd832e1d62044b18b8b771f6586ee2a Mon Sep 17 00:00:00 2001
> From: Fabian Stelzer <fs@xxxxxxxxxxxx>
> Date: Fri, 12 Nov 2021 16:43:18 +0100
> Subject: [RFC PATCH 2/2] test-lib: introduce required prereq for test runs
>
> Allows setting GIT_TEST_REQUIRE_PREREQ to a number of prereqs that must
> succeed for this run. Otherwise the test run will abort.
I am not quite sure what the sentence means, so let me read the code
first before commenting.
> Signed-off-by: Fabian Stelzer <fs@xxxxxxxxxxxx>
> ---
> t/test-lib-functions.sh | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index eef2262a36..d65995cd15 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -669,6 +669,14 @@ test_have_prereq () {
> satisfied_this_prereq=t
> ;;
> *)
At this point, we know $prerequisite we are looking at (note that
what is written as a guard for a particular test might be negated,
e.g. "test_expect_success !WINDOWS 'title' 'code'" that runs on
non-WINDOWS platforms, but here the negation has been stripped away,
so the test says "I require to be on non-Windows", but this new code
only knows that WINDOWS prereq has failed)
> + if ! test -z $GIT_TEST_REQUIRE_PREREQ
Why not
if test -n "$GIT_TEST_REQUIRE_PREREQ"
?
> + then
> + case ",$GIT_TEST_REQUIRE_PREREQ," in
> + *,$prerequisite,*)
> + error "required prereq $prerequisite failed"
> + ;;
So GIT_TEST_REQUIRE_PREREQ could be set to a comma separated list of
prerequisites, e.g. WINDOWS,PDP11,CRAY, and we see if $prerequisite
we have just found out is missing is any one of them. And abort the
test if that is true. Makes sense, except for the negation. You
want to be able to say GIT_TEST_REQUIRE_PREREQ=!WINDOWS,PERL to
require that you are not on Windows and have PERL, for example.
Perhaps this new block should be moved a bit further down in the
code, i.e.
| total_prereq=$(($total_prereq + 1))
| case "$satisfied_prereq" in
| *" $prerequisite "*)
| satisfied_this_prereq=t
| ;;
| *)
... you are inserting the new code here, but don't do that yet, and ...
| satisfied_this_prereq=
| esac
|
| case "$satisfied_this_prereq,$negative_prereq" in
| t,|,t)
| ok_prereq=$(($ok_prereq + 1))
| ;;
| *)
| # Keep a list of missing prerequisites; restore
| # the negative marker if necessary.
| prerequisite=${negative_prereq:+!}$prerequisite
... do it here instead? We have restored the negation in prerequisite
at this point, so we can say
case ",$GIT_TEST_REQUIRE_PREREQ," in
*,$prerequisite,*)
error you do not have $prerequisite.
;;
esac
safely here, before we accumulate it into $missing_prereq variable.
| if test -z "$missing_prereq"
| then
| missing_prereq=$prerequisite
| else
| missing_prereq="$prerequisite,$missing_prereq"
| fi
| esac
Thanks for working on this.
Looking good.
