| Message ID | patch-12.15-f3cc5bc7eb9-20220302T171755Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tests: don't ignore "git" exit codes | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > -test_have_prereq GETTEXT_LOCALE && > -test-tool regex "HALLÓ" "Halló" ICASE && > -test_set_prereq REGEX_LOCALE > +test_expect_success GETTEXT_LOCALE 'setup REGEX_LOCALE prerequisite' ' > + if test-tool regex "HALLÓ" "Halló" ICASE > + then > + test_set_prereq REGEX_LOCALE This looks sensible but > + else > + test_must_fail test-tool regex "HALLÓ" "Halló" ICASE this side looks puzzling. I think this way to avoid counting abort etc as passing "must fail" test would be the least bad that we can do. Nicely done. > + fi > +'
On Wed, Mar 02 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> -test_have_prereq GETTEXT_LOCALE && >> -test-tool regex "HALLÓ" "Halló" ICASE && >> -test_set_prereq REGEX_LOCALE >> +test_expect_success GETTEXT_LOCALE 'setup REGEX_LOCALE prerequisite' ' >> + if test-tool regex "HALLÓ" "Halló" ICASE >> + then >> + test_set_prereq REGEX_LOCALE > > This looks sensible but > >> + else >> + test_must_fail test-tool regex "HALLÓ" "Halló" ICASE > > this side looks puzzling. I think this way to avoid counting abort > etc as passing "must fail" test would be the least bad that we can > do. > > Nicely done. Thanks. For the purposes of a re-roll I'll note this as a "nothing to change", since the commit message explains why we're doing this, unless you have comments on that explanation (the last paragraph of the commit message).
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Wed, Mar 02 2022, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> -test_have_prereq GETTEXT_LOCALE && >>> -test-tool regex "HALLÓ" "Halló" ICASE && >>> -test_set_prereq REGEX_LOCALE >>> +test_expect_success GETTEXT_LOCALE 'setup REGEX_LOCALE prerequisite' ' >>> + if test-tool regex "HALLÓ" "Halló" ICASE >>> + then >>> + test_set_prereq REGEX_LOCALE >> >> This looks sensible but >> >>> + else >>> + test_must_fail test-tool regex "HALLÓ" "Halló" ICASE >> >> this side looks puzzling. I think this way to avoid counting abort >> etc as passing "must fail" test would be the least bad that we can >> do. >> >> Nicely done. > > Thanks. For the purposes of a re-roll I'll note this as a "nothing to > change", since the commit message explains why we're doing this, unless > you have comments on that explanation (the last paragraph of the commit > message). The comment was mostly if it is more appropriate to explain the puzzling code with an in-code comment, rather than the log message. In-code comment is for those who may find the current code strange. The log message is for those who wonder why the current code came to be in today's shape.
diff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh index ca3f24f8079..347bf4a12f3 100755 --- a/t/t7812-grep-icase-non-ascii.sh +++ b/t/t7812-grep-icase-non-ascii.sh @@ -11,9 +11,14 @@ test_expect_success GETTEXT_LOCALE 'setup' ' export LC_ALL ' -test_have_prereq GETTEXT_LOCALE && -test-tool regex "HALLÓ" "Halló" ICASE && -test_set_prereq REGEX_LOCALE +test_expect_success GETTEXT_LOCALE 'setup REGEX_LOCALE prerequisite' ' + if test-tool regex "HALLÓ" "Halló" ICASE + then + test_set_prereq REGEX_LOCALE + else + test_must_fail test-tool regex "HALLÓ" "Halló" ICASE + fi +' test_expect_success REGEX_LOCALE 'grep literal string, no -F' ' git grep -i "TILRAUN: Halló Heimur!" &&
Amend a prerequisite check added in 5c1ebcca4d1 (grep/icase: avoid kwsset on literal non-ascii strings, 2016-06-25) to do invoke 'test-tool regex' in such a way that we'll notice if it dies under SANITIZE=leak due to having a memory leak, as opposed to us not having the "ICASE" support we're checking for. Because we weren't making a distinction between the two I'd marked these tests as passing under SANITIZE=leak in 03d85e21951 (leak tests: mark remaining leak-free tests as such, 2021-12-17). Doing this is tricky. Ideally "test_lazy_prereq" would materialize as a "real" test that we could check the exit code of with the same signal matching that "test_must_fail" does. However lazy prerequisites aren't real tests, and are instead lazily materialized in the guts of "test_have_prereq" when we've already started another test. We could detect the abort() (or similar) there and pass that exit code down, and fail the test that caused the prerequisites to be materialized. But that would require extensive changes to test-lib.sh and test-lib-functions.sh. Let's instead simply check if the exit code of "test-tool regex" is zero, and if so set the prerequisites. If it's non-zero let's run it again with "test_must_fail". We'll thus make a distinction between "bad" non-zero (segv etc) and "good" (exit 1 etc.). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t7812-grep-icase-non-ascii.sh | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)