Chandra Pratap <chandrapratap3519@xxxxxxxxx> writes:
> reftable_log_record_compare_key() is a function defined by
> reftable/record.{c, h} and is used to compare the keys of two
> log records when sorting multiple log records using 'qsort'.
> In the current testing setup, this function is left unexercised.
> Add a testing function for the same.
>
> 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 | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> index f45f2fdef2..cac8f632f9 100644
> --- a/t/unit-tests/t-reftable-record.c
> +++ b/t/unit-tests/t-reftable-record.c
> @@ -208,6 +208,37 @@ static void test_reftable_log_record_comparison(void)
> check(!reftable_record_cmp(&in[0], &in[1]));
> }
>
> +static void test_reftable_log_record_compare_key(void)
> +{
> + struct reftable_log_record logs[14] = { 0 };
> + size_t N = ARRAY_SIZE(logs), i;
> +
> + for (i = 0; i < N; i++) {
> + if (i < N / 2) {
> + logs[i].refname = xstrfmt("%02"PRIuMAX, (uintmax_t)i);
> + logs[i].update_index = i;
> + } else {
> + logs[i].refname = xstrdup("refs/heads/master");
> + logs[i].update_index = i;
> + }
> + }
> +
So we split the array into two sets, the first containing "00" ... "06"
and the next seven containing "refs/heads/master". It would be nice if
there was a comment here explaining why.
> + QSORT(logs, N, reftable_log_record_compare_key);
> +
> + 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);
> + }
> +
The same comments as the previous patch apply here too.
So the splitting of the array into two was mostly to show that for log
records, the update index is what determines the comparison factor when
the refname is the same I assume.
I can think of the following scenarios:
1. diff refnames, diff update index
2. diff refnames, same update index
3. same refnames, diff update index
4. same refnames, same update index
Seems like we test 1, 3 & 4. We should also test scenario 2.
Speaking of which, I also noticed that for scenario 4, we test this by
passing the same record '&logs[i]'. While this is useful, we should also
be testing passing different logs with the same value.
The difference is subtle here, but from a unit test point of view, we
want to ensure that the function works the same for records which have
same values and records which have the same address. This ensures to
test for function which would contain code like
int reftable_log_record_compare_key(const void *a, const void *b)
{
const struct reftable_log_record *la = a;
const struct reftable_log_record *lb = b;
if (la == lb)
return 0;
...
but forgot to check for value similarity.
> + for (i = 0; i < N; i++)
> + reftable_log_record_release(&logs[i]);
> +}
> +
> static void test_reftable_log_record_roundtrip(void)
> {
> struct reftable_log_record in[] = {
> @@ -513,6 +544,7 @@ int cmd_main(int argc, const char *argv[])
> TEST(test_reftable_index_record_comparison(), "comparison operations work on index record");
> TEST(test_reftable_obj_record_comparison(), "comparison operations work on obj record");
> TEST(test_reftable_ref_record_compare_name(), "reftable_ref_record_compare_name works");
> + TEST(test_reftable_log_record_compare_key(), "reftable_log_record_compare_key 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");
> --
> 2.45.2.404.g9eaef5822c
Attachment:
signature.asc
Description: PGP signature
