Re: [GSoC][PATCH v2] t: port helper/test-oid-array.c to unit-tests/t-oid-array.c

["Ghanshyam Thakkar" <shyamthakkar001@gmail.com>]

Christian Couder <christian.couder@gmail.com> wrote:
> On Sat, Aug 3, 2024 at 3:22 PM Ghanshyam Thakkar
> <shyamthakkar001@gmail.com> wrote:
> > Migrate them to the unit testing framework for better runtime
> > performance and efficiency. Also 'the_hash_algo' is used internally in
>
> It doesn't seem to me that a variable called 'the_hash_algo' is used
> internally in oid_array_lookup() anymore.

It is. oid_array_lookup() uses oid_pos():hash-lookup.c, which uses
'the_hash_algo'.

> > +static void t_enumeration(const char **input_args, size_t input_sz,
> > +                         const char **result, size_t result_sz)
> > +{
> > +       struct oid_array input = OID_ARRAY_INIT, expect = OID_ARRAY_INIT,
> > +                        actual = OID_ARRAY_INIT;
> > +       size_t i;
> > +
> > +       if (fill_array(&input, input_args, input_sz))
> > +               return;
> > +       if (fill_array(&expect, result, result_sz))
> > +               return;
>
> It would have been nice if the arguments were called 'expect_args' and
> 'expect_sz' in the same way as for 'input'. Is there a reason why we
> couldn't just use 'expect' (or maybe 'expected') everywhere instead of
> 'result'?

I have changed them to 'expect' in v3.

> Also after the above 'input.nr' is equal to 'input_sz' and 'expect.nr'
> is equal to 'result_sz' otherwise we would have already returned fron
> the current function.
>
> > +       oid_array_for_each_unique(&input, add_to_oid_array, &actual);
> > +       check_uint(actual.nr, ==, expect.nr);
>
> I think it might be better to return if this check fails. Otherwise it
> means that we likely messed something up in the 'input_args' or
> 'result' arguments we passed to the function, and then...
>
> > +       for (i = 0; i < test_min(actual.nr, expect.nr); i++) {
> > +               if (!check(oideq(&actual.oid[i], &expect.oid[i])))
>
> ...we might not compare here the input oid with the corresponding
> result oid we intended to compare it to. This might result in a lot of
> not very relevant output.
>
> Returning if check_uint(actual.nr, ==, expect.nr) fails would avoid
> such output and also enable us to just use 'actual.nr' instead of
> 'test_min(actual.nr, expect.nr)' in the 'for' loop above.

Changed this in v3.

>
> > +                       test_msg("expected: %s\n       got: %s\n     index: %" PRIuMAX,
> > +                                oid_to_hex(&expect.oid[i]), oid_to_hex(&actual.oid[i]),
> > +                                (uintmax_t)i);
> > +       }
> > +       check_uint(i, ==, result_sz);
>
> As we saw above that 'expect.nr' is equal to 'result_sz', this check
> can fail only if 'actual.nr' is different from 'expect.nr' which we
> already checked above. So I think this check is redundant and we might
> want to get rid of it.

Removed in v3.

>
> In fill_array() above, we use check_int() to check the result of
> get_oid_arbitrary_hex() like this:
>
> if (!check_int(get_oid_arbitrary_hex(hexes[i], &oid), ==, 0))
>
> It doesn't look consistent to not use check_int() to check the result
> of get_oid_arbitrary_hex() here. Or is there a specific reason to do
> it in one place but not in another?

Not in particular. Added check_int() in v3.

Thanks for the review.