Re: [PATCH] ls-tree: test for the regression in 9c4d58ff2c3
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jun 2, 2022, 15:18 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2206021703110.349@tvgsbejvaqbjf.bet>
- In-Reply-To
- <patch-1.1-0fdfec624eb-20220531T171908Z-avarab@gmail.com>
Hi Ævar,
On Tue, 31 May 2022, Ævar Arnfjörð Bjarmason wrote:
Show 5 quoted lines
> Add a test for the regression introduced in my 9c4d58ff2c3 (ls-tree: > split up "fast path" callbacks, 2022-03-23) and fixed in > 350296cc789 (ls-tree: `-l` should not imply recursive listing, > 2022-04-04), and test for the test of ls-tree option/mode combinations > to make sure we don't have other blind spots.
That's adding a lot of new test cases, which is not free of cost.
> Range-diff: > [... 433 lines, essentially a rewrite...]
Pasting 443 lines of range-diff also is not free of cost.
Show 15 quoted lines
> diff --git a/t/t3105-ls-tree-output.sh b/t/t3105-ls-tree-output.sh
> new file mode 100755
> index 00000000000..29511d9331b
> --- /dev/null
> +++ b/t/t3105-ls-tree-output.sh
> @@ -0,0 +1,192 @@
> +#!/bin/sh
> +
> +test_description='ls-tree output'
> +
> [...]
> +'
> +
> +test_ls_tree_format_mode_output () {
> + local opts=$1 &&This line does not quote `$1`, but...
Show 39 quoted lines
> + shift &&
> + cat >expect &&
> +
> + while test $# -gt 0
> + do
> + local mode="$1" &&
> + shift &&
> +
> + test_expect_success "'ls-tree $opts${mode:+ $mode}' output" '
> + git ls-tree ${mode:+$mode }$opts HEAD >actual &&
> + test_cmp expect actual
> + '
> +
> + case "$opts" in
> + --full-tree)
> + test_expect_success "'ls-tree $opts${mode:+ $mode}' output (via subdir, fails)" '
> + test_must_fail git -C dir ls-tree --full-name ${mode:+$mode }$opts HEAD -- ../
> + '
> + ;;
> + *)
> + test_expect_success "'ls-tree $opts${mode:+ $mode}' output (via subdir)" '
> + git -C dir ls-tree --full-name ${mode:+$mode }$opts HEAD -- ../ >actual &&
> + test_cmp expect actual
> + '
> + ;;
> + esac
> + done
> +}
> +
> [...]
> +## opt = --object-only --abbrev
> +test_expect_success 'setup: HEAD_short_* variables' '
> + HEAD_short_gitmodules=$(git rev-parse --short HEAD:.gitmodules) &&
> + HEAD_short_dir=$(git rev-parse --short HEAD:dir) &&
> + HEAD_short_top_file=$(git rev-parse --short HEAD:top-file.t) &&
> + HEAD_short_submodule=$(git rev-parse --short HEAD:submodule) &&
> + HEAD_short_dir_sub_file=$(git rev-parse --short HEAD:dir/sub-file.t)
> +'
> +test_ls_tree_format_mode_output "--object-only --abbrev" "" "-t" <<-EOF... this line passes a first argument that contains spaces. Hence the tests fail in CI: https://github.com/git/git/runs/6703333447?check_suite_focus=true
Further, since this failure is outside of any `test_expect_success` or `test_expect_failure`, the error message about this is not even included in the weblogs (but of course it is included in the full logs that are included in the build artifacts). For the record, here is the error message:
[...]
ok 35 - 'ls-tree --object-only -r' output (via subdir) + git rev-parse --short HEAD:.gitmodules + HEAD_short_gitmodules=6da7993 + git rev-parse --short HEAD:dir + HEAD_short_dir=aba57e0 + git rev-parse --short HEAD:top-file.t + HEAD_short_top_file=02dad95 + git rev-parse --short HEAD:submodule + HEAD_short_submodule=61a63ac + git rev-parse --short HEAD:dir/sub-file.t + HEAD_short_dir_sub_file=a150abd
ok 36 - setup: HEAD_short_* variables t3105-ls-tree-output.sh: 20: local: --abbrev: bad variable name FATAL: Unexpected exit with code 2
Note: I am only pointing out why the CI/PR run fails, I have not formed any opinion on the actual code other than noticing a lot of what might be repetitive lines and suspecting that adding this many new test cases increases the runtime of the test suite and thereby adds to developer friction and the benefit (namely, to catch future regressions) might not justify that. But again, I have not fully formed an opinion about this patch.
Ciao, Johannes