Re: [RFC PATCH] lib-test: show failed prereq was Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure

[Fabian Stelzer <fs@gigacodes.de>]

On 12.11.2021 22:10, Junio C Hamano wrote:
>Fabian Stelzer <fs@gigacodes.de> 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@gigacodes.de>
>> 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.