git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] ls-files: update test style
@ 2022-06-23  8:46 Li Linchao via GitGitGadget
  2022-06-23 10:50 ` Ævar Arnfjörð Bjarmason
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Li Linchao via GitGitGadget @ 2022-06-23  8:46 UTC (permalink / raw)
  To: git; +Cc: Li Linchao, Li Linchao

From: Li Linchao <lilinchao@oschina.cn>

Update test style in t/t30[*].sh for uniformity, that's to
keep test title the same line with helper function itself.

And update t/README to describe this test style.

Signed-off-by: Li Linchao <lilinchao@oschina.cn>
---
    ls-files: update test style
    
    Update test style in t/t30[*].sh for uniformity, that's to keep test
    title the same line with helper function itself.
    
    And update t/README to describe this test style.
    
    Signed-off-by: Li Linchao lilinchao@oschina.cn

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1269%2FCactusinhand%2Fllc%2Ffix-test-title-style-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1269/Cactusinhand/llc/fix-test-title-style-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1269

 t/README                           | 15 ++++++++++++++
 t/t3001-ls-files-others-exclude.sh | 24 +++++++++++-----------
 t/t3002-ls-files-dashpath.sh       | 33 +++++++++++++-----------------
 t/t3020-ls-files-error-unmatch.sh  | 12 +++++------
 t/t3060-ls-files-with-tree.sh      |  6 +++---
 5 files changed, 50 insertions(+), 40 deletions(-)

diff --git a/t/README b/t/README
index 309a31133c6..70205fba41b 100644
--- a/t/README
+++ b/t/README
@@ -560,6 +560,21 @@ Here are the "do's:"
    Even code that isn't a test per se, but merely some setup code
    should be inside a test assertion.
 
+ - Keep test title the same line with test helper function itself,
+   and end the line with a single quote.
+
+   Take test_expect_success helper for example, write it like:
+
+  test_expect_success 'test title to describe this test case' '
+  # test body
+  '
+
+   Instead of:
+
+  test_expect_success \
+  'test title to describe this test case' \
+  '# test body'
+
  - Chain your test assertions
 
    Write test code like this:
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index 48cec4e5f88..76361b92336 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -67,26 +67,26 @@ echo '!*.2
 
 allignores='.gitignore one/.gitignore one/two/.gitignore'
 
-test_expect_success \
-    'git ls-files --others with various exclude options.' \
-    'git ls-files --others \
+test_expect_success 'git ls-files --others with various exclude options.' '
+	git ls-files --others \
        --exclude=\*.6 \
        --exclude-per-directory=.gitignore \
        --exclude-from=.git/ignore \
        >output &&
-     test_cmp expect output'
+     test_cmp expect output
+'
 
 # Test \r\n (MSDOS-like systems)
 printf '*.1\r\n/*.3\r\n!*.6\r\n' >.gitignore
 
-test_expect_success \
-    'git ls-files --others with \r\n line endings.' \
-    'git ls-files --others \
+test_expect_success 'git ls-files --others with \r\n line endings.' '
+	git ls-files --others \
        --exclude=\*.6 \
        --exclude-per-directory=.gitignore \
        --exclude-from=.git/ignore \
        >output &&
-     test_cmp expect output'
+     test_cmp expect output
+'
 
 test_expect_success 'setup skip-worktree gitignore' '
 	git add $allignores &&
@@ -94,14 +94,14 @@ test_expect_success 'setup skip-worktree gitignore' '
 	rm $allignores
 '
 
-test_expect_success \
-    'git ls-files --others with various exclude options.' \
-    'git ls-files --others \
+test_expect_success 'git ls-files --others with various exclude options.' '
+	git ls-files --others \
        --exclude=\*.6 \
        --exclude-per-directory=.gitignore \
        --exclude-from=.git/ignore \
        >output &&
-     test_cmp expect output'
+     test_cmp expect output
+'
 
 test_expect_success !SANITIZE_LEAK 'restore gitignore' '
 	git checkout --ignore-skip-worktree-bits $allignores &&
diff --git a/t/t3002-ls-files-dashpath.sh b/t/t3002-ls-files-dashpath.sh
index 54d22a45dfb..adbe96fa2df 100755
--- a/t/t3002-ls-files-dashpath.sh
+++ b/t/t3002-ls-files-dashpath.sh
@@ -16,15 +16,14 @@ filesystem.
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-test_expect_success \
-	setup \
-	'echo frotz >path0 &&
+test_expect_success 'setup' '
+    echo frotz >path0 &&
 	echo frotz >./-foo &&
-	echo frotz >./--'
+	echo frotz >./--
+'
 
-test_expect_success \
-    'git ls-files without path restriction.' \
-    'git ls-files --others >output &&
+test_expect_success 'git ls-files without path restriction.' '
+    git ls-files --others >output &&
      test_cmp output - <<EOF
 --
 -foo
@@ -33,33 +32,29 @@ path0
 EOF
 '
 
-test_expect_success \
-    'git ls-files with path restriction.' \
-    'git ls-files --others path0 >output &&
+test_expect_success 'git ls-files with path restriction.' '
+    git ls-files --others path0 >output &&
 	test_cmp output - <<EOF
 path0
 EOF
 '
 
-test_expect_success \
-    'git ls-files with path restriction with --.' \
-    'git ls-files --others -- path0 >output &&
+test_expect_success 'git ls-files with path restriction with --.' '
+    git ls-files --others -- path0 >output &&
 	test_cmp output - <<EOF
 path0
 EOF
 '
 
-test_expect_success \
-    'git ls-files with path restriction with -- --.' \
-    'git ls-files --others -- -- >output &&
+test_expect_success 'git ls-files with path restriction with -- --.' '
+    git ls-files --others -- -- >output &&
 	test_cmp output - <<EOF
 --
 EOF
 '
 
-test_expect_success \
-    'git ls-files with no path restriction.' \
-    'git ls-files --others -- >output &&
+test_expect_success 'git ls-files with no path restriction.' '
+    git ls-files --others -- >output &&
 	test_cmp output - <<EOF
 --
 -foo
diff --git a/t/t3020-ls-files-error-unmatch.sh b/t/t3020-ls-files-error-unmatch.sh
index 2cbcbc0721b..8dd520bb331 100755
--- a/t/t3020-ls-files-error-unmatch.sh
+++ b/t/t3020-ls-files-error-unmatch.sh
@@ -19,12 +19,12 @@ test_expect_success 'setup' '
 	git commit -m "add foo bar"
 '
 
-test_expect_success \
-    'git ls-files --error-unmatch should fail with unmatched path.' \
-    'test_must_fail git ls-files --error-unmatch foo bar-does-not-match'
+test_expect_success 'git ls-files --error-unmatch should fail with unmatched path.' '
+    test_must_fail git ls-files --error-unmatch foo bar-does-not-match
+'
 
-test_expect_success \
-    'git ls-files --error-unmatch should succeed with matched paths.' \
-    'git ls-files --error-unmatch foo bar'
+test_expect_success 'git ls-files --error-unmatch should succeed with matched paths.' '
+    git ls-files --error-unmatch foo bar
+'
 
 test_done
diff --git a/t/t3060-ls-files-with-tree.sh b/t/t3060-ls-files-with-tree.sh
index b257c792a46..c350b4641f3 100755
--- a/t/t3060-ls-files-with-tree.sh
+++ b/t/t3060-ls-files-with-tree.sh
@@ -62,9 +62,9 @@ test_expect_success 'git ls-files --with-tree should succeed from subdir' '
 	)
 '
 
-test_expect_success \
-    'git ls-files --with-tree should add entries from named tree.' \
-    'test_cmp expected output'
+test_expect_success 'git ls-files --with-tree should add entries from named tree.' '
+	test_cmp expected output
+'
 
 test_expect_success 'no duplicates in --with-tree output' '
 	git ls-files --with-tree=HEAD >actual &&

base-commit: 5b71c59bc3b9365075e2a175aa7b6f2b0c84ce44
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ls-files: update test style
  2022-06-23  8:46 [PATCH] ls-files: update test style Li Linchao via GitGitGadget
@ 2022-06-23 10:50 ` Ævar Arnfjörð Bjarmason
  2022-06-24  4:57   ` lilinchao
  2022-06-23 17:09 ` Junio C Hamano
  2022-06-28  9:14 ` [PATCH v2] " Li Linchao via GitGitGadget
  2 siblings, 1 reply; 9+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-23 10:50 UTC (permalink / raw)
  To: Li Linchao via GitGitGadget; +Cc: git, Li Linchao


On Thu, Jun 23 2022, Li Linchao via GitGitGadget wrote:

> From: Li Linchao <lilinchao@oschina.cn>
>
> Update test style in t/t30[*].sh for uniformity, that's to
> keep test title the same line with helper function itself.

We have a few of these sorts of old style tests, and it's good to update
them.

>     Write test code like this:
> diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
> index 48cec4e5f88..76361b92336 100755
> --- a/t/t3001-ls-files-others-exclude.sh
> +++ b/t/t3001-ls-files-others-exclude.sh
> @@ -67,26 +67,26 @@ echo '!*.2
>  
>  allignores='.gitignore one/.gitignore one/two/.gitignore'
>  
> -test_expect_success \
> -    'git ls-files --others with various exclude options.' \
> -    'git ls-files --others \
> +test_expect_success 'git ls-files --others with various exclude options.' '
> +	git ls-files --others \
>         --exclude=\*.6 \
>         --exclude-per-directory=.gitignore \
>         --exclude-from=.git/ignore \
>         >output &&

This though really stops too short, here we end up with:

	<TAB>git-ls-files --others \
	<7 spaces>--exclude [...]

> -     test_cmp expect output'
> +     test_cmp expect output

And you've space-indented this test_cmp, presumably the below has the
same issues (I didn't check in detail)

Instead the argument lists should be <TAB><TAB> indented, and the rest
should be TAB indented.

> +'
>  
>  # Test \r\n (MSDOS-like systems)
>  printf '*.1\r\n/*.3\r\n!*.6\r\n' >.gitignore
>  
> -test_expect_success \
> -    'git ls-files --others with \r\n line endings.' \
> -    'git ls-files --others \
> +test_expect_success 'git ls-files --others with \r\n line endings.' '
> +	git ls-files --others \
>         --exclude=\*.6 \
>         --exclude-per-directory=.gitignore \
>         --exclude-from=.git/ignore \
>         >output &&
> -     test_cmp expect output'
> +     test_cmp expect output
> +'

Aside from the above I think it's also worth incorporating all the
"printf", "echo", "cat" etc. that we do into the "test_expect_success"
themselves, and if they're needed by more than one test perhaps make
them a "setup" helper function (which would test_when_finished "rm -f
.gitignore" clean up after itself).

That's obviously bigger than some whitespace changes, so we could punt
on it for now, but as we're looking at this anyway we could convert
fully to a more modern style in a follow-up commit...

> -test_expect_success \
> -    'git ls-files with path restriction with --.' \
> -    'git ls-files --others -- path0 >output &&
> +test_expect_success 'git ls-files with path restriction with --.' '
> +    git ls-files --others -- path0 >output &&
>  	test_cmp output - <<EOF
>  path0
>  EOF
>  '

On the topic of leaving things on the table: here we could use "<<-EOF"
(or actually better "<<-\EOF") instead, and indent the here-doc, as we
usually do.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ls-files: update test style
  2022-06-23  8:46 [PATCH] ls-files: update test style Li Linchao via GitGitGadget
  2022-06-23 10:50 ` Ævar Arnfjörð Bjarmason
