| Message ID | 20230531155142.3359886-1-siddhartth@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Unit tests for khash.h | expand |
Hi Siddharth, On Wed, May 31, 2023 at 05:51:41PM +0200, 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. Thanks for working on this, and for opening the discussion up. I took only a brief look through the actual changes. But I think the much more interesting discussion is on the approach, so I'll refrain from commenting on the tests themselves. I am somewhat skeptical of this as a productive direction, primarily because khash already has tests [1] that exercise its functionality. I'm not necessarily opposed to (light) testing of oid_set, oid_map, and oid_pos, which are khash structures, but declared by Git. Even still, I don't think that we are testing much in that case, since the boundary between Git and khash is limited to the KHASH_INIT macro. So... I dunno. I'm not strongly opposed here, but I think this is probably not the most productive place to start adding tests. Thanks, Taylor [1]: https://github.com/attractivechaos/klib/blob/master/test/khash_test.c
Siddharth Singh <siddhartth@google.com> wrote:
> This RFC patch adds unit tests for khash.h.
I agree with what Taylor said about being skeptical of these
tests being productive.
I'm curious about switching parts of git to use AC's newer `khashl'.
khashl is smaller than the original khash but slower for deletes.
Since git hardly deletes and khash memory use is high (especially on
packing); it should be a benefit all around.
Also, see Documentation/CodingGuidelines for coding style.
Taylor Blau <me@ttaylorr.com> writes: > Hi Siddharth, > > On Wed, May 31, 2023 at 05:51:41PM +0200, 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. > > Thanks for working on this, and for opening the discussion up. I took > only a brief look through the actual changes. But I think the much more > interesting discussion is on the approach, so I'll refrain from > commenting on the tests themselves. > > I am somewhat skeptical of this as a productive direction, primarily > because khash already has tests [1] that exercise its functionality. I'm > not necessarily opposed to (light) testing of oid_set, oid_map, and > oid_pos, which are khash structures, but declared by Git. Yes, as a fairly isolated and supposedly well tested piece of code, using khash as a guinea pig for the technology demonstration of C TAP harness they propose to use would be an excellent choice, and I found the request not to talk about the harness quite odd. It felt almost pointless. The "main()" did illustrate a few things people found problematic with the harness, among which having to declare plan(9) upfront. An obvious disregard of the coding guidelines did not help well, either. Overall, I am not impressed. Thanks.
Hi Taylor On 31/05/2023 23:35, Taylor Blau wrote: > Hi Siddharth, > > On Wed, May 31, 2023 at 05:51:41PM +0200, 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. > > Thanks for working on this, and for opening the discussion up. I took > only a brief look through the actual changes. But I think the much more > interesting discussion is on the approach, so I'll refrain from > commenting on the tests themselves. > > I am somewhat skeptical of this as a productive direction, primarily > because khash already has tests [1] that exercise its functionality. I'm > not necessarily opposed to (light) testing of oid_set, oid_map, and > oid_pos, which are khash structures, but declared by Git. I agree that starting by adding tests for third party code is a bit curious, but those tests you linked to look like they're testing performance rather than correctness. Best Wishes Phillip > Even still, I don't think that we are testing much in that case, since > the boundary between Git and khash is limited to the KHASH_INIT macro. > > So... I dunno. I'm not strongly opposed here, but I think this is > probably not the most productive place to start adding tests. > > Thanks, > Taylor > > [1]: https://github.com/attractivechaos/klib/blob/master/test/khash_test.c
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 Phillip [1] https://lore.kernel.org/git/230502.861qjyj0cb.gmgdl@evledraar.gmail.com > 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] https://lore.kernel.org/git/20230427175007.902278-1-calvinwan@google.com/T/#me379c06257ebe2847d71b604c031328c7edc3843 > > > 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 >