[v3,02/10] trailer: add unit tests for trailer iterator

Commit Message

Linus Arver April 26, 2024, 12:26 a.m. UTC

From: Linus Arver <[e-mail]>

Test the number of trailers found by the iterator (to be more precise,
the parsing mechanism which the iterator just walks over) when given
some some arbitrary log message.

We test the iterator because it is a public interface function exposed
by the trailer API (we generally don't want to test internal
implementation details which are, unlike the API, subject to drastic
changes).

Signed-off-by: Linus Arver <[e-mail]>
---
 Makefile                 |   1 +
 t/unit-tests/t-trailer.c | 174 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 175 insertions(+)
 create mode 100644 t/unit-tests/t-trailer.c

Comments

Christian Couder April 26, 2024, 2:51 p.m. UTC | #1

On Fri, Apr 26, 2024 at 2:26 AM Linus Arver via GitGitGadget
<[e-mail]> wrote:
>
> From: Linus Arver <[e-mail]>
>
> Test the number of trailers found by the iterator (to be more precise,
> the parsing mechanism which the iterator just walks over) when given
> some some arbitrary log message.

s/some some/some/

> We test the iterator because it is a public interface function exposed
> by the trailer API (we generally don't want to test internal
> implementation details which are, unlike the API, subject to drastic
> changes).
>
> Signed-off-by: Linus Arver <[e-mail]>


> +static void run_t_trailer_iterator(void)
> +{
> +       static struct test_cases {
> +               const char *name;
> +               const char *msg;
> +               size_t num_expected_trailers;
> +       } tc[] = {

...

> +       };
> +
> +       for (int i = 0; i < sizeof(tc) / sizeof(tc[0]); i++) {
> +               TEST(t_trailer_iterator(tc[i].msg,
> +                                       tc[i].num_expected_trailers),
> +                    "%s", tc[i].name);

Nit: the members of struct test_cases are used in the (msg,
num_expected_trailers, name) order, while they are declared in the
(name, msg, num_expected_trailers) order. I think it would make it a
bit easier to use in struct test_cases the same order in which they
are used in the TEST() macro.

> +       }
> +}
> +
> +int cmd_main(int argc, const char **argv)
> +{
> +       run_t_trailer_iterator();
> +       return test_done();
> +}

LGTM otherwise.

Junio C Hamano April 26, 2024, 4:20 p.m. UTC | #2

Christian Couder <[e-mail]> writes:

> On Fri, Apr 26, 2024 at 2:26 AM Linus Arver via GitGitGadget
> <[e-mail]> wrote:
>>
>> From: Linus Arver <[e-mail]>
>>
>> Test the number of trailers found by the iterator (to be more precise,
>> the parsing mechanism which the iterator just walks over) when given
>> some some arbitrary log message.
>
> s/some some/some/

Right.

>> +static void run_t_trailer_iterator(void)
>> +{
>> +       static struct test_cases {
>> +               const char *name;
>> +               const char *msg;
>> +               size_t num_expected_trailers;
>> +       } tc[] = {
>
> ...
>
>> +       };
>> +
>> +       for (int i = 0; i < sizeof(tc) / sizeof(tc[0]); i++) {
>> +               TEST(t_trailer_iterator(tc[i].msg,
>> +                                       tc[i].num_expected_trailers),
>> +                    "%s", tc[i].name);
>
> Nit: the members of struct test_cases are used in the (msg,
> num_expected_trailers, name) order, while they are declared in the
> (name, msg, num_expected_trailers) order. I think it would make it a
> bit easier to use in struct test_cases the same order in which they
> are used in the TEST() macro.

I am not sure if I agree.  In the array of struct, being able to
identify each array item with its .name component makes quite a lot
of sense, especially when the .name member is not really part of the
data used in tests but is used as an identifier for the tuple made
of other members (i.e., <msg, num_expected_trailers> in this case).

The TEST() macro is unable to take "name" as an early parameter than
others due to how it wants to create the identifying string (i.e.,
doing an equivalent of strfmt() on tc[i].name), but reordering the
struct members to match the peculiar order the members are used
smells like a tail wagging a dog.

>
>> +       }
>> +}
>> +
>> +int cmd_main(int argc, const char **argv)
>> +{
>> +       run_t_trailer_iterator();
>> +       return test_done();
>> +}
>
> LGTM otherwise.

