Re: [PATCH v2 1/4] t: move reftable/readwrite_test.c to the unit testing framework

From: Chandra Pratap <chandrapratap3519@gmail.com>
Date: Aug 12, 2024, 14:50 UTC
Message-ID: <CA+J6zkRTRQ9o=CDgsFbJx5csjDxLfQC_E+dw+Csz3hp=_c8Ueg@mail.gmail.com>
In-Reply-To: <xmqqwmkpd0qs.fsf@gitster.g>

On Fri, 9 Aug 2024 at 23:42, Junio C Hamano <gitster@pobox.com> wrote:

Show 26 quoted lines

>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > reftable/readwrite_test.c exercises the functions defined in
> > reftable/reader.{c,h} and reftable/writer.{c,h}. Migrate
> > reftable/readwrite_test.c to the unit testing framework. Migration
> > involves refactoring the tests to use the unit testing framework
> > instead of reftable's test framework and renaming the tests to
> > align with unit-tests' naming conventions.
> >
> > Since some tests in reftable/readwrite_test.c use the functions
> > set_test_hash(), noop_flush() and strbuf_add_void() defined in
> > reftable/test_framework.{c,h} but these files are not #included
> > in the ported unit test, copy these functions in the new test file.
> >
> > While at it, ensure structs are 0-initialized with '= { 0 }'
> > instead of '= { NULL }'.
>
> OK.
>
> > -             EXPECT(buf->buf[off] == 'r');
> > +             if (!off)
> > +                     off = header_size((hash_id == GIT_SHA256_FORMAT_ID) ? 2 : 1);
> > +             check(buf->buf[off] == 'r');
>
> Why not "check_char(buf->buf[off], ==, 'r')"?

I wrote this series quite some time ago when this functionality was not yet introduced to the unit testing framework. I'll commit this change in the next reroll.

Show 7 quoted lines

> >       }
> >
> > -     EXPECT(stats->log_stats.blocks > 0);
> > +     check(stats->log_stats.blocks > 0);
>
> Why not "check_int(stats->log_stats.blocks, >, 0)", which you used
> in the t_log_write_read() function?

Looks like a case of too-mechanical-a-translation to me. I'll fix this in the next version.

> While reading this step, I looked for use of check() that is not
> rewriting EXPECT_ERR(x) to check(!x) as suspicious.  The above two
> (and a !memcmp() that is OK) were the only three such uses of
> check(), I think.

I went through this series again and I agree on not encountering any other subpar translations of EXPECT() to its counterparts in the unit testing framework. I'll reroll the series with only these changes until someone else finds any other corrections.