Re: [PATCH 6/7] worktree: add tests for `list` verbose and annotations

> On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> Add tests for "git worktree list" verbose mode, prunable and locked
>> annotations for both default and porcelain format and ensure the
>> "prunable" annotation is consistent with what "git worktree prune"
>> command will eventually remove. Additionally, add one test case to
>> ensure any newline characters are escaped from locked reason for the
>> porcelain format to prevent breaking the format.
>>
>> The c57b3367be (worktree: teach `list` to annotate locked worktree,
>> 2020-10-11) introduced a new test to ensure locked worktrees are listed
>> with "locked" annotation. However, the test does not remove the worktree
>> as the "git worktree prune" is not going to remove any locked worktrees.
>> Let's fix that by unlocking the worktree before the "prune" command.
>>
>> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
>> ---
>> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
>> @@ -62,7 +62,9 @@ test_expect_success '"list" all worktrees --porcelain' '
>>  test_expect_success '"list" all worktrees with locked annotation' '
>> -       test_when_finished "rm -rf locked unlocked out && git worktree prune" &&
>> +       test_when_finished "rm -rf locked unlocked out &&
>> +               git worktree unlock locked &&
>> +               git worktree prune" &&
>>         git worktree add --detach locked master &&
>>         git worktree add --detach unlocked master &&
>>         git worktree lock locked &&
>
> You might need to be a bit more careful here. If the test fails before
> the worktree is locked, then the `git worktree unlock` in the cleanup
> code will return an error, which will make the code executed by
> test_when_finished() fail, as well, which makes it harder to debug
> problems. Moving the `unlock` cleanup after you know the lock
> succeeded will address this issue:
>
>     test_when_finished "rm -rf locked unlocked out && git worktree prune" &&
>     git worktree add --detach locked master &&
>     git worktree add --detach unlocked master &&
>     git worktree lock locked &&
>     test_when_finished "git worktree unlock locked" &&
>     ...
>
> Same comment applies to other new tests added by this patch which lock
> worktrees.
>

>> @@ -71,6 +73,96 @@ test_expect_success '"list" all worktrees with locked annotation' '
>> +test_expect_success '"list" all worktrees with prunable consistent with "prune"' '
>> +       test_when_finished "rm -rf prunable out && git worktree prune" &&
>> +       git worktree add --detach prunable &&
>> +       rm -rf prunable &&
>> +       git worktree list >out &&
>> +       grep "/prunable  *[0-9a-f].* prunable$" out &&
>> +       git worktree prune --verbose >out &&
>> +       test_i18ngrep "^Removing worktrees/prunable" out
>> +'
>
> To be trustworthy, doesn't this test also need to have an unprunable
> worktree, and test that `git worktree list` doesn't annotate it as
> "prunable" _and_ that `git worktree prune` didn't prune it? Otherwise,
> we really don't know that the two commands agree about what is and is
> not prunable -- we only know they agree that this particular worktree
> was prunable.
>

>> +test_expect_success '"list" all worktrees --porcelain with newline escaped locked reason' '
>> +       test_when_finished "rm -rf locked_lf locked_crlf reason_lf reason_crlf out actual expect reason &&
>> +               git worktree unlock locked_lf &&
>> +               git worktree unlock locked_crlf &&
>> +               git worktree prune" &&
>
> Nit: It's not a big deal, but we don't normally bother cleaning up
> every junk file in tests, such as `out`, `actual`, `expect` if those
> files aren't going to be a problem for subsequent tests. We are
> explicitly cleaning up the worktrees in these tests because this
> script is all about testing worktree behavior, and some random
> leftover worktree could be a problem for other tests. I don't care
> strongly one way or the other, but I worry a tiny bit that the list of
> files being cleaned up could become outdated as changes are made to
> the tests later...
>

>> +test_expect_success '"list" all worktrees --porcelain with prunable' '
>> +       test_when_finished "rm -rf prunable list out && git worktree prune" &&
>> +       git worktree add --detach prunable &&
>> +       rm -rf prunable &&
>> +       git worktree list --porcelain >out &&
>> +       test_i18ngrep "^prunable gitdir file points to non-existent location$" out
>> +'
>
> ... for instance, the file `list` being cleaned up in this test is not
> even created by this test.
>