| Message ID | 20240621115708.3626-2-chandrapratap3519@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] t: move reftable/record_test.c to the unit testing framework | expand |
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(©, 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. > reftable_record_copy_from(©, rec, GIT_SHA1_RAWSZ); > - EXPECT(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); > + check(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); > > - puts("testing print coverage:\n"); > + test_msg("testing print coverage:"); > reftable_record_print(©, 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. > reftable_record_release(©); > @@ -43,8 +39,8 @@ static void test_varint_roundtrip(void) > 4096, > ((uint64_t)1 << 63), > ((uint64_t)1 << 63) + ((uint64_t)1 << 63) - 1 }; > - int i = 0; > - for (i = 0; i < ARRAY_SIZE(inputs); i++) { > + > + for (size_t i = 0; i < ARRAY_SIZE(inputs); i++) { > uint8_t dest[10]; > > struct string_view out = { > @@ -55,29 +51,26 @@ static void test_varint_roundtrip(void) > int n = put_var_int(&out, in); > uint64_t got = 0; > > - EXPECT(n > 0); > + check_int(n, >, 0); > out.len = n; > n = get_var_int(&got, &out); > - EXPECT(n > 0); > + check_int(n, >, 0); > > - EXPECT(got == in); > + check_int(got, ==, in); > } > } > > static void set_hash(uint8_t *h, int j) > { > - int i = 0; > - for (i = 0; i < hash_size(GIT_SHA1_FORMAT_ID); i++) { > + for (int i = 0; i < hash_size(GIT_SHA1_FORMAT_ID); i++) > h[i] = (j >> i) & 0xff; > - } > } > > static void test_reftable_ref_record_roundtrip(void) > { > struct strbuf scratch = STRBUF_INIT; > - int i = 0; > > - for (i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) { > + for (int i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) { > struct reftable_record in = { > .type = BLOCK_TYPE_REF, > }; > @@ -109,17 +102,17 @@ static void test_reftable_ref_record_roundtrip(void) > > test_copy(&in); > > - EXPECT(reftable_record_val_type(&in) == i); > + check_int(reftable_record_val_type(&in), ==, i); > > reftable_record_key(&in, &key); > n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ); > - EXPECT(n > 0); > + check_int(n, >, 0); > > /* decode into a non-zero reftable_record to test for leaks. */ > m = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ, &scratch); > - EXPECT(n == m); > + check_int(n, ==, m); > > - EXPECT(reftable_ref_record_equal(&in.u.ref, &out.u.ref, > + check(reftable_ref_record_equal(&in.u.ref, &out.u.ref, > GIT_SHA1_RAWSZ)); > reftable_record_release(&in); > > @@ -143,16 +136,15 @@ static void test_reftable_log_record_equal(void) > } > }; > > - EXPECT(!reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); > + check(!reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); > in[1].update_index = in[0].update_index; > - EXPECT(reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); > + check(reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); > reftable_log_record_release(&in[0]); > reftable_log_record_release(&in[1]); > } > > static void test_reftable_log_record_roundtrip(void) > { > - int i; > struct reftable_log_record in[] = { > { > .refname = xstrdup("refs/heads/master"), > @@ -180,12 +172,12 @@ static void test_reftable_log_record_roundtrip(void) > } > }; > struct strbuf scratch = STRBUF_INIT; > + set_hash(in[0].value.update.new_hash, 1); > + set_hash(in[0].value.update.old_hash, 2); > + set_hash(in[2].value.update.new_hash, 3); > + set_hash(in[2].value.update.old_hash, 4); > > - set_test_hash(in[0].value.update.new_hash, 1); > - set_test_hash(in[0].value.update.old_hash, 2); > - set_test_hash(in[2].value.update.new_hash, 3); > - set_test_hash(in[2].value.update.old_hash, 4); > - for (i = 0; i < ARRAY_SIZE(in); i++) { > + for (size_t i = 0; i < ARRAY_SIZE(in); i++) { > struct reftable_record rec = { .type = BLOCK_TYPE_LOG }; > struct strbuf key = STRBUF_INIT; > uint8_t buffer[1024] = { 0 }; > @@ -217,13 +209,13 @@ static void test_reftable_log_record_roundtrip(void) > reftable_record_key(&rec, &key); > > n = reftable_record_encode(&rec, dest, GIT_SHA1_RAWSZ); > - EXPECT(n >= 0); > + check_int(n, >=, 0); > valtype = reftable_record_val_type(&rec); > m = reftable_record_decode(&out, key, valtype, dest, > GIT_SHA1_RAWSZ, &scratch); > - EXPECT(n == m); > + check_int(n, ==, m); > > - EXPECT(reftable_log_record_equal(&in[i], &out.u.log, > + check(reftable_log_record_equal(&in[i], &out.u.log, > GIT_SHA1_RAWSZ)); > reftable_log_record_release(&in[i]); > strbuf_release(&key); > @@ -252,14 +244,14 @@ static void test_key_roundtrip(void) > strbuf_addstr(&key, "refs/tags/bla"); > extra = 6; > n = reftable_encode_key(&restart, dest, last_key, key, extra); > - EXPECT(!restart); > - EXPECT(n > 0); > + check(!restart); > + check_int(n, >, 0); > > strbuf_addstr(&roundtrip, "refs/heads/master"); > m = reftable_decode_key(&roundtrip, &rt_extra, dest); > - EXPECT(n == m); > - EXPECT(0 == strbuf_cmp(&key, &roundtrip)); > - EXPECT(rt_extra == extra); > + check_int(n, ==, m); > + check(!strbuf_cmp(&key, &roundtrip)); > + check_int(rt_extra, ==, extra); > > strbuf_release(&last_key); > strbuf_release(&key); > @@ -289,9 +281,8 @@ static void test_reftable_obj_record_roundtrip(void) > }, > }; > struct strbuf scratch = STRBUF_INIT; > - int i = 0; > > - for (i = 0; i < ARRAY_SIZE(recs); i++) { > + for (size_t i = 0; i < ARRAY_SIZE(recs); i++) { > uint8_t buffer[1024] = { 0 }; > struct string_view dest = { > .buf = buffer, > @@ -311,13 +302,13 @@ static void test_reftable_obj_record_roundtrip(void) > test_copy(&in); > reftable_record_key(&in, &key); > n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ); > - EXPECT(n > 0); > + check_int(n, >, 0); > extra = reftable_record_val_type(&in); > m = reftable_record_decode(&out, key, extra, dest, > GIT_SHA1_RAWSZ, &scratch); > - EXPECT(n == m); > + check_int(n, ==, m); > > - EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); > + check(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); > strbuf_release(&key); > reftable_record_release(&out); > } > @@ -352,16 +343,16 @@ static void test_reftable_index_record_roundtrip(void) > reftable_record_key(&in, &key); > test_copy(&in); > > - EXPECT(0 == strbuf_cmp(&key, &in.u.idx.last_key)); > + check(!strbuf_cmp(&key, &in.u.idx.last_key)); > n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ); > - EXPECT(n > 0); > + check_int(n, >, 0); > > extra = reftable_record_val_type(&in); > m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ, > &scratch); > - EXPECT(m == n); > + check_int(m, ==, n); > > - EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); > + check(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); > > reftable_record_release(&out); > strbuf_release(&key); > @@ -369,14 +360,15 @@ static void test_reftable_index_record_roundtrip(void) > strbuf_release(&in.u.idx.last_key); > } > > -int record_test_main(int argc, const char *argv[]) > +int cmd_main(int argc, const char *argv[]) > { > - RUN_TEST(test_reftable_log_record_equal); > - RUN_TEST(test_reftable_log_record_roundtrip); > - RUN_TEST(test_reftable_ref_record_roundtrip); > - RUN_TEST(test_varint_roundtrip); > - RUN_TEST(test_key_roundtrip); > - RUN_TEST(test_reftable_obj_record_roundtrip); > - RUN_TEST(test_reftable_index_record_roundtrip); > - return 0; > + TEST(test_reftable_log_record_equal(), "reftable_log_record_equal works"); > + TEST(test_reftable_log_record_roundtrip(), "record operations work on log record"); > + TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record"); > + TEST(test_varint_roundtrip(), "put_var_int and get_var_int work"); > + TEST(test_key_roundtrip(), "reftable_encode_key and reftable_decode_key work"); > + TEST(test_reftable_obj_record_roundtrip(), "record operations work on obj record"); > + TEST(test_reftable_index_record_roundtrip(), "record operations work on index record"); > + All other tests in the 'unit-tests/' folder use a `t_<name>` format for the tests. Here we seem to diverge and use a `test_<name>` format. I think the best outcome would be some documentation around this, but it would still be nice if we follow this pattern nevertheless. > + return test_done(); > } > -- > 2.45.2.404.g9eaef5822c
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(©, 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 Should we get rid of this after all? > > reftable_record_copy_from(©, rec, GIT_SHA1_RAWSZ); > > - EXPECT(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); > > + check(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); > > > > - puts("testing print coverage:\n"); > > + test_msg("testing print coverage:"); > > reftable_record_print(©, 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? > > reftable_record_release(©); > > @@ -43,8 +39,8 @@ static void test_varint_roundtrip(void) > > 4096, > > ((uint64_t)1 << 63), > > ((uint64_t)1 << 63) + ((uint64_t)1 << 63) - 1 }; > > - int i = 0; > > - for (i = 0; i < ARRAY_SIZE(inputs); i++) { > > + > > + for (size_t i = 0; i < ARRAY_SIZE(inputs); i++) { > > uint8_t dest[10]; > > > > struct string_view out = { > > @@ -55,29 +51,26 @@ static void test_varint_roundtrip(void) > > int n = put_var_int(&out, in); > > uint64_t got = 0; > > > > - EXPECT(n > 0); > > + check_int(n, >, 0); > > out.len = n; > > n = get_var_int(&got, &out); > > - EXPECT(n > 0); > > + check_int(n, >, 0); > > > > - EXPECT(got == in); > > + check_int(got, ==, in); > > } > > } > > > > static void set_hash(uint8_t *h, int j) > > { > > - int i = 0; > > - for (i = 0; i < hash_size(GIT_SHA1_FORMAT_ID); i++) { > > + for (int i = 0; i < hash_size(GIT_SHA1_FORMAT_ID); i++) > > h[i] = (j >> i) & 0xff; > > - } > > } > > > > static void test_reftable_ref_record_roundtrip(void) > > { > > struct strbuf scratch = STRBUF_INIT; > > - int i = 0; > > > > - for (i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) { > > + for (int i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) { > > struct reftable_record in = { > > .type = BLOCK_TYPE_REF, > > }; > > @@ -109,17 +102,17 @@ static void test_reftable_ref_record_roundtrip(void) > > > > test_copy(&in); > > > > - EXPECT(reftable_record_val_type(&in) == i); > > + check_int(reftable_record_val_type(&in), ==, i); > > > > reftable_record_key(&in, &key); > > n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ); > > - EXPECT(n > 0); > > + check_int(n, >, 0); > > > > /* decode into a non-zero reftable_record to test for leaks. */ > > m = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ, &scratch); > > - EXPECT(n == m); > > + check_int(n, ==, m); > > > > - EXPECT(reftable_ref_record_equal(&in.u.ref, &out.u.ref, > > + check(reftable_ref_record_equal(&in.u.ref, &out.u.ref, > > GIT_SHA1_RAWSZ)); > > reftable_record_release(&in); > > > > @@ -143,16 +136,15 @@ static void test_reftable_log_record_equal(void) > > } > > }; > > > > - EXPECT(!reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); > > + check(!reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); > > in[1].update_index = in[0].update_index; > > - EXPECT(reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); > > + check(reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); > > reftable_log_record_release(&in[0]); > > reftable_log_record_release(&in[1]); > > } > > > > static void test_reftable_log_record_roundtrip(void) > > { > > - int i; > > struct reftable_log_record in[] = { > > { > > .refname = xstrdup("refs/heads/master"), > > @@ -180,12 +172,12 @@ static void test_reftable_log_record_roundtrip(void) > > } > > }; > > struct strbuf scratch = STRBUF_INIT; > > + set_hash(in[0].value.update.new_hash, 1); > > + set_hash(in[0].value.update.old_hash, 2); > > + set_hash(in[2].value.update.new_hash, 3); > > + set_hash(in[2].value.update.old_hash, 4); > > > > - set_test_hash(in[0].value.update.new_hash, 1); > > - set_test_hash(in[0].value.update.old_hash, 2); > > - set_test_hash(in[2].value.update.new_hash, 3); > > - set_test_hash(in[2].value.update.old_hash, 4); > > - for (i = 0; i < ARRAY_SIZE(in); i++) { > > + for (size_t i = 0; i < ARRAY_SIZE(in); i++) { > > struct reftable_record rec = { .type = BLOCK_TYPE_LOG }; > > struct strbuf key = STRBUF_INIT; > > uint8_t buffer[1024] = { 0 }; > > @@ -217,13 +209,13 @@ static void test_reftable_log_record_roundtrip(void) > > reftable_record_key(&rec, &key); > > > > n = reftable_record_encode(&rec, dest, GIT_SHA1_RAWSZ); > > - EXPECT(n >= 0); > > + check_int(n, >=, 0); > > valtype = reftable_record_val_type(&rec); > > m = reftable_record_decode(&out, key, valtype, dest, > > GIT_SHA1_RAWSZ, &scratch); > > - EXPECT(n == m); > > + check_int(n, ==, m); > > > > - EXPECT(reftable_log_record_equal(&in[i], &out.u.log, > > + check(reftable_log_record_equal(&in[i], &out.u.log, > > GIT_SHA1_RAWSZ)); > > reftable_log_record_release(&in[i]); > > strbuf_release(&key); > > @@ -252,14 +244,14 @@ static void test_key_roundtrip(void) > > strbuf_addstr(&key, "refs/tags/bla"); > > extra = 6; > > n = reftable_encode_key(&restart, dest, last_key, key, extra); > > - EXPECT(!restart); > > - EXPECT(n > 0); > > + check(!restart); > > + check_int(n, >, 0); > > > > strbuf_addstr(&roundtrip, "refs/heads/master"); > > m = reftable_decode_key(&roundtrip, &rt_extra, dest); > > - EXPECT(n == m); > > - EXPECT(0 == strbuf_cmp(&key, &roundtrip)); > > - EXPECT(rt_extra == extra); > > + check_int(n, ==, m); > > + check(!strbuf_cmp(&key, &roundtrip)); > > + check_int(rt_extra, ==, extra); > > > > strbuf_release(&last_key); > > strbuf_release(&key); > > @@ -289,9 +281,8 @@ static void test_reftable_obj_record_roundtrip(void) > > }, > > }; > > struct strbuf scratch = STRBUF_INIT; > > - int i = 0; > > > > - for (i = 0; i < ARRAY_SIZE(recs); i++) { > > + for (size_t i = 0; i < ARRAY_SIZE(recs); i++) { > > uint8_t buffer[1024] = { 0 }; > > struct string_view dest = { > > .buf = buffer, > > @@ -311,13 +302,13 @@ static void test_reftable_obj_record_roundtrip(void) > > test_copy(&in); > > reftable_record_key(&in, &key); > > n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ); > > - EXPECT(n > 0); > > + check_int(n, >, 0); > > extra = reftable_record_val_type(&in); > > m = reftable_record_decode(&out, key, extra, dest, > > GIT_SHA1_RAWSZ, &scratch); > > - EXPECT(n == m); > > + check_int(n, ==, m); > > > > - EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); > > + check(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); > > strbuf_release(&key); > > reftable_record_release(&out); > > } > > @@ -352,16 +343,16 @@ static void test_reftable_index_record_roundtrip(void) > > reftable_record_key(&in, &key); > > test_copy(&in); > > > > - EXPECT(0 == strbuf_cmp(&key, &in.u.idx.last_key)); > > + check(!strbuf_cmp(&key, &in.u.idx.last_key)); > > n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ); > > - EXPECT(n > 0); > > + check_int(n, >, 0); > > > > extra = reftable_record_val_type(&in); > > m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ, > > &scratch); > > - EXPECT(m == n); > > + check_int(m, ==, n); > > > > - EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); > > + check(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); > > > > reftable_record_release(&out); > > strbuf_release(&key); > > @@ -369,14 +360,15 @@ static void test_reftable_index_record_roundtrip(void) > > strbuf_release(&in.u.idx.last_key); > > } > > > > -int record_test_main(int argc, const char *argv[]) > > +int cmd_main(int argc, const char *argv[]) > > { > > - RUN_TEST(test_reftable_log_record_equal); > > - RUN_TEST(test_reftable_log_record_roundtrip); > > - RUN_TEST(test_reftable_ref_record_roundtrip); > > - RUN_TEST(test_varint_roundtrip); > > - RUN_TEST(test_key_roundtrip); > > - RUN_TEST(test_reftable_obj_record_roundtrip); > > - RUN_TEST(test_reftable_index_record_roundtrip); > > - return 0; > > + TEST(test_reftable_log_record_equal(), "reftable_log_record_equal works"); > > + TEST(test_reftable_log_record_roundtrip(), "record operations work on log record"); > > + TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record"); > > + TEST(test_varint_roundtrip(), "put_var_int and get_var_int work"); > > + TEST(test_key_roundtrip(), "reftable_encode_key and reftable_decode_key work"); > > + TEST(test_reftable_obj_record_roundtrip(), "record operations work on obj record"); > > + TEST(test_reftable_index_record_roundtrip(), "record operations work on index record"); > > + > > All other tests in the 'unit-tests/' folder use a `t_<name>` format for > the tests. Here we seem to diverge and use a `test_<name>` format. I > think the best outcome would be some documentation around this, but it > would still be nice if we follow this pattern nevertheless. > > > + return test_done(); > > } > > -- > > 2.45.2.404.g9eaef5822c
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(©, 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(©, rec, GIT_SHA1_RAWSZ); >> > - EXPECT(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); >> > + check(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); >> > >> > - puts("testing print coverage:\n"); >> > + test_msg("testing print coverage:"); >> > reftable_record_print(©, 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]
On Wed, 26 Jun 2024 at 17:22, Karthik Nayak <karthik.188@gmail.com> wrote: > > 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(©, 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(©, rec, GIT_SHA1_RAWSZ); > >> > - EXPECT(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); > >> > + check(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); > >> > > >> > - puts("testing print coverage:\n"); > >> > + test_msg("testing print coverage:"); > >> > reftable_record_print(©, 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.
On Wed, Jun 26, 2024 at 1:52 PM Karthik Nayak <karthik.188@gmail.com> wrote: > But to summarize, seems like we're not sure why this was added. CC'ing > Han-Wen here incase he remembers the intent. The copy functions are like C++ copy constructors. They discard the old data, and copy over the new data into the destination. If we forget to free() something in the "discard old data" step, that would be exercised by initializing the record to hold data (1st copy call) and then triggering the cleanup (2nd copy call.). Valgrind or similar tooling would then detect the leak.
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(©, rec, GIT_SHA1_RAWSZ); /* do it twice to catch memory leaks */ reftable_record_copy_from(©, rec, GIT_SHA1_RAWSZ); - EXPECT(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); + check(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); - puts("testing print coverage:\n"); + test_msg("testing print coverage:"); reftable_record_print(©, GIT_SHA1_RAWSZ); reftable_record_release(©); @@ -43,8 +39,8 @@ static void test_varint_roundtrip(void) 4096, ((uint64_t)1 << 63), ((uint64_t)1 << 63) + ((uint64_t)1 << 63) - 1 }; - int i = 0; - for (i = 0; i < ARRAY_SIZE(inputs); i++) { + + for (size_t i = 0; i < ARRAY_SIZE(inputs); i++) { uint8_t dest[10]; struct string_view out = { @@ -55,29 +51,26 @@ static void test_varint_roundtrip(void) int n = put_var_int(&out, in); uint64_t got = 0; - EXPECT(n > 0); + check_int(n, >, 0); out.len = n; n = get_var_int(&got, &out); - EXPECT(n > 0); + check_int(n, >, 0); - EXPECT(got == in); + check_int(got, ==, in); } } static void set_hash(uint8_t *h, int j) { - int i = 0; - for (i = 0; i < hash_size(GIT_SHA1_FORMAT_ID); i++) { + for (int i = 0; i < hash_size(GIT_SHA1_FORMAT_ID); i++) h[i] = (j >> i) & 0xff; - } } static void test_reftable_ref_record_roundtrip(void) { struct strbuf scratch = STRBUF_INIT; - int i = 0; - for (i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) { + for (int i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) { struct reftable_record in = { .type = BLOCK_TYPE_REF, }; @@ -109,17 +102,17 @@ static void test_reftable_ref_record_roundtrip(void) test_copy(&in); - EXPECT(reftable_record_val_type(&in) == i); + check_int(reftable_record_val_type(&in), ==, i); reftable_record_key(&in, &key); n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ); - EXPECT(n > 0); + check_int(n, >, 0); /* decode into a non-zero reftable_record to test for leaks. */ m = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ, &scratch); - EXPECT(n == m); + check_int(n, ==, m); - EXPECT(reftable_ref_record_equal(&in.u.ref, &out.u.ref, + check(reftable_ref_record_equal(&in.u.ref, &out.u.ref, GIT_SHA1_RAWSZ)); reftable_record_release(&in); @@ -143,16 +136,15 @@ static void test_reftable_log_record_equal(void) } }; - EXPECT(!reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); + check(!reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); in[1].update_index = in[0].update_index; - EXPECT(reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); + check(reftable_log_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); reftable_log_record_release(&in[0]); reftable_log_record_release(&in[1]); } static void test_reftable_log_record_roundtrip(void) { - int i; struct reftable_log_record in[] = { { .refname = xstrdup("refs/heads/master"), @@ -180,12 +172,12 @@ static void test_reftable_log_record_roundtrip(void) } }; struct strbuf scratch = STRBUF_INIT; + set_hash(in[0].value.update.new_hash, 1); + set_hash(in[0].value.update.old_hash, 2); + set_hash(in[2].value.update.new_hash, 3); + set_hash(in[2].value.update.old_hash, 4); - set_test_hash(in[0].value.update.new_hash, 1); - set_test_hash(in[0].value.update.old_hash, 2); - set_test_hash(in[2].value.update.new_hash, 3); - set_test_hash(in[2].value.update.old_hash, 4); - for (i = 0; i < ARRAY_SIZE(in); i++) { + for (size_t i = 0; i < ARRAY_SIZE(in); i++) { struct reftable_record rec = { .type = BLOCK_TYPE_LOG }; struct strbuf key = STRBUF_INIT; uint8_t buffer[1024] = { 0 }; @@ -217,13 +209,13 @@ static void test_reftable_log_record_roundtrip(void) reftable_record_key(&rec, &key); n = reftable_record_encode(&rec, dest, GIT_SHA1_RAWSZ); - EXPECT(n >= 0); + check_int(n, >=, 0); valtype = reftable_record_val_type(&rec); m = reftable_record_decode(&out, key, valtype, dest, GIT_SHA1_RAWSZ, &scratch); - EXPECT(n == m); + check_int(n, ==, m); - EXPECT(reftable_log_record_equal(&in[i], &out.u.log, + check(reftable_log_record_equal(&in[i], &out.u.log, GIT_SHA1_RAWSZ)); reftable_log_record_release(&in[i]); strbuf_release(&key); @@ -252,14 +244,14 @@ static void test_key_roundtrip(void) strbuf_addstr(&key, "refs/tags/bla"); extra = 6; n = reftable_encode_key(&restart, dest, last_key, key, extra); - EXPECT(!restart); - EXPECT(n > 0); + check(!restart); + check_int(n, >, 0); strbuf_addstr(&roundtrip, "refs/heads/master"); m = reftable_decode_key(&roundtrip, &rt_extra, dest); - EXPECT(n == m); - EXPECT(0 == strbuf_cmp(&key, &roundtrip)); - EXPECT(rt_extra == extra); + check_int(n, ==, m); + check(!strbuf_cmp(&key, &roundtrip)); + check_int(rt_extra, ==, extra); strbuf_release(&last_key); strbuf_release(&key); @@ -289,9 +281,8 @@ static void test_reftable_obj_record_roundtrip(void) }, }; struct strbuf scratch = STRBUF_INIT; - int i = 0; - for (i = 0; i < ARRAY_SIZE(recs); i++) { + for (size_t i = 0; i < ARRAY_SIZE(recs); i++) { uint8_t buffer[1024] = { 0 }; struct string_view dest = { .buf = buffer, @@ -311,13 +302,13 @@ static void test_reftable_obj_record_roundtrip(void) test_copy(&in); reftable_record_key(&in, &key); n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ); - EXPECT(n > 0); + check_int(n, >, 0); extra = reftable_record_val_type(&in); m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ, &scratch); - EXPECT(n == m); + check_int(n, ==, m); - EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); + check(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); strbuf_release(&key); reftable_record_release(&out); } @@ -352,16 +343,16 @@ static void test_reftable_index_record_roundtrip(void) reftable_record_key(&in, &key); test_copy(&in); - EXPECT(0 == strbuf_cmp(&key, &in.u.idx.last_key)); + check(!strbuf_cmp(&key, &in.u.idx.last_key)); n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ); - EXPECT(n > 0); + check_int(n, >, 0); extra = reftable_record_val_type(&in); m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ, &scratch); - EXPECT(m == n); + check_int(m, ==, n); - EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); + check(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ)); reftable_record_release(&out); strbuf_release(&key); @@ -369,14 +360,15 @@ static void test_reftable_index_record_roundtrip(void) strbuf_release(&in.u.idx.last_key); } -int record_test_main(int argc, const char *argv[]) +int cmd_main(int argc, const char *argv[]) { - RUN_TEST(test_reftable_log_record_equal); - RUN_TEST(test_reftable_log_record_roundtrip); - RUN_TEST(test_reftable_ref_record_roundtrip); - RUN_TEST(test_varint_roundtrip); - RUN_TEST(test_key_roundtrip); - RUN_TEST(test_reftable_obj_record_roundtrip); - RUN_TEST(test_reftable_index_record_roundtrip); - return 0; + TEST(test_reftable_log_record_equal(), "reftable_log_record_equal works"); + TEST(test_reftable_log_record_roundtrip(), "record operations work on log record"); + TEST(test_reftable_ref_record_roundtrip(), "record operations work on ref record"); + TEST(test_varint_roundtrip(), "put_var_int and get_var_int work"); + TEST(test_key_roundtrip(), "reftable_encode_key and reftable_decode_key work"); + TEST(test_reftable_obj_record_roundtrip(), "record operations work on obj record"); + TEST(test_reftable_index_record_roundtrip(), "record operations work on index record"); + + return test_done(); }
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%)