Git Mailing List Archive mirror
 help / color / mirror / Atom feed
From: Phillip Wood <>
To: Siddharth Singh <>,
Cc: Taylor Blau <>, Junio C Hamano <>
Subject: Re: [RFC PATCH 0/1] Unit tests for khash.h
Date: Thu, 1 Jun 2023 14:23:19 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

Hi Siddharth

On 31/05/2023 16:51, Siddharth Singh wrote:
> This RFC patch adds unit tests for khash.h. It uses the C TAP harness to illustrate the test cases [1]. This is not intended to be a complete implementation. The purpose of this patch to get your thoughts on the unit test content, not the test framework itself.

It is a bit hard to comment on the test content when you say they are 
incomplete so we don't know what you're planning on adding in the 
future. I agree with Taylor and Junio that khash is probably not the 
best place to start adding tests (I've not checked how far away from 
upstream our code has drifted over time) but from a quick glance I found 
these tests rather tame. I'd like to see tests that are trying to break 
things, for example adding keys with hash collisions and checking that 
they can still be retrieved, updated  after some have been deleted. You 
could use a hash function that returns the first character of a string 
and add words starting with the same letter to generate collisions.

It is also hard to separate the tests from the framework as a good test 
framework will make it easy to write tests that have good diagnostic 
messages when they fail that pin-point the line where the failure 
occurred and the values used in the comparison that failed. A good 
framework will also minimize the amount of " if (...) return 0" boiler 
plate required when a comparison fails. The tests here do not benefit 
from such a framework.

I think this patch also raises the question of what should be tested by 
our test-tool helper and what should be tested in unit tests. Ævar 
raised the idea of improving our test-helper tests rather than adding 
unit tests [1]. It would be good to get a response to that point of view.

Finally I would recommend that you chat to your colleagues about the 
expectations around including an explanation of the reasons for a change 
in the commit message and code style for patches posted on this list.

Best Wishes



> The tests cover a wide range of functionality, including the creation and destruction of hash tables, the insertion and deletion of elements and the querying of hash tables. I would appreciate feedback from reviewers on what other tests would be useful for khash.h and if these tests provide enough value.
> [1]
> Siddharth Singh (1):
>    khash_test.c: add unit tests
>   Makefile       |   1 +
>   t/khash_test.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 174 insertions(+)
>   create mode 100644 t/khash_test.c

  parent reply	other threads:[~2023-06-01 13:23 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 [this message]
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

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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* 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).