On Thu, Dec 1, 2022 at 7:07 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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...
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> 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.
