On Wed, 26 Jun 2024 at 17:22, Karthik Nayak [off-list ref] wrote:

Chandra Pratap [off-list ref] writes:

quoted

On Tue, 25 Jun 2024 at 13:56, Karthik Nayak [off-list ref] wrote:

quoted

Chandra Pratap [off-list ref] writes:

quoted

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 [off-list ref]
Mentored-by: Christian Couder [off-list ref]
Signed-off-by: Chandra Pratap <redacted>
---
 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.

quoted

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.

quoted

quoted

quoted

      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.

While that is true, it makes me wonder the reason behind adding it
in the original test. Maybe we're supposed to manually verify the output?

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

The thing is, all the 'print' functions defined in reftable/ directory are
meant to be used for debugging. So while they're not used anywhere
in production, they still serve an important purpose during development.