| Message ID | 20240824172028.39419-1-shyamthakkar001@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [GSoC] unit-tests: add tests for oidset.h | expand |
On Sat, Aug 24, 2024 at 10:50:23PM +0530, Ghanshyam Thakkar wrote: > Add tests for oidset.h library, which were not previously present using > the unit testing framework. > > This imposes a new restriction of running the test from the 't/' and > 't/unit-tests/bin' 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 restriction is similar to the one we already have for > end-to-end tests, wherein, we can only run those tests from 't/'. The > addition of allowing 't/unit-tests/bin' for allowing to run tests from > is for running 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' > > 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. Another option would be to set up a preprocessor define that gives us the path to the unit test root directory. But I'm also okayish with the current version. If it turns out to be annoying we can still iterate. > 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 */ Let's add a comment to explain what this does. Also, do we maybe want to give it a name that ties it to the unit tests? `init_hash_algo()` is quite generic and may easily lead to conflicting symbol names. Maybe something like `t_oid_init_hash_algo()`. `t_` to indicate that it is testing-related, `oid_` indicates that it's part of "lib-oid.h", and the remainder describes what it does. > diff --git a/t/unit-tests/t-oidset.c b/t/unit-tests/t-oidset.c > new file mode 100644 > index 0000000000..4a63f9ea94 > --- /dev/null > +++ b/t/unit-tests/t-oidset.c > @@ -0,0 +1,222 @@ > +#include "test-lib.h" > +#include "oidset.h" > +#include "lib-oid.h" > +#include "hex.h" > +#include "strbuf.h" > + > +static const char *const hex_input[] = { "00", "11", "22", "33", "aa", "cc" }; I think we typically write this `const char * const`, with another space between `*` and `const`. > +static void strbuf_test_data_path(struct strbuf *buf, int hash_algo) > +{ > + strbuf_getcwd(buf); > + strbuf_strip_suffix(buf, "/unit-tests/bin"); > + strbuf_addf(buf, "/unit-tests/t-oidset/%s", > + hash_algo == GIT_HASH_SHA1 ? "sha1-oids" : "sha256-oids"); > +} I wouldn't prefix this with `strbuf_`, as it is not part of the strbuf subsystem. The function just happens to use a strbuf. > +static void setup(void (*f)(struct oidset *st)) I was wondering what `st` stands for. I'd either call it just `s` or `set`. > +{ > + struct oidset st = OIDSET_INIT; > + struct object_id oid; > + int ret = 0; > + > + if (!check_int(oidset_size(&st), ==, 0)) { > + test_skip_all("OIDSET_INIT is broken"); > + return; > + } > + > + for (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) { > + if ((ret = get_oid_arbitrary_hex(hex_input[i], &oid))) > + break; > + if (!check_int((ret = oidset_insert(&st, &oid)), ==, 0)) > + break; > + } In both of these cases I'd split out the assignment into a separate line. While the first instance is likely fine, the second instance makes me a bit uneasy as it is a macro. I generally do not trust macros to do the correct thing when being passed a statement with side effects. [snip] > +static int input_contains(struct object_id *oid, char *seen) > +{ > + for (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) { > + struct object_id oid_input; > + if (get_oid_arbitrary_hex(hex_input[i], &oid_input)) > + return -1; > + if (oideq(&oid_input, oid)) { > + if (seen[i]) > + return 2; > + seen[i] = 1; > + return 0; > + } > + } > + return 1; > +} This function is somewhat confusing. Contains what? What are the parameters? Is one of them the expectation, the other one the actual state? What do we compare against? I think it would help to get it a more descriptive name that says what we check for and remove the dependence on global state. Also, the `seen` array does not seem to be used by any caller. So maybe we should allocate it ourselves in this function such that it is self-contained. It requires more allocations, sure, but I highly doubt that this is going to be important in this test. Patrick
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. > 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. > 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.
Christian Couder <christian.couder@gmail.com> writes: >> 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. Thanks for pointing these out; 100% agreed. >> This imposes a new restriction of running the test from the 't/' and >> 't/unit-tests/bin' I thought we just got rid of such an restriction during the review of another unit-test topic? If this is a recurring theme, perhaps we should teach t/unit-test/test-lib.c a few ways to specify where the auxiliary files for unit-tests are (e.g. "-d <datadir>" command line option, or $GIT_UNIT_TEST_DATA_DIR environment variable). Even though the end-to-end tests do not allow you to start them from an arbitrary directory (it shouldn't be a rocket science to teach them to do so, though), they can run in an arbitrary place with the "--root" option without hindering its ability to read its auxiliary data files, because they can learn where the t/ directory is by looking at $TEST_DIRECTORY and a few other variables. A similar idea should be applicable to the unit-tests framework. Thanks.
diff --git a/Makefile b/Makefile index e298c8b55e..5c1762fa1b 100644 --- a/Makefile +++ b/Makefile @@ -1338,6 +1338,7 @@ UNIT_TEST_PROGRAMS += t-hash UNIT_TEST_PROGRAMS += t-hashmap UNIT_TEST_PROGRAMS += t-mem-pool UNIT_TEST_PROGRAMS += t-oidmap +UNIT_TEST_PROGRAMS += t-oidset UNIT_TEST_PROGRAMS += t-oidtree UNIT_TEST_PROGRAMS += t-prio-queue UNIT_TEST_PROGRAMS += t-reftable-basics 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 */ diff --git a/t/unit-tests/t-oidset.c b/t/unit-tests/t-oidset.c new file mode 100644 index 0000000000..4a63f9ea94 --- /dev/null +++ b/t/unit-tests/t-oidset.c @@ -0,0 +1,222 @@ +#include "test-lib.h" +#include "oidset.h" +#include "lib-oid.h" +#include "hex.h" +#include "strbuf.h" + +static const char *const hex_input[] = { "00", "11", "22", "33", "aa", "cc" }; + +static void strbuf_test_data_path(struct strbuf *buf, int hash_algo) +{ + strbuf_getcwd(buf); + strbuf_strip_suffix(buf, "/unit-tests/bin"); + strbuf_addf(buf, "/unit-tests/t-oidset/%s", + hash_algo == GIT_HASH_SHA1 ? "sha1-oids" : "sha256-oids"); +} + +static void setup(void (*f)(struct oidset *st)) +{ + struct oidset st = OIDSET_INIT; + struct object_id oid; + int ret = 0; + + if (!check_int(oidset_size(&st), ==, 0)) { + test_skip_all("OIDSET_INIT is broken"); + return; + } + + for (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) { + if ((ret = get_oid_arbitrary_hex(hex_input[i], &oid))) + break; + if (!check_int((ret = oidset_insert(&st, &oid)), ==, 0)) + break; + } + + if (!ret && check_int(oidset_size(&st), ==, ARRAY_SIZE(hex_input))) + f(&st); + + oidset_clear(&st); +} + +static void t_contains(struct oidset *st) +{ + struct object_id oid; + + for (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) { + if (!get_oid_arbitrary_hex(hex_input[i], &oid)) { + if (!check_int(oidset_contains(st, &oid), ==, 1)) + test_msg("oid: %s", oid_to_hex(&oid)); + } + } + + if (!get_oid_arbitrary_hex("55", &oid)) + check_int(oidset_contains(st, &oid), ==, 0); +} + +static void t_insert_dup(struct oidset *st) +{ + struct object_id oid; + + if (!get_oid_arbitrary_hex("11", &oid)) + check_int(oidset_insert(st, &oid), ==, 1); + + if (!get_oid_arbitrary_hex("aa", &oid)) + check_int(oidset_insert(st, &oid), ==, 1); + + check_int(oidset_size(st), ==, ARRAY_SIZE(hex_input)); +} + +static void t_insert_from_set(struct oidset *st_src) +{ + struct oidset st_dest = OIDSET_INIT; + struct oidset_iter iter_src, iter_dest; + struct object_id *oid_src, *oid_dest; + struct object_id oid; + size_t count = 0; + + oidset_insert_from_set(&st_dest, st_src); + check_int(oidset_size(st_src), ==, ARRAY_SIZE(hex_input)); + check_int(oidset_size(&st_dest), ==, oidset_size(st_src)); + + oidset_iter_init(st_src, &iter_src); + oidset_iter_init(&st_dest, &iter_dest); + + /* check that oidset_insert_from_set() makes a copy of the object_ids */ + while ((oid_src = oidset_iter_next(&iter_src)) && + (oid_dest = oidset_iter_next(&iter_dest))) { + check(oid_src != oid_dest); + count++; + } + check_int(count, ==, ARRAY_SIZE(hex_input)); + + for (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) { + if (!get_oid_arbitrary_hex(hex_input[i], &oid)) { + if (!check_int(oidset_contains(&st_dest, &oid), ==, 1)) + test_msg("oid: %s", oid_to_hex(&oid)); + } + } + + if (!get_oid_arbitrary_hex("55", &oid)) + check_int(oidset_contains(&st_dest, &oid), ==, 0); + oidset_clear(&st_dest); +} + +static void t_remove(struct oidset *st) +{ + struct object_id oid; + + if (!get_oid_arbitrary_hex("55", &oid)) { + check_int(oidset_remove(st, &oid), ==, 0); + check_int(oidset_size(st), ==, ARRAY_SIZE(hex_input)); + } + + if (!get_oid_arbitrary_hex("22", &oid)) { + check_int(oidset_remove(st, &oid), ==, 1); + check_int(oidset_size(st), ==, ARRAY_SIZE(hex_input) - 1); + check_int(oidset_contains(st, &oid), ==, 0); + } + + if (!get_oid_arbitrary_hex("cc", &oid)) { + check_int(oidset_remove(st, &oid), ==, 1); + check_int(oidset_size(st), ==, ARRAY_SIZE(hex_input) - 2); + check_int(oidset_contains(st, &oid), ==, 0); + } + + if (!get_oid_arbitrary_hex("00", &oid)) + { + /* remove a value inserted more than once */ + check_int(oidset_insert(st, &oid), ==, 1); + check_int(oidset_remove(st, &oid), ==, 1); + check_int(oidset_size(st), ==, ARRAY_SIZE(hex_input) - 3); + check_int(oidset_contains(st, &oid), ==, 0); + } + + if (!get_oid_arbitrary_hex("22", &oid)) + check_int(oidset_remove(st, &oid), ==, 0); +} + +static int input_contains(struct object_id *oid, char *seen) +{ + for (size_t i = 0; i < ARRAY_SIZE(hex_input); i++) { + struct object_id oid_input; + if (get_oid_arbitrary_hex(hex_input[i], &oid_input)) + return -1; + if (oideq(&oid_input, oid)) { + if (seen[i]) + return 2; + seen[i] = 1; + return 0; + } + } + return 1; +} + +static void t_iterate(struct oidset *st) +{ + struct oidset_iter iter; + struct object_id *oid; + char seen[ARRAY_SIZE(hex_input)] = { 0 }; + int count = 0; + + oidset_iter_init(st, &iter); + while ((oid = oidset_iter_next(&iter))) { + int ret; + if (!check_int((ret = input_contains(oid, seen)), ==, 0)) { + switch (ret) { + case -1: + break; /* handled by get_oid_arbitrary_hex() */ + case 1: + test_msg("obtained object_id was not given in the input\n" + " object_id: %s", oid_to_hex(oid)); + break; + case 2: + test_msg("duplicate object_id detected\n" + " object_id: %s", oid_to_hex(oid)); + break; + } + } else { + count++; + } + } + check_int(count, ==, ARRAY_SIZE(hex_input)); + check_int(oidset_size(st), ==, ARRAY_SIZE(hex_input)); +} + +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; + + 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); +} + +int cmd_main(int argc UNUSED, const char **argv UNUSED) +{ + TEST(setup(t_contains), "contains works"); + TEST(setup(t_insert_dup), "insert an already inserted value works"); + TEST(setup(t_insert_from_set), "insert from one set to another works"); + TEST(setup(t_remove), "remove works"); + TEST(setup(t_iterate), "iteration works"); + TEST(t_parse_file(), "parsing from file works"); + return test_done(); +} diff --git a/t/unit-tests/t-oidset/sha1-oids b/t/unit-tests/t-oidset/sha1-oids new file mode 100644 index 0000000000..881f45e661 --- /dev/null +++ b/t/unit-tests/t-oidset/sha1-oids @@ -0,0 +1,10 @@ +# comments are ignored +0000000000000000000000000000000000000000 +9900000000000000000000000000000000000000 +dd00000000000000000000000000000000000000 + + 4400000000000000000000000000000000000000 + +bb00000000000000000000000000000000000000 # test comment +cc00000000000000000000000000000000000000 +# 1100000000000000000000000000000000000000 diff --git a/t/unit-tests/t-oidset/sha256-oids b/t/unit-tests/t-oidset/sha256-oids new file mode 100644 index 0000000000..3c1c687812 --- /dev/null +++ b/t/unit-tests/t-oidset/sha256-oids @@ -0,0 +1,10 @@ +# comments are ignored +0000000000000000000000000000000000000000000000000000000000000000 +9900000000000000000000000000000000000000000000000000000000000000 +dd00000000000000000000000000000000000000000000000000000000000000 + + 4400000000000000000000000000000000000000000000000000000000000000 + +bb00000000000000000000000000000000000000000000000000000000000000 # test comment +cc00000000000000000000000000000000000000000000000000000000000000 +# 1100000000000000000000000000000000000000000000000000000000000000
Add tests for oidset.h library, which were not previously present using the unit testing framework. This imposes a new restriction of running the test from the 't/' and 't/unit-tests/bin' 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 restriction is similar to the one we already have for end-to-end tests, wherein, we can only run those tests from 't/'. The addition of allowing 't/unit-tests/bin' for allowing to run tests from is for running 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' 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. Thanks. Makefile | 1 + t/unit-tests/lib-oid.c | 2 +- t/unit-tests/lib-oid.h | 1 + t/unit-tests/t-oidset.c | 222 ++++++++++++++++++++++++++++++ t/unit-tests/t-oidset/sha1-oids | 10 ++ t/unit-tests/t-oidset/sha256-oids | 10 ++ 6 files changed, 245 insertions(+), 1 deletion(-) create mode 100644 t/unit-tests/t-oidset.c create mode 100644 t/unit-tests/t-oidset/sha1-oids create mode 100644 t/unit-tests/t-oidset/sha256-oids