@ 2022-06-23 17:09 ` Junio C Hamano
  2022-06-24  5:05   ` lilinchao
  2022-06-28  9:14 ` [PATCH v2] " Li Linchao via GitGitGadget
  2 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2022-06-23 17:09 UTC (permalink / raw)
  To: Li Linchao via GitGitGadget; +Cc: git, Li Linchao

"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/t/README b/t/README
> index 309a31133c6..70205fba41b 100644
> --- a/t/README
> +++ b/t/README
> @@ -560,6 +560,21 @@ Here are the "do's:"
>     Even code that isn't a test per se, but merely some setup code
>     should be inside a test assertion.
>  
> + - Keep test title the same line with test helper function itself,
> +   and end the line with a single quote.
> +
> +   Take test_expect_success helper for example, write it like:
> +
> +  test_expect_success 'test title to describe this test case' '
> +  # test body
> +  '

If you want to show the pretty layout, then the test body should
be shown indented, i.e.

	test_expect_success 'title' '
		... test body ...
	'

But I am not sure if this belongs to the existing "Do's and don'ts"
section, which lists tips that matter for correctness.

This new one certainly encouraged as a more modern style, but is not
about correctness at all.

A separate "recommended style" section might make sense, but there
will be a lot more entries, like when to quote and not to quote EOF
marker for here-document, indenting the body of here-document, etc.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Re: [PATCH] ls-files: update test style
  2022-06-23 10:50 ` Ævar Arnfjörð Bjarmason
