Chandra Pratap <chandrapratap3519@xxxxxxxxx> 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@xxxxxx>
> Mentored-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> Signed-off-by: Chandra Pratap <chandrapratap3519@xxxxxxxxx>
>
> ---
> 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@xxxxxx>
> Mentored-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> Signed-off-by: Chandra Pratap <chandrapratap3519@xxxxxxxxx>
> @@ 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]);
Attachment:
signature.asc
Description: PGP signature
