[v5,3/3] t4113: put executable lines to test_expect_success

Commit Message

Shuqi Liang Feb. 9, 2023, 3:44 p.m. UTC

As t/README says, put all code inside test_expect_success and
other assertions. This script is written in old style,where there are
some executable lines outside test_expect_success. Put the executable
lines inside the test_expect_success.

As t/README says,use "<<-" instead of "<<"
to strip leading TABs used for indentation. Change the "<<" to "<<-"

for example:
-cat >test-patch <<\EOF
-diff a/file b/file

 test_expect_success 'apply at the beginning' '
+       cat >test-patch <<-\EOF
+       diff a/file b/file
+       --- a/file

As t/README says,chain test assertions.Chain this test assertions
with &&.

For example:

-cat >test-patch <<\EOF
-diff --git a/file b/file

+ cat >test-patch <<-\EOF &&
+ diff --git a/file b/file

This script is written in old style,where there are something like

        echo x >file &&
        echo y >>file &&
        echo z >>file

  Change it to this stlye :
        {
        echo x &&
        echo y &&
        echo z
        } >file

In order to escape for executable lines inside the test_expect_success.
Change ' in executable lines to '\'' in order to escape.

Signed-off-by: Shuqi Liang <[e-mail]>
---
 t/t4113-apply-ending.sh | 61 ++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 31 deletions(-)

Comments

Eric Sunshine Feb. 15, 2023, 1:09 a.m. UTC | #1

On Thu, Feb 9, 2023 at 11:00 AM Shuqi Liang <[e-mail]> wrote:
> As t/README says, put all code inside test_expect_success and
> other assertions. This script is written in old style,where there are
> some executable lines outside test_expect_success. Put the executable
> lines inside the test_expect_success.

Although it's true that t/README explains why code should be placed
inside tests, you can help readers out by simply explaining the reason
here in the commit message. For instance, you might replace the above
paragraph with:

    Some old test scripts have setup code outside of tests. This
    is problematic since any failures of the setup code will go
    unnoticed. Therefore, move setup code into the tests themselves
    so that failures are properly flagged.

As for the rest of the commit message...

> As t/README says,use "<<-" instead of "<<"
> to strip leading TABs used for indentation. Change the "<<" to "<<-"
>
> for example:
> -cat >test-patch <<\EOF
> -diff a/file b/file
>
>  test_expect_success 'apply at the beginning' '
> +       cat >test-patch <<-\EOF
> +       diff a/file b/file
> +       --- a/file

Certain changes are considered obvious by reviewers, so you don't need
to mention them explicitly in the commit message. This is one such
change. Any reviewer who sees that you indented the here-doc body to
match the indentation of the rest of the test body will understand why
you changed `<<` to `<<-` without the commit message having to explain
it.

> As t/README says,chain test assertions.Chain this test assertions
> with &&.
>
> For example:
>
> -cat >test-patch <<\EOF
> -diff --git a/file b/file
>
> + cat >test-patch <<-\EOF &&
> + diff --git a/file b/file

Same thing. Reviewers understand that all code inside a test body must
have an intact &&-chain, so you needn't mention this in the commit
message.

> This script is written in old style,where there are something like
>
>         echo x >file &&
>         echo y >>file &&
>         echo z >>file
>
>   Change it to this stlye :
>         {
>         echo x &&
>         echo y &&
>         echo z
>         } >file

This is similar. This is such a simple style change, and the code
fragment itself is so tiny, that a reviewer can understand this change
without the commit message spelling it out.

> In order to escape for executable lines inside the test_expect_success.
> Change ' in executable lines to '\'' in order to escape.

Likewise.

Reviewers appreciate well-explained commit messages, but they also
appreciate succinctness. Although it may not always be obvious how
much to write in a commit message, you can assume that reviewers will
understand obvious changes simply by reading the patch itself, thus
you don't need to mention every little detail in the commit message.
The important thing to mention in the commit message is the
explanation of _why_ the change is being made, plus any changes which
might not be obvious. In this case, all the changes are obvious, so,
really, you can collapse this entire commit message to just the first
paragraph.

> Signed-off-by: Shuqi Liang <[e-mail]>
> ---
> diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
> @@ -8,46 +8,45 @@ test_description='git apply trying to add an ending line.
> -# setup
> -

Good to see that you got rid of the now-unnecessary comment.

> -cat >test-patch <<\EOF
> -diff --git a/file b/file
> ---- a/file
> -+++ b/file
> -@@ -1,2 +1,3 @@
> - a
> - b
> -+c
> -EOF
> -
> -echo 'a' >file
> -echo 'b' >>file
> -echo 'c' >>file
> -
>  test_expect_success setup '
> +       cat >test-patch <<-\EOF &&
> +       diff --git a/file b/file
> +       --- a/file
> +       +++ b/file
> +       @@ -1,2 +1,3 @@
> +        a
> +        b
> +       +c
> +       EOF