@ 2022-06-24  4:57   ` lilinchao
  0 siblings, 0 replies; 9+ messages in thread
From: lilinchao @ 2022-06-24  4:57 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, Li Linchao via GitGitGadget; +Cc: git

>
>On Thu, Jun 23 2022, Li Linchao via GitGitGadget wrote:
>
>> From: Li Linchao <lilinchao@oschina.cn>
>>
>> Update test style in t/t30[*].sh for uniformity, that's to
>> keep test title the same line with helper function itself.
>
>We have a few of these sorts of old style tests, and it's good to update
>them. 
Yes. Currently there are at least 400+ old style tests :). It not easy for me
to fix them all at once with some magic regex expressions, so I'm not going
to update them all in one patch. But I think, first of all, we can explicitly
document which test style we prefer first.
>
>>     Write test code like this:
>> diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
>> index 48cec4e5f88..76361b92336 100755
>> --- a/t/t3001-ls-files-others-exclude.sh
>> +++ b/t/t3001-ls-files-others-exclude.sh
>> @@ -67,26 +67,26 @@ echo '!*.2
>> 
>>  allignores='.gitignore one/.gitignore one/two/.gitignore'
>> 
>> -test_expect_success \
>> -    'git ls-files --others with various exclude options.' \
>> -    'git ls-files --others \
>> +test_expect_success 'git ls-files --others with various exclude options.' '
>> +	git ls-files --others \
>>         --exclude=\*.6 \
>>         --exclude-per-directory=.gitignore \
>>         --exclude-from=.git/ignore \
>>         >output &&
>
>This though really stops too short, here we end up with:
>
>	<TAB>git-ls-files --others \
>	<7 spaces>--exclude [...]
> 
OK.
>> -     test_cmp expect output'
>> +     test_cmp expect output
>
>And you've space-indented this test_cmp, presumably the below has the
>same issues (I didn't check in detail) 
>
>Instead the argument lists should be <TAB><TAB> indented, and the rest
>should be TAB indented. 
OK.
>
>> +'
>> 
>>  # Test \r\n (MSDOS-like systems)
>>  printf '*.1\r\n/*.3\r\n!*.6\r\n' >.gitignore
>> 
>> -test_expect_success \
>> -    'git ls-files --others with \r\n line endings.' \
>> -    'git ls-files --others \
>> +test_expect_success 'git ls-files --others with \r\n line endings.' '
>> +	git ls-files --others \
>>         --exclude=\*.6 \
>>         --exclude-per-directory=.gitignore \
>>         --exclude-from=.git/ignore \
>>         >output &&
>> -     test_cmp expect output'
>> +     test_cmp expect output
>> +'
>
>Aside from the above I think it's also worth incorporating all the
>"printf", "echo", "cat" etc. that we do into the "test_expect_success"
>themselves, and if they're needed by more than one test perhaps make
>them a "setup" helper function (which would test_when_finished "rm -f
>.gitignore" clean up after itself). 
Yes, make sense.
>
>That's obviously bigger than some whitespace changes, so we could punt
>on it for now, but as we're looking at this anyway we could convert
>fully to a more modern style in a follow-up commit...
>
>> -test_expect_success \
>> -    'git ls-files with path restriction with --.' \
>> -    'git ls-files --others -- path0 >output &&
>> +test_expect_success 'git ls-files with path restriction with --.' '
>> +    git ls-files --others -- path0 >output &&
>>  test_cmp output - <<EOF
>>  path0
>>  EOF
>>  '
>
>On the topic of leaving things on the table: here we could use "<<-EOF"
>(or actually better "<<-\EOF") instead, and indent the here-doc, as we
>usually do. 
OK, will do.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Re: [PATCH] ls-files: update test style
  2022-06-23 17:09 ` Junio C Hamano
