Re: [GSoC][PATCH] unit-tests: add tests for oidset.h

From: Christian Couder <christian.couder@gmail.com>
Date: Aug 26, 2024, 09:31 UTC
Message-ID: <CAP8UFD2yTMNmx0n1jhOu7dz_4XeOyTy1iLmRWYmuf9QJf75hsQ@mail.gmail.com>
In-Reply-To: <20240824172028.39419-1-shyamthakkar001@gmail.com>

On Sat, Aug 24, 2024 at 7:20 PM Ghanshyam Thakkar <shyamthakkar001@gmail.com> wrote:

>
> Add tests for oidset.h library, which were not previously present using
> the unit testing framework.

It might be interesting to also say if there are tests for oidset in the end-to-end tests, not just in the unit test framework. Also I think oidset.h is more an API than a library.

> This imposes a new restriction of running the test from the 't/' and
> 't/unit-tests/bin'

Either "from 't/' and 't/unit-tests/bin'" or "from the 't/' and 't/unit-tests/bin' directory" would be a bit better.

> for constructing the path to the test files which
> are used by t_parse_file(), which tests the parsing of object_ids from
> a file.

This might be clearer if it mentioned that t_parse_file() actually tests oidset_parse_file() which is part of the oidset.h API.

> This restriction is similar to the one we already have for
> end-to-end tests, wherein, we can only run those tests from 't/'.

Ok.

> The
> addition of allowing 't/unit-tests/bin' for allowing to run tests from
> is for running individual unit tests,

Maybe: "Allowing to run tests from 't/unit-tests/bin', in addition to
't/', makes it possible to run individual unit tests,"

> which is not currently possible
> via any 'make' target. And 'make unit-tests-test-tool' also runs from
> 't/unit-tests/bin'

It would be nice if you gave a few examples of commands that can be run after this patch while they didn't work before it.

Show 8 quoted lines

> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
> I know there is some hesitance from the community in imposing the
> restriction of running the unit tests from certain directories, so
> if this case does not justify imposing such a restriction, I am fine
> with removing t_parse_file() in the next version.

My opinion is that it might be good to remove t_parse_file() for now, as testing oidset_parse_file() might not be so important. Later an iteration in a separate patch could then perhap add it while better discussing if the restrictions that come with adding it are worth it.

Show 23 quoted lines

> diff --git a/t/unit-tests/lib-oid.c b/t/unit-tests/lib-oid.c
> index 37105f0a8f..8f0ccac532 100644
> --- a/t/unit-tests/lib-oid.c
> +++ b/t/unit-tests/lib-oid.c
> @@ -3,7 +3,7 @@
>  #include "strbuf.h"
>  #include "hex.h"
>
> -static int init_hash_algo(void)
> +int init_hash_algo(void)
>  {
>         static int algo = -1;
>
> diff --git a/t/unit-tests/lib-oid.h b/t/unit-tests/lib-oid.h
> index 8d2acca768..fc3e7aa376 100644
> --- a/t/unit-tests/lib-oid.h
> +++ b/t/unit-tests/lib-oid.h
> @@ -14,4 +14,5 @@
>   */
>  int get_oid_arbitrary_hex(const char *s, struct object_id *oid);
>
> +int init_hash_algo(void);
>  #endif /* LIB_OID_H */

It seems that the changes above will go away when some patches you already sent will be merged, which should happen soon. It might have been nice to say this in the section after the "---" line.

Show 9 quoted lines

> +static void t_parse_file(void)
> +{
> +       struct strbuf path = STRBUF_INIT;
> +       struct oidset st = OIDSET_INIT;
> +       struct object_id oid;
> +       int hash_algo = init_hash_algo();
> +
> +       if (!check_int(hash_algo, !=, GIT_HASH_UNKNOWN))
> +               return;

If initializing the hash algo fails here, it is likely because it already failed when get_oid_arbitrary_hex() (which initializes it) was called in the tests before this one. So I think it might be even better to move the above hash algo initialization code to setup() and make setup() error out in case the initialization fails. Then setup() could pass 'hash_algo' to all the functions it calls, even if some of them don't use it.

Show 17 quoted lines

> +       strbuf_test_data_path(&path, hash_algo);
> +       oidset_parse_file(&st, path.buf, &hash_algos[hash_algo]);
> +       check_int(oidset_size(&st), ==, 6);
> +
> +       if (!get_oid_arbitrary_hex("00", &oid))
> +               check_int(oidset_contains(&st, &oid), ==, 1);
> +       if (!get_oid_arbitrary_hex("44", &oid))
> +               check_int(oidset_contains(&st, &oid), ==, 1);
> +       if (!get_oid_arbitrary_hex("cc", &oid))
> +               check_int(oidset_contains(&st, &oid), ==, 1);
> +
> +       if (!get_oid_arbitrary_hex("11", &oid))
> +               check_int(oidset_contains(&st, &oid), ==, 0);
> +
> +       oidset_clear(&st);
> +       strbuf_release(&path);
> +}

Thanks.