Re: Re: [PATCH] ls-files: update test style
- From
- lilinchao@oschina.cn <lilinchao@oschina.cn>
- Date
- Jun 24, 2022, 04:57 UTC
- Message-ID
- <2022062412565939884621@oschina.cn>
- In-Reply-To
- <220623.86wnd7k5un.gmgdl@evledraar.gmail.com>
Show 10 quoted lines
> >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.
Show 25 quoted lines
> >> 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.
Show 8 quoted lines
>> - 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.
Show 24 quoted lines
> >> +' >> >> # 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.
Show 18 quoted lines
> >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.