@ 2022-06-24  5:05   ` lilinchao
  0 siblings, 0 replies; 9+ messages in thread
From: lilinchao @ 2022-06-24  5:05 UTC (permalink / raw)
  To: Junio C Hamano, Li Linchao via GitGitGadget; +Cc: git


>"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/t/README b/t/README
>> index 309a31133c6..70205fba41b 100644
>> --- a/t/README
>> +++ b/t/README
>> @@ -560,6 +560,21 @@ Here are the "do's:"
>>     Even code that isn't a test per se, but merely some setup code
>>     should be inside a test assertion.
>> 
>> + - Keep test title the same line with test helper function itself,
>> +   and end the line with a single quote.
>> +
>> +   Take test_expect_success helper for example, write it like:
>> +
>> +  test_expect_success 'test title to describe this test case' '
>> +  # test body
>> +  '
>
>If you want to show the pretty layout, then the test body should
>be shown indented, i.e.
>
>	test_expect_success 'title' '
>	... test body ...
>	'
OK.
>
>But I am not sure if this belongs to the existing "Do's and don'ts"
>section, which lists tips that matter for correctness.
>
>This new one certainly encouraged as a more modern style, but is not
>about correctness at all.
OK, I will remove it from "Do's and don'ts" section.
>
>A separate "recommended style" section might make sense, but there
>will be a lot more entries, like when to quote and not to quote EOF
>marker for here-document, indenting the body of here-document, etc.
Yes, a "recommended style" section needed.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] ls-files: update test style
  2022-06-23  8:46 [PATCH] ls-files: update test style Li Linchao via GitGitGadget
  2022-06-23 10:50 ` Ævar Arnfjörð Bjarmason
  2022-06-23 17:09 ` Junio C Hamano
@ 2022-06-28  9:14 ` Li Linchao via GitGitGadget
  2022-06-28  9:51   ` [PATCH v3] " Li Linchao via GitGitGadget
  2 siblings, 1 reply; 9+ messages in thread
From: Li Linchao via GitGitGadget @ 2022-06-28  9:14 UTC (permalink / raw)
  To: git; +Cc: Ævar Arnfjörð Bjarmason, Li Linchao, Li Linchao

From: Li Linchao <lilinchao@oschina.cn>

Update test style in t/t30[*].sh for uniformity, that's to
keep test title the same line with helper function itself.

And update t/README to describe this test style.

Signed-off-by: Li Linchao <lilinchao@oschina.cn>
---
    ls-files: update test style
    
    Update test style in t/t30[*].sh for uniformity, that's to keep test
    title the same line with helper function itself.
    
    And update t/README to describe this test style.
    
    Signed-off-by: Li Linchao lilinchao@oschina.cn

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1269%2FCactusinhand%2Fllc%2Ffix-test-title-style-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1269/Cactusinhand/llc/fix-test-title-style-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1269

Range-diff vs v1:

 1:  e18a6172cff ! 1:  775c17499d5 ls-files: update test style
     @@ Commit message
          Signed-off-by: Li Linchao <lilinchao@oschina.cn>
      
       ## t/README ##
     -@@ t/README: Here are the "do's:"
     -    Even code that isn't a test per se, but merely some setup code
     -    should be inside a test assertion.
     +@@ t/README: This test harness library does the following things:
     +    consistently when command line arguments --verbose (or -v),
     +    --debug (or -d), and --immediate (or -i) is given.
       
     -+ - Keep test title the same line with test helper function itself,
     -+   and end the line with a single quote.
     ++Recommended style
     ++-----------------
     ++Here are some recommented style that you should follow when you write some test cases.
     ++
     ++ - Keep test title the same line with test helper function itself.
      +
      +   Take test_expect_success helper for example, write it like:
      +
     -+  test_expect_success 'test title to describe this test case' '
     -+  # test body
     ++  test_expect_success 'test title' '
     ++  ... test body ...
      +  '
      +
      +   Instead of:
      +
      +  test_expect_success \
     -+  'test title to describe this test case' \
     -+  '# test body'
     ++  'test title' \
     ++  '... test body ...'
     ++
     ++
     ++ - End the line with a single quote.
     ++
     ++ - Indent the body of here-document, and cut off prefix TAB by using
     ++ "<<-":
      +
     -  - Chain your test assertions
     ++  test_expect_success 'test something' '
     ++      cat >expect <<-\EOF &&
     ++      one
     ++      two
     ++      three
     ++      EOF
     ++      test_something > actual &&
     ++      test_cmp expect actual
     ++  '
     ++
     ++   Instead of:
     ++
     ++  test_expect_success 'test something' '
     ++      cat >expect <<\EOF &&
     ++  one
     ++  two
     ++  three
     ++  EOF
     ++      test_something > actual &&
     ++      test_cmp expect actual
     ++  '
     ++
     ++
     + Do's & don'ts
     + -------------
       
     -    Write test code like this:
      
       ## t/t3001-ls-files-others-exclude.sh ##
      @@ t/t3001-ls-files-others-exclude.sh: echo '!*.2
     @@ t/t3001-ls-files-others-exclude.sh: echo '!*.2
              --exclude=\*.6 \
              --exclude-per-directory=.gitignore \
              --exclude-from=.git/ignore \
     -        >output &&
     +-       >output &&
      -     test_cmp expect output'
     -+     test_cmp expect output
     ++	>output &&
     ++	test_cmp expect output
      +'
       
       # Test \r\n (MSDOS-like systems)
     @@ t/t3001-ls-files-others-exclude.sh: echo '!*.2
              --exclude=\*.6 \
              --exclude-per-directory=.gitignore \
              --exclude-from=.git/ignore \
     -        >output &&
     +-       >output &&
      -     test_cmp expect output'
     -+     test_cmp expect output
     ++	>output &&
     ++	test_cmp expect output
      +'
       
       test_expect_success 'setup skip-worktree gitignore' '
     @@ t/t3001-ls-files-others-exclude.sh: test_expect_success 'setup skip-worktree git
              --exclude=\*.6 \
              --exclude-per-directory=.gitignore \
              --exclude-from=.git/ignore \
     -        >output &&
     +-       >output &&
      -     test_cmp expect output'
     -+     test_cmp expect output
     ++	>output &&
     ++	test_cmp expect output
      +'
       
       test_expect_success !SANITIZE_LEAK 'restore gitignore' '
       	git checkout --ignore-skip-worktree-bits $allignores &&
     +@@ t/t3001-ls-files-others-exclude.sh: test_expect_success 'pattern matches prefix completely' '
     + '
     + 
     + test_expect_success 'ls-files with "**" patterns' '
     +-	cat <<\EOF >expect &&
     +-a.1
     +-one/a.1
     +-one/two/a.1
     +-three/a.1
     +-EOF
     ++	cat <<-\EOF >expect &&
     ++	a.1
     ++	one/a.1
     ++	one/two/a.1
     ++	three/a.1
     ++	EOF
     + 	git ls-files -o -i --exclude "**/a.1" >actual &&
     + 	test_cmp expect actual
     + '
      
       ## t/t3002-ls-files-dashpath.sh ##
      @@ t/t3002-ls-files-dashpath.sh: filesystem.
     @@ t/t3002-ls-files-dashpath.sh: filesystem.
      -	setup \
      -	'echo frotz >path0 &&
      +test_expect_success 'setup' '
     -+    echo frotz >path0 &&
     ++	echo frotz >path0 &&
       	echo frotz >./-foo &&
      -	echo frotz >./--'
      +	echo frotz >./--
     @@ t/t3002-ls-files-dashpath.sh: filesystem.
      -test_expect_success \
      -    'git ls-files without path restriction.' \
      -    'git ls-files --others >output &&
     +-     test_cmp output - <<EOF
     +---
     +--foo
     +-output
     +-path0
     +-EOF
      +test_expect_success 'git ls-files without path restriction.' '
     -+    git ls-files --others >output &&
     -      test_cmp output - <<EOF
     - --
     - -foo
     -@@ t/t3002-ls-files-dashpath.sh: path0
     - EOF
     ++	test_when_finished "rm -f expect" &&
     ++	git ls-files --others >output &&
     ++	cat >expect <<-\EOF &&
     ++	--
     ++	-foo
     ++	output
     ++	path0
     ++	EOF
     ++	test_cmp output expect
       '
       
      -test_expect_success \
      -    'git ls-files with path restriction.' \
      -    'git ls-files --others path0 >output &&
     +-	test_cmp output - <<EOF
     +-path0
     +-EOF
      +test_expect_success 'git ls-files with path restriction.' '
     -+    git ls-files --others path0 >output &&
     - 	test_cmp output - <<EOF
     - path0
     - EOF
     ++	test_when_finished "rm -f expect" &&
     ++	git ls-files --others path0 >output &&
     ++	cat >expect <<-\EOF &&
     ++	path0
     ++	EOF
     ++	test_cmp output expect
       '
       
      -test_expect_success \
      -    'git ls-files with path restriction with --.' \
      -    'git ls-files --others -- path0 >output &&
     +-	test_cmp output - <<EOF
     +-path0
     +-EOF
      +test_expect_success 'git ls-files with path restriction with --.' '
     -+    git ls-files --others -- path0 >output &&
     - 	test_cmp output - <<EOF
     - path0
     - EOF
     ++	test_when_finished "rm -f expect" &&
     ++	git ls-files --others -- path0 >output &&
     ++	cat >expect <<-\EOF &&
     ++	path0
     ++	EOF
     ++	test_cmp output expect
       '
       
      -test_expect_success \
      -    'git ls-files with path restriction with -- --.' \
      -    'git ls-files --others -- -- >output &&
     +-	test_cmp output - <<EOF
     +---
     +-EOF
      +test_expect_success 'git ls-files with path restriction with -- --.' '
     -+    git ls-files --others -- -- >output &&
     - 	test_cmp output - <<EOF
     - --
     - EOF
     ++	test_when_finished "rm -f expect" &&
     ++	git ls-files --others -- -- >output &&
     ++	cat >expect <<-\EOF &&
     ++	--
     ++	EOF
     ++	test_cmp output expect
       '
       
      -test_expect_success \
      -    'git ls-files with no path restriction.' \
      -    'git ls-files --others -- >output &&
     +-	test_cmp output - <<EOF
     +---
     +--foo
     +-output
     +-path0
     +-EOF
      +test_expect_success 'git ls-files with no path restriction.' '
     -+    git ls-files --others -- >output &&
     - 	test_cmp output - <<EOF
     - --
     - -foo
     ++	test_when_finished "rm -f expect" &&
     ++	git ls-files --others -- >output &&
     ++	cat >expect <<-\EOF &&
     ++	--
     ++	-foo
     ++	output
     ++	path0
     ++	EOF
     ++	test_cmp output expect
     ++
     + '
     + 
     + test_done
     +
     + ## t/t3007-ls-files-recurse-submodules.sh ##
     +@@ t/t3007-ls-files-recurse-submodules.sh: test_expect_success '--recurse-submodules does not support --error-unmatch' '
     + 
     + test_incompatible_with_recurse_submodules () {
     + 	test_expect_success "--recurse-submodules and $1 are incompatible" "
     +-		test_must_fail git ls-files --recurse-submodules $1 2>actual &&
     +-		test_i18ngrep 'unsupported mode' actual
     ++	test_must_fail git ls-files --recurse-submodules $1 2>actual &&
     ++	test_i18ngrep 'unsupported mode' actual
     + 	"
     + }
     + 
      
       ## t/t3020-ls-files-error-unmatch.sh ##
      @@ t/t3020-ls-files-error-unmatch.sh: test_expect_success 'setup' '
     @@ t/t3020-ls-files-error-unmatch.sh: test_expect_success 'setup' '
      -    'git ls-files --error-unmatch should fail with unmatched path.' \
      -    'test_must_fail git ls-files --error-unmatch foo bar-does-not-match'
      +test_expect_success 'git ls-files --error-unmatch should fail with unmatched path.' '
     -+    test_must_fail git ls-files --error-unmatch foo bar-does-not-match
     ++	test_must_fail git ls-files --error-unmatch foo bar-does-not-match
      +'
       
      -test_expect_success \
      -    'git ls-files --error-unmatch should succeed with matched paths.' \
      -    'git ls-files --error-unmatch foo bar'
      +test_expect_success 'git ls-files --error-unmatch should succeed with matched paths.' '
     -+    git ls-files --error-unmatch foo bar
     ++	git ls-files --error-unmatch foo bar
      +'
       
       test_done
      
       ## t/t3060-ls-files-with-tree.sh ##
     +@@ t/t3060-ls-files-with-tree.sh: a scenario known to trigger a crash with some versions of git.
     + '
     + . ./test-lib.sh
     + 
     +-test_expect_success setup '
     ++test_expect_success 'setup' '
     + 
     + 	# The bug we are exercising requires a fair number of entries
     + 	# in a sub-directory so that add_index_entry will trigger a
      @@ t/t3060-ls-files-with-tree.sh: test_expect_success 'git ls-files --with-tree should succeed from subdir' '
       	)
       '


 t/README                               | 47 ++++++++++++++
 t/t3001-ls-files-others-exclude.sh     | 42 ++++++-------
 t/t3002-ls-files-dashpath.sh           | 86 ++++++++++++++------------
 t/t3007-ls-files-recurse-submodules.sh |  4 +-
 t/t3020-ls-files-error-unmatch.sh      | 12 ++--
 t/t3060-ls-files-with-tree.sh          |  8 +--
 6 files changed, 126 insertions(+), 73 deletions(-)

diff --git a/t/README b/t/README
index 309a31133c6..845af1883c3 100644
--- a/t/README
+++ b/t/README
@@ -547,6 +547,53 @@ This test harness library does the following things:
    consistently when command line arguments --verbose (or -v),
    --debug (or -d), and --immediate (or -i) is given.
 
+Recommended style
+-----------------
+Here are some recommented style that you should follow when you write some test cases.
+
+ - Keep test title the same line with test helper function itself.
+
+   Take test_expect_success helper for example, write it like:
+
+  test_expect_success 'test title' '
+  ... test body ...
+  '
+
+   Instead of:
+
+  test_expect_success \
+  'test title' \
+  '... test body ...'
+
+
+ - End the line with a single quote.
+
+ - Indent the body of here-document, and cut off prefix TAB by using
+ "<<-":
+
+  test_expect_success 'test something' '
+      cat >expect <<-\EOF &&
+      one
+      two
+      three
+      EOF
+      test_something > actual &&
+      test_cmp expect actual
+  '
+
+   Instead of:
+
+  test_expect_success 'test something' '
+      cat >expect <<\EOF &&
+  one
+  two
+  three
+  EOF
+      test_something > actual &&
+      test_cmp expect actual
+  '
+
+
 Do's & don'ts
 -------------
 
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index 48cec4e5f88..e07ac6c6dce 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -67,26 +67,26 @@ echo '!*.2
 
 allignores='.gitignore one/.gitignore one/two/.gitignore'
 
-test_expect_success \
-    'git ls-files --others with various exclude options.' \
-    'git ls-files --others \
+test_expect_success 'git ls-files --others with various exclude options.' '
+	git ls-files --others \
        --exclude=\*.6 \
        --exclude-per-directory=.gitignore \
        --exclude-from=.git/ignore \
-       >output &&
-     test_cmp expect output'
+	>output &&
+	test_cmp expect output
+'
 
 # Test \r\n (MSDOS-like systems)
 printf '*.1\r\n/*.3\r\n!*.6\r\n' >.gitignore
 
-test_expect_success \
-    'git ls-files --others with \r\n line endings.' \
-    'git ls-files --others \
+test_expect_success 'git ls-files --others with \r\n line endings.' '
+	git ls-files --others \
        --exclude=\*.6 \
        --exclude-per-directory=.gitignore \
        --exclude-from=.git/ignore \
-       >output &&
-     test_cmp expect output'
+	>output &&
+	test_cmp expect output
+'
 
 test_expect_success 'setup skip-worktree gitignore' '
 	git add $allignores &&
@@ -94,14 +94,14 @@ test_expect_success 'setup skip-worktree gitignore' '
 	rm $allignores
 '
 
-test_expect_success \
-    'git ls-files --others with various exclude options.' \
-    'git ls-files --others \
+test_expect_success 'git ls-files --others with various exclude options.' '
+	git ls-files --others \
        --exclude=\*.6 \
        --exclude-per-directory=.gitignore \
        --exclude-from=.git/ignore \
-       >output &&
-     test_cmp expect output'
+	>output &&
+	test_cmp expect output
+'
 
 test_expect_success !SANITIZE_LEAK 'restore gitignore' '
 	git checkout --ignore-skip-worktree-bits $allignores &&
@@ -283,12 +283,12 @@ test_expect_success 'pattern matches prefix completely' '
 '
 
 test_expect_success 'ls-files with "**" patterns' '
-	cat <<\EOF >expect &&
-a.1
-one/a.1
-one/two/a.1
-three/a.1
-EOF
+	cat <<-\EOF >expect &&
+	a.1
+	one/a.1
+	one/two/a.1
+	three/a.1
+	EOF
 	git ls-files -o -i --exclude "**/a.1" >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t3002-ls-files-dashpath.sh b/t/t3002-ls-files-dashpath.sh
index 54d22a45dfb..4dd24550eba 100755
--- a/t/t3002-ls-files-dashpath.sh
+++ b/t/t3002-ls-files-dashpath.sh
@@ -16,56 +16,62 @@ filesystem.
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-test_expect_success \
-	setup \
-	'echo frotz >path0 &&
+test_expect_success 'setup' '
+	echo frotz >path0 &&
 	echo frotz >./-foo &&
-	echo frotz >./--'
+	echo frotz >./--
+'
 
-test_expect_success \
-    'git ls-files without path restriction.' \
-    'git ls-files --others >output &&
-     test_cmp output - <<EOF
---
--foo
-output
-path0
-EOF
+test_expect_success 'git ls-files without path restriction.' '
+	test_when_finished "rm -f expect" &&
+	git ls-files --others >output &&
+	cat >expect <<-\EOF &&
+	--
+	-foo
+	output
+	path0
+	EOF
+	test_cmp output expect
 '
 
-test_expect_success \
-    'git ls-files with path restriction.' \
-    'git ls-files --others path0 >output &&
-	test_cmp output - <<EOF
-path0
-EOF
+test_expect_success 'git ls-files with path restriction.' '
+	test_when_finished "rm -f expect" &&
+	git ls-files --others path0 >output &&
+	cat >expect <<-\EOF &&
+	path0
+	EOF
+	test_cmp output expect
 '
 
-test_expect_success \
-    'git ls-files with path restriction with --.' \
-    'git ls-files --others -- path0 >output &&
-	test_cmp output - <<EOF
-path0
-EOF
+test_expect_success 'git ls-files with path restriction with --.' '
+	test_when_finished "rm -f expect" &&
+	git ls-files --others -- path0 >output &&
+	cat >expect <<-\EOF &&
+	path0
+	EOF
+	test_cmp output expect
 '
 
-test_expect_success \
-    'git ls-files with path restriction with -- --.' \
-    'git ls-files --others -- -- >output &&
-	test_cmp output - <<EOF
---
-EOF
+test_expect_success 'git ls-files with path restriction with -- --.' '
+	test_when_finished "rm -f expect" &&
+	git ls-files --others -- -- >output &&
+	cat >expect <<-\EOF &&
+	--
+	EOF
+	test_cmp output expect
 '
 
-test_expect_success \
-    'git ls-files with no path restriction.' \
-    'git ls-files --others -- >output &&
-	test_cmp output - <<EOF
---
--foo
-output
-path0
-EOF
+test_expect_success 'git ls-files with no path restriction.' '
+	test_when_finished "rm -f expect" &&
+	git ls-files --others -- >output &&
+	cat >expect <<-\EOF &&
+	--
+	-foo
+	output
+	path0
+	EOF
+	test_cmp output expect
+
 '
 
 test_done
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index dd7770e85de..2a057e987fe 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -301,8 +301,8 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '
 
 test_incompatible_with_recurse_submodules () {
 	test_expect_success "--recurse-submodules and $1 are incompatible" "
-		test_must_fail git ls-files --recurse-submodules $1 2>actual &&
-		test_i18ngrep 'unsupported mode' actual
+	test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+	test_i18ngrep 'unsupported mode' actual
 	"
 }
 
diff --git a/t/t3020-ls-files-error-unmatch.sh b/t/t3020-ls-files-error-unmatch.sh
index 2cbcbc0721b..133593d23c0 100755
--- a/t/t3020-ls-files-error-unmatch.sh
+++ b/t/t3020-ls-files-error-unmatch.sh
@@ -19,12 +19,12 @@ test_expect_success 'setup' '
 	git commit -m "add foo bar"
 '
 
-test_expect_success \
-    'git ls-files --error-unmatch should fail with unmatched path.' \
-    'test_must_fail git ls-files --error-unmatch foo bar-does-not-match'
+test_expect_success 'git ls-files --error-unmatch should fail with unmatched path.' '
+	test_must_fail git ls-files --error-unmatch foo bar-does-not-match
+'
 
-test_expect_success \
-    'git ls-files --error-unmatch should succeed with matched paths.' \
-    'git ls-files --error-unmatch foo bar'
+test_expect_success 'git ls-files --error-unmatch should succeed with matched paths.' '
+	git ls-files --error-unmatch foo bar
+'
 
 test_done
diff --git a/t/t3060-ls-files-with-tree.sh b/t/t3060-ls-files-with-tree.sh
index b257c792a46..52f76f7b57f 100755
--- a/t/t3060-ls-files-with-tree.sh
+++ b/t/t3060-ls-files-with-tree.sh
@@ -10,7 +10,7 @@ a scenario known to trigger a crash with some versions of git.
 '
 . ./test-lib.sh
 
-test_expect_success setup '
+test_expect_success 'setup' '
 
 	# The bug we are exercising requires a fair number of entries
 	# in a sub-directory so that add_index_entry will trigger a
@@ -62,9 +62,9 @@ test_expect_success 'git ls-files --with-tree should succeed from subdir' '
 	)
 '
 
-test_expect_success \
-    'git ls-files --with-tree should add entries from named tree.' \
-    'test_cmp expected output'
+test_expect_success 'git ls-files --with-tree should add entries from named tree.' '
+	test_cmp expected output
+'
 
 test_expect_success 'no duplicates in --with-tree output' '
 	git ls-files --with-tree=HEAD >actual &&

base-commit: e4a4b31577c7419497ac30cebe30d755b97752c5
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3] ls-files: update test style
  2022-06-28  9:14 ` [PATCH v2] " Li Linchao via GitGitGadget
