Re: [PATCH v2] add -i tests: mark "TODO" depending on GIT_TEST_ADD_I_USE_BUILTIN

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jun 14 2022, Todd Zullinger wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> Fix an issue that existed before 0527ccb1b55 (add -i: default to the
>> built-in implementation, 2021-11-30), but which became the default
>> with that change, we should not be marking tests that are known to
>> pass as "TODO" tests.
> [...]
>> ---
>> Just converting it to "test_expect_success" will break CI and other
>> setups that are testing with GIT_TEST_ADD_I_USE_BUILTIN=false.
>> 
>> The below fixes it, however.
>
> Nice catch.  FWIW, I tested w/GIT_TEST_ADD_I_USE_BUILTIN=0
> and without.

My patch landed on "master" as 7ccbea564e8 (add -i tests: mark "TODO"
depending on GIT_TEST_ADD_I_USE_BUILTIN, 2022-06-14) so this is water
under the bridge.

But just to tie this loose knot I think something went wrong in your
testing.

If I:

    git checkout v2.37.0-rc0
    # Apply your patch from <20220614185218.1091413-1-tmz@pobox.com>

I'll consistently get a failure from:

    GIT_TEST_ADD_I_USE_BUILTIN=false ./t3701-add-interactive.sh

Since we do fail that test with the Perl implementation, and now it's no
longer a TODO test.

Perhaps you used it as a parameter to "make"? I.e.:

    make GIT_TEST_ADD_I_USE_BUILTIN=false
    make test

Which isn't how it works, just speculating...
 
>> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
>> index bc3f69b4b1d..a5822e41af2 100755
>> --- a/t/t2016-checkout-patch.sh
>> +++ b/t/t2016-checkout-patch.sh
>> @@ -4,7 +4,7 @@ test_description='git checkout --patch'
>>  
>>  . ./lib-patch-mode.sh
>>  
>> -if ! test_bool_env GIT_TEST_ADD_I_USE_BUILTIN true && ! test_have_prereq PERL
>> +if ! test_have_prereq ADD_I_USE_BUILTIN && ! test_have_prereq PERL
>>  then
>>  	skip_all='skipping interactive add tests, PERL not set'
>
> It's not the fault of this patch, but it makes it obvious
> that the `skip_all` message is no longer accurate.  Perhaps
> somethine like this?
>
>     skip_all='skipping interactive add tests, missing ADD_I_USE_BUILTIN or PERL'
>
> Maybe a separate `ADD_I` prereq would be better?  Though
> without looking closer, I don't know if that would end up
> being clearer to anyone running the tests without either
> PERL or the add -i builtin enabled.

Yeah seems like a good idea for a follow-up, but since it's landed I'll
probably forget :)

> Thanks for the keen eye and attention to detail, Ævar,

Happy to have it fixed!