On Sat, Aug 24, 2024 at 7:20 PM Ghanshyam Thakkar
<shyamthakkar001@xxxxxxxxx> 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.
> Mentored-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx>
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@xxxxxxxxx>
> ---
> 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.
> 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.
> +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.
> + 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.
