From: Glen Choo <chooglen@google.com>
To: Siddharth Singh <siddhartth@google.com>, git@vger.kernel.org
Subject: Re: [RFC PATCH v2] khash_test.c : add unit tests
Date: Thu, 29 Jun 2023 18:05:05 -0700 [thread overview]
Message-ID: <kl6l5y75rc8u.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <20230627132026.3204186-1-siddhartth@google.com>
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 parent reply other threads:[~2023-06-30 1:05 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-31 15:51 [RFC PATCH 0/1] Unit tests for khash.h Siddharth Singh
2023-05-31 15:51 ` [RFC PATCH 1/1] khash_test.c: add unit tests Siddharth Singh
2023-05-31 22:35 ` [RFC PATCH 0/1] Unit tests for khash.h Taylor Blau
2023-06-01 6:26 ` Junio C Hamano
2023-06-01 13:02 ` Phillip Wood
2023-05-31 23:35 ` Eric Wong
2023-06-01 13:23 ` Phillip Wood
2023-06-27 13:20 ` [RFC PATCH v2] khash_test.c : add unit tests Siddharth Singh
2023-06-30 0:53 ` Glen Choo
2023-07-07 15:04 ` Siddharth Singh
2023-07-07 21:12 ` Glen Choo
2023-06-30 1:05 ` Glen Choo [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=kl6l5y75rc8u.fsf@chooglen-macbookpro.roam.corp.google.com \
--to=chooglen@google.com \
--cc=git@vger.kernel.org \
--cc=siddhartth@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).