From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sun, 08 Aug 2010 13:29:22 GMT
Subject: Re: [PATCH 1/3] test-lib: Add support for multiple test prerequisites
Message-ID: <AANLkTikFUH38mzuPo8D-MwJQZdUPEaBUVKgrrfYX=5=p@mail.gmail.com>
In-Reply-To: <20100808015729.GA9284@burratino>

On Sun, Aug 8, 2010 at 01:57, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> Change the test_have_prereq function in test-lib.sh to support a
>> comma-separated list of prerequisites. This is useful for tests that
>> need e.g. both POSIXPERM and SANITY.
>>
>> The implementation was stolen from Junio C Hamano and Johannes Sixt,
>> the tests and documentation were not.
>
> I think you can sell it better. :)

As Johannes points out the patch is different enough that I changed
the authorship. Actually I wouldn't modify any patch under someone
else's name without getting them to sign off on it.

>> +++ b/t/t0000-basic.sh
>> @@ -73,6 +73,23 @@ then
>>       exit 1
>>  fi
>>
>> +test_set_prereq HAVETHIS
>> +haveit=no
>> +test_expect_success HAVETHIS,HAVEIT 'test runs if prerequisites are satisfied' '
>> +    test_have_prereq HAVEIT &&
>> +    test_have_prereq HAVETHIS &&
>
> I think the similar code above was just a way to sneak in a sanity
> check for test_have_prereq().  I’d leave it out.

It's a sanity test. If that wasn't there the test might succeed
e.g. if the implementation was broken and only did a OR on the comma
delimited list, not a AND.

>> +    haveit=yes
>> +'
>> +donthaveit=yes
>> +test_expect_success HAVEIT,DONTHAVEIT 'unmet prerequisites causes test to be skipped' '
>> +    donthaveit=no
>> +'
>> +if test $haveit$donthaveit != yesyes
>> +then
>> +     say "bug in test framework: multiple prerequisite tags do not work reliably"
>> +     exit 1
>> +fi
>
> Maybe it would be simpler to squash this in with the other similar checks.

I think that's too much complexity for one test, especially in the
sanity file. I didn't squash it with the existing "yesyes" test
because we're testing basic functionality first (one prereq) then the
fancy stuff (multiple).

>> +++ b/t/test-lib.sh
>> @@ -327,12 +327,20 @@ test_set_prereq () {
>>  satisfied=" "
>>
>>  test_have_prereq () {
>> -     case $satisfied in
>> -     *" $1 "*)
>> -             : yes, have it ;;
>> -     *)
>> -             ! : nope ;;
>> -     esac
>> +     # prerequisites can be concatenated with ','
>> +     save_IFS=$IFS
>> +     IFS=,
>> +     set -- $*
>> +     IFS=$save_IFS
>> +     for prerequisite
>> +     do
>> +             case $satisfied in
>> +             *" $prerequisite "*)
>> +                     : yes, have it ;;
>> +             *)
>> +                     ! : nope ;;
>> +             esac
>> +     done
>
> Does that work?

It passes all the tests :)

> Except as noted above,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Thanks.

Likewise.