Re: [PATCH v2 4/8] test-lib-functions: add and use test_cmp_cmd

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 2, 2022, 01:33 UTC
Message-ID: <CAPig+cTfx4QYBdtSomPRM+BpW+3ELqaQxD7okKhXzykVkyH9NA@mail.gmail.com>
In-Reply-To: <patch-v2-4.8-c36060934a6-20221202T000227Z-avarab@gmail.com>

On Thu, Dec 1, 2022 at 7:07 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> Add a "test_cmp_cmd" helper for the common pattern discussed in the
> documentation being added here to "t/test-lib-functions.sh".

I'm somewhat "meh" on this, at least as it's sold here. Perhaps if it was sold as producing better diagnostic report for:

    test "$something" = "$(git blah)"

which covers many of the "fixes" in this patch then I'd be less jaded...

> By using this in we'll catch cases where "git" or "test-tool"
> errors (such as segfaults or abort()) were previously hidden, and we'd
> either pass the test, or fail in some subsequent assertion.

...which, I suppose, may be what this paragraph is saying, but it's not immediately obvious.

Minor subjective stuff aside...

Show 20 quoted lines

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> +# test_cmp_cmd is a convenience helper for doing the more verbose:
> +#
> +#      echo something >expect &&
> +#      <some-command-and-args> >actual &&
> +#      test_cmp expect actual
> +#
> +# As:
> +#
> +#      test_cmp_cmd something <some-command-and-args>
> +test_cmp_cmd () {
> +       local expect="$1" &&
> +       shift &&
> +       printf "%s\n" "$expect" >expect &&
> +       "$@" >actual 2>err &&
> +       test_must_be_empty err
> +       test_cmp expect actual
> +}

I do find it concerning that this clobbers 'actual', 'expect', and 'err', which are common enough names. I'd suggest using names unique to this function (perhaps 'test_cmp_cmd.actual', 'test_cmp_cmd.expect', etc.).

I think you also need to document the fact that it expects stderr output to be empty.