[v2,2/8] trailer: add unit tests for trailer iterator

Commit Message

Linus Arver April 19, 2024, 5:22 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 | 175 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 176 insertions(+)
 create mode 100644 t/unit-tests/t-trailer.c

Comments

Linus Arver April 19, 2024, 5:33 a.m. UTC | #1

"Linus Arver via GitGitGadget" <[e-mail]> writes:

> From: Linus Arver <[e-mail]>
> [...]
> +		{
> +			"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.
> +			 */
 
In the next reroll (sometime next week?), I should put the

    ... (see git_generated_prefixes[] in trailer.c).

comment up here (where we first mention "Git-generated" trailers)
instead of down in the last test case below.

> +			"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 because the
> +			 * 25% threshold only applies to cases where there was a
> +			 * Git-generated trailer (see git_generated_prefixes[]
> +			 * in trailer.c).
> +			 */
> +			"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
> +		},
> +	};

Linus Arver April 19, 2024, 6:46 p.m. UTC | #2

"Linus Arver via GitGitGadget" <[e-mail]> writes:

> From: Linus Arver <[e-mail]>
> 
> [...]
> 
> +		{
> +			"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 because the

s/a trailer/a trailer block

> +			 * 25% threshold only applies to cases where there was a
> +			 * Git-generated trailer (see git_generated_prefixes[]
> +			 * in trailer.c).
> +			 */
> +			"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
> +		},

Junio C Hamano April 19, 2024, 9:52 p.m. UTC | #3

"Linus Arver via GitGitGadget" <[e-mail]> writes:

> +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

Totally offtopic, but does it bother folks who are interested in
adding more unit tests that they do not seem to interact very well
with GIT_SKIP_TESTS environment variable?

> diff --git a/t/unit-tests/t-trailer.c b/t/unit-tests/t-trailer.c
> new file mode 100644
> index 00000000000..147a51b66b9
> --- /dev/null
> +++ b/t/unit-tests/t-trailer.c
> @@ -0,0 +1,175 @@
> +#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++;
> +	}

Unnecessary {braces} around a single-statement block?

> +	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;

This is more like number of lines in the trailer block, not
limiting its count only to true trailers, no?

> +	} tc[] = {
> ...
> +		{
> +			"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.
> +			 */
> +			"not a trailer line\n"
> +			"not a trailer line\n"
> +			"not a trailer line\n"
> +			"Signed-off-by: x\n",
> +			1
> +		},

It is OK to leave it num_expected_trailers in this step and then
rename it when you update this "1" (number of real trailer lines)
to "4" (number of lines in the trailer block).

I wonder if you'd want to make more data available to the test.  At
least it would be more useful if the number of true trailer lines
and the number of lines in the trialer block are available
separately.

The interface into the trailers that is being tested by this code is
"the caller repeatedly calls the iterator, and the caller can
inspect the iterator's state available as its .raw, .key and .val
members and use them as it sees fit", so you could check, if you
wanted to, the following given the above sample data:

 * the first iteration finds no key/value pair (optionally, the
   contents found in the .raw member is as expected).
 * the second iteration finds no key/value pair (ditto).
 * the third iteration finds no key/value pair (ditto).
 * the fourth iteration finds key="Signed-off-by" value="x".
 * there is no fifth iteration.

but the current code only checks the last condition and nothing
else.  I dunno.

Linus Arver April 20, 2024, 12:14 a.m. UTC | #4

Junio C Hamano <[e-mail]> writes:

> "Linus Arver via GitGitGadget" <[e-mail]> writes:
>
>> +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
>
> Totally offtopic, but does it bother folks who are interested in
> adding more unit tests that they do not seem to interact very well
> with GIT_SKIP_TESTS environment variable?

FWIW I am not bothered (not that I've actually used GIT_SKIP_TESTS)
mainly because the unit tests finish so quickly.

>
>> diff --git a/t/unit-tests/t-trailer.c b/t/unit-tests/t-trailer.c
>> new file mode 100644
>> index 00000000000..147a51b66b9
>> --- /dev/null
>> +++ b/t/unit-tests/t-trailer.c
>> @@ -0,0 +1,175 @@
>> +#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++;
>> +	}
>
> Unnecessary {braces} around a single-statement block?

Gah, I blame writing too much Go. Will fix.

I also wonder if there's a C linter that could catch this... I am not
very familiar with C tooling. Would be great to run that in CI (GGG).

>> +	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;
>
> This is more like number of lines in the trailer block, not
> limiting its count only to true trailers, no?

Yes, but to be even more precise, it would be the number of trailer
objects in the trailer block (a single trailer could be folded over
multiple lines). Will update to "num_expected_objects".

>
>> +	} tc[] = {
>> ...
>> +		{
>> +			"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.
>> +			 */
>> +			"not a trailer line\n"
>> +			"not a trailer line\n"
>> +			"not a trailer line\n"
>> +			"Signed-off-by: x\n",
>> +			1
>> +		},
>
> It is OK to leave it num_expected_trailers in this step and then
> rename it when you update this "1" (number of real trailer lines)
> to "4" (number of lines in the trailer block).
>
> I wonder if you'd want to make more data available to the test.  At
> least it would be more useful if the number of true trailer lines
> and the number of lines in the trialer block are available
> separately.

I purposely did the simplest test possible in order to keep the patch
simple. Totally OK with expanding the data available to the test though,
if you'd prefer that (although that could also be in a separate series
later when we start converting some of the existing shell tests to these
unit tests).

> The interface into the trailers that is being tested by this code is
> "the caller repeatedly calls the iterator, and the caller can
> inspect the iterator's state available as its .raw, .key and .val
> members and use them as it sees fit", so you could check, if you
> wanted to, the following given the above sample data:
>
>  * the first iteration finds no key/value pair (optionally, the
>    contents found in the .raw member is as expected).
>  * the second iteration finds no key/value pair (ditto).
>  * the third iteration finds no key/value pair (ditto).
>  * the fourth iteration finds key="Signed-off-by" value="x".
>  * there is no fifth iteration.
>
> but the current code only checks the last condition and nothing
> else.  I dunno.

Yeah, this sounds like the natural thing to do. Basically have an exact
list of "this is the linked list of trailer objects I expect to see
after parsing is complete".

I do plan on making the trailer iterator struct private in a future
series though, so maybe it's best to do the above after that series (to
avoid churn)? IDK.

@Christian thoughts?

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..147a51b66b9
--- /dev/null
+++ b/t/unit-tests/t-trailer.c
@@ -0,0 +1,175 @@ 
+#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.
+			 */
+			"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 because the
+			 * 25% threshold only applies to cases where there was a
+			 * Git-generated trailer (see git_generated_prefixes[]
+			 * in trailer.c).
+			 */
+			"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();
+}