Re: [PATCH 01/11] t: move reftable/record_test.c to the unit testing framework

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jun 26, 2024, 11:52 UTC
Message-ID: <CAOLa=ZQG4S6oJ_YTvc9LjV9C+THKcr_4xMsrOB2Mw6CZYfK9GA@mail.gmail.com>
In-Reply-To: <CA+J6zkSymKxSRnAVx7oeAoPpMxrRsVRPGO-X0tVbc0VtKOe3ig@mail.gmail.com>

Chandra Pratap <chandrapratap3519@gmail.com> writes:

Show 97 quoted lines

> On Tue, 25 Jun 2024 at 13:56, Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>>
>> > reftable/record_test.c exercises the functions defined in
>> > reftable/record.{c, h}. Migrate reftable/record_test.c to the
>> > unit testing framework. Migration involves refactoring the tests
>> > to use the unit testing framework instead of reftable's test
>> > framework.
>> > While at it, change the type of index variable 'i' to 'size_t'
>> > from 'int'. This is because 'i' is used in comparison against
>> > 'ARRAY_SIZE(x)' which is of type 'size_t'.
>> >
>> > Also, use set_hash() which is defined locally in the test file
>> > instead of set_test_hash() which is defined by
>> > reftable/test_framework.{c, h}. This is fine to do as both these
>> > functions are similarly implemented, and
>> > reftable/test_framework.{c, h} is not #included in the ported test.
>> >
>> > Mentored-by: Patrick Steinhardt <ps@pks.im>
>> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>> > ---
>> >  Makefile                                      |   2 +-
>> >  t/helper/test-reftable.c                      |   1 -
>> >  .../unit-tests/t-reftable-record.c            | 106 ++++++++----------
>> >  3 files changed, 50 insertions(+), 59 deletions(-)
>> >  rename reftable/record_test.c => t/unit-tests/t-reftable-record.c (77%)
>> >
>> > diff --git a/Makefile b/Makefile
>> > index f25b2e80a1..def3700b4d 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -1338,6 +1338,7 @@ UNIT_TEST_PROGRAMS += t-hash
>> >  UNIT_TEST_PROGRAMS += t-mem-pool
>> >  UNIT_TEST_PROGRAMS += t-prio-queue
>> >  UNIT_TEST_PROGRAMS += t-reftable-basics
>> > +UNIT_TEST_PROGRAMS += t-reftable-record
>> >  UNIT_TEST_PROGRAMS += t-strbuf
>> >  UNIT_TEST_PROGRAMS += t-strcmp-offset
>> >  UNIT_TEST_PROGRAMS += t-strvec
>> > @@ -2678,7 +2679,6 @@ REFTABLE_TEST_OBJS += reftable/block_test.o
>> >  REFTABLE_TEST_OBJS += reftable/dump.o
>> >  REFTABLE_TEST_OBJS += reftable/merged_test.o
>> >  REFTABLE_TEST_OBJS += reftable/pq_test.o
>> > -REFTABLE_TEST_OBJS += reftable/record_test.o
>> >  REFTABLE_TEST_OBJS += reftable/readwrite_test.o
>> >  REFTABLE_TEST_OBJS += reftable/stack_test.o
>> >  REFTABLE_TEST_OBJS += reftable/test_framework.o
>> > diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c
>> > index 9160bc5da6..aa6538a8da 100644
>> > --- a/t/helper/test-reftable.c
>> > +++ b/t/helper/test-reftable.c
>> > @@ -5,7 +5,6 @@
>> >  int cmd__reftable(int argc, const char **argv)
>> >  {
>> >       /* test from simple to complex. */
>> > -     record_test_main(argc, argv);
>> >       block_test_main(argc, argv);
>> >       tree_test_main(argc, argv);
>> >       pq_test_main(argc, argv);
>> > diff --git a/reftable/record_test.c b/t/unit-tests/t-reftable-record.c
>> > similarity index 77%
>> > rename from reftable/record_test.c
>> > rename to t/unit-tests/t-reftable-record.c
>> > index 58290bdba3..1b357e6c7f 100644
>> > --- a/reftable/record_test.c
>> > +++ b/t/unit-tests/t-reftable-record.c
>> > @@ -6,13 +6,9 @@
>> >    https://developers.google.com/open-source/licenses/bsd
>> >  */
>> >
>> > -#include "record.h"
>> > -
>> > -#include "system.h"
>> > -#include "basics.h"
>> > -#include "constants.h"
>> > -#include "test_framework.h"
>> > -#include "reftable-tests.h"
>> > +#include "test-lib.h"
>> > +#include "reftable/constants.h"
>> > +#include "reftable/record.h"
>> >
>> >  static void test_copy(struct reftable_record *rec)
>> >  {
>> > @@ -24,9 +20,9 @@ static void test_copy(struct reftable_record *rec)
>> >       reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
>> >       /* do it twice to catch memory leaks */
>>
>> I'm curious why we do this, and if it is still needed. The original
>> commit (e303bf22f reftable: (de)serialization for the polymorphic record
>> type) doesn't mention any particular reasoning.
>
> Yeah, I was confused about this as well. I asked Patrick about it some time
> ago and it seems like he had no clue about it either:
> https://gitlab.slack.com/archives/C071PDKNCHM/p1717479205788209
>

Just to note, this is an internal GitLab link and not accessible to others on the list.

But to summarize, seems like we're not sure why this was added. CC'ing Han-Wen here incase he remembers the intent.

> Should we get rid of this after all?

The best solution would be to understand its reasoning and incorporate that, but otherwise its best to remove it. We do have CI pipelines to capture leaks in a general sense.

Show 19 quoted lines

>> >       reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
>> > -     EXPECT(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
>> > +     check(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
>> >
>> > -     puts("testing print coverage:\n");
>> > +     test_msg("testing print coverage:");
>> >       reftable_record_print(&copy, GIT_SHA1_RAWSZ);
>> >
>>
>> This prints for any test that uses this function. As I see from the
>> current usage of the testing library, we only print debug information
>> when we encounter something unexpected.
>>
>> This also clogs up the unit-test's output. So I would remove this from
>> here.
>
> That's true, but that would also mean the print functions are no longer
> exercised. Is that a fine tradeoff?
>

I don't see it this way. Just exercising the function doesn't test it in any way. Since the function just prints to stdout without an option to pick any other file descriptor, there is no way to test it currently either.

On the contrary if no one is using the function, perhaps we can even remove it.

[snip]