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

From: Andrei Rybak <rybak.a.v@gmail.com>
Date: Feb 1, 2023, 02:21 UTC
Message-ID: <b9a38b83-cdfe-0389-3097-c20a699f183c@gmail.com>
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

Show 22 quoted lines

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

Show 10 quoted lines

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

Show 7 quoted lines

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

Show 20 quoted lines

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