Some comments on mechanical issues... It seems that you dropped the other participants from your reply. https://git-scm.com/docs/MyFirstContribution#v2-git-send-email Do CC them as it helps them keep track of discussions they were in. I think they will not mind if you send a new message to this thread, CC them, and explain that you didn't mean to omit them. Siddharth Singh <siddhartth@xxxxxxxxxx> writes: > Signed-off-by: Siddharth Singh <siddhartth@xxxxxxxxxx> > --- > Since v1 > - rewrote the code keeping coding style guidelines in mind. > - refactored tests to improve their implementation.. > - added a few more tests that cause collisions in the hashmap. > In the v1 patch, there were concerns that new tests were unnecessary because of upstream tests. However, I believe it would be beneficial to have tests based on the khash implementation in the git codebase because the existing tests[1] for khash do not use the same code for khash as the git codebase. > E.g. in khash.h file of “attractivechaos/klib/khash.h” [2]. There's an implementation for `KHASH_MAP_INIT_INT64` which isn’t defined in “git/git/khash.h”[3]. So, there’s a possibility that the khash.h in a different repository might move forward and end up being different from out implementation, so writing tests for “git/git/khash.h” would ensure that our tests are relevant to the actual usage of the library. > While my colleagues are currently investigating whether C Tap harness is the best way to test libified code, this RFC is intended to discuss the content of unit tests and what other tests would be useful for khash.h. I am unsure of what tests will be valuable in the future, so I would appreciate your thoughts on the matter. Many test cases are easier to construct in C TAP harness than in test-tool. For example, In C TAP harness, non-string values can be used directly in hash maps. However, in test-tool, non-string values must be passed as an argument to a shell executable, parsed into the correct type, and then operated on. > I'm also very new to writing unit tests in C and git codebase in general, so I'll appreciate constructive feedback and discussion.. Do rewrap the line to 80 characters, otherwise it is quite hard to read on most text-based clients. You can preview your patches with "git send-email --annotate" and see if they are a reasonable column width. > diff --git a/Makefile b/Makefile > index 660c72d72e..d3ad2fa23c 100644 > --- a/Makefile > +++ b/Makefile > @@ -3847,11 +3847,13 @@ $(UNIT_TEST_RUNNER): t/runtests.c > > UNIT_TEST_PROGS += t/unit-tests/unit-test-t > UNIT_TEST_PROGS += t/unit-tests/strbuf-t > +UNIT_TEST_PROGS += t/unit-tests/khash-t > > $(UNIT_TEST_PROGS): $(CTAP_OBJS) $(LIBS) > $(QUIET)mkdir -p t/unit-tests > $(QUIET_CC)$(CC) -It -o t/unit-tests/unit-test-t t/unit-test.c $(CTAP_OBJS) > $(QUIET_CC)$(CC) -It -o t/unit-tests/strbuf-t t/strbuf-test.c -DSKIP_COMMON_MAIN common-main.c $(CTAP_OBJS) $(LIBS) Many reviewers like to apply the patches for review. It's typically assumed that patches are based on 'master', but this looks like it is not. That's okay, though you should call it out. If it is based on something not in Junio's tree (and thus a reviewer can't apply your patch), it would be appreciated if you share this commit via a Git fork, e.g. GitHub. > +/* > + These tests are for base assumptions; if they fails, library is broken Missing *. > +int hash_string_succeeds() { > + const char *str = "test_string"; We use tabs, not spaces. I think "make style" catches this. > +int initialized_hashmap_pointer_not_null() { > + kh_str_t *hashmap = kh_init_str(); > + > + if(hashmap != NULL){ "if ", not "if". "make style" will catch this. > +int update_value_after_deletion_succeeds() { > + int ret, value1 = 14, value2 = 15; > + khint_t pos1, pos2; > + kh_str_t *hashmap = kh_init_str(); > + // adding the kv to the hashmap Comments use /* */, not //.
