| Message ID | pull.1269.git.1655974015414.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ls-files: update test style | expand |
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.
"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.
> >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.
>"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.
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 &&