Thanks.

Linus Arver April 26, 2024, 4:25 p.m. UTC | #3

Hello Christian!

Christian Couder <[e-mail]> writes:

> On Fri, Apr 26, 2024 at 2:26 AM Linus Arver via GitGitGadget
> <[e-mail]> wrote:
>>
>> From: Linus Arver <[e-mail]>
>>
>> Test the number of trailers found by the iterator (to be more precise,
>> the parsing mechanism which the iterator just walks over) when given
>> some some arbitrary log message.
>
> s/some some/some/

Fixed locally, thanks. Will send as part of a reroll pending further
review comments.

>> We test the iterator because it is a public interface function exposed
>> by the trailer API (we generally don't want to test internal
>> implementation details which are, unlike the API, subject to drastic
>> changes).
>>
>> Signed-off-by: Linus Arver <[e-mail]>
>
>
>> +static void run_t_trailer_iterator(void)
>> +{
>> +       static struct test_cases {
>> +               const char *name;
>> +               const char *msg;
>> +               size_t num_expected_trailers;
>> +       } tc[] = {
>
> ...
>
>> +       };
>> +
>> +       for (int i = 0; i < sizeof(tc) / sizeof(tc[0]); i++) {
>> +               TEST(t_trailer_iterator(tc[i].msg,
>> +                                       tc[i].num_expected_trailers),
>> +                    "%s", tc[i].name);
>
> Nit: the members of struct test_cases are used in the (msg,
> num_expected_trailers, name) order, while they are declared in the
> (name, msg, num_expected_trailers) order. I think it would make it a
> bit easier to use in struct test_cases the same order in which they
> are used in the TEST() macro.

This bothered me as well, but ultimately I preferred to see the test
names first in the actual test cases where each one is defined like

     {
        "name of test",
        ...
     },
     {
        "name of another test",
        ...
     }
     ...

instead of the other way around. FWIW this style comes from Golang where
it is the standard practice there. I suppose in this instance we have
test cases like

     {
             "without body text",
             "subject: foo bar\n"
             "\n"
             "Fixes: x\n"
             "Acked-by: x\n"
             "Reviewed-by: x\n",
             3
     },

and the separation between "name" vs "msg" could be a bit confusing on
first glance, but I don't think that's a big deal. Plus our
test_expect_success shell functions also expect the name as the first
parameter, so it would be consistent with that style.

It's unfortunate that we cannot put __VA_ARGS__ as the "first parameter"
to the TEST() macro, like

    TEST("%s", tc[i].name,
         t_trailer_iterator(tc[i].msg,
                            tc[i].num_expected_trailers),
        );

but I suppose that's a limitation of __VA_ARGS__. I also do wonder
whether we even need the test case name to be __VA_ARGS__ at all though
(we certainly don't *need* it here as the test case names are already
unique) --- so it might be fine to have another macro that only takes
the test name and a test function. Something like

    #define TC(name, t) ...

on top of the

    #define TEST(t, ...) ...

we already have, perhaps? IDK.

Patch

diff --git a/Makefile b/Makefile
index d3a3f16f076..5418ddd03be 100644
--- a/Makefile
+++ b/Makefile
@@ -1347,6 +1347,7 @@  UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-prio-queue
 UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-trailer
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/unit-tests/t-trailer.c b/t/unit-tests/t-trailer.c
new file mode 100644
index 00000000000..c1f897235c7
--- /dev/null
+++ b/t/unit-tests/t-trailer.c
@@ -0,0 +1,174 @@ 
+#include "test-lib.h"
+#include "trailer.h"
+
+static void t_trailer_iterator(const char *msg, size_t num_expected_trailers)
+{
+	struct trailer_iterator iter;
+	size_t i = 0;
+
+	trailer_iterator_init(&iter, msg);
+	while (trailer_iterator_advance(&iter))
+		i++;
+	trailer_iterator_release(&iter);
+
+	check_uint(i, ==, num_expected_trailers);
+}
+
+static void run_t_trailer_iterator(void)
+{
+	static struct test_cases {
+		const char *name;
+		const char *msg;
+		size_t num_expected_trailers;
+	} tc[] = {
+		{
+			"empty input",
+			"",
+			0
+		},
+		{
+			"no newline at beginning",
+			"Fixes: x\n"
+			"Acked-by: x\n"
+			"Reviewed-by: x\n",
+			0
+		},
+		{
+			"newline at beginning",
+			"\n"
+			"Fixes: x\n"
+			"Acked-by: x\n"
+			"Reviewed-by: x\n",
+			3
+		},
+		{
+			"without body text",
+			"subject: foo bar\n"
+			"\n"
+			"Fixes: x\n"
+			"Acked-by: x\n"
+			"Reviewed-by: x\n",
+			3
+		},
+		{
+			"with body text, without divider",
+			"my subject\n"
+			"\n"
+			"my body which is long\n"
+			"and contains some special\n"
+			"chars like : = ? !\n"
+			"hello\n"
+			"\n"
+			"Fixes: x\n"
+			"Acked-by: x\n"
+			"Reviewed-by: x\n"
+			"Signed-off-by: x\n",
+			4
+		},
+		{
+			"with body text, without divider (second trailer block)",
+			"my subject\n"
+			"\n"
+			"my body which is long\n"
+			"and contains some special\n"
+			"chars like : = ? !\n"
+			"hello\n"
+			"\n"
+			"Fixes: x\n"
+			"Acked-by: x\n"
+			"Reviewed-by: x\n"
+			"Signed-off-by: x\n"
+			"\n"
+			/*
+			 * Because this is the last trailer block, it takes
+			 * precedence over the first one encountered above.
+			 */
+			"Helped-by: x\n"
+			"Signed-off-by: x\n",
+			2
+		},
+		{
+			"with body text, with divider",
+			"my subject\n"
+			"\n"
+			"my body which is long\n"
+			"and contains some special\n"
+			"chars like : = ? !\n"
+			"hello\n"
+			"\n"
+			"---\n"
+			"\n"
+			/*
+			 * This trailer still counts because the iterator
+			 * always ignores the divider.
+			 */
+			"Signed-off-by: x\n",
+			1
+		},
+		{
+			"with non-trailer lines in trailer block",
+			"subject: foo bar\n"
+			"\n"
+			/*
+			 * Even though this trailer block has a non-trailer line
+			 * in it, it's still a valid trailer block because it's
+			 * at least 25% trailers and is Git-generated (see
+			 * git_generated_prefixes[] in trailer.c).
+			 */
+			"not a trailer line\n"
+			"not a trailer line\n"
+			"not a trailer line\n"
+			"Signed-off-by: x\n",
+			1
+		},
+		{
+			"with non-trailer lines (one too many) in trailer block",
+			"subject: foo bar\n"
+			"\n"
+			/*
+			 * This block has only 20% trailers, so it's below the
+			 * 25% threshold.
+			 */
+			"not a trailer line\n"
+			"not a trailer line\n"
+			"not a trailer line\n"
+			"not a trailer line\n"
+			"Signed-off-by: x\n",
+			0
+		},
+		{
+			"with non-trailer lines (only 1) in trailer block, but no Git-generated trailers",
+			"subject: foo bar\n"
+			"\n"
+			/*
+			 * This block has only 1 non-trailer out of 10 (IOW, 90%
+			 * trailers) but is not considered a trailer block
+			 * because the 25% threshold only applies to cases where
+			 * there was a Git-generated trailer.
+			 */
+			"Reviewed-by: x\n"
+			"Reviewed-by: x\n"
+			"Reviewed-by: x\n"
+			"Helped-by: x\n"
+			"Helped-by: x\n"
+			"Helped-by: x\n"
+			"Acked-by: x\n"
+			"Acked-by: x\n"
+			"Acked-by: x\n"
+			"not a trailer line\n",
+			0
+		},
+	};
+
+	for (int i = 0; i < sizeof(tc) / sizeof(tc[0]); i++) {
+		TEST(t_trailer_iterator(tc[i].msg,
+					tc[i].num_expected_trailers),
+		     "%s", tc[i].name);
+	}
+}
+
+int cmd_main(int argc, const char **argv)
+{
+	run_t_trailer_iterator();
+	return test_done();
+}