| Message ID | pull.1675.v2.git.1709243831190.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] tests: modernize the test script t0010-racy-git.sh | expand |
On Thu, Feb 29, 2024 at 4:57 PM Aryan Gupta via GitGitGadget <gitgitgadget@gmail.com> wrote: > Modernize the formatting of the test script to align with current > standards and improve its overall readability. > > Signed-off-by: Aryan Gupta <garyan447@gmail.com> > --- > diff --git a/t/t0010-racy-git.sh b/t/t0010-racy-git.sh > @@ -16,19 +16,18 @@ do > files=$(git diff-files -p) > - test_expect_success \ > - "Racy GIT trial #$trial part A" \ > - 'test "" != "$files"' > + test_expect_success "Racy git trial #$trial part A" ' > + test "" != "$files" > + ' This version (v2) addresses my review comments about v1. Thanks.
"Aryan Gupta via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Aryan Gupta <garyan447@gmail.com> > > Modernize the formatting of the test script to align with current > standards and improve its overall readability. OK. > diff --git a/t/t0010-racy-git.sh b/t/t0010-racy-git.sh > index 837c8b7228b..2ceac06c9c2 100755 > --- a/t/t0010-racy-git.sh > +++ b/t/t0010-racy-git.sh > @@ -1,6 +1,6 @@ > #!/bin/sh > > -test_description='racy GIT' > +test_description='racy git' This does not look like updating formatting to the current standard, or improving readability, though. > - test_expect_success \ > - "Racy GIT trial #$trial part A" \ > - 'test "" != "$files"' > + test_expect_success "Racy git trial #$trial part A" ' > + test "" != "$files" > + ' > ... > - test_expect_success \ > - "Racy GIT trial #$trial part B" \ > - 'test "" != "$files"' > - > + test_expect_success "Racy git trial #$trial part B" ' > + test "" != "$files" > + ' These are not wrong per-se, but if we are updating, I wonder if we want to get rid of statements outside the test_expect_success block, not just the formatting of individual test_expect_success tests. Looking at an iteration of the loop, the executable statements from here ... rm -f .git/index echo frotz >infocom git update-index --add infocom echo xyzzy >infocom files=$(git diff-files -p) ... to here is what we would make part of one test, namely ... test_expect_success \ "Racy GIT trial #$trial part A" \ 'test "" != "$files"' ... this one. Then sleep 1 is a cricial delay to have before the next test, which we may want to have outside to make it clear what is going on to readers. But the following parts ... echo xyzzy >cornerstone git update-index --add cornerstone files=$(git diff-files -p) ... up to here, we would make part of the next test ... test_expect_success \ "Racy GIT trial #$trial part B" \ 'test "" != "$files"' ... in the modern style. So, we may want to do it more like this, perhaps? test_expect_success "Racy GIT trial #$trial part A" ' rm -f .git/index && echo frotz >infocom && git update-index --add infocom && echo xyzzy >infocom && files=$(git diff-files -p) && test "" != "$files" ' sleep 1 test_expect_success "Racy GIT trial #$trial part B" ' echo xyzzy >cornerstone && git update-index --add cornerstone && files=$(git diff-files -p) && test "" != "$files" ' An added benefit of the more modern style to place as much as possible to &&-chain in test_expect_success block is that we would catch breakage in "git update-index" and other things used to set-up the test as well. With the original loop, breakages in things outside the test_expect_success blocks will go unchecked.
On Thu, Feb 29, 2024 at 6:14 PM Junio C Hamano <gitster@pobox.com> wrote: > So, we may want to do it more like this, perhaps? > > test_expect_success "Racy GIT trial #$trial part A" ' > rm -f .git/index && > echo frotz >infocom && > git update-index --add infocom && > echo xyzzy >infocom && > > files=$(git diff-files -p) && > test "" != "$files" > ' If taking it to this extent, then the modernized version of the last couple lines would be: git diff-files -p >out && test_file_not_empty out
Eric Sunshine <sunshine@sunshineco.com> writes: > On Thu, Feb 29, 2024 at 6:14 PM Junio C Hamano <gitster@pobox.com> wrote: >> So, we may want to do it more like this, perhaps? >> >> test_expect_success "Racy GIT trial #$trial part A" ' >> rm -f .git/index && >> echo frotz >infocom && >> git update-index --add infocom && >> echo xyzzy >infocom && >> >> files=$(git diff-files -p) && >> test "" != "$files" >> ' > > If taking it to this extent, then the modernized version of the last > couple lines would be: > > git diff-files -p >out && > test_file_not_empty out Yes. The modern style seems to prefer temporary files over variables; the reason probably is because it tends to be easier to remotely post-mortem?
On Thu, Feb 29, 2024 at 6:36 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > If taking it to this extent, then the modernized version of the last > > couple lines would be: > > > > git diff-files -p >out && > > test_file_not_empty out > > Yes. The modern style seems to prefer temporary files over > variables; the reason probably is because it tends to be easier to > remotely post-mortem? Yes, that seems likely. Functions such as test_file_not_empty() also provide an immediate indication of what went wrong without having to spelunk the test detritus: 'foo' is not a non-empty file. [*] whereas `test "" != "$foo"` provides no output at all, so it's not immediately clear which part of the test failed. Peff's `verbose` function was intended to mitigate that problem by making the expression verbose upon failure: verbose test "" != "$foo" && but never really caught on and was eventually retired by 8ddfce7144 (t: drop "verbose" helper function, 2023-05-08). [*]: Admittedly, the double-negative in "'foo' is not a non-empty file." is more than a little confusing. It probably would have been better phrased as "'foo' should be empty but is not".
On Thu, Feb 29, 2024 at 6:52 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > [*]: Admittedly, the double-negative in "'foo' is not a non-empty > file." is more than a little confusing. It probably would have been > better phrased as "'foo' should be empty but is not". The double-negative confused me even when suggesting a replacement. What I meant was that a better phrasing would perhaps have been: 'foo' is empty but should not be
Eric Sunshine <sunshine@sunshineco.com> writes: > The double-negative confused me even when suggesting a replacement. > What I meant was that a better phrasing would perhaps have been: > > 'foo' is empty but should not be +1. Thanks for caring.
diff --git a/t/t0010-racy-git.sh b/t/t0010-racy-git.sh index 837c8b7228b..2ceac06c9c2 100755 --- a/t/t0010-racy-git.sh +++ b/t/t0010-racy-git.sh @@ -1,6 +1,6 @@ #!/bin/sh -test_description='racy GIT' +test_description='racy git' TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh @@ -16,19 +16,18 @@ do echo xyzzy >infocom files=$(git diff-files -p) - test_expect_success \ - "Racy GIT trial #$trial part A" \ - 'test "" != "$files"' + test_expect_success "Racy git trial #$trial part A" ' + test "" != "$files" + ' sleep 1 echo xyzzy >cornerstone git update-index --add cornerstone files=$(git diff-files -p) - test_expect_success \ - "Racy GIT trial #$trial part B" \ - 'test "" != "$files"' - + test_expect_success "Racy git trial #$trial part B" ' + test "" != "$files" + ' done test_done