Re: [PATCH v3 10/11] t-reftable-record: add tests for reftable_ref_record_compare_name()

>
> Chandra Pratap <chandrapratap3519@gmail.com> writes:
>
> > On Mon, 1 Jul 2024 at 00:29, Karthik Nayak <karthik.188@gmail.com> wrote:
> >>
> >> Chandra Pratap <chandrapratap3519@gmail.com> 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@pks.im>
> >> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> >> > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> >> > ---
> >> >  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