Re: [PATCH 2/2] diff tests: rewrite flakyness-causing test "aid"

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

On Tue, Apr 13 2021, Matheus Tavares Bernardino wrote:

> On Tue, Apr 13, 2021 at 9:31 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> If a new test is added to this "while read magic cmd" test facility
>> added in 3c2f75b590c (t4013: add tests for diff/log family output
>> options., 2006-06-26) but no test file is added it'll fail the first
>> time, but then succeed on subsequent runs as a new file has been added
>> in t4013.
>>
>> Let's accomplish the same aim in way that doesn't cause subsequent
>
> s/in way/in a way/ ?

*nod*

>> test runs to succeed. If we can't find the file we'll BUG out, and
>> suggest to the developer that they copy our "expect.new" file over,
>> unlike the previous "expect" file this won't be picked up on
>> subsequent runs.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/t4013-diff-various.sh | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
>> index 67f6411aff9..228ff100c61 100755
>> --- a/t/t4013-diff-various.sh
>> +++ b/t/t4013-diff-various.sh
>> @@ -200,10 +200,12 @@ do
>>         esac
>>         test=$(echo "$label" | sed -e 's|[/ ][/ ]*|_|g')
>>         pfx=$(printf "%04d" $test_count)
>> -       expect="$TEST_DIRECTORY/t4013/diff.$test"
>> +       expect_relative="t4013/diff.$test"
>> +       expect="$TEST_DIRECTORY/$expect_relative"
>>         actual="$pfx-diff.$test"
>>
>>         test_expect_$status "git $cmd # magic is ${magic:-(not used)}" '
>> +               test_when_finished "rm $actual" &&
>
> Nit: before these two patches, "$actual" was only removed when the
> test succeeded. So, in case of failure, the failed output files would
> still be there for debugging. It might be interesting to keep this
> behavior and only remove "$actual" at the end of the test.

Either I'm missing something or you are, that's how test_when_finished
works.

It's skipped under e.g. "--immediate --debug". See b586744a864 (test:
skip clean-up when running under --immediate mode, 2011-06-27)

Maybe there's some edge case where we'd like to keep the files that it's
not covering, but then we should patch it to do the right thing, not use
manual "rm" at the end instead.

>>                 {
>>                         echo "$ git $cmd"
>>                         case "$magic" in
>> @@ -216,16 +218,19 @@ do
>>                             -e "s/^\\(.*mixed; boundary=\"-*\\)$V\\(-*\\)\"\$/\\1g-i-t--v-e-r-s-i-o-n\2\"/"
>>                         echo "\$"
>>                 } >"$actual" &&
>> -               if test -f "$expect"
>> +
>> +               if ! test -f "$expect"
>>                 then
>> -                       process_diffs "$actual" >actual &&
>> -                       process_diffs "$expect" >expect &&
>> -                       test_cmp expect actual
>> -               else
>> -                       # this is to help developing new tests.
>> -                       cp "$actual" "$expect"
>> -                       false
>> -               fi
>> +                       expect_new="$expect.new" &&
>> +                       cp "$actual" "$expect_new" &&
>> +                       BUG "Have no \"$expect_relative\", new test? The output is in \"$expect_new\", maybe use that?"
>> +               fi &&
>> +
>> +               test_when_finished "rm actual" &&
>> +               process_diffs "$actual" >actual &&
>> +               test_when_finished "rm expect" &&
>> +               process_diffs "$expect" >expect &&
>> +               test_cmp expect actual
>>         '
>>  done <<\EOF
>>  diff-tree initial
>
> The rest LGTM, thanks.

Thanks a lot for the review!