Re: [PATCH v4 29/29] t7527: test status with untracked-cache and fsmonitor--daemon

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Oct 27, 2021, 20:06 UTC
Message-ID: <d9c3ef61-768c-3560-2858-3438c355a742@jeffhostetler.com>
In-Reply-To: <CAPig+cT1j6NsZaSM+_JgGksmH4XGsnh7Pe=dbELoBU+FisHGag@mail.gmail.com>

On 10/22/21 1:23 AM, Eric Sunshine wrote:

Show 31 quoted lines

> On Thu, Oct 21, 2021 at 10:26 AM Jeff Hostetler via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> Create 2x2 test matrix with the untracked-cache and fsmonitor--daemon
>> features and a series of edits and verify that status output is
>> identical.
>>
>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>> ---
>> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
>> @@ -508,4 +510,98 @@ test_expect_success 'cleanup worktrees' '
>> +test_lazy_prereq UNTRACKED_CACHE '
>> +       { git update-index --test-untracked-cache; ret=$?; } &&
>> +       test $ret -ne 1
>> +'
> 
> I may be missing something obvious, but can't this be expressed more simply as:
> 
>      test_lazy_prereq UNTRACKED_CACHE '
>          git update-index --test-untracked-cache
>          test $? -ne 1
>      '
> 
> How significant is it to test specifically against 1? If not, then
> even simpler would be:
> 
>      test_lazy_prereq UNTRACKED_CACHE '
>          git update-index --test-untracked-cache
>      '
> 
> which is also more robust since it won't be fooled by die() or crashes.
>

IIRC I stole that from t7063. It didn't occur to me to try to simplify it.

Show 7 quoted lines

>> +test_expect_success 'Matrix: setup for untracked-cache,fsmonitor matrix' '
>> +       test_might_fail git config --unset core.useBuiltinFSMonitor &&
> 
> More idiomatic:
> 
>      test_unconfig core.useBuiltinFSMonitor &&
>

Good to know, thanks! I'll take a look at this and the rest of your comments below.

thanks Jeff

Show 110 quoted lines

>> +       git update-index --no-fsmonitor &&
>> +       test_might_fail git fsmonitor--daemon stop
>> +'
>> +
>> +matrix_clean_up_repo () {
>> +       git reset --hard HEAD
>> +       git clean -fd
>> +}
> 
> Since calls to this function are part of the &&-chain in tests, it
> probably would be a good idea to maintain the &&-chain within the body
> of the function, as well.
> 
>> +matrix_try () {
>> +       test_expect_success "Matrix[uc:$uc][fsm:$fsm] $fn" '
>> +               matrix_clean_up_repo &&
>> +               $fn &&
>> +               if test $uc = false -a $fsm = false
> 
> We avoid -a and -o with `test` and instead chain them with &&:
> 
>      if test $uc = false && test $fsm = false
> 
> Documentation/CodingGuidelines mentions this. Also see [1] & [2].
> 
> [1]: https://lore.kernel.org/git/xmqqa6qkb5fi.fsf@gitster.g/
> [2]: https://lore.kernel.org/git/CAPig+cQFFsLeE921WpzTxVnBMnNRiKs4N=hUQ2UQi1VznNEQwg@mail.gmail.com/
> 
>> +               then
>> +                       git status --porcelain=v1 >.git/expect.$fn
>> +               else
>> +                       git status --porcelain=v1 >.git/actual.$fn
>> +                       test_cmp .git/expect.$fn .git/actual.$fn
>> +               fi
>> +       '
> 
> Broken &&-chain in the `else` arm.
> 
>> +       return $?
>> +}
> 
> No callers care about the return value of this function, so the
> `return $?` can be dropped.
> 
>> +uc_values="false"
>> +test_have_prereq UNTRACKED_CACHE && uc_values="false true"
>> +for uc_val in $uc_values
>> +do
>> +       if test $uc_val = false
>> +       then
>> +               test_expect_success "Matrix[uc:$uc_val] disable untracked cache" '
>> +                       git config core.untrackedcache false &&
>> +                       git update-index --no-untracked-cache
>> +               '
>> +       else
>> +               test_expect_success "Matrix[uc:$uc_val] enable untracked cache" '
>> +                       git config core.untrackedcache true &&
>> +                       git update-index --untracked-cache
>> +               '
>> +       fi
>> +
>> +       fsm_values="false true"
>> +       for fsm_val in $fsm_values
>> +       do
>> +               if test $fsm_val = false
>> +               then
>> +                       test_expect_success "Matrix[uc:$uc_val][fsm:$fsm_val] disable fsmonitor" '
>> +                               test_might_fail git config --unset core.useBuiltinFSMonitor &&
> 
> Ditto: test_unconfig()
> 
>> +                               git update-index --no-fsmonitor &&
>> +                               test_might_fail git fsmonitor--daemon stop 2>/dev/null
>> +                       '
> 
> stderr is redirected within tests anyhow, so we normally don't
> suppress it manually like this (especially since it may come in handy
> when debugging a failing test).
> 
>> +               else
>> +                       test_expect_success "Matrix[uc:$uc_val][fsm:$fsm_val] enable fsmonitor" '
>> +                               git config core.useBuiltinFSMonitor true &&
>> +                               git fsmonitor--daemon start &&
>> +                               git update-index --fsmonitor
>> +                       '
>> +               fi
>> +
>> +               matrix_try $uc_val $fsm_val edit_files
>> +               matrix_try $uc_val $fsm_val delete_files
>> +               matrix_try $uc_val $fsm_val create_files
>> +               matrix_try $uc_val $fsm_val rename_files
>> +               matrix_try $uc_val $fsm_val file_to_directory
>> +               matrix_try $uc_val $fsm_val directory_to_file
>> +
>> +               if test $fsm_val = true
>> +               then
>> +                       test_expect_success "Matrix[uc:$uc_val][fsm:$fsm_val] disable fsmonitor at end" '
>> +                               test_might_fail git config --unset core.useBuiltinFSMonitor &&
> 
> Ditto: test_unconfig()
> 
>> +                               git update-index --no-fsmonitor &&
>> +                               test_might_fail git fsmonitor--daemon stop 2>/dev/null
> 
> Ditto: stderr
> 
>> +                       '
>> +               fi
>> +       done
>> +done