Re: [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script

From: Andrei Rybak <rybak.a.v@gmail.com>
To: Shuqi Liang <cheskaqiqi@gmail.com>, git@vger.kernel.org
Subject: Re: [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script
Date: Wed, 1 Feb 2023 03:21:03 +0100	[thread overview]
Message-ID: <b9a38b83-cdfe-0389-3097-c20a699f183c@gmail.com> (raw)
In-Reply-To: <20230131224929.2018546-1-cheskaqiqi@gmail.com>

Hi Shuqi Liang,

> Subject: [GSoC][PATCH] t/t4113-apply-ending.sh: Modernize a test script

For patches that change a single test, the subject line can include just
the "t" and the number.  The part after the colon should start with a
lowercase letter.  Something like

     t4113: modernize test style

On 31/01/2023 23:49, Shuqi Liang wrote:
> 
> I cleaned up some old style in test script.

Commit message should start with description of the existing problem
in present tense, something like:

     Test scripts in file t4113-apply-ending.sh are written in old style,
     where the test_expect_success command and test title are written on
     separate lines ...

Then changes should be described using imperative mood, as if you are
giving commands to the codebase.  See section "[[describe-changes]]"
in "Documentation/SubmittingPatches" for details.

You can also find examples of existing commit messages for similar
changes:

     $ git log --no-merges --grep='modernize' -- t

> 
> for example :
> 
> * old style:
> 
>      test_expect_success \
>          'title' \
>          'body line 1 &&
>          body line 2'
> 
>    should become:
> 
>      test_expect_success 'title' '
>          body line 1 &&
>          body line 2
>      '
> 
> 
> 
> 
> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
 > Hi,I'm Shuqi Liang.a junior student majors in Computer Science at
 > University of Western Ontario.

Welcome!

>   t/t4113-apply-ending.sh | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
> index 66fa51591e..aa57895b22 100755
> --- a/t/t4113-apply-ending.sh
> +++ b/t/t4113-apply-ending.sh
> @@ -24,13 +24,14 @@ echo 'a' >file
>   echo 'b' >>file
>   echo 'c' >>file

A "modern" test could also do such preparation for test files as
part of its "setup" step.  This could its own patch in the same
series, separate from style changes.

In case of t4113, files "test-patch" and "file" are created twice.
The second creation of the files could be either its own step
'setup for apply at the beginning', or incorporated into the step
'apply at the beginning'.

Section "Recommended style" in t/README also has some notes about
how heredocs should be indented.

>   
> -test_expect_success setup \
> -    'git update-index --add file'
> -
> +test_expect_success setup '
> +    git update-index --add file
> +'

While changing the quoting around test tiles and commands, the
indentation with spaces could also be changed to TABs.

>   # test

If the setup code on top level of the file is moved into test
steps, this comment and the "# setup" comment at line 11 will
become unnecessary.

>   
> -test_expect_success 'apply at the end' \
> -    'test_must_fail git apply --index test-patch'
> +test_expect_success 'apply at the end' '
> +    test_must_fail git apply --index test-patch
> +'
>   
>   cat >test-patch <<\EOF
>   diff a/file b/file
> @@ -47,7 +48,8 @@ b
>   c'
>   git update-index file
>   
> -test_expect_success 'apply at the beginning' \
> -	'test_must_fail git apply --index test-patch'
> +test_expect_success 'apply at the beginning' '
> +    test_must_fail git apply --index test-patch
> +'
>   
>   test_done

Thanks.