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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 14, 2024, 13:08 UTC
Message-ID: <ZrysVg04x_uIdNio@tanuki>
In-Reply-To: <2rxxfpzijfmvo65xournnmx4oawzqlhgipje4cxzxvo5aqzt6u@xppoikj262cp>

On Tue, Aug 13, 2024 at 03:33:04PM -0700, Josh Steadmon wrote:

Show 20 quoted lines

> On 2024.08.13 20:04, Chandra Pratap wrote:
> > 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.
> 
> I'm assuming that eventually, reftable/test_framework (and all the rest
> of reftable/libreftable_test.a) will be removed after all the tests are
> converted to the unit test framework, is that correct? Will other tests
> need these test_framework functions? If so, I'd rather not end up with
> duplicates in each test file, even if these are small functions. Is
> there a reason why we can't link the reftable/test_framework object (or
> the whole reftable/libreftable_test.a library)?

The reason is likely that they use different infra, e.g. `EXPECT()` vs `check()`. So instead of linking `libreftable_test.a`, I think it is fine to duplicate the functionality in `t/unit-tests`. In not too distant of a future we're going to get rid of everything in the reftable tests anyway, including the `libreftable_test.a` library. So avoiding the duplication doesn't make a ton of sense to me.

That being said, I think we should not duplicate functionality in `t/unit-tests`. So if there is functionality used by multiple tests, we should likely move it into a new `t/unit-tests/lib-reftable.c` file.

Patrick