@ 2022-06-28  9:51   ` Li Linchao via GitGitGadget
  2022-06-28 20:12     ` Junio C Hamano
  0 siblings, 1 reply; 9+ messages in thread
From: Li Linchao via GitGitGadget @ 2022-06-28  9:51 UTC (permalink / raw)
  To: git; +Cc: Ævar Arnfjörð Bjarmason, Li Linchao, Li Linchao

From: Li Linchao <lilinchao@oschina.cn>

Update test style in t/t30[*].sh for uniformity, that's to
keep test title the same line with helper function itself,
and fix some indentions.

Add a new section "recommended style" in t/README to
encourage people to use more modern style in test.

Signed-off-by: Li Linchao <lilinchao@oschina.cn>
---
    ls-files: update test style
    
    Update test style in t/t30[*].sh for uniformity, that's to keep test
    title the same line with helper function itself.
    
    And update t/README to describe this test style.
    
    Signed-off-by: Li Linchao lilinchao@oschina.cn

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1269%2FCactusinhand%2Fllc%2Ffix-test-title-style-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1269/Cactusinhand/llc/fix-test-title-style-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1269

Range-diff vs v2:

 1:  775c17499d5 ! 1:  b3d80bd9cd5 ls-files: update test style
     @@ Commit message
          ls-files: update test style
      
          Update test style in t/t30[*].sh for uniformity, that's to
     -    keep test title the same line with helper function itself.
     +    keep test title the same line with helper function itself,
     +    and fix some indentions.
      
     -    And update t/README to describe this test style.
     +    Add a new section "recommended style" in t/README to
     +    encourage people to use more modern style in test.
      
          Signed-off-by: Li Linchao <lilinchao@oschina.cn>
      
     @@ t/README: This test harness library does the following things:
       
      +Recommended style
      +-----------------
     -+Here are some recommented style that you should follow when you write some test cases.
     ++Here are some recommented styles when writing test case.
      +
      + - Keep test title the same line with test helper function itself.
      +
     @@ t/README: This test harness library does the following things:
      +
      + - End the line with a single quote.
      +
     -+ - Indent the body of here-document, and cut off prefix TAB by using
     -+ "<<-":
     ++ - Indent the body of here-document, and use "<< -" instead of "<<" to strip prefix TAB:
      +
      +  test_expect_success 'test something' '
      +      cat >expect <<-\EOF &&
     @@ t/t3002-ls-files-dashpath.sh: filesystem.
       
       test_done
      
     - ## t/t3007-ls-files-recurse-submodules.sh ##
     -@@ t/t3007-ls-files-recurse-submodules.sh: test_expect_success '--recurse-submodules does not support --error-unmatch' '
     - 
     - test_incompatible_with_recurse_submodules () {
     - 	test_expect_success "--recurse-submodules and $1 are incompatible" "
     --		test_must_fail git ls-files --recurse-submodules $1 2>actual &&
     --		test_i18ngrep 'unsupported mode' actual
     -+	test_must_fail git ls-files --recurse-submodules $1 2>actual &&
     -+	test_i18ngrep 'unsupported mode' actual
     - 	"
     - }
     - 
     -
       ## t/t3020-ls-files-error-unmatch.sh ##
      @@ t/t3020-ls-files-error-unmatch.sh: test_expect_success 'setup' '
       	git commit -m "add foo bar"


 t/README                           | 46 ++++++++++++++++
 t/t3001-ls-files-others-exclude.sh | 42 +++++++--------
 t/t3002-ls-files-dashpath.sh       | 86 ++++++++++++++++--------------
 t/t3020-ls-files-error-unmatch.sh  | 12 ++---
 t/t3060-ls-files-with-tree.sh      |  8 +--
 5 files changed, 123 insertions(+), 71 deletions(-)

