On 12.11.2021 22:10, Junio C Hamano wrote:
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.
+ 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.
You are right. I'll change it to ',' as well. It makes the following
unique logic easier.
Would something like this work better, I wonder?
unique_missing_prereq=$(
echo "$missing_prereq" |
tr -s "," "\012" |
grep -v "^$" |
sort -u |
paste -s -d ,
)
Ok. Took me a moment to understand since i didn't realize tr did the
newline expansion as well. But yeah, this is nicer.
+ 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.
True. I'll add an if.
+ 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.
Ok, i'll use the same logic as in the test_have_prereq func here as
well.
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.
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)
I will write some clearer commit messages and then re-send as a normal
patch.
+ if ! test -z $GIT_TEST_REQUIRE_PREREQ
Why not
if test -n "$GIT_TEST_REQUIRE_PREREQ"
?
Obviously, yes...
+ 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.
Thanks, yes i did not notice the negation issue.
Thanks for working on this.
Looking good.
Thanks for your review.
