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

>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > 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.
>
> >> >       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.