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

> 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.