Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 7, 2025, 18:41 UTC
Message-ID: <xmqqv7uqsc03.fsf@gitster.g>
In-Reply-To: <xmqq7c76trpa.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 24 quoted lines

> A quick peek at [PATCH 2/2] tells me that this is not even something
> that would make it easier to port the existing tests by allowing
> more straight line-by-line copies or something.  The patch splits
> many in-line test pieces in the "main" into separate functions, and
> it does so in a rather unusual format, e.g.,
>
>   void test_hash__multi_character(void) TEST_HASH_STR("abc",
>           "a9993e364706816aba3e25717850c26c9cd0d89d",
>           "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad")
>
> where TEST_HASH_STR() expands to the function body that starts with
> a "{" and ends with a "}".  It can well be written more like
>
>     void test_hash__multi_character(void)
>     {
> 	TEST_HASH_STR("abc",
>         	"a9993e364706816aba3e25717850c26c9cd0d89d",
> 		"ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
>     }
>
> and we do not need this step at all if we did so.  Such a construct
> would be a lot friendlier to the editors that auto-indent, too.
>
> So, I do not quite see much value in this particular change.

Having said that, if this were more like that you write a series of

    DEF_HASH_TEST(multi_character, "abc", "a9993e...", "ba7816bf...")

and they expand to

    void test_hash__multi_character(void)
    {
	const char *expected[] = {"a9993e...", "ba7816bf..."};
	check_hash_data("abc", strlen("abc"), expected);
    }

then a preparatory step like this patch _might_ be justifiable. You may want to avoid having to write too many boilerplate, and a special rule to find "DEF_HASH_TEST(name, ...)" and it might make sense to add support to extract the name of the test function being defined by the macro automatically.

Not that I think such a sequence of DEF_HASH_TEST(), one per line, is an improvement at all (it also is unfriendly to editors that auto-indent the same way as your original version). I just wanted to say that a change to the pattern to pick up the function name may be justifiable if it were so.

Thanks.