Okay.

> +       {
> +       echo '\''a'\'' &&
> +       echo '\''b'\'' &&
> +       echo '\''c'\''
> +       } >file &&

A few comments:

This is unnecessarily confusing. Although this does work, it would be
sufficient just to change the single-quotes to double-quotes, like
this:

    {
    echo "a" &&
    echo "b" &&
    echo "c"
    } >file &&

Even simpler, you could drop the quotes altogether for such a simple case:

    {
    echo a &&
    echo b &&
    echo c
    } >file &&

However, as mentioned elsewhere in this thread, a really succinct way
to do this, taking advantage of modern style would be to use
test_write_lines(), so the five lines collapse to a single line:

    test_write_lines a b c >file &&

> -cat >test-patch <<\EOF
> -diff a/file b/file
> ---- a/file
> -+++ b/file
> -@@ -1,2 +1,3 @@
> -+a
> - b
> - c
> -EOF
> -
> -echo >file 'a
> -b
> -c'
> -git update-index file
>
>  test_expect_success 'apply at the beginning' '
> +       cat >test-patch <<-\EOF &&
> +       diff a/file b/file
> +       --- a/file
> +       +++ b/file
> +       @@ -1,2 +1,3 @@
> +       +a
> +        b
> +        c
> +       EOF
> +
> +       echo >file '\''a
> +       b
> +       c'\'' &&

Same comment about simply using double-quotes instead of
single-quotes, however, this is also another really good place to use
test_write_lines:

    test_write_lines a b c >file &&

Shuqi Liang Feb. 15, 2023, 2:40 a.m. UTC | #2

On Tue, Feb 14, 2023 at 8:09 PM Eric Sunshine <[e-mail]> wrote:

> Although it's true that t/README explains why code should be placed
> inside tests, you can help readers out by simply explaining the reason
> here in the commit message. For instance, you might replace the above
> paragraph with:
>
>     Some old test scripts have setup code outside of tests. This
>     is problematic since any failures of the setup code will go
>     unnoticed. Therefore, move setup code into the tests themselves
>     so that failures are properly flagged.
>
Thanks, that really makes the change more clear for the readers. I learn a lot.

> Reviewers appreciate well-explained commit messages, but they also
> appreciate succinctness. Although it may not always be obvious how
> much to write in a commit message, you can assume that reviewers will
> understand obvious changes simply by reading the patch itself, thus
> you don't need to mention every little detail in the commit message.
> The important thing to mention in the commit message is the
> explanation of _why_ the change is being made, plus any changes which
> might not be obvious. In this case, all the changes are obvious, so,
> really, you can collapse this entire commit message to just the first
> paragraph.

Yeah, the commit looks very wordy. I'll make it more succinct.
>     test_write_lines a b c >file &&

> Same comment about simply using double quotes instead of
> single-quotes, however, this is also another really good place to use
> test_write_lines:
>
>     test_write_lines a b c >file &&

Thanks for the tips!

(Sorry for sending the V6 to you twice I send it by accident .)

-------------------------
Thanks
Shuqi

Patch

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index a470c9ce7b..c70429bd07 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -8,46 +8,45 @@  test_description='git apply trying to add an ending line.
 '
 . ./test-lib.sh
 
-# setup
-
-cat >test-patch <<\EOF
-diff --git a/file b/file
---- a/file
-+++ b/file
-@@ -1,2 +1,3 @@
- a
- b
-+c
-EOF
-
-echo 'a' >file
-echo 'b' >>file
-echo 'c' >>file
-
 test_expect_success setup '
+	cat >test-patch <<-\EOF &&
+	diff --git a/file b/file
+	--- a/file
+	+++ b/file
+	@@ -1,2 +1,3 @@
+	 a
+	 b
+	+c
+	EOF
+
+	{
+	echo '\''a'\'' &&
+	echo '\''b'\'' &&
+	echo '\''c'\''
+	} >file &&
 	git update-index --add file
 '
-# test
 
 test_expect_success 'apply at the end' '
 	test_must_fail git apply --index test-patch
 '
-cat >test-patch <<\EOF
-diff a/file b/file
---- a/file
-+++ b/file
-@@ -1,2 +1,3 @@
-+a
- b
- c
-EOF
-
-echo >file 'a
-b
-c'
-git update-index file
 
 test_expect_success 'apply at the beginning' '
+	cat >test-patch <<-\EOF &&
+	diff a/file b/file
+	--- a/file
+	+++ b/file
+	@@ -1,2 +1,3 @@
+	+a
+	 b
+	 c
+	EOF
+
+	echo >file '\''a
+	b
+	c'\'' &&
+	git update-index file &&
 	test_must_fail git apply --index test-patch
 '
+
 test_done