Re: [PATCH v2 2/8] trailer: add unit tests for trailer iterator
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 19, 2024, 21:52 UTC
- Message-ID
- <xmqq5xwd58b9.fsf@gitster.g>
- In-Reply-To
- <e1fa05143ac63e8fe8dbc8ccb76a89b7a008c412.1713504153.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> 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?
Show 18 quoted lines
> 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?Show 11 quoted lines
> + 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?
Show 17 quoted lines
> + } 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.