Re: [PATCH v6 1/6] t6030-bisect-porcelain: add tests to control bisect run exit cases

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 2, 2021, 21:44 UTC
Message-ID: <xmqqv93iirev.fsf@gitster.g>
In-Reply-To: <20210902090421.93113-2-mirucam@gmail.com>

Miriam Rubio <mirucam@gmail.com> writes:

Show 26 quoted lines

> There is a gap on bisect run test coverage related with error exits.
> Add two tests to control these error cases.
>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
>  t/t6030-bisect-porcelain.sh | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index a1baf4e451..e61b8143fd 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -962,4 +962,15 @@ test_expect_success 'bisect handles annotated tags' '
>  	grep "$bad is the first bad commit" output
>  '
>  
> +test_expect_success 'bisect run fails with exit code equals or greater than 128' '
> +	write_script test_script.sh <<-\EOF &&
> +	exit 128 >/dev/null
> +	EOF
> +	test_must_fail git bisect run ./test_script.sh > my_bisect_log.txt &&
> +	write_script test_script.sh <<-\EOF &&
> +	exit 255 >/dev/null
> +	EOF
> +	test_must_fail git bisect run ./test_script.sh >> my_bisect_log.txt
> +'

Two and a half glitches.

 * It is not obvious why you need to redirect output from "exit" to
   /dev/null; drop them or explain the reason in the proposed log
   message, perhaps.

 * The contents of my_bisect_log.txt is never inspected.  If it does
   not matter how the command fails, not inspecting is perfectly OK,
   but then perhaps not capturing it is the right thing to do?  We
   do not even want to redirect the output to /dev/null, as the
   output from the commands run in these test pieces will not be
   shown unless the test scripts are run under an option for
   debugging purposes.

 * Style: no space after ">" or ">>" before my_bisect_log.txt

Thanks.