| Message ID | 20240628063625.4092-11-chandrapratap3519@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: port reftable/record_test.c to the unit testing framework | expand |
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. > + > + 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. > + for (i = 0; i < N; i++) > + reftable_ref_record_release(&recs[i]); > +} > + Nit: The top three loops could possibly be combined. > 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
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. > > + > > + 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. > > + 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
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
On Mon, 1 Jul 2024 at 15:21, Karthik Nayak <karthik.188@gmail.com> wrote: > > 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); > } I agree, this seems much simpler than the dance we have to do when using qsort. I'll reimplement this and the 'log_compare_key' test with hard-coded input instead of qsort. > >> > + 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
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); + + QSORT(recs, N, reftable_ref_record_compare_name); + + 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]); +} + 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");
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(+)