| Message ID | patch-v2-1.8-7c9f8d2830f-20221202T000227Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tests: fix ignored & hidden exit codes | expand |
On Thu, Dec 1, 2022 at 7:07 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > Change an "exit 1" added in ac52d9410e5 (t4205: cover `git log > --reflog -z` blindspot, 2019-11-19) to use "return 1" instead, which > curiously was done in an adjacent test case added in the same commit. > > Using "exit 1" outside a sub-shell will cause the test framework > itself to exit on failure, which isn't what we want to do here. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh > @@ -156,7 +156,7 @@ test_expect_success 'NUL termination with --reflog --pretty=oneline' ' > for r in $revs > do > git show -s --pretty=oneline "$r" >raw && > - cat raw | lf_to_nul || exit 1 > + cat raw | lf_to_nul || return 1 > done >expect && Using `return 1` here is "obviously correct" since this code is not inside a subshell. Furthermore, the exit code of the for-loop itself is not being lost down a pipe, so `return 1` is indeed an appropriate way to signal failure. Good.
Eric Sunshine <sunshine@sunshineco.com> writes: >> git show -s --pretty=oneline "$r" >raw && >> - cat raw | lf_to_nul || exit 1 >> + cat raw | lf_to_nul || return 1 >> done >expect && > > Using `return 1` here is "obviously correct" since this code is not > inside a subshell. Furthermore, the exit code of the for-loop itself > is not being lost down a pipe, so `return 1` is indeed an appropriate > way to signal failure. Good. "return 1" is obvious and safe correction. I have to wonder if test_expect_success can be taught to be smarter to intercept "exit" so we do not have to bo so careful, but that would be a much more involved change to the lower-level of test framework.
On Fri, Dec 02 2022, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >>> git show -s --pretty=oneline "$r" >raw && >>> - cat raw | lf_to_nul || exit 1 >>> + cat raw | lf_to_nul || return 1 >>> done >expect && >> >> Using `return 1` here is "obviously correct" since this code is not >> inside a subshell. Furthermore, the exit code of the for-loop itself >> is not being lost down a pipe, so `return 1` is indeed an appropriate >> way to signal failure. Good. > > "return 1" is obvious and safe correction. I have to wonder if > test_expect_success can be taught to be smarter to intercept "exit" > so we do not have to bo so careful, but that would be a much more > involved change to the lower-level of test framework. I can't think of a way to do so that wouldn't involve running the test in a sub-shell, which I think would bring us to the state management problems noted in [1] for Phillip's "test_todo" series, except in this case we'd have those issues trying to pass state back from the "test_expect_success". It's possible, but we'd need to change a lot of code that's expecting to talk to itself via variables in the same shell to use IPC between shells, wouldn't we? 1. https://lore.kernel.org/git/221006.86v8owr986.gmgdl@evledraar.gmail.com/
Eric Sunshine <sunshine@sunshineco.com> writes: >> for r in $revs >> do >> git show -s --pretty=oneline "$r" >raw && >> - cat raw | lf_to_nul || exit 1 >> + cat raw | lf_to_nul || return 1 >> done >expect && > > Using `return 1` here is "obviously correct" since this code is not > inside a subshell. While it is safe and correct, I wonder if test_expect_success can be made a bit smarter by somehow capturing what happens inside its body. We of course cannot just wrap everything inside a subshell, as we do want to allow the test body to affect the shell environment it is running in with its side effects.
On Fri, Dec 2, 2022 at 3:55 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Fri, Dec 02 2022, Junio C Hamano wrote: > > "return 1" is obvious and safe correction. I have to wonder if > > test_expect_success can be taught to be smarter to intercept "exit" > > so we do not have to bo so careful, but that would be a much more > > involved change to the lower-level of test framework. > > I can't think of a way to do so that wouldn't involve running the test > in a sub-shell, which I think would bring us to the state management > problems noted in [1] for Phillip's "test_todo" series, except in this > case we'd have those issues trying to pass state back from the > "test_expect_success". > > It's possible, but we'd need to change a lot of code that's expecting to > talk to itself via variables in the same shell to use IPC between > shells, wouldn't we? It might make more sense to turn this on its head and make it a linting issue and simply throw a "?!FOO?!" as is done for other suspect shell code. In fact, I already have local chainlint.pl patches which detect whether a subshell is active so that the linter can complain if it sees `cd` outside of a subshell. I would think that warning about misuse of `exit 1` outside a subshell (and perhaps `return 1` inside a subshell) should be possible, though I haven't thought through all the possibilities.
On Fri, Dec 02 2022, Eric Sunshine wrote: > On Fri, Dec 2, 2022 at 3:55 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> On Fri, Dec 02 2022, Junio C Hamano wrote: >> > "return 1" is obvious and safe correction. I have to wonder if >> > test_expect_success can be taught to be smarter to intercept "exit" >> > so we do not have to bo so careful, but that would be a much more >> > involved change to the lower-level of test framework. >> >> I can't think of a way to do so that wouldn't involve running the test >> in a sub-shell, which I think would bring us to the state management >> problems noted in [1] for Phillip's "test_todo" series, except in this >> case we'd have those issues trying to pass state back from the >> "test_expect_success". >> >> It's possible, but we'd need to change a lot of code that's expecting to >> talk to itself via variables in the same shell to use IPC between >> shells, wouldn't we? > > It might make more sense to turn this on its head and make it a > linting issue and simply throw a "?!FOO?!" as is done for other > suspect shell code. In fact, I already have local chainlint.pl patches > which detect whether a subshell is active so that the linter can > complain if it sees `cd` outside of a subshell. I would think that > warning about misuse of `exit 1` outside a subshell (and perhaps > `return 1` inside a subshell) should be possible, though I haven't > thought through all the possibilities. That would be great. As a reminder I think (maybe it's not what you have in mind exactly?) that we had a brief discussion on this topic starting at [1]. I.e. I was hoping chainlint.pl could be extended to detect exactly these sort of "test...$(git" patterns (among other things). This topic doesn't get us there, but once we finally get rid of some of those patterns it would be nice to have assertions to ensure they don't come back. 1. https://lore.kernel.org/git/221024.865yg9ecsx.gmgdl@evledraar.gmail.com/
On Fri, Dec 2, 2022 at 5:08 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Fri, Dec 02 2022, Eric Sunshine wrote: > > It might make more sense to turn this on its head and make it a > > linting issue and simply throw a "?!FOO?!" as is done for other > > suspect shell code. In fact, I already have local chainlint.pl patches > > which detect whether a subshell is active so that the linter can > > complain if it sees `cd` outside of a subshell. I would think that > > warning about misuse of `exit 1` outside a subshell (and perhaps > > `return 1` inside a subshell) should be possible, though I haven't > > thought through all the possibilities. > > That would be great. As a reminder I think (maybe it's not what you have > in mind exactly?) that we had a brief discussion on this topic starting > at [1]. I.e. I was hoping chainlint.pl could be extended to detect > exactly these sort of "test...$(git" patterns (among other things). I recall it but haven't really put any additional thought into it. I did put a little bit of thought into how to upgrade chainlint.pl to supplant check-non-portable-shell.pl, though that doesn't help much with your wish list. More recently, I've been thinking that it might make more sense for chainlint.pl to generate an AST rather than working against the raw token stream, which may mesh better with the sort of rewriting you mentioned, and may simplify (or complexify) some of the linting heuristics. I haven't really thought it through yet, and I'm always concerned about slowing down the script too much -- but perhaps I'm worrying unnecessarily if your Makefile-based parallelization patches land, or if we just rip out parallelization altogether as Peff was suggesting. > This topic doesn't get us there, but once we finally get rid of some of > those patterns it would be nice to have assertions to ensure they don't > come back. Ya.
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index e448ef2928a..0404491d6ee 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -156,7 +156,7 @@ test_expect_success 'NUL termination with --reflog --pretty=oneline' ' for r in $revs do git show -s --pretty=oneline "$r" >raw && - cat raw | lf_to_nul || exit 1 + cat raw | lf_to_nul || return 1 done >expect && # the trailing NUL is already produced so we do not need to # output another one
Change an "exit 1" added in ac52d9410e5 (t4205: cover `git log --reflog -z` blindspot, 2019-11-19) to use "return 1" instead, which curiously was done in an adjacent test case added in the same commit. Using "exit 1" outside a sub-shell will cause the test framework itself to exit on failure, which isn't what we want to do here. This issue was spotted with the in-flight "GIT_TEST_PASSING_SANITIZE_LEAK=check" test mode[1]. This "git show" invocation currently leaks memory, and we'd thus "exit 1". This change was initially part of that topic[2] to demonstrate the correctness of the "check" implementation. 1. https://lore.kernel.org/git/patch-07.10-0961df2ab6c-20220719T205710Z-avarab@gmail.com/ 2. https://lore.kernel.org/git/patch-10.10-9cedf0cb0e2-20220719T205710Z-avarab@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t4205-log-pretty-formats.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)