From: Linus Arver <linusa@google.com>
To: Junio C Hamano <gitster@pobox.com>,
Linus Arver via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Christian Couder <chriscool@tuxfamily.org>,
Emily Shaffer <nasamuffin@google.com>,
Josh Steadmon <steadmon@google.com>,
"Randall S. Becker" <rsbecker@nexbridge.com>,
Christian Couder <christian.couder@gmail.com>,
Kristoffer Haugsbakk <code@khaugsbakk.name>
Subject: Re: [PATCH v2 2/8] trailer: add unit tests for trailer iterator
Date: Fri, 19 Apr 2024 17:14:00 -0700 [thread overview]
Message-ID: <owly7cgs6gc7.fsf@fine.c.googlers.com> (raw)
In-Reply-To: <xmqq5xwd58b9.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "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?
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?
next prev parent reply other threads:[~2024-04-20 0:14 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-16 6:27 [PATCH 0/6] Make trailer_info struct private (plus sequencer cleanup) Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 1/6] trailer: teach iterator about non-trailer lines Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 2/6] sequencer: use the trailer iterator Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 3/6] interpret-trailers: access trailer_info with new helpers Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 4/6] trailer: make parse_trailers() return trailer_info pointer Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 5/6] trailer: make trailer_info struct private Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 6/6] trailer: retire trailer_info_get() from API Linus Arver via GitGitGadget
2024-03-16 17:06 ` [PATCH 0/6] Make trailer_info struct private (plus sequencer cleanup) Junio C Hamano
2024-03-26 22:00 ` Junio C Hamano
2024-04-19 5:36 ` Linus Arver
2024-04-19 5:22 ` [PATCH v2 0/8] " Linus Arver via GitGitGadget
2024-04-19 5:22 ` [PATCH v2 1/8] Makefile: sort UNIT_TEST_PROGRAMS Linus Arver via GitGitGadget
2024-04-19 5:22 ` [PATCH v2 2/8] trailer: add unit tests for trailer iterator Linus Arver via GitGitGadget
2024-04-19 5:33 ` Linus Arver
2024-04-19 18:46 ` Linus Arver
2024-04-19 21:52 ` Junio C Hamano
2024-04-20 0:14 ` Linus Arver [this message]
2024-04-19 5:22 ` [PATCH v2 3/8] trailer: teach iterator about non-trailer lines Linus Arver via GitGitGadget
2024-04-19 5:22 ` [PATCH v2 4/8] sequencer: use the trailer iterator Linus Arver via GitGitGadget
2024-04-23 21:19 ` Junio C Hamano
2024-04-19 5:22 ` [PATCH v2 5/8] interpret-trailers: access trailer_info with new helpers Linus Arver via GitGitGadget
2024-04-19 5:22 ` [PATCH v2 6/8] trailer: make parse_trailers() return trailer_info pointer Linus Arver via GitGitGadget
2024-04-23 23:17 ` Junio C Hamano
2024-04-19 5:22 ` [PATCH v2 7/8] trailer: make trailer_info struct private Linus Arver via GitGitGadget
2024-04-23 23:27 ` Junio C Hamano
2024-04-25 3:17 ` Linus Arver
2024-04-19 5:22 ` [PATCH v2 8/8] trailer: retire trailer_info_get() from API Linus Arver via GitGitGadget
2024-04-23 23:27 ` Junio C Hamano
2024-04-24 0:27 ` [PATCH v2 0/8] Make trailer_info struct private (plus sequencer cleanup) Junio C Hamano
2024-04-26 0:26 ` [PATCH v3 00/10] " Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 01/10] Makefile: sort UNIT_TEST_PROGRAMS Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 02/10] trailer: add unit tests for trailer iterator Linus Arver via GitGitGadget
2024-04-26 14:51 ` Christian Couder
2024-04-26 16:20 ` Junio C Hamano
2024-04-26 16:25 ` Linus Arver
2024-04-26 0:26 ` [PATCH v3 03/10] trailer: teach iterator about non-trailer lines Linus Arver via GitGitGadget
2024-04-27 12:50 ` Christian Couder
2024-04-30 4:42 ` Linus Arver
2024-04-30 4:55 ` Linus Arver
2024-04-26 0:26 ` [PATCH v3 04/10] sequencer: use the trailer iterator Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 05/10] interpret-trailers: access trailer_info with new helpers Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 06/10] trailer: make parse_trailers() return trailer_info pointer Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 07/10] trailer: make trailer_info struct private Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 08/10] trailer: retire trailer_info_get() from API Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 09/10] trailer: document parse_trailers() usage Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 10/10] trailer unit tests: inspect iterator contents Linus Arver via GitGitGadget
2024-04-27 12:51 ` [PATCH v3 00/10] Make trailer_info struct private (plus sequencer cleanup) Christian Couder
2024-05-02 4:54 ` [PATCH v4 " Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 01/10] Makefile: sort UNIT_TEST_PROGRAMS Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 02/10] trailer: add unit tests for trailer iterator Linus Arver via GitGitGadget
2024-05-02 16:54 ` Junio C Hamano
2024-05-02 4:54 ` [PATCH v4 03/10] trailer: teach iterator about non-trailer lines Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 04/10] sequencer: use the trailer iterator Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 05/10] interpret-trailers: access trailer_info with new helpers Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 06/10] trailer: make parse_trailers() return trailer_info pointer Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 07/10] trailer: make trailer_info struct private Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 08/10] trailer: retire trailer_info_get() from API Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 09/10] trailer: document parse_trailers() usage Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 10/10] trailer unit tests: inspect iterator contents Linus Arver via GitGitGadget
2024-05-02 17:15 ` [PATCH v4 00/10] Make trailer_info struct private (plus sequencer cleanup) Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=owly7cgs6gc7.fsf@fine.c.googlers.com \
--to=linusa@google.com \
--cc=chriscool@tuxfamily.org \
--cc=christian.couder@gmail.com \
--cc=code@khaugsbakk.name \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=nasamuffin@google.com \
--cc=rsbecker@nexbridge.com \
--cc=steadmon@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).