| Message ID | 20240628063625.4092-1-chandrapratap3519@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | t: port reftable/record_test.c to the unit testing framework | expand |
Chandra Pratap <chandrapratap3519@gmail.com> writes: > In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit > testing framework written entirely in C was introduced to the Git project > aimed at simplifying testing and reducing test run times. > Currently, tests for the reftable refs-backend are performed by a custom > testing framework defined by reftable/test_framework.{c, h}. Port > reftable/record_test.c to the unit testing framework and improve upon > the ported test. > > The first patch in the series moves the test to the unit testing framework, > and the rest of the patches improve upon 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> > > --- > Changes in v3: > - Remove reftable_record_print() from tests to reduce clutter in test output. > - Add more explanation to in-code comment in 1st patch. > - Use string literals instead of xstrdup() when possible to prevent the cost > of mallocing and freeing. > - make reftable_record_is_deletion() test for refs conciser in 6th patch. > - Update commit messages for 7th, 8th and 9th patch to better reflect changes. > - Add tests for '== 0' and '> 0' cases n 10th and 11th patch. > I've reviewed the series and left a few comments, I like how it is shaping up though. Thanks! > CI/PR: https://github.com/gitgitgadget/git/pull/1750 > > Chandra Pratap (11): > t: move reftable/record_test.c to the unit testing framework > t-reftable-record: add reftable_record_cmp() tests for log records > t-reftable-record: add comparison tests for ref records > t-reftable-record: add comparison tests for index records > t-reftable-record: add comparison tests for obj records > t-reftable-record: add reftable_record_is_deletion() test for ref records > t-reftable-record: add reftable_record_is_deletion() test for log records > t-reftable-record: add reftable_record_is_deletion() test for obj records > t-reftable-record: add reftable_record_is_deletion() test for index records > t-reftable-record: add tests for reftable_ref_record_compare_name() > t-reftable-record: add tests for reftable_log_record_compare_key() > > Makefile | 2 +- > reftable/record_test.c | 382 ------------------------- > t/helper/test-reftable.c | 1 - > t/unit-tests/t-reftable-record.c | 554 +++++++++++++++++++++++++++++++++++++++++++++ > > Range-diff against v2: > 1: d0646af549 ! 1: c88fb5bcfa t: move reftable/record_test.c to the unit testing framework > @@ Commit message > functions are similarly implemented, and > reftable/test_framework.{c, h} is not #included in the ported test. > > + Get rid of reftable_record_print() from the tests as well, because > + it clutters the test framework's output and we have no way of > + verifying the output. > + > Mentored-by: Patrick Steinhardt <ps@pks.im> > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com> > @@ t/unit-tests/t-reftable-record.c: static void test_copy(struct reftable_record * > /* 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_print(©, GIT_SHA1_RAWSZ); > ++ check(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); > > reftable_record_release(©); > + } > @@ t/unit-tests/t-reftable-record.c: static void test_varint_roundtrip(void) > 4096, > ((uint64_t)1 << 63), > 2: 90feb4168c ! 2: 45ac972538 t-reftable-record: add reftable_record_cmp() tests for log records > @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip > - .refname = xstrdup("refs/heads/master"), > - .update_index = 42, > + .type = BLOCK_TYPE_LOG, > -+ .u.log.refname = xstrdup("refs/heads/master"), > ++ .u.log.refname = (char *) "refs/heads/master", > + .u.log.update_index = 42, > }, > { > @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip > - .update_index = 22, > - } > + .type = BLOCK_TYPE_LOG, > -+ .u.log.refname = xstrdup("refs/heads/master"), > ++ .u.log.refname = (char *) "refs/heads/master", > + .u.log.update_index = 22, > + }, > + { > + .type = BLOCK_TYPE_LOG, > -+ .u.log.refname = xstrdup("refs/heads/main"), > ++ .u.log.refname = (char *) "refs/heads/main", > + .u.log.update_index = 22, > + }, > }; > @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip > + check(!reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); > + check(!reftable_record_equal(&in[1], &in[2], GIT_SHA1_RAWSZ)); > + check_int(reftable_record_cmp(&in[1], &in[2]), >, 0); > -+ /* comparison should be reversed for equal keys */ > ++ /* comparison should be reversed for equal keys, because > ++ * comparison is now performed on the basis of update indices */ > + check_int(reftable_record_cmp(&in[0], &in[1]), <, 0); > + > + in[1].u.log.update_index = in[0].u.log.update_index; > + check(reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); > + check(!reftable_record_cmp(&in[0], &in[1])); > -+ > -+ for (size_t i = 0; i < ARRAY_SIZE(in); i++) > -+ reftable_record_release(&in[i]); > } > > static void test_reftable_log_record_roundtrip(void) > 3: e435166a78 ! 3: db76851f4b t-reftable-record: add comparison tests for ref records > @@ t/unit-tests/t-reftable-record.c: static void set_hash(uint8_t *h, int j) > + struct reftable_record in[3] = { > + { > + .type = BLOCK_TYPE_REF, > -+ .u.ref.refname = xstrdup("refs/heads/master"), > ++ .u.ref.refname = (char *) "refs/heads/master", > + .u.ref.value_type = REFTABLE_REF_VAL1, > + }, > + { > + .type = BLOCK_TYPE_REF, > -+ .u.ref.refname = xstrdup("refs/heads/master"), > ++ .u.ref.refname = (char *) "refs/heads/master", > + .u.ref.value_type = REFTABLE_REF_DELETION, > + }, > + { > + .type = BLOCK_TYPE_REF, > -+ .u.ref.refname = xstrdup("HEAD"), > ++ .u.ref.refname = (char *) "HEAD", > + .u.ref.value_type = REFTABLE_REF_SYMREF, > -+ .u.ref.value.symref = xstrdup("refs/heads/master"), > ++ .u.ref.value.symref = (char *) "refs/heads/master", > + }, > + }; > + > @@ t/unit-tests/t-reftable-record.c: static void set_hash(uint8_t *h, int j) > + in[1].u.ref.value_type = in[0].u.ref.value_type; > + check(reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); > + check(!reftable_record_cmp(&in[0], &in[1])); > -+ > -+ for (size_t i = 0; i < ARRAY_SIZE(in); i++) > -+ reftable_record_release(&in[i]); > +} > + > static void test_reftable_ref_record_roundtrip(void) > 4: ad014db045 = 4: 78aff923c6 t-reftable-record: add comparison tests for index records > 5: 69c1f3891a = 5: b0b3c98042 t-reftable-record: add comparison tests for obj records > 6: dca1a016da < -: ---------- t-reftable-record: add ref tests for reftable_record_is_deletion() > -: ---------- > 6: 5e6b004216 t-reftable-record: add ref tests for reftable_record_is_deletion() > 7: c7ffff71b0 ! 7: a68be88ccb t-reftable-record: add log tests for reftable_record_is_deletion() > @@ Commit message > reftable_record_is_deletion() is a function defined in > reftable/record.{c, h} that determines whether a record is of > type deletion or not. In the current testing setup, this function > - is left untested for all the four record types (ref, log, obj, index). > + is left untested for three of the four record types (log, obj, index). > > Add tests for this function in the case of log records. > > 8: f3e0c2aaf5 ! 8: 02516add15 t-reftable-record: add obj tests for reftable_record_is_deletion() > @@ Commit message > reftable_record_is_deletion() is a function defined in > reftable/record.{c, h} that determines whether a record is of > type deletion or not. In the current testing setup, this function > - is left untested for all the four record types (ref, log, obj, index). > + is left untested for two of the four record types (obj, index). > > Add tests for this function in the case of obj records. > Note that since obj records cannot be of type deletion, this function > 9: 8eeeb63982 ! 9: 541f9811d3 t-reftable-record: add index tests for reftable_record_is_deletion() > @@ Commit message > reftable_record_is_deletion() is a function defined in > reftable/record.{c, h} that determines whether a record is of > type deletion or not. In the current testing setup, this function > - is left untested for all the four record types (ref, log, obj, index). > + is left untested for index records. > > Add tests for this function in the case of index records. > Note that since index records cannot be of type deletion, this function > 10: 979db146a0 ! 10: c2aff283b1 t-reftable-record: add tests for reftable_ref_record_compare_name() > @@ Commit message > > ## t/unit-tests/t-reftable-record.c ## > @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_comparison(void) > - reftable_record_release(&in[i]); > + check(!reftable_record_cmp(&in[0], &in[1])); > } > > +static void test_reftable_ref_record_compare_name(void) > @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_compariso > + > + QSORT(recs, N, reftable_ref_record_compare_name); > + > -+ for (i = 1; i < N; i++) > -+ check(reftable_ref_record_compare_name(&recs[i - 1], &recs[i]) < 0); > ++ for (i = 1; i < N; i++) { > ++ check_int(strcmp(recs[i - 1].refname, recs[i].refname), <, 0); > ++ check_int(reftable_ref_record_compare_name(&recs[i], &recs[i]), ==, 0); > ++ } > ++ > ++ for (i = 0; i < N - 1; i++) > ++ check_int(reftable_ref_record_compare_name(&recs[i + 1], &recs[i]), >, 0); > + > + for (i = 0; i < N; i++) > + reftable_ref_record_release(&recs[i]); > 11: fe044f186b ! 11: 7bdfca3744 t-reftable-record: add tests for reftable_log_record_compare_key() > @@ Commit message > > ## t/unit-tests/t-reftable-record.c ## > @@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_comparison(void) > - reftable_record_release(&in[i]); > + check(!reftable_record_cmp(&in[0], &in[1])); > } > > +static void test_reftable_log_record_compare_key(void) > @@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_compariso > + > + QSORT(logs, N, reftable_log_record_compare_key); > + > -+ for (i = 1; i < N; i++) > -+ check(reftable_log_record_compare_key(&logs[i - 1], &logs[i]) < 0); > ++ for (i = 1; i < N / 2; i++) > ++ check_int(strcmp(logs[i - 1].refname, logs[i].refname), <, 0); > ++ for (i = N / 2 + 1; i < N; i++) > ++ check_int(logs[i - 1].update_index, >, logs[i].update_index); > ++ > ++ for (i = 0; i < N - 1; i++) { > ++ check_int(reftable_log_record_compare_key(&logs[i], &logs[i]), ==, 0); > ++ check_int(reftable_log_record_compare_key(&logs[i + 1], &logs[i]), >, 0); > ++ } > + > + for (i = 0; i < N; i++) > + reftable_log_record_release(&logs[i]);
In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit testing framework written entirely in C was introduced to the Git project aimed at simplifying testing and reducing test run times. Currently, tests for the reftable refs-backend are performed by a custom testing framework defined by reftable/test_framework.{c, h}. Port reftable/record_test.c to the unit testing framework and improve upon the ported test. The first patch in the series moves the test to the unit testing framework, and the rest of the patches improve upon 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> --- Changes in v3: - Remove reftable_record_print() from tests to reduce clutter in test output. - Add more explanation to in-code comment in 1st patch. - Use string literals instead of xstrdup() when possible to prevent the cost of mallocing and freeing. - make reftable_record_is_deletion() test for refs conciser in 6th patch. - Update commit messages for 7th, 8th and 9th patch to better reflect changes. - Add tests for '== 0' and '> 0' cases n 10th and 11th patch. CI/PR: https://github.com/gitgitgadget/git/pull/1750 Chandra Pratap (11): t: move reftable/record_test.c to the unit testing framework t-reftable-record: add reftable_record_cmp() tests for log records t-reftable-record: add comparison tests for ref records t-reftable-record: add comparison tests for index records t-reftable-record: add comparison tests for obj records t-reftable-record: add reftable_record_is_deletion() test for ref records t-reftable-record: add reftable_record_is_deletion() test for log records t-reftable-record: add reftable_record_is_deletion() test for obj records t-reftable-record: add reftable_record_is_deletion() test for index records t-reftable-record: add tests for reftable_ref_record_compare_name() t-reftable-record: add tests for reftable_log_record_compare_key() Makefile | 2 +- reftable/record_test.c | 382 ------------------------- t/helper/test-reftable.c | 1 - t/unit-tests/t-reftable-record.c | 554 +++++++++++++++++++++++++++++++++++++++++++++ Range-diff against v2: 1: d0646af549 ! 1: c88fb5bcfa t: move reftable/record_test.c to the unit testing framework @@ Commit message functions are similarly implemented, and reftable/test_framework.{c, h} is not #included in the ported test. + Get rid of reftable_record_print() from the tests as well, because + it clutters the test framework's output and we have no way of + verifying the output. + Mentored-by: Patrick Steinhardt <ps@pks.im> Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com> @@ t/unit-tests/t-reftable-record.c: static void test_copy(struct reftable_record * /* 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_print(©, GIT_SHA1_RAWSZ); ++ check(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ)); reftable_record_release(©); + } @@ t/unit-tests/t-reftable-record.c: static void test_varint_roundtrip(void) 4096, ((uint64_t)1 << 63), 2: 90feb4168c ! 2: 45ac972538 t-reftable-record: add reftable_record_cmp() tests for log records @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip - .refname = xstrdup("refs/heads/master"), - .update_index = 42, + .type = BLOCK_TYPE_LOG, -+ .u.log.refname = xstrdup("refs/heads/master"), ++ .u.log.refname = (char *) "refs/heads/master", + .u.log.update_index = 42, }, { @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip - .update_index = 22, - } + .type = BLOCK_TYPE_LOG, -+ .u.log.refname = xstrdup("refs/heads/master"), ++ .u.log.refname = (char *) "refs/heads/master", + .u.log.update_index = 22, + }, + { + .type = BLOCK_TYPE_LOG, -+ .u.log.refname = xstrdup("refs/heads/main"), ++ .u.log.refname = (char *) "refs/heads/main", + .u.log.update_index = 22, + }, }; @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_roundtrip + check(!reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); + check(!reftable_record_equal(&in[1], &in[2], GIT_SHA1_RAWSZ)); + check_int(reftable_record_cmp(&in[1], &in[2]), >, 0); -+ /* comparison should be reversed for equal keys */ ++ /* comparison should be reversed for equal keys, because ++ * comparison is now performed on the basis of update indices */ + check_int(reftable_record_cmp(&in[0], &in[1]), <, 0); + + in[1].u.log.update_index = in[0].u.log.update_index; + check(reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); + check(!reftable_record_cmp(&in[0], &in[1])); -+ -+ for (size_t i = 0; i < ARRAY_SIZE(in); i++) -+ reftable_record_release(&in[i]); } static void test_reftable_log_record_roundtrip(void) 3: e435166a78 ! 3: db76851f4b t-reftable-record: add comparison tests for ref records @@ t/unit-tests/t-reftable-record.c: static void set_hash(uint8_t *h, int j) + struct reftable_record in[3] = { + { + .type = BLOCK_TYPE_REF, -+ .u.ref.refname = xstrdup("refs/heads/master"), ++ .u.ref.refname = (char *) "refs/heads/master", + .u.ref.value_type = REFTABLE_REF_VAL1, + }, + { + .type = BLOCK_TYPE_REF, -+ .u.ref.refname = xstrdup("refs/heads/master"), ++ .u.ref.refname = (char *) "refs/heads/master", + .u.ref.value_type = REFTABLE_REF_DELETION, + }, + { + .type = BLOCK_TYPE_REF, -+ .u.ref.refname = xstrdup("HEAD"), ++ .u.ref.refname = (char *) "HEAD", + .u.ref.value_type = REFTABLE_REF_SYMREF, -+ .u.ref.value.symref = xstrdup("refs/heads/master"), ++ .u.ref.value.symref = (char *) "refs/heads/master", + }, + }; + @@ t/unit-tests/t-reftable-record.c: static void set_hash(uint8_t *h, int j) + in[1].u.ref.value_type = in[0].u.ref.value_type; + check(reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); + check(!reftable_record_cmp(&in[0], &in[1])); -+ -+ for (size_t i = 0; i < ARRAY_SIZE(in); i++) -+ reftable_record_release(&in[i]); +} + static void test_reftable_ref_record_roundtrip(void) 4: ad014db045 = 4: 78aff923c6 t-reftable-record: add comparison tests for index records 5: 69c1f3891a = 5: b0b3c98042 t-reftable-record: add comparison tests for obj records 6: dca1a016da < -: ---------- t-reftable-record: add ref tests for reftable_record_is_deletion() -: ---------- > 6: 5e6b004216 t-reftable-record: add ref tests for reftable_record_is_deletion() 7: c7ffff71b0 ! 7: a68be88ccb t-reftable-record: add log tests for reftable_record_is_deletion() @@ Commit message reftable_record_is_deletion() is a function defined in reftable/record.{c, h} that determines whether a record is of type deletion or not. In the current testing setup, this function - is left untested for all the four record types (ref, log, obj, index). + is left untested for three of the four record types (log, obj, index). Add tests for this function in the case of log records. 8: f3e0c2aaf5 ! 8: 02516add15 t-reftable-record: add obj tests for reftable_record_is_deletion() @@ Commit message reftable_record_is_deletion() is a function defined in reftable/record.{c, h} that determines whether a record is of type deletion or not. In the current testing setup, this function - is left untested for all the four record types (ref, log, obj, index). + is left untested for two of the four record types (obj, index). Add tests for this function in the case of obj records. Note that since obj records cannot be of type deletion, this function 9: 8eeeb63982 ! 9: 541f9811d3 t-reftable-record: add index tests for reftable_record_is_deletion() @@ Commit message reftable_record_is_deletion() is a function defined in reftable/record.{c, h} that determines whether a record is of type deletion or not. In the current testing setup, this function - is left untested for all the four record types (ref, log, obj, index). + is left untested for index records. Add tests for this function in the case of index records. Note that since index records cannot be of type deletion, this function 10: 979db146a0 ! 10: c2aff283b1 t-reftable-record: add tests for reftable_ref_record_compare_name() @@ Commit message ## t/unit-tests/t-reftable-record.c ## @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_comparison(void) - reftable_record_release(&in[i]); + check(!reftable_record_cmp(&in[0], &in[1])); } +static void test_reftable_ref_record_compare_name(void) @@ t/unit-tests/t-reftable-record.c: static void test_reftable_ref_record_compariso + + QSORT(recs, N, reftable_ref_record_compare_name); + -+ for (i = 1; i < N; i++) -+ check(reftable_ref_record_compare_name(&recs[i - 1], &recs[i]) < 0); ++ for (i = 1; i < N; i++) { ++ check_int(strcmp(recs[i - 1].refname, recs[i].refname), <, 0); ++ check_int(reftable_ref_record_compare_name(&recs[i], &recs[i]), ==, 0); ++ } ++ ++ for (i = 0; i < N - 1; i++) ++ check_int(reftable_ref_record_compare_name(&recs[i + 1], &recs[i]), >, 0); + + for (i = 0; i < N; i++) + reftable_ref_record_release(&recs[i]); 11: fe044f186b ! 11: 7bdfca3744 t-reftable-record: add tests for reftable_log_record_compare_key() @@ Commit message ## t/unit-tests/t-reftable-record.c ## @@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_comparison(void) - reftable_record_release(&in[i]); + check(!reftable_record_cmp(&in[0], &in[1])); } +static void test_reftable_log_record_compare_key(void) @@ t/unit-tests/t-reftable-record.c: static void test_reftable_log_record_compariso + + QSORT(logs, N, reftable_log_record_compare_key); + -+ for (i = 1; i < N; i++) -+ check(reftable_log_record_compare_key(&logs[i - 1], &logs[i]) < 0); ++ for (i = 1; i < N / 2; i++) ++ check_int(strcmp(logs[i - 1].refname, logs[i].refname), <, 0); ++ for (i = N / 2 + 1; i < N; i++) ++ check_int(logs[i - 1].update_index, >, logs[i].update_index); ++ ++ for (i = 0; i < N - 1; i++) { ++ check_int(reftable_log_record_compare_key(&logs[i], &logs[i]), ==, 0); ++ check_int(reftable_log_record_compare_key(&logs[i + 1], &logs[i]), >, 0); ++ } + + for (i = 0; i < N; i++) + reftable_log_record_release(&logs[i]);