On Mon, Jul 22, 2024 at 1:31 PM René Scharfe <l.s.r@xxxxxx> wrote:
>
> Am 22.07.24 um 21:36 schrieb Junio C Hamano:
> > Kyle Lippincott <spectral@xxxxxxxxxx> writes:
> >
> >>> + for_test ("for_test passing test")
> >>> + check_int(1, ==, 1);
> >>
> >> I'm concerned that users will write this like:
> >> + for_test ("for_test passing test");
> >> + check_int(1, ==, 1);
> >
> > And that is exactly why we want the macro name to include C keyword
> > for control structures.
> >
> >> And the issue won't be caught.
> >
> > You are right. Making an empty body somehow catchable by the
> > compiler would be a vast improvement.
>
> That would be nice, but I have no idea how to do that without compiler
> changes. In the meantime the existing runtime checks will catch both
> the empty test in the first line and the out-of-test check in the second
> one and report them like this:
>
> # BUG: test has no checks at t/helper/test-example-tap.c:75
> not ok 1 - for_test passing test
> Assertion failed: (ctx.running), function test_assert, file test-lib.c, line 267.
>
> File name and line in the second one are not as helpful as they could
> be. Here's a patch to improve that output.
>
> --- >8 ---
> Subject: [PATCH] unit-tests: show location of checks outside of tests
>
> Checks outside of tests are caught at runtime and reported like this:
>
> Assertion failed: (ctx.running), function test_assert, file test-lib.c, line 267.
>
> The assert() call aborts the unit test and doesn't reveal the location
> or even the type of the offending check, as test_assert() is called by
> all of them.
>
> Handle it like the opposite case, a test without any checks: Don't
> abort, but report the location of the actual check, along with a message
> explaining the situation. The output for example above becomes:
Oh, I referenced "detect tests without checks" as a possible
improvement in my previous message, and didn't actually check whether
it was there. This statement made me go and check, and you're right,
it is there already. So I think we're well defended against the empty
check case, especially with the improvement here. Thanks!
If we have a good set of protections against misuse, does this mean
we're free to rename it? :) The concern raised for why `for` had to be
in the name was because it was using a control statement (a
at-most-1-execution for loop) to achieve its magic, and the control
statement puts certain restrictions and obligations on how to use it
correctly. If the misuse is detected reliably, we can choose a name
that's more descriptive about what it's doing.
>
> # BUG: check outside of test at t/helper/test-example-tap.c:75
>
> ... and the unit test program continues.
>
> Signed-off-by: René Scharfe <l.s.r@xxxxxx>
> ---
> t/unit-tests/test-lib.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
> index 3c513ce59a..9977c81739 100644
> --- a/t/unit-tests/test-lib.c
> +++ b/t/unit-tests/test-lib.c
> @@ -264,7 +264,11 @@ static void test_todo(void)
>
> int test_assert(const char *location, const char *check, int ok)
> {
> - assert(ctx.running);
> + if (!ctx.running) {
> + test_msg("BUG: check outside of test at %s",
> + make_relative(location));
> + return 0;
> + }
>
> if (ctx.result == RESULT_SKIP) {
> test_msg("skipping check '%s' at %s", check,
> --
> 2.45.2
