Re: [PATCH v3 11/11] t-reftable-record: add tests for reftable_log_record_compare_key()
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jun 30, 2024, 19:11 UTC
- Message-ID
- <CAOLa=ZSC9BFHBuqwnHj6VZDAL5Xuh0tkxAnXjEUkvh8ZpoZPQw@mail.gmail.com>
- In-Reply-To
- <20240628063625.4092-12-chandrapratap3519@gmail.com>
Chandra Pratap <chandrapratap3519@gmail.com> writes:
Show 36 quoted lines
> 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@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> 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.
Show 12 quoted lines
> + 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.
Show 17 quoted lines
> + 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