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

[Eric Sunshine <sunshine@sunshineco.com>]

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.

> +
> +test_expect_success '"list" all worktrees --verbose and --porcelain mutually exclusive' '
> +       test_must_fail git worktree list --verbose --porcelain
> +'

Nit: This test title could probably be shortened to:

    '"list' --verbose and --porcelain mutually exclusive' '

Finally, I haven't tried it myself, but I was wondering if it is
possible to make a test which shows both "locked" and "prunable"
annotations for the same worktree. Would that be possible by
specifying a particular value for `--expire`? If it's too much work,
don't worry about it.