Re: [PATCH v7 01/12] t1800: add hook output stream tests

[Junio C Hamano <gitster@pobox.com>]

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

> Lack of test coverage in this area led to some regressions while
> converting the remaining hooks to the newer hook.[ch] API.
>
> Add some tests to verify hooks write to the expected output streams.
>
> Suggested-by: Patrick Steinhardt <ps@pks.im>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  t/t1800-hook.sh | 127 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 127 insertions(+)
>
> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> index 4feaf0d7be..0e4f93fb31 100755
> --- a/t/t1800-hook.sh
> +++ b/t/t1800-hook.sh
> @@ -184,4 +184,131 @@ test_expect_success 'stdin to hooks' '
>  	test_cmp expect actual
>  '
>  
> +check_stdout_separate_from_stderr () {
> +	for hook in "$@"
> +	do
> +		test_grep ! "Hook $hook stdout" stderr.actual &&
> +		test_grep ! "Hook $hook stderr" stdout.actual &&
> +		test_grep "Hook $hook stderr" stderr.actual &&
> +		test_grep "Hook $hook stdout" stdout.actual || return 1
> +	done
> +}
> +
> +check_stdout_merged_to_stderr () {
> +	test_grep ! "Hook .* stdout" stdout.actual &&
> +	test_grep ! "Hook .* stderr" stdout.actual &&
> +	for hook in "$@"
> +	do
> +		test_grep "Hook $hook stdout" stderr.actual &&
> +		test_grep "Hook $hook stderr" stderr.actual || return 1
> +	done
> +}

Asymmetry between the above two was a bit surprising, but the string
"the word 'hook' followed by something ending with 'stdout' or
'stderr'" is specific enough that the way the check makes sure
everything goes to stderr is probably fine.

> +test_expect_success 'client pre-push hook expects separate stdout and stderr' '
> +	test_when_finished "rm -f stdout.actual stderr.actual" &&
> +	git init --bare remote &&
> +	git remote add origin remote &&
> +	test_commit A &&
> +
> +	hook=pre-push &&
> +	test_hook $hook <<-EOF &&
> +	echo >&1 Hook $hook stdout
> +	echo >&2 Hook $hook stderr
> +	EOF
> +
> +	git push origin HEAD:main >stdout.actual 2>stderr.actual &&
> +	check_stdout_separate_from_stderr pre-push
> +'
> +
> +test_expect_success 'client hooks expect stdout redirected to stderr' '
> +	test_when_finished "rm -f stdout.actual stderr.actual" &&
> +	for hook in pre-commit post-commit post-checkout pre-merge-commit \
> +		prepare-commit-msg commit-msg post-merge post-rewrite reference-transaction \
> +		applypatch-msg pre-applypatch post-applypatch pre-rebase post-index-change

Slightly overlong lines above...

> +	do
> +		test_hook $hook <<-EOF || return 1
> +		echo >&1 Hook $hook stdout
> +		echo >&2 Hook $hook stderr
> +		EOF
> +	done &&
> +
> +	git checkout -B main &&
> +	git checkout -b branch-a &&
> +	test_commit commit-on-branch-a &&
> +
> +	# Trigger pre-commit, prepare-commit-msg, commit-msg, post-commit, reference-transaction

... and this one ...


> +	git commit --allow-empty -m "Test" >stdout.actual 2>stderr.actual &&
> +	check_stdout_merged_to_stderr pre-commit prepare-commit-msg commit-msg post-commit reference-transaction &&

... and this one.  I'll stop counting.

You commit, and then checkout, and then merge, and then
amend/rewrite, etc., all of which look quite sensible.  

These separate steps not being in individual test_expect_success and
instead in a single one chained together with &&- makes me suspect
that it would be inconvenient to tell which step is failing and to
debug when things start to break, though.

> +test_expect_success 'server hooks expect stdout redirected to stderr' '
> +	test_when_finished "rm -f stdout.actual stderr.actual" &&
> +	git init --bare remote-server &&
> +	git remote add origin-server remote-server &&
> +
> +	for hook in pre-receive update post-receive post-update
> +	do
> +		write_script remote-server/hooks/$hook <<-EOF || return 1
> +		echo >&1 Hook $hook stdout
> +		echo >&2 Hook $hook stderr
> +		EOF
> +	done &&
> +
> +	# Trigger pre-receive update post-receive post-update
> +	git push origin-server HEAD:new-branch >stdout.actual 2>stderr.actual &&
> +	check_stdout_merged_to_stderr pre-receive update post-receive post-update
> +'
> +
> +test_expect_success 'server push-to-checkout hook expects stdout redirected to stderr' '
> +	test_when_finished "rm -f stdout.actual stderr.actual" &&
> +	git init server &&
> +	git -C server checkout -b main &&
> +	test_config -C server receive.denyCurrentBranch updateInstead &&
> +	git remote add origin-server-2 server &&
> +
> +	write_script server/.git/hooks/push-to-checkout <<-EOF &&
> +	echo >&1 Hook push-to-checkout stdout
> +	echo >&2 Hook push-to-checkout stderr
> +	EOF
> +
> +	# Trigger push-to-checkout
> +	git push origin-server-2 HEAD:main >stdout.actual 2>stderr.actual &&
> +	check_stdout_merged_to_stderr push-to-checkout
> +'
> +
>  test_done

Looking good otherwise.  Thanks.