[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [RFC PATCH v2] khash_test.c : add unit tests
From:       Glen Choo <chooglen () google ! com>
Date:       2023-06-30 1:05:05
Message-ID: kl6l5y75rc8u.fsf () chooglen-macbookpro ! roam ! corp ! google ! com
[Download RAW message or body]

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@google.com> writes:

> Signed-off-by: Siddharth Singh <siddhartth@google.com>
> ---
> 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 //.


[prev in list] [next in list] [prev in thread] [next in thread]