diff --git a/t/README b/t/README
index 309a31133c6..5e0539412b4 100644
--- a/t/README
+++ b/t/README
@@ -547,6 +547,52 @@ This test harness library does the following things:
    consistently when command line arguments --verbose (or -v),
    --debug (or -d), and --immediate (or -i) is given.
 
+Recommended style
+-----------------
+Here are some recommented styles when writing test case.
+
+ - Keep test title the same line with test helper function itself.
+
+   Take test_expect_success helper for example, write it like:
+
+  test_expect_success 'test title' '
+  ... test body ...
+  '
+
+   Instead of:
+
+  test_expect_success \
+  'test title' \
+  '... test body ...'
+
+
+ - End the line with a single quote.
+
+ - Indent the body of here-document, and use "<< -" instead of "<<" to strip prefix TAB:
+
+  test_expect_success 'test something' '
+      cat >expect <<-\EOF &&
+      one
+      two
+      three
+      EOF
+      test_something > actual &&
+      test_cmp expect actual
+  '
+
+   Instead of:
+
+  test_expect_success 'test something' '
+      cat >expect <<\EOF &&
+  one
+  two
+  three
+  EOF
+      test_something > actual &&
+      test_cmp expect actual
+  '
+
+
 Do's & don'ts
 -------------
 
diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
index 48cec4e5f88..e07ac6c6dce 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-exclude.sh
@@ -67,26 +67,26 @@ echo '!*.2
 
 allignores='.gitignore one/.gitignore one/two/.gitignore'
 
-test_expect_success \
-    'git ls-files --others with various exclude options.' \
-    'git ls-files --others \
+test_expect_success 'git ls-files --others with various exclude options.' '
+	git ls-files --others \
        --exclude=\*.6 \
        --exclude-per-directory=.gitignore \
        --exclude-from=.git/ignore \
-       >output &&
-     test_cmp expect output'
+	>output &&
+	test_cmp expect output
+'
 
 # Test \r\n (MSDOS-like systems)
 printf '*.1\r\n/*.3\r\n!*.6\r\n' >.gitignore
 
-test_expect_success \
-    'git ls-files --others with \r\n line endings.' \
-    'git ls-files --others \
+test_expect_success 'git ls-files --others with \r\n line endings.' '
+	git ls-files --others \
        --exclude=\*.6 \
        --exclude-per-directory=.gitignore \
        --exclude-from=.git/ignore \
-       >output &&
-     test_cmp expect output'
+	>output &&
+	test_cmp expect output
+'
 
 test_expect_success 'setup skip-worktree gitignore' '
 	git add $allignores &&
@@ -94,14 +94,14 @@ test_expect_success 'setup skip-worktree gitignore' '
 	rm $allignores
 '
 
-test_expect_success \
-    'git ls-files --others with various exclude options.' \
-    'git ls-files --others \
+test_expect_success 'git ls-files --others with various exclude options.' '
+	git ls-files --others \
        --exclude=\*.6 \
        --exclude-per-directory=.gitignore \
        --exclude-from=.git/ignore \
-       >output &&
-     test_cmp expect output'
+	>output &&
+	test_cmp expect output
+'
 
 test_expect_success !SANITIZE_LEAK 'restore gitignore' '
 	git checkout --ignore-skip-worktree-bits $allignores &&
@@ -283,12 +283,12 @@ test_expect_success 'pattern matches prefix completely' '
 '
 
 test_expect_success 'ls-files with "**" patterns' '
-	cat <<\EOF >expect &&
-a.1
-one/a.1
-one/two/a.1
-three/a.1
-EOF
+	cat <<-\EOF >expect &&
+	a.1
+	one/a.1
+	one/two/a.1
+	three/a.1
+	EOF
 	git ls-files -o -i --exclude "**/a.1" >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t3002-ls-files-dashpath.sh b/t/t3002-ls-files-dashpath.sh
index 54d22a45dfb..4dd24550eba 100755
--- a/t/t3002-ls-files-dashpath.sh
+++ b/t/t3002-ls-files-dashpath.sh
@@ -16,56 +16,62 @@ filesystem.
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-test_expect_success \
-	setup \
-	'echo frotz >path0 &&
+test_expect_success 'setup' '
+	echo frotz >path0 &&
 	echo frotz >./-foo &&
-	echo frotz >./--'
+	echo frotz >./--
+'
 
-test_expect_success \
-    'git ls-files without path restriction.' \
-    'git ls-files --others >output &&
-     test_cmp output - <<EOF
---
--foo
-output
-path0
-EOF
+test_expect_success 'git ls-files without path restriction.' '
+	test_when_finished "rm -f expect" &&
+	git ls-files --others >output &&
+	cat >expect <<-\EOF &&
+	--
+	-foo
+	output
+	path0
+	EOF
+	test_cmp output expect
 '
 
-test_expect_success \
-    'git ls-files with path restriction.' \
-    'git ls-files --others path0 >output &&
-	test_cmp output - <<EOF
-path0
-EOF
+test_expect_success 'git ls-files with path restriction.' '
+	test_when_finished "rm -f expect" &&
+	git ls-files --others path0 >output &&
+	cat >expect <<-\EOF &&
+	path0
+	EOF
+	test_cmp output expect
 '
 
-test_expect_success \
-    'git ls-files with path restriction with --.' \
-    'git ls-files --others -- path0 >output &&
-	test_cmp output - <<EOF
-path0
-EOF
+test_expect_success 'git ls-files with path restriction with --.' '
+	test_when_finished "rm -f expect" &&
+	git ls-files --others -- path0 >output &&
+	cat >expect <<-\EOF &&
+	path0
+	EOF
+	test_cmp output expect
 '
 
-test_expect_success \
-    'git ls-files with path restriction with -- --.' \
-    'git ls-files --others -- -- >output &&
-	test_cmp output - <<EOF
---
-EOF
+test_expect_success 'git ls-files with path restriction with -- --.' '
+	test_when_finished "rm -f expect" &&
+	git ls-files --others -- -- >output &&
+	cat >expect <<-\EOF &&
+	--
+	EOF
+	test_cmp output expect
 '
 
-test_expect_success \
-    'git ls-files with no path restriction.' \
-    'git ls-files --others -- >output &&
-	test_cmp output - <<EOF
---
--foo
-output
-path0
-EOF
+test_expect_success 'git ls-files with no path restriction.' '
+	test_when_finished "rm -f expect" &&
+	git ls-files --others -- >output &&
+	cat >expect <<-\EOF &&
+	--
+	-foo
+	output
+	path0
+	EOF
+	test_cmp output expect
+
 '
 
 test_done
