threads / patch / 54356

patcht5534: split stdout and stderr redirection

Subject: [PATCH] t5534: split stdout and stderr redirection

## tl;dr

4 messages between Oct 6, 2020 and Oct 7, 2020. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Đoàn Trần Công Danh· Oct 6, 2020, 15:08 UTC · lore

On atomic pushing failure with GnuPG, we expect a very specific output in stdout due to `--porcelain` switch.

On such failure, we also write down some helpful hint into stderr in order to help user understand what happens and how to continue from those failures.

On a lot of system, those hint (in stderr) will be flushed first, then those messages in stdout will be flushed. In such systems, the current test code is fine as is.

However, we don't have such guarantee, (at least) there're some real systems that writes those stream interleaved. On such systems, we may see the stderr stream written in the middle of stdout stream.

Let's split those stream redirection. By splitting those stream, the output stream will contain exactly what we want to compare, thus, saving us a "sed" invocation.

While we're at it, change the `test_i18ncmp` to `test_cmp` because we will never translate those messages (because of `--porcelain`).

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 Arguably, I would say it's OK to change the:
 	
 	test_i18ngrep ! "gpg failed to sign"
 to:
 	! grep "gpg failed to sign"
 since the latter will be correct even if GIT_TEST_GETTEXT_POISON=true
 t/t5534-push-signed.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Show changes to t/t5534-push-signed.sh +3 −4
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 7e928aff66..af0385fb89 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -282,10 +282,9 @@ test_expect_success GPG 'failed atomic push does not execute GPG' '
 	EOF
 	test_must_fail env PATH="$TRASH_DIRECTORY:$PATH" git push \
 			--signed --atomic --porcelain \
-			dst noop ff noff >out 2>&1 &&
+			dst noop ff noff >out 2>err &&
 
-	test_i18ngrep ! "gpg failed to sign" out &&
-	sed -n -e "/^To dst/,$ p" out >actual &&
+	test_i18ngrep ! "gpg failed to sign" err &&
 	cat >expect <<-EOF &&
 	To dst
 	=	refs/heads/noop:refs/heads/noop	[up to date]
@@ -293,7 +292,7 @@ test_expect_success GPG 'failed atomic push does not execute GPG' '
 	!	refs/heads/noff:refs/heads/noff	[rejected] (non-fast-forward)
 	Done
 	EOF
-	test_i18ncmp expect actual
+	test_cmp expect out
 '
 
 test_done

-- 
2.28.0.849.g665beee440
Junio C Hamano· Oct 6, 2020, 19:11 UTC · re: Đoàn Trần Công Danh · lore

Re: [PATCH] t5534: split stdout and stderr redirection

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
Show 18 quoted lines
> On atomic pushing failure with GnuPG, we expect a very specific output
> in stdout due to `--porcelain` switch.
>
> On such failure, we also write down some helpful hint into stderr
> in order to help user understand what happens and how to continue from
> those failures.
>
> On a lot of system, those hint (in stderr) will be flushed first,
> then those messages in stdout will be flushed. In such systems, the
> current test code is fine as is.
>
> However, we don't have such guarantee, (at least) there're some real
> systems that writes those stream interleaved. On such systems, we may
> see the stderr stream written in the middle of stdout stream.
>
> Let's split those stream redirection. By splitting those stream,
> the output stream will contain exactly what we want to compare,
> thus, saving us a "sed" invocation.
Makes sense.
> While we're at it, change the `test_i18ncmp` to `test_cmp` because we
> will never translate those messages (because of `--porcelain`).

Good thinking. It would make sure that we will catch when we accidentally mark messages meant for --porcelain with the gettext-poison tests.

Show 11 quoted lines
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  Arguably, I would say it's OK to change the:
>  	
>  	test_i18ngrep ! "gpg failed to sign"
>
>  to:
>
>  	! grep "gpg failed to sign"
>
>  since the latter will be correct even if GIT_TEST_GETTEXT_POISON=true

Is it because we haven't managed to translate this particular message, or is it because we should never ever translate it perhaps because the message is meant for machine consumption? If the latter, yes, I agree with the reasoning, but I do not see a reason why this message should never be translated.

Show 29 quoted lines
>  t/t5534-push-signed.sh | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
> index 7e928aff66..af0385fb89 100755
> --- a/t/t5534-push-signed.sh
> +++ b/t/t5534-push-signed.sh
> @@ -282,10 +282,9 @@ test_expect_success GPG 'failed atomic push does not execute GPG' '
>  	EOF
>  	test_must_fail env PATH="$TRASH_DIRECTORY:$PATH" git push \
>  			--signed --atomic --porcelain \
> -			dst noop ff noff >out 2>&1 &&
> +			dst noop ff noff >out 2>err &&
>  
> -	test_i18ngrep ! "gpg failed to sign" out &&
> -	sed -n -e "/^To dst/,$ p" out >actual &&
> +	test_i18ngrep ! "gpg failed to sign" err &&
>  	cat >expect <<-EOF &&
>  	To dst
>  	=	refs/heads/noop:refs/heads/noop	[up to date]
> @@ -293,7 +292,7 @@ test_expect_success GPG 'failed atomic push does not execute GPG' '
>  	!	refs/heads/noff:refs/heads/noff	[rejected] (non-fast-forward)
>  	Done
>  	EOF
> -	test_i18ncmp expect actual
> +	test_cmp expect out
>  '
>  
>  test_done
Đoàn Trần Công Danh· Oct 7, 2020, 00:15 UTC · re: Junio C Hamano · lore

Re: [PATCH] t5534: split stdout and stderr redirection

On 2020-10-06 12:11:00-0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
> 
> >  Arguably, I would say it's OK to change the:
> >  	
> >  	test_i18ngrep ! "gpg failed to sign"
> >
> >  to:
> >
> >  	! grep "gpg failed to sign"
> >
> >  since the latter will be correct even if GIT_TEST_GETTEXT_POISON=true
> 
> Is it because we haven't managed to translate this particular
> message, or is it because we should never ever translate it perhaps
> because the message is meant for machine consumption?
We translated that message.
> If the latter, yes, I agree with the reasoning,
> but I do not see a reason why this message should never be translated.

I was wrong, I thought test_i18ngrep will blindly "return 0", it does call grep on "test_i18ngrep !"

Please ignore my non-sense in the foot-note. I guess the main part is still good, though.

Thanks,
-- 
Danh
Junio C Hamano· Oct 7, 2020, 16:21 UTC · re: Đoàn Trần Công Danh · lore

Re: [PATCH] t5534: split stdout and stderr redirection

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
Show 18 quoted lines
> On 2020-10-06 12:11:00-0700, Junio C Hamano <gitster@pobox.com> wrote:
>> Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
>> 
>> >  Arguably, I would say it's OK to change the:
>> >  	
>> >  	test_i18ngrep ! "gpg failed to sign"
>> >
>> >  to:
>> >
>> >  	! grep "gpg failed to sign"
>> >
>> >  since the latter will be correct even if GIT_TEST_GETTEXT_POISON=true
>> 
>> Is it because we haven't managed to translate this particular
>> message, or is it because we should never ever translate it perhaps
>> because the message is meant for machine consumption?
>
> We translated that message.

If so wouldn't the message shown under GIT_TEST_GETTEXT_POISON as something that does not even remotely resemble 'gpg failed to sign'?

... Ahh, hmm, cute but true. As long as our assertion is "the output should not have that message in it", '! grep' would do the job.

But that probably is not a good example to leave for others to copy and paste without thinking.

Thanks.

← back to recent threads