diff mbox series

[1/2] t6300: unify %(trailers) and %(contents:trailers) tests

Message ID bd0bb8d0ef0936866c2a957e5391424a7481a33c.1597841551.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Fix trailers atom bug and improved tests | expand

Commit Message

Philippe Blain via GitGitGadget Aug. 19, 2020, 12:52 p.m. UTC 
From: Hariom Verma <hariom18599@gmail.com>

Currently, there are different tests for testing %(trailers) and
%(contents:trailers) causing redundant copy.

Its time to get rid of duplicate code.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Heba Waly <heba.waly@gmail.com>
Signed-off-by: Hariom Verma <hariom18599@gmail.com>
---
 t/t6300-for-each-ref.sh | 50 +++++++++--------------------------------
 1 file changed, 11 insertions(+), 39 deletions(-)

Comments

Junio C Hamano Aug. 19, 2020, 5:31 p.m. UTC | #1 
"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index a83579fbdf..495848c881 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -776,60 +776,39 @@ test_expect_success 'set up trailers for next test' '
>  '
>  
>  test_expect_success '%(trailers:unfold) unfolds trailers' '
> -	git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
>  	{
>  		unfold <trailers
>  		echo
>  	} >expect &&
> +	git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
> +	test_cmp expect actual &&
> +	git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
>  	test_cmp expect actual
>  '

Hmph, what is this one doing?  Ah, OK, trailers:unfold is tested as
before (just the steps to prepare 'expect' and 'actual' got swapped),
and because the same expectation holds for contents:trailers:unfold,
we can test it at the same.   Makes sense.

>  test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
> -	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
> -	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
> -	test_cmp actual reverse &&
>  	{
>  		grep -v patch.description <trailers | unfold &&
>  		echo
>  	} >expect &&
> +	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
> +	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
> +	test_cmp actual reverse &&
> +	test_cmp expect actual &&

This uses different pattern.  It may be cleaner to test one side at
a time, as we have prepared the 'expect' that should be the same for
both, and compare with the expected pattern one at a time; that would
eliminate the need for 'reverse', too.  I.e.

	{
		grep -v patch.description trailers | unfold && echo
	} >expect &&
	git for-each-ref ... only,unfold ... >actual &&
	test_cmp expect actual &&
	git for-each-ref ... unfold,only ... >actual &&
	test_cmp expect actual &&

> @@ -839,14 +818,7 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
>  	fatal: unknown %(trailers) argument: unsupported
>  	EOF
>  	test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
> -	test_i18ncmp expect actual
> -'
> -
> -test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
> -	# error message cannot be checked under i18n
> -	cat >expect <<-EOF &&
> -	fatal: unknown %(trailers) argument: unsupported
> -	EOF
> +	test_i18ncmp expect actual &&
>  	test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
>  	test_i18ncmp expect actual
>  '

Doesn't this highlight a small bug, where an end-user request for an
unknown %(contents:trailers:unsupported) is flagged as an error
about %(trailers)?  Is it OK because we expect that users who use
the longer %(contents:trailers) to know that it is a synonym for
%(trailers) and the latter is the official way to write it?

Thanks.
Hariom verma Aug. 21, 2020, 10:03 a.m. UTC | #2 
Hi,

On Wed, Aug 19, 2020 at 11:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > @@ -839,14 +818,7 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '
> >       fatal: unknown %(trailers) argument: unsupported
> >       EOF
> >       test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
> > -     test_i18ncmp expect actual
> > -'
> > -
> > -test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
> > -     # error message cannot be checked under i18n
> > -     cat >expect <<-EOF &&
> > -     fatal: unknown %(trailers) argument: unsupported
> > -     EOF
> > +     test_i18ncmp expect actual &&
> >       test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
> >       test_i18ncmp expect actual
> >  '
>
> Doesn't this highlight a small bug, where an end-user request for an
> unknown %(contents:trailers:unsupported) is flagged as an error
> about %(trailers)?  Is it OK because we expect that users who use
> the longer %(contents:trailers) to know that it is a synonym for
> %(trailers) and the latter is the official way to write it?

Maybe.

Another way of thinking is...
'trailers' is an argument to 'contents', likewise here 'unsupported'
is an argument to trailers.
Technically, the error message is correct.

Again, I think views on this are highly subjective.

Thanks,
Hariom
diff mbox series

Patch

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index a83579fbdf..495848c881 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -776,60 +776,39 @@  test_expect_success 'set up trailers for next test' '
 '
 
 test_expect_success '%(trailers:unfold) unfolds trailers' '
-	git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
 	{
 		unfold <trailers
 		echo
 	} >expect &&
+	git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success '%(trailers:only) shows only "key: value" trailers' '
-	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
 	{
 		grep -v patch.description <trailers &&
 		echo
 	} >expect &&
+	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
+	test_cmp expect actual &&
+	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '
-	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
-	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
-	test_cmp actual reverse &&
 	{
 		grep -v patch.description <trailers | unfold &&
 		echo
 	} >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success '%(contents:trailers:unfold) unfolds trailers' '
-	git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual &&
-	{
-		unfold <trailers
-		echo
-	} >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' '
-	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
-	{
-		grep -v patch.description <trailers &&
-		echo
-	} >expect &&
-	test_cmp expect actual
-'
-
-test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '
+	git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual &&
+	git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse &&
+	test_cmp actual reverse &&
+	test_cmp expect actual &&
 	git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual &&
 	git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse &&
 	test_cmp actual reverse &&
-	{
-		grep -v patch.description <trailers | unfold &&
-		echo
-	} >expect &&
 	test_cmp expect actual
 '
 
@@ -839,14 +818,7 @@  test_expect_success '%(trailers) rejects unknown trailers arguments' '
 	fatal: unknown %(trailers) argument: unsupported
 	EOF
 	test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual &&
-	test_i18ncmp expect actual
-'
-
-test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '
-	# error message cannot be checked under i18n
-	cat >expect <<-EOF &&
-	fatal: unknown %(trailers) argument: unsupported
-	EOF
+	test_i18ncmp expect actual &&
 	test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual &&
 	test_i18ncmp expect actual
 '