diff --git a/t/t3020-ls-files-error-unmatch.sh b/t/t3020-ls-files-error-unmatch.sh
index 2cbcbc0721b..133593d23c0 100755
--- a/t/t3020-ls-files-error-unmatch.sh
+++ b/t/t3020-ls-files-error-unmatch.sh
@@ -19,12 +19,12 @@ test_expect_success 'setup' '
 	git commit -m "add foo bar"
 '
 
-test_expect_success \
-    'git ls-files --error-unmatch should fail with unmatched path.' \
-    'test_must_fail git ls-files --error-unmatch foo bar-does-not-match'
+test_expect_success 'git ls-files --error-unmatch should fail with unmatched path.' '
+	test_must_fail git ls-files --error-unmatch foo bar-does-not-match
+'
 
-test_expect_success \
-    'git ls-files --error-unmatch should succeed with matched paths.' \
-    'git ls-files --error-unmatch foo bar'
+test_expect_success 'git ls-files --error-unmatch should succeed with matched paths.' '
+	git ls-files --error-unmatch foo bar
+'
 
 test_done
diff --git a/t/t3060-ls-files-with-tree.sh b/t/t3060-ls-files-with-tree.sh
index b257c792a46..52f76f7b57f 100755
--- a/t/t3060-ls-files-with-tree.sh
+++ b/t/t3060-ls-files-with-tree.sh
@@ -10,7 +10,7 @@ a scenario known to trigger a crash with some versions of git.
 '
 . ./test-lib.sh
 
-test_expect_success setup '
+test_expect_success 'setup' '
 
 	# The bug we are exercising requires a fair number of entries
 	# in a sub-directory so that add_index_entry will trigger a
@@ -62,9 +62,9 @@ test_expect_success 'git ls-files --with-tree should succeed from subdir' '
 	)
 '
 
-test_expect_success \
-    'git ls-files --with-tree should add entries from named tree.' \
-    'test_cmp expected output'
+test_expect_success 'git ls-files --with-tree should add entries from named tree.' '
+	test_cmp expected output
+'
 
 test_expect_success 'no duplicates in --with-tree output' '
 	git ls-files --with-tree=HEAD >actual &&

base-commit: e4a4b31577c7419497ac30cebe30d755b97752c5
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3] ls-files: update test style
  2022-06-28  9:51   ` [PATCH v3] " Li Linchao via GitGitGadget
@ 2022-06-28 20:12     ` Junio C Hamano
  2022-06-29  7:12       ` lilinchao
  0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2022-06-28 20:12 UTC (permalink / raw)
  To: Li Linchao via GitGitGadget
  Cc: git, Ævar Arnfjörð Bjarmason, Li Linchao

"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/t/README b/t/README
> index 309a31133c6..5e0539412b4 100644
> --- a/t/README
> +++ b/t/README
> @@ -547,6 +547,52 @@ This test harness library does the following things:
>     consistently when command line arguments --verbose (or -v),
>     --debug (or -d), and --immediate (or -i) is given.
>  
> +Recommended style
> +-----------------
> +Here are some recommented styles when writing test case.
> +
> + - Keep test title the same line with test helper function itself.
> +
> +   Take test_expect_success helper for example, write it like:
> +
> +  test_expect_success 'test title' '
> +  ... test body ...
> +  '

Indent the body further to the right?

> + - Indent the body of here-document, and use "<< -" instead of "<<" to strip prefix TAB:

Overly long line.

Did you mean to have a space between "<<" and "-"?

"prefix TAB" -> "leading TABs used for indentation" (plural is the
important part)?  

Mention end of here-document marker should by default be quoted,
unless the body needs $variable_interpolation?

> +  test_expect_success 'test something' '
> +      cat >expect <<-\EOF &&
> +      one
> +      two
> +      three
> +      EOF
> +      test_something > actual &&
> +      test_cmp expect actual
> +  '
> +
> +   Instead of:
> +
> +  test_expect_success 'test something' '
> +      cat >expect <<\EOF &&
> +  one
> +  two
> +  three
> +  EOF
> +      test_something > actual &&
> +      test_cmp expect actual
> +  '


Thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Re: [PATCH v3] ls-files: update test style
  2022-06-28 20:12     ` Junio C Hamano
@ 2022-06-29  7:12       ` lilinchao
  0 siblings, 0 replies; 9+ messages in thread
From: lilinchao @ 2022-06-29  7:12 UTC (permalink / raw)
  To: Junio C Hamano, Li Linchao via GitGitGadget
  Cc: git, Ævar Arnfjörð Bjarmason


>"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/t/README b/t/README
>> index 309a31133c6..5e0539412b4 100644
>> --- a/t/README
>> +++ b/t/README
>> @@ -547,6 +547,52 @@ This test harness library does the following things:
>>     consistently when command line arguments --verbose (or -v),
>>     --debug (or -d), and --immediate (or -i) is given.
>> 
>> +Recommended style
>> +-----------------
>> +Here are some recommented styles when writing test case.
>> +
>> + - Keep test title the same line with test helper function itself.
>> +
>> +   Take test_expect_success helper for example, write it like:
>> +
>> +  test_expect_success 'test title' '
>> +  ... test body ...
>> +  '
>
>Indent the body further to the right?
>
>> + - Indent the body of here-document, and use "<< -" instead of "<<" to strip prefix TAB:
>
>Overly long line.
>
>Did you mean to have a space between "<<" and "-"? 
Ops, I'll fix it. This came from an extension in VS code which is to preview AsciiDoc,
it mistakenly render "<<-" into "<←", so I put a space between "<<" and "-".
>
>"prefix TAB" -> "leading TABs used for indentation" (plural is the
>important part)?  
>
>Mention end of here-document marker should by default be quoted,
>unless the body needs $variable_interpolation? 
Sorry, I don't get it. I don't see many of ending "EOF" are quoted in our tests.
>
>> +  test_expect_success 'test something' '
>> +      cat >expect <<-\EOF &&
>> +      one
>> +      two
>> +      three
>> +      EOF
>> +      test_something > actual &&
>> +      test_cmp expect actual
>> +  '
>> +
>> +   Instead of:
>> +
>> +  test_expect_success 'test something' '
>> +      cat >expect <<\EOF &&
>> +  one
>> +  two
>> +  three
>> +  EOF
>> +      test_something > actual &&
>> +      test_cmp expect actual
>> +  '
>
>
>Thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-06-29  7:14 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-23  8:46 [PATCH] ls-files: update test style Li Linchao via GitGitGadget
2022-06-23 10:50 ` Ævar Arnfjörð Bjarmason
2022-06-24  4:57   ` lilinchao
2022-06-23 17:09 ` Junio C Hamano
2022-06-24  5:05   ` lilinchao
2022-06-28  9:14 ` [PATCH v2] " Li Linchao via GitGitGadget
2022-06-28  9:51   ` [PATCH v3] " Li Linchao via GitGitGadget
2022-06-28 20:12     ` Junio C Hamano
2022-06-29  7:12       ` lilinchao

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).