Chandra Pratap <chandrapratap3519@xxxxxxxxx> writes:
> On Mon, 1 Jul 2024 at 00:29, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>>
>> Chandra Pratap <chandrapratap3519@xxxxxxxxx> writes:
>>
>> > reftable_ref_record_compare_name() is a function defined by
>> > reftable/record.{c, h} and is used to compare the refname of two
>> > ref records when sorting multiple ref 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 | 23 +++++++++++++++++++++++
>> > 1 file changed, 23 insertions(+)
>> >
>> > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
>> > index 55b8d03494..f45f2fdef2 100644
>> > --- a/t/unit-tests/t-reftable-record.c
>> > +++ b/t/unit-tests/t-reftable-record.c
>> > @@ -95,6 +95,28 @@ static void test_reftable_ref_record_comparison(void)
>> > check(!reftable_record_cmp(&in[0], &in[1]));
>> > }
>> >
>> > +static void test_reftable_ref_record_compare_name(void)
>> > +{
>> > + struct reftable_ref_record recs[14] = { 0 };
>> > + size_t N = ARRAY_SIZE(recs), i;
>> > +
>> > + for (i = 0; i < N; i++)
>> > + recs[i].refname = xstrfmt("%02"PRIuMAX, (uintmax_t)i);
>>
>> This needs to be free'd too right?
>>
>> So we create an array of 14 records, with refnames "00", "01", "02" ...
>> "13", here.
>>
>> > +
>> > + QSORT(recs, N, reftable_ref_record_compare_name);
>> > +
>>
>> We then use `reftable_ref_record_compare_name` as the comparison
>> function to sort them.
>>
>> > + 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);
>> > + }
>>
>> Here we use `strcmp` to ensure that the ordering done by
>> `reftable_ref_record_compare_name` is correct. This makes sense,
>> although I would have expected this to be done the other way around.
>> i.e. we should use `strcmp` as the function used in `QSORT` and in this
>> loop we validate that `reftable_ref_record_compare_name` also produces
>> the same result when comparing.
>
> The first parameter to QSORT is an array of 'struct reftable_record' so I don't
> think it's possible to use strcmp() as the comparison function. We do, however,
> use strcmp() internally to compare the ref records.
>
Well, yes, not directly, but you can create your own function and pass
it to QSORT. This will mostly replicate what
`reftable_ref_record_compare_name` is doing. But I think you're missing
what I'm trying to say however.
I'm not really talking about the semantics of it. I'm talking more about
the concept of it. See the next section...
>> > +
>> > + for (i = 0; i < N - 1; i++)
>> > + check_int(reftable_ref_record_compare_name(&recs[i + 1], &recs[i]), >, 0);
>> > +
>>
>> Also, with the current setup, we use `reftable_ref_record_compare_name`
>> to sort the first array and then use `reftable_ref_record_compare_name`
>> to check if it is correct? This doesn't work, we need to isolate the
>> data creation from the inference, if the same function can influence
>> both, then we are not really testing the function.
>
> The validity of `reftable_ref_record_compare_name()` is checked by the first
> loop. Since we're already sure of the order of 'recs' at this point (increasing
> order), this loop is supposed to test the function for ' > 0' case.
>
Yes, the first loop uses 'strcmp' to validate and that's perfectly
correct. But this operation here is kinda pointless in my opinion. My
point being that if there is a list x[] and you use a function f() to
sort that list, validating that x[] is sorted with f() again, doesn't
test f().
It might be much simpler to just test
`reftable_ref_record_compare_name()` as so:
static void test_reftable_ref_record_compare_name(void)
{
struct reftable_ref_record recs[3] = {
{
.refname = (char *) "refs/heads/a"
},
{
.refname = (char *) "refs/heads/b"
},
{
.refname = (char *) "refs/heads/a"
},
};
check_int(reftable_ref_record_compare_name(&recs[0], &recs[1]), ==, -1);
check_int(reftable_ref_record_compare_name(&recs[1], &recs[0]), ==, 1);
check_int(reftable_ref_record_compare_name(&recs[0], &recs[2]), ==, 0);
}
>> > + for (i = 0; i < N; i++)
>> > + reftable_ref_record_release(&recs[i]);
>> > +}
>> > +
>>
>> Nit: The top three loops could possibly be combined.
>
> The limiting as well as initial value for the array indices are all
> different so I'm not sure how to go about this.
>
>> > static void test_reftable_ref_record_roundtrip(void)
>> > {
>> > struct strbuf scratch = STRBUF_INIT;
>> > @@ -490,6 +512,7 @@ int cmd_main(int argc, const char *argv[])
>> > TEST(test_reftable_log_record_comparison(), "comparison operations work on log record");
>> > 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_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
