Re: [GSOC][PATCH] t4121: modernize test style
- From
Victoria Dye <vdye@github.com>
- Date
- Feb 21, 2023, 17:22 UTC
- Message-ID
- <38cb184d-e47a-2129-a93e-16ffd2afe67a@github.com>
- In-Reply-To
- <20230220235121.34375-1-gvivan6@gmail.com>
Vivan Garg wrote:
> Test scripts in file t4121-apply-diffs.sh are written in old style, > where the test_expect_success command and test title are written on > separate lines
nit: period at the end of the sentence (s/lines/lines.)
Also, this commit message explains *why* you're making the change, but not what the commit actually does (that is, update the tests to adhere to the new style). Would you mind adding a note about that to the message?
Show 16 quoted lines
> > Signed-off-by: Vivan Garg <gvivan6@gmail.com> > --- > Greetings, my name is Vivan Garg. I am currently pursuing a double major > in computer science and finance at the University of Waterloo in Canada. > I am currently completing my third software developer internship at Morgan > Stanley. As part of my coursework, I studied C and shell scripting, which > I then applied in internships and personal projects. C++ is the programming > language that I am most comfortable with right now. Please feel free to > address me as Vivan, and my pronouns are he/him/his. I meet the requirements > for GSOC participation. So far, I've either read or skimmed the following > documents based on prior knowledge: Submitting patches, Coding guidelines, > Myfirstcontribution.txt, gittutorial, Giteveryday, readme, Hacking-Git, > General-Microproject-Information, SoC-2023-Ideas, and > General-Application-Information. I'm looking forward to having a fantastic > time here!
Welcome to the Git community, and thanks for your contribution! :)
Show 26 quoted lines
> > t/t4121-apply-diffs.sh | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/t/t4121-apply-diffs.sh b/t/t4121-apply-diffs.sh > index a80cec9d11..2ff38ededa 100755 > --- a/t/t4121-apply-diffs.sh > +++ b/t/t4121-apply-diffs.sh > @@ -16,8 +16,8 @@ echo '1 > 7 > 8' >file > > -test_expect_success 'setup' \ > - 'git add file && > +test_expect_success 'setup' ' > + git add file && > git commit -q -m 1 && > git checkout -b test && > mv file file.tmp && > @@ -27,10 +27,11 @@ test_expect_success 'setup' \ > git commit -a -q -m 2 && > echo 9 >>file && > git commit -a -q -m 3 && > - git checkout main' > + git checkout main > +'
This test looks good.
Show 7 quoted lines
> > -test_expect_success \ > - 'check if contextually independent diffs for the same file apply' \ > - '( git diff test~2 test~1 && git diff test~1 test~0 )| git apply' > +test_expect_success 'check if contextually independent diffs for the same file apply' ' > + ( git diff test~2 test~1 && git diff test~1 test~0 )| git apply > +'
As for this one, the test is correctly updated to the new style (per the microproject prompt). However, the spacing around the '|' is a little weird - I think there should be a space after ')'. On your next re-roll, could you fix that spacing (in this patch is fine - it's not a substantial enough change to warrant its own commit)?
> > test_done