* Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail' [not found] ` <YDLXf+OoJabrJTWu@coredump.intra.peff.net> @ 2026-04-14 20:52 ` SZEDER Gábor 2026-04-14 21:11 ` Junio C Hamano 2026-04-14 22:14 ` Jeff King 0 siblings, 2 replies; 5+ messages in thread From: SZEDER Gábor @ 2026-04-14 20:52 UTC (permalink / raw) To: Jeff King; +Cc: git, Denton Liu On Sun, Feb 21, 2021 at 04:58:23PM -0500, Jeff King wrote: > On Sun, Feb 21, 2021 at 08:25:12PM +0100, SZEDER Gábor wrote: > > > In many test helper functions we verify that they were invoked with > > sensible parameters, and call BUG() to abort the test script when the > > parameters are buggy. 6a67c75948 (test-lib-functions: restrict > > test_must_fail usage, 2020-07-07) added such a parameter verification > > to 'test_must_fail', but it didn't report the error with BUG(), like > > we usually do. > > OK. I do not care all that much between BUG() and not-BUG here, since we > are unlikely to have a test where test_must_fail returning 0 yields > success. I guess the most interesting outcome is that we would notice a > bug in a test_expect_failure block. If I had managed to send a new version of this patch series in the last 5 years :), then this would have caught the issue noted in: https://public-inbox.org/git/ad6hovxCkwMTG11U@szeder.dev/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail' 2026-04-14 20:52 ` [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail' SZEDER Gábor @ 2026-04-14 21:11 ` Junio C Hamano 2026-04-14 22:18 ` Jeff King 2026-04-14 22:14 ` Jeff King 1 sibling, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2026-04-14 21:11 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Jeff King, git, Denton Liu SZEDER Gábor <szeder.dev@gmail.com> writes: > On Sun, Feb 21, 2021 at 04:58:23PM -0500, Jeff King wrote: >> On Sun, Feb 21, 2021 at 08:25:12PM +0100, SZEDER Gábor wrote: >> >> > In many test helper functions we verify that they were invoked with >> > sensible parameters, and call BUG() to abort the test script when the >> > parameters are buggy. 6a67c75948 (test-lib-functions: restrict >> > test_must_fail usage, 2020-07-07) added such a parameter verification >> > to 'test_must_fail', but it didn't report the error with BUG(), like >> > we usually do. >> >> OK. I do not care all that much between BUG() and not-BUG here, since we >> are unlikely to have a test where test_must_fail returning 0 yields >> success. I guess the most interesting outcome is that we would notice a >> bug in a test_expect_failure block. > > If I had managed to send a new version of this patch series in the > last 5 years :), then this would have caught the issue noted in: > > https://public-inbox.org/git/ad6hovxCkwMTG11U@szeder.dev/ I was wondering if we should remove "test_might_fail". Its use case is rather limited to very narrow cases, like * we want to kill something but it may have exited on its own * we want "git config --unset" but the variable may or may not be set * we want "git foo --abort" just in case we are in the middle of "git foo" all of which is clearer with "|| :", and more importantly, the thing whose "failure" is protected against the test framework declaring a test failure is *not* what we are testing (these "config --unset" are not about testing "git config", in other words). So the extra ability test_must_fail and test_might_fail have that they can detect uncontrolled death with non-zero exit status (aka "crash") is not very interesting---it is more like "As we are running a git command here, it would be better to catch than not catch a segfault here as well", i.e., a nice to have item. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail' 2026-04-14 21:11 ` Junio C Hamano @ 2026-04-14 22:18 ` Jeff King 2026-04-15 15:25 ` Junio C Hamano 0 siblings, 1 reply; 5+ messages in thread From: Jeff King @ 2026-04-14 22:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: SZEDER Gábor, git, Denton Liu On Tue, Apr 14, 2026 at 02:11:00PM -0700, Junio C Hamano wrote: > I was wondering if we should remove "test_might_fail". Its use case > is rather limited to very narrow cases, like > > * we want to kill something but it may have exited on its own > > * we want "git config --unset" but the variable may or may not be set > > * we want "git foo --abort" just in case we are in the middle of > "git foo" > > all of which is clearer with "|| :", and more importantly, the thing > whose "failure" is protected against the test framework declaring a > test failure is *not* what we are testing (these "config --unset" > are not about testing "git config", in other words). > > So the extra ability test_must_fail and test_might_fail have that > they can detect uncontrolled death with non-zero exit status (aka > "crash") is not very interesting---it is more like "As we are > running a git command here, it would be better to catch than not > catch a segfault here as well", i.e., a nice to have item. I think the main value of both (but especially test_might_fail) is that they slot naturally into &&-chains. I left a similar comment in that other thread, but to expand a bit, if you do: false && true || : && echo everything ok you will get "everything ok", even though step 1 failed. You need: false && { true || : } && echo everything ok except that because it is shell you have to add an extra semicolon after the ":". ;) Syntax-complaints aside, I think it is a very easy thing for contributors to get wrong. So I think test_might_fail has value, though I do not care if it has a different name. -Peff ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail' 2026-04-14 22:18 ` Jeff King @ 2026-04-15 15:25 ` Junio C Hamano 0 siblings, 0 replies; 5+ messages in thread From: Junio C Hamano @ 2026-04-15 15:25 UTC (permalink / raw) To: Jeff King; +Cc: SZEDER Gábor, git, Denton Liu Jeff King <peff@peff.net> writes: > I think the main value of both (but especially test_might_fail) is that > they slot naturally into &&-chains. I left a similar comment in that > other thread, but to expand a bit, if you do: > > false && > true || : && > echo everything ok > > you will get "everything ok", even though step 1 failed. You need: > > false && > { true || : } && > echo everything ok > > except that because it is shell you have to add an extra semicolon after > the ":". ;) > > Syntax-complaints aside, I think it is a very easy thing for > contributors to get wrong. So I think test_might_fail has value, though > I do not care if it has a different name. Yup, I recall that I recently said that I hate that semicolon ;-) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail' 2026-04-14 20:52 ` [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail' SZEDER Gábor 2026-04-14 21:11 ` Junio C Hamano @ 2026-04-14 22:14 ` Jeff King 1 sibling, 0 replies; 5+ messages in thread From: Jeff King @ 2026-04-14 22:14 UTC (permalink / raw) To: SZEDER Gábor; +Cc: git, Denton Liu On Tue, Apr 14, 2026 at 10:52:33PM +0200, SZEDER Gábor wrote: > On Sun, Feb 21, 2021 at 04:58:23PM -0500, Jeff King wrote: > > On Sun, Feb 21, 2021 at 08:25:12PM +0100, SZEDER Gábor wrote: > > > > > In many test helper functions we verify that they were invoked with > > > sensible parameters, and call BUG() to abort the test script when the > > > parameters are buggy. 6a67c75948 (test-lib-functions: restrict > > > test_must_fail usage, 2020-07-07) added such a parameter verification > > > to 'test_must_fail', but it didn't report the error with BUG(), like > > > we usually do. > > > > OK. I do not care all that much between BUG() and not-BUG here, since we > > are unlikely to have a test where test_must_fail returning 0 yields > > success. I guess the most interesting outcome is that we would notice a > > bug in a test_expect_failure block. > > If I had managed to send a new version of this patch series in the > last 5 years :), then this would have caught the issue noted in: > > https://public-inbox.org/git/ad6hovxCkwMTG11U@szeder.dev/ Looking at that old thread, I do not see any reason you should not re-send it (with or without the cosmetic fixups I suggested on top). -Peff ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-04-15 15:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20210221192512.3096291-1-szeder.dev@gmail.com>
[not found] ` <20210221192512.3096291-2-szeder.dev@gmail.com>
[not found] ` <YDLXf+OoJabrJTWu@coredump.intra.peff.net>
2026-04-14 20:52 ` [PATCH 2/2] test-lib-functions: use BUG() in 'test_must_fail' SZEDER Gábor
2026-04-14 21:11 ` Junio C Hamano
2026-04-14 22:18 ` Jeff King
2026-04-15 15:25 ` Junio C Hamano
2026-04-14 22:14 ` Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox