{"thread":{"id":"59814","subject":"[RFC PATCH 0/1] Unit tests for khash.h","startedAt":"2023-05-31T15:52:55Z","lastAt":"2023-07-07T21:13:20Z","messageCount":12,"participants":["Siddharth Singh","Taylor Blau","Eric Wong","Junio C Hamano","Phillip Wood","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"477835","messageId":"20230531155142.3359886-1-siddhartth@google.com","threadId":"59814","inReplyTo":null,"subject":"[RFC PATCH 0/1] Unit tests for khash.h","fromName":"Siddharth Singh","fromEmail":"siddhartth@google.com","sentAt":"2023-05-31T15:51:41Z","receivedAt":"2023-05-31T15:52:55Z","isPatch":true,"sender":{"key":"siddhartth@google.com","avatar":"https://avatars.githubusercontent.com/u/68241942?v=4"},"body":"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.\n\nThe 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.\n\n[1] https://lore.kernel.org/git/20230427175007.902278-1-calvinwan@google.com/T/#me379c06257ebe2847d71b604c031328c7edc3843\n\n\nSiddharth Singh (1):\n  khash_test.c: add unit tests\n\n Makefile       |   1 +\n t/khash_test.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 174 insertions(+)\n create mode 100644 t/khash_test.c\n\n-- \n2.40.1.606.ga4b1b128d6-goog\n\n"},{"id":"477836","messageId":"20230531155142.3359886-2-siddhartth@google.com","threadId":"59814","inReplyTo":"20230531155142.3359886-1-siddhartth@google.com","subject":"[RFC PATCH 1/1] khash_test.c: add unit tests","fromName":"Siddharth Singh","fromEmail":"siddhartth@google.com","sentAt":"2023-05-31T15:51:42Z","receivedAt":"2023-05-31T15:53:00Z","isPatch":true,"sender":{"key":"siddhartth@google.com","avatar":"https://avatars.githubusercontent.com/u/68241942?v=4"},"body":"The tests check if a hashmap\n1. Gets hash value for a string\n2. Has correct value for '__ac_HASH_UPPER'\n3. Initializes succesfully.\n4. Put a key value pair.\n5. Gets the value from key.\n6. Delete the key-value pair.\n7. Updates the value after deleting it.\n8. Update Value of before deleting.\n9. Contains the right number of elements.\n\nSigned-off-by: Siddharth Singh <siddhartth@google.com>\n---\n Makefile       |   1 +\n t/khash_test.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 174 insertions(+)\n create mode 100644 t/khash_test.c\n\ndiff --git a/Makefile b/Makefile\nindex 660c72d72e..59cb76e5e5 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3852,6 +3852,7 @@ $(UNIT_TEST_PROGS): $(CTAP_OBJS) $(LIBS)\n \t$(QUIET)mkdir -p t/unit-tests\n \t$(QUIET_CC)$(CC) -It -o t/unit-tests/unit-test-t t/unit-test.c $(CTAP_OBJS)\n \t$(QUIET_CC)$(CC) -It -o t/unit-tests/strbuf-t t/strbuf-test.c -DSKIP_COMMON_MAIN common-main.c $(CTAP_OBJS) $(LIBS)\n+\t$(QUIET_CC)$(CC) -It -o t/unit-tests/khash-t t/khash_test.c -DSKIP_COMMON_MAIN common-main.c $(CTAP_OBJS) $(LIBS)\n \n .PHONY: unit-tests\n unit-tests: $(UNIT_TEST_PROGS) $(UNIT_TEST_RUNNER)\ndiff --git a/t/khash_test.c b/t/khash_test.c\nnew file mode 100644\nindex 0000000000..cd27618d4d\n--- /dev/null\n+++ b/t/khash_test.c\n@@ -0,0 +1,173 @@\n+#include <tap/basic.h>\n+#include \"../git-compat-util.h\"\n+\n+#include \"../khash.h\"\n+\n+int khash_testACX31HashString(){\n+    const char * str = \"foobar\";\n+    khint_t value = __ac_X31_hash_string(str);\n+    khint_t expected = 3026088333;\n+    if(value == expected)\n+        return 1;\n+    return 0;\n+}\n+\n+int khash_testAC_HASH_UPPER(){\n+    const double expected = 0.77;\n+    if(expected == __ac_HASH_UPPER){\n+        return 1;\n+    }\n+    return 0;\n+}\n+\n+KHASH_INIT(str, const char *, int *, 1, kh_str_hash_func, kh_str_hash_equal);\n+\n+int khash_testInit(){\n+    kh_str_t * hashmap = kh_init_str();\n+    if(hashmap != NULL){\n+        return 1;\n+    }\n+    return 0;\n+}\n+\n+int khash_testPut(){\n+    kh_str_t * hashmap = kh_init_str();\n+    int ret;\n+    int pos = kh_put_str(hashmap,\"test_key\",&ret);\n+    int value = 14;\n+    if(ret){\n+        kh_key(hashmap, pos) = xstrdup(\"test_key\");\n+\t\tkh_value(hashmap, pos) = &value;\n+    }else return 0;\n+    if(*kh_value(hashmap,pos) == value){\n+        return 1;\n+    }\n+    return 0;\n+}\n+\n+int khash_testGet(){\n+    kh_str_t * hashmap = kh_init_str();\n+    int ret;\n+    int pos = kh_put_str(hashmap,\"test_key\",&ret);\n+    int value = 14;\n+    if(ret){\n+        kh_key(hashmap, pos) = xstrdup(\"test_key\");\n+\t\tkh_value(hashmap, pos) = &value;\n+    }else return 0;\n+    int value_pos = kh_get_str(hashmap,\"test_key\");    \n+    int returned_value = *kh_value(hashmap,value_pos);\n+    if(returned_value == value){\n+        return 1;\n+    }\n+    return 0;\n+}\n+\n+int khash_testDelete(){\n+    kh_str_t * hashmap = kh_init_str();\n+    int ret;\n+    int pos = kh_put_str(hashmap,\"test_key\",&ret);\n+    int value = 14;\n+    if(ret){\n+        kh_key(hashmap, pos) = xstrdup(\"test_key\");\n+\t\tkh_value(hashmap, pos) = &value;\n+    }else return 0;\n+    int current_size = hashmap->size;\n+    kh_del_str(hashmap,pos);\n+    if(hashmap->size + 1 == current_size && !kh_exist(hashmap, pos)){\n+        return 1;\n+    }\n+    return 0;\n+}\n+\n+int khash_testUpdateValueAfterDeleting(){\n+    kh_str_t * hashmap = kh_init_str();\n+    int ret;\n+    int pos1 = kh_put_str(hashmap,\"test_key\",&ret);\n+    int value1 = 14;\n+    if(ret){\n+        kh_key(hashmap, pos1) = xstrdup(\"test_key\");\n+\t\tkh_value(hashmap, pos1) = &value1;\n+    }else return 0;\n+\n+    kh_del_str(hashmap,pos1);\n+    int pos2 = kh_put_str(hashmap,\"test_key\",&ret);\n+    int value2 = 15;\n+    \n+    if(ret){\n+        kh_key(hashmap, pos2) = xstrdup(\"test_key\");\n+\t\tkh_value(hashmap, pos2) = &value2;\n+    }else return 0;\n+\n+    int value_pos = kh_get_str(hashmap,\"test_key\");    \n+    int returned_value = *kh_value(hashmap,value_pos);\n+    if(returned_value == value2){\n+        return 1;\n+    }\n+    return 0;\n+}\n+\n+int khash_testUpdateValueBeforeDeleting(){\n+    kh_str_t * hashmap = kh_init_str();\n+    int ret;\n+    int pos1 = kh_put_str(hashmap,\"test_key\",&ret);\n+    int value1 = 14;\n+    if(ret){\n+        kh_key(hashmap, pos1) = xstrdup(\"test_key\");\n+\t\tkh_value(hashmap, pos1) = &value1;\n+    }else return 0;\n+\n+    int pos2 = kh_put_str(hashmap,\"test_key\",&ret);\n+    int value2 = 15;\n+    \n+    if(ret == 0){\n+        return 1;\n+    }\n+    return 0;\n+}\n+\n+int khash_testSize(){\n+    kh_str_t * hashmap = kh_init_str();\n+    int ret;\n+    int pos1 = kh_put_str(hashmap,\"test_key1\",&ret);\n+    int value1 = 14;\n+    if(ret){\n+        kh_key(hashmap, pos1) = xstrdup(\"test_key1\");\n+\t\tkh_value(hashmap, pos1) = &value1;\n+    }else return 0;\n+\n+    int pos2 = kh_put_str(hashmap,\"test_key2\",&ret);\n+    int value2 = 15;\n+    if(ret){\n+        kh_key(hashmap, pos2) = xstrdup(\"test_key2\");\n+\t\tkh_value(hashmap, pos2) = &value2;\n+    }else return 0;\n+\n+\n+    int pos3 = kh_put_str(hashmap,\"test_key3\",&ret);\n+    int value3 = 16;\n+    if(ret){\n+        kh_key(hashmap, pos3) = xstrdup(\"test_key3\");\n+\t\tkh_value(hashmap, pos3) = &value3;\n+    }else return 0;\n+\n+    if(kh_size(hashmap) == 3){\n+        return 1;\n+    }\n+    return 0;\n+}\n+\n+int main(void){\n+    plan(9);\n+    \n+    ok(khash_testACX31HashString(),\"__ac_X31_hash_string works\");\n+    ok(khash_testAC_HASH_UPPER(),\"__ac_HASH_UPPER has correct value\");\n+    ok(khash_testInit(), \"Initialize hashmap\");\n+    ok(khash_testPut(), \"Put Key-Value pair\");\n+    ok(khash_testGet(), \"Get the value from hashmap\");\n+    ok(khash_testDelete(), \"Delete the key-value from hashmap\");\n+    ok(khash_testUpdateValueAfterDeleting(), \"Update Value of a Key after deleting\");\n+    ok(khash_testUpdateValueBeforeDeleting(), \"Update Value of a Key before deleting\");\n+    ok(khash_testSize(),\"Test if 3 elements are present in hashmap\");\n+\n+    return 0;\n+}\n\\ No newline at end of file\n-- \n2.40.1.606.ga4b1b128d6-goog\n\n"},{"id":"477839","messageId":"ZHfLwpX7nNVjBvE5@nand.local","threadId":"59814","inReplyTo":"20230531155142.3359886-1-siddhartth@google.com","subject":"Re: [RFC PATCH 0/1] Unit tests for khash.h","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-31T22:35:46Z","receivedAt":"2023-05-31T22:35:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Siddharth,\n\nOn Wed, May 31, 2023 at 05:51:41PM +0200, Siddharth Singh wrote:\n> This RFC patch adds unit tests for khash.h. It uses the C TAP harness\n> to illustrate the test cases [1]. This is not intended to be a\n> complete implementation. The purpose of this patch to get your\n> thoughts on the unit test content, not the test framework itself.\n\nThanks for working on this, and for opening the discussion up. I took\nonly a brief look through the actual changes. But I think the much more\ninteresting discussion is on the approach, so I'll refrain from\ncommenting on the tests themselves.\n\nI am somewhat skeptical of this as a productive direction, primarily\nbecause khash already has tests [1] that exercise its functionality. I'm\nnot necessarily opposed to (light) testing of oid_set, oid_map, and\noid_pos, which are khash structures, but declared by Git.\n\nEven still, I don't think that we are testing much in that case, since\nthe boundary between Git and khash is limited to the KHASH_INIT macro.\n\nSo... I dunno. I'm not strongly opposed here, but I think this is\nprobably not the most productive place to start adding tests.\n\nThanks,\nTaylor\n\n[1]: https://github.com/attractivechaos/klib/blob/master/test/khash_test.c\n"},{"id":"477845","messageId":"20230531233526.M226883@dcvr","threadId":"59814","inReplyTo":"20230531155142.3359886-1-siddhartth@google.com","subject":"Re: [RFC PATCH 0/1] Unit tests for khash.h","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2023-05-31T23:35:26Z","receivedAt":"2023-05-31T23:40:54Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Siddharth Singh <siddhartth@google.com> wrote:\n> This RFC patch adds unit tests for khash.h.\n\nI agree with what Taylor said about being skeptical of these\ntests being productive.\n\nI'm curious about switching parts of git to use AC's newer `khashl'.\nkhashl is smaller than the original khash but slower for deletes.\nSince git hardly deletes and khash memory use is high (especially on\npacking); it should be a benefit all around.\n\nAlso, see Documentation/CodingGuidelines for coding style.\n"},{"id":"477856","messageId":"xmqqr0qvbsvp.fsf@gitster.g","threadId":"59814","inReplyTo":"ZHfLwpX7nNVjBvE5@nand.local","subject":"Re: [RFC PATCH 0/1] Unit tests for khash.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-01T06:26:18Z","receivedAt":"2023-06-01T06:27:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Hi Siddharth,\n>\n> On Wed, May 31, 2023 at 05:51:41PM +0200, Siddharth Singh wrote:\n>> This RFC patch adds unit tests for khash.h. It uses the C TAP harness\n>> to illustrate the test cases [1]. This is not intended to be a\n>> complete implementation. The purpose of this patch to get your\n>> thoughts on the unit test content, not the test framework itself.\n>\n> Thanks for working on this, and for opening the discussion up. I took\n> only a brief look through the actual changes. But I think the much more\n> interesting discussion is on the approach, so I'll refrain from\n> commenting on the tests themselves.\n>\n> I am somewhat skeptical of this as a productive direction, primarily\n> because khash already has tests [1] that exercise its functionality. I'm\n> not necessarily opposed to (light) testing of oid_set, oid_map, and\n> oid_pos, which are khash structures, but declared by Git.\n\nYes, as a fairly isolated and supposedly well tested piece of code,\nusing khash as a guinea pig for the technology demonstration of C\nTAP harness they propose to use would be an excellent choice, and I\nfound the request not to talk about the harness quite odd.  It felt\nalmost pointless.\n\nThe \"main()\" did illustrate a few things people found problematic\nwith the harness, among which having to declare plan(9) upfront.  An\nobvious disregard of the coding guidelines did not help well,\neither.  Overall, I am not impressed.\n\nThanks.\n"},{"id":"477874","messageId":"35bd6c86-66e8-13c8-4a8d-5822953fedb1@gmail.com","threadId":"59814","inReplyTo":"ZHfLwpX7nNVjBvE5@nand.local","subject":"Re: [RFC PATCH 0/1] Unit tests for khash.h","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-06-01T13:02:54Z","receivedAt":"2023-06-01T13:03:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Taylor\n\nOn 31/05/2023 23:35, Taylor Blau wrote:\n> Hi Siddharth,\n> \n> On Wed, May 31, 2023 at 05:51:41PM +0200, Siddharth Singh wrote:\n>> This RFC patch adds unit tests for khash.h. It uses the C TAP harness\n>> to illustrate the test cases [1]. This is not intended to be a\n>> complete implementation. The purpose of this patch to get your\n>> thoughts on the unit test content, not the test framework itself.\n> \n> Thanks for working on this, and for opening the discussion up. I took\n> only a brief look through the actual changes. But I think the much more\n> interesting discussion is on the approach, so I'll refrain from\n> commenting on the tests themselves.\n> \n> I am somewhat skeptical of this as a productive direction, primarily\n> because khash already has tests [1] that exercise its functionality. I'm\n> not necessarily opposed to (light) testing of oid_set, oid_map, and\n> oid_pos, which are khash structures, but declared by Git.\n\nI agree that starting by adding tests for third party code is a bit \ncurious, but those tests you linked to look like they're testing \nperformance rather than correctness.\n\nBest Wishes\n\nPhillip\n\n> Even still, I don't think that we are testing much in that case, since\n> the boundary between Git and khash is limited to the KHASH_INIT macro.\n> \n> So... I dunno. I'm not strongly opposed here, but I think this is\n> probably not the most productive place to start adding tests.\n> \n> Thanks,\n> Taylor\n> \n> [1]: https://github.com/attractivechaos/klib/blob/master/test/khash_test.c\n\n"},{"id":"477875","messageId":"30c735ea-cd8d-d82f-a5dc-da2bd591f76e@gmail.com","threadId":"59814","inReplyTo":"20230531155142.3359886-1-siddhartth@google.com","subject":"Re: [RFC PATCH 0/1] Unit tests for khash.h","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-06-01T13:23:19Z","receivedAt":"2023-06-01T13:23:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Siddharth\n\nOn 31/05/2023 16:51, Siddharth Singh wrote:\n> 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.\n\nIt is a bit hard to comment on the test content when you say they are \nincomplete so we don't know what you're planning on adding in the \nfuture. I agree with Taylor and Junio that khash is probably not the \nbest place to start adding tests (I've not checked how far away from \nupstream our code has drifted over time) but from a quick glance I found \nthese tests rather tame. I'd like to see tests that are trying to break \nthings, for example adding keys with hash collisions and checking that \nthey can still be retrieved, updated  after some have been deleted. You \ncould use a hash function that returns the first character of a string \nand add words starting with the same letter to generate collisions.\n\nIt is also hard to separate the tests from the framework as a good test \nframework will make it easy to write tests that have good diagnostic \nmessages when they fail that pin-point the line where the failure \noccurred and the values used in the comparison that failed. A good \nframework will also minimize the amount of \" if (...) return 0\" boiler \nplate required when a comparison fails. The tests here do not benefit \nfrom such a framework.\n\nI think this patch also raises the question of what should be tested by \nour test-tool helper and what should be tested in unit tests. Ævar \nraised the idea of improving our test-helper tests rather than adding \nunit tests [1]. It would be good to get a response to that point of view.\n\nFinally I would recommend that you chat to your colleagues about the \nexpectations around including an explanation of the reasons for a change \nin the commit message and code style for patches posted on this list.\n\nBest Wishes\n\nPhillip\n\n[1] https://lore.kernel.org/git/230502.861qjyj0cb.gmgdl@evledraar.gmail.com\n\n> 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.\n> \n> [1] https://lore.kernel.org/git/20230427175007.902278-1-calvinwan@google.com/T/#me379c06257ebe2847d71b604c031328c7edc3843\n> \n> \n> Siddharth Singh (1):\n>    khash_test.c: add unit tests\n> \n>   Makefile       |   1 +\n>   t/khash_test.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++++\n>   2 files changed, 174 insertions(+)\n>   create mode 100644 t/khash_test.c\n> \n\n"},{"id":"478881","messageId":"20230627132026.3204186-1-siddhartth@google.com","threadId":"59814","inReplyTo":"20230531155142.3359886-1-siddhartth@google.com","subject":"[RFC PATCH v2] khash_test.c : add unit tests","fromName":"Siddharth Singh","fromEmail":"siddhartth@google.com","sentAt":"2023-06-27T13:20:26Z","receivedAt":"2023-06-27T13:21:58Z","isPatch":true,"sender":{"key":"siddhartth@google.com","avatar":"https://avatars.githubusercontent.com/u/68241942?v=4"},"body":"Signed-off-by: Siddharth Singh <siddhartth@google.com>\n---\nSince v1\n- rewrote the code keeping coding style guidelines in mind.\n- refactored tests to improve their implementation..\n- added a few more tests that cause collisions in the hashmap.\nIn 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. \nE.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.\nWhile 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.\nI'm also very new to writing unit tests in C and git codebase in general, so I'll appreciate constructive feedback and discussion..\nWith this RFC, I hope you can help me answer these questions.\nWhat are the unit test cases to include in khash.h?\nWhat are the other types of tests that would be useful for khash.h?\n[1] https://github.com/attractivechaos/klib/blob/master/test/khash_test.c\n[2] https://github.com/attractivechaos/klib/blob/master/khash.h\n[3] https://github.com/git/git/blob/master/khash.h\n\n Makefile       |   2 +\n t/khash_test.c | 214 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 216 insertions(+)\n create mode 100644 t/khash_test.c\n\ndiff --git a/Makefile b/Makefile\nindex 660c72d72e..d3ad2fa23c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3847,11 +3847,13 @@ $(UNIT_TEST_RUNNER): t/runtests.c\n \n UNIT_TEST_PROGS += t/unit-tests/unit-test-t\n UNIT_TEST_PROGS += t/unit-tests/strbuf-t\n+UNIT_TEST_PROGS += t/unit-tests/khash-t\n \n $(UNIT_TEST_PROGS): $(CTAP_OBJS) $(LIBS)\n \t$(QUIET)mkdir -p t/unit-tests\n \t$(QUIET_CC)$(CC) -It -o t/unit-tests/unit-test-t t/unit-test.c $(CTAP_OBJS)\n \t$(QUIET_CC)$(CC) -It -o t/unit-tests/strbuf-t t/strbuf-test.c -DSKIP_COMMON_MAIN common-main.c $(CTAP_OBJS) $(LIBS)\n+\t$(QUIET_CC)$(CC) -It -o t/unit-tests/khash-t t/khash_test.c -DSKIP_COMMON_MAIN common-main.c $(CTAP_OBJS) $(LIBS)\n \n .PHONY: unit-tests\n unit-tests: $(UNIT_TEST_PROGS) $(UNIT_TEST_RUNNER)\ndiff --git a/t/khash_test.c b/t/khash_test.c\nnew file mode 100644\nindex 0000000000..d1a43add13\n--- /dev/null\n+++ b/t/khash_test.c\n@@ -0,0 +1,214 @@\n+#include \"../git-compat-util.h\"\n+#include \"../khash.h\"\n+#include <tap/basic.h>\n+\n+/*\n+  These tests are for base assumptions; if they fails, library is broken\n+*/\n+int hash_string_succeeds() {\n+  const char *str = \"test_string\";\n+  khint_t value = __ac_X31_hash_string(str);\n+  return value == 0xf1a6305e;\n+}\n+\n+int hash_string_with_non_alphabets_succeeds() {\n+  const char *str = \"test_string #2\";\n+  khint_t value = __ac_X31_hash_string(str);\n+  return value == 0xfa970771;\n+}\n+\n+KHASH_INIT(str, const char *, int *, 1, kh_str_hash_func, kh_str_hash_equal);\n+\n+int initialized_hashmap_pointer_not_null() {\n+  kh_str_t *hashmap = kh_init_str();\n+\n+  if(hashmap != NULL){\n+    free(hashmap);\n+    return 1;\n+  }\n+  return 0;\n+}\n+\n+int put_key_into_hashmap_once_succeeds() {\n+  int ret, value;\n+  khint_t pos;\n+  kh_str_t *hashmap = kh_init_str();\n+  pos = kh_put_str(hashmap, \"test_key\", &ret);\n+  if(!ret)\n+    return 0;\n+  return pos != 0;\n+}\n+\n+int put_same_key_into_hashmap_twice_fails() {\n+  int first_return_value, second_return_value, value;\n+  khint_t pos;\n+  kh_str_t *hashmap = kh_init_str();\n+  kh_put_str(hashmap, \"test_key\", &first_return_value);\n+  kh_put_str(hashmap, \"test_key\", &second_return_value);\n+  return !second_return_value;\n+}\n+\n+int put_value_into_hashmap_once_succeeds() {\n+  int ret, value = 14;\n+  khint_t pos;\n+  kh_str_t *hashmap = kh_init_str();\n+  pos = kh_put_str(hashmap, \"test_key\", &ret);\n+  if (!ret)\n+    return 0;\n+  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n+  kh_value(hashmap, pos) = &value;\n+  return *kh_value(hashmap, pos) == value;\n+}\n+\n+int put_same_value_into_hashmap_twice_fails() {\n+  int first_return_value, second_return_value, value = 14;\n+  khint_t pos;\n+  kh_str_t *hashmap = kh_init_str();\n+  pos = kh_put_str(hashmap, \"test_key\", &first_return_value);\n+  if (!first_return_value)\n+    return 0;\n+  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n+  kh_value(hashmap, pos) = &value;\n+  kh_put_str(hashmap, \"test_key\", &second_return_value);\n+  return !second_return_value;\n+}\n+\n+int get_existing_kv_from_hashmap_succeeds() {\n+  int ret, value =14;\n+  int expected = value;\n+  khint_t pos;\n+  kh_str_t *hashmap = kh_init_str();\n+  pos = kh_put_str(hashmap, \"test_key\", &ret);\n+  if (!ret)\n+    return 0;\n+  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n+  kh_value(hashmap, pos) = &value;\n+  return *kh_value(hashmap, kh_get_str(hashmap, \"test_key\")) == expected;\n+}\n+\n+int get_nonexisting_kv_from_hashmap_fails() {\n+  int value = 14;\n+  khint_t pos;\n+  kh_str_t *hashmap = kh_init_str();\n+  return !kh_get_str(hashmap, \"test_key\");\n+}\n+\n+int deletion_from_hashmap_sets_flag() {\n+  int ret, value = 14;\n+  khint_t pos;\n+  kh_str_t *hashmap = kh_init_str();\n+  pos = kh_put_str(hashmap, \"test_key\", &ret);\n+  if (!ret)\n+    return 0;\n+  if(!kh_exist(hashmap, pos))\n+    return 0;\n+  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n+  kh_value(hashmap, pos) = &value;\n+  kh_del_str(hashmap, pos);\n+  return !kh_exist(hashmap, pos);\n+}\n+\n+int deletion_from_hashmap_updates_size() {\n+  int ret, value = 14, current_size;\n+  khint_t pos;\n+  kh_str_t *hashmap = kh_init_str();\n+  pos = kh_put_str(hashmap, \"test_key\", &ret);\n+  if (!ret)\n+    return 0;\n+  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n+  kh_value(hashmap, pos) = &value;\n+  current_size = hashmap->size;\n+  kh_del_str(hashmap, pos);\n+  return hashmap->size + 1 == current_size;\n+}\n+\n+int deletion_from_hashmap_does_not_update_kv() {\n+  int ret, value = 14, current_size;\n+  khint_t pos;\n+  kh_str_t *hashmap = kh_init_str();\n+  pos = kh_put_str(hashmap, \"test_key\", &ret);\n+  if (!ret)\n+    return 0;\n+  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n+  kh_value(hashmap, pos) = &value;\n+  kh_del_str(hashmap, pos);\n+  return !strcmp(kh_key(hashmap, pos),\"test_key\");\n+}\n+\n+int update_value_after_deletion_succeeds() {\n+  int ret, value1 = 14, value2 = 15;\n+  khint_t pos1, pos2;\n+  kh_str_t *hashmap = kh_init_str();\n+  // adding the kv to the hashmap\n+  pos1 = kh_put_str(hashmap, \"test_key\", &ret);\n+  if (!ret)\n+    return 0;\n+  kh_key(hashmap, pos1) = xstrdup(\"test_key\");\n+  kh_value(hashmap, pos1) = &value1;\n+  // delete the kv from the hashmap\n+  kh_del_str(hashmap, pos1);\n+  // adding the same key with different value\n+  pos2 = kh_put_str(hashmap, \"test_key\", &ret);\n+  if (!ret)\n+    return 0;\n+  kh_key(hashmap, pos2) = xstrdup(\"test_key\");\n+  kh_value(hashmap, pos2) = &value2;\n+  // check if the value is different\n+  return *kh_value(hashmap, kh_get_str(hashmap, \"test_key\")) == value2;\n+}\n+\n+int hashmap_size_matches_number_of_added_elements() {\n+  int ret, value1 = 14, value2 = 15, value3 = 16;\n+  khint_t pos1, pos2, pos3;\n+  kh_str_t *hashmap = kh_init_str();\n+  pos1 = kh_put_str(hashmap, \"test_key1\", &ret);\n+  if (!ret)\n+    return 0;\n+  kh_key(hashmap, pos1) = xstrdup(\"test_key1\");\n+  kh_value(hashmap, pos1) = &value1;\n+  pos2 = kh_put_str(hashmap, \"test_key2\", &ret);\n+  if (!ret)\n+    return 0;\n+  kh_key(hashmap, pos2) = xstrdup(\"test_key2\");\n+  kh_value(hashmap, pos2) = &value2;\n+  pos3 = kh_put_str(hashmap, \"test_key3\", &ret);\n+  if (!ret)\n+    return 0;\n+  kh_key(hashmap, pos3) = xstrdup(\"test_key3\");\n+  kh_value(hashmap, pos3) = &value3;\n+  return kh_size(hashmap) == 3;\n+}\n+\n+int main(void) {\n+  plan(14);\n+\n+  ok(hash_string_succeeds(), \"X31 hash value of a string is correct\");\n+  ok(hash_string_with_non_alphabets_succeeds(),\n+     \"Get X31 hash value for a string with non-alphabets in it.\");\n+  ok(initialized_hashmap_pointer_not_null(),\n+     \"Successfully initialize a hashmap and it's not NULL\");\n+  ok(put_key_into_hashmap_once_succeeds(),\n+     \"Put a key into hashmap that doesn't exist.\");\n+  ok(put_same_key_into_hashmap_twice_fails(),\n+     \"Put a key into hashmap twice that causes collision.\");\n+  ok(put_value_into_hashmap_once_succeeds(),\n+     \"Put a unique key-value pair into hashmap.\");\n+  ok(put_same_value_into_hashmap_twice_fails(),\n+     \"Put a key-value pair into hashmap twice that causes collision.\");\n+  ok(get_existing_kv_from_hashmap_succeeds(),\n+     \"Get the value from a key that exists in hashmap\");\n+  ok(get_nonexisting_kv_from_hashmap_fails(),\n+     \"Get the value from a key that doesn't exist in hashmap\");\n+  ok(deletion_from_hashmap_sets_flag(),\n+     \"Delete the key-value from hashmap and the flag is set to false\");\n+  ok(deletion_from_hashmap_updates_size(),\n+     \"Delete the key-value from hashmap and the size changes\");\n+  ok(deletion_from_hashmap_does_not_update_kv(),\n+     \"Delete the key-value from hashmap but it can be fetched using iterator\");\n+  ok(update_value_after_deletion_succeeds(),\n+     \"Updates the value of a key after deleting it\");\n+  ok(hashmap_size_matches_number_of_added_elements(),\n+     \"size of hashmap is correct after adding 3 elements\");\n+\n+  return 0;\n+}\n-- \n2.40.GIT\n\n"},{"id":"479038","messageId":"kl6l8rc1rcsj.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59814","inReplyTo":"20230627132026.3204186-1-siddhartth@google.com","subject":"Re: [RFC PATCH v2] khash_test.c : add unit tests","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-06-30T00:53:16Z","receivedAt":"2023-06-30T00:53:22Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Siddharth Singh <siddhartth@google.com> writes:\n\n> - rewrote the code keeping coding style guidelines in mind.\n\nI noticed several style issues in the patch. I'll avoid commenting on\nthose here so that we can focus on the other, big questions.\n\n> I'm also very new to writing unit tests in C and git codebase in general, so I'll appreciate constructive feedback and discussion..\n\nOkay. I think this is a good chance for the ML to decide on what sorts\nof unit test practices we want too.\n\n> What are the unit test cases to include in khash.h?\n> What are the other types of tests that would be useful for khash.h?\n\nFor a third party library, I mostly agree with upthread commenters that\ntesting the correctness of the _library_ code isn't that valuable on its\nown, but unit tests are also useful for demonstrating how the code\nshould be used. In a lot of projects, unit tests are a pseudo-official\ndemonstration of how an interface should be used, and that's sometimes\nbetter than seeing the code in production, because unit tests are\ntypically a lot simpler and easier to reason about.\n\nAs an aside, this is something that a unit test framework will do _much\nbetter_ than test-tool, because it makes it easy to read and write these\nidtiomatic demos without worrying about how the test-tool executable\nwill be used.\n\nI think that if we consider these two factors together (correctness and\ndeveloper education), the fact that we don't have great documentation of\nkhash.h in-tree, and that the external docs aren't that comprehensive\neither, I think it would be useful to exercise each of the khash\n'functions' to show how they work.\n\nAs Taylor mentioned upthread, I think the most valuable thing to test is\nthe way our code uses khash.h (not the library code itself). IOW:\n\n  KHASH_INIT(oid_set, struct object_id, int, 0, oidhash_by_value, oideq_by_value)\n  KHASH_INIT(oid_map, struct object_id, void *, 1, oidhash_by_value, oideq_by_value)\n  KHASH_INIT(oid_pos, struct object_id, int, 1, oidhash_by_value, oideq_by_value)\n\nSo we would focus on oidhash_by_value and oideq_by_value in particular,\nsince that's provided by us. It would be very useful to verify that\nthese functions are doing exactly what we think they should be doing,\nand we could do this with _every_ khash function to really explore the\nAPI surface.\n\nIn fact... I suspect that they _not_ doing what we think they should,\nor at the very least, might be displaying some very unintuitive\nbehavior. object_id contains both the hash value _and_ the hash\nalgorithm that generated it. If we have the same hash value, but\ngenerated by different algorithms, are they considered the same equal or\nnot? To me, the intuitive answer is \"no\", but oidhash_by_value() only\nconsiders the hash value and ignores the algorithm altogether! I think\nthat testing collisions between SHA1 and SHA256 values would be very\nuseful.\n\n> +/*\n> +  These tests are for base assumptions; if they fails, library is broken\n> +*/\n> +int hash_string_succeeds() {\n> +  const char *str = \"test_string\";\n> +  khint_t value = __ac_X31_hash_string(str);\n> +  return value == 0xf1a6305e;\n> +}\n> +\n> +int hash_string_with_non_alphabets_succeeds() {\n> +  const char *str = \"test_string #2\";\n> +  khint_t value = __ac_X31_hash_string(str);\n> +  return value == 0xfa970771;\n> +}\n\nFor a unit test, it is a best practice to test the external interface\nthat callers are depending on, and to avoid implementation\ndetails/internals. If we do the latter, we end up with brittle tests\nthat break on non-important changes. E.g. we definitely should not be\ntesting the exact value of the hash function. The khash library\nmaintainer could decide to change the hash function, and we should not\nbe broken by that.\n\nFor khash.h, it appears that that anything prepended with \"__\" is meant\nto be internal, and everything else is external. E.g.\n__ac_X31_hash_string is certainly not meant to be used directly by end\nusers., especially since there's a human-friendly macro for it:\n\n  #define kh_str_hash_func(key) __ac_X31_hash_string(key)\n\nHowever, because something is external, doesn't mean we need to be\ntesting its implementation - we still shouldn't be testing the\nreturn value of kh_str_hash_func(). We may want to test certain\n_properties_ of the external function (e.g. does it provide good\ncollision resistance?), but in this case, I think we can trust that what\nthe library author wrote is a good default.\n\n> +KHASH_INIT(str, const char *, int *, 1, kh_str_hash_func, kh_str_hash_equal);\n\nAs mentioned earlier, I think testing oid_[set|map|pos] would be much\nmore useful, and we should try to demonstrate each of the\nmacros/functions and how they interact, e.g. if I kh_put_*, I should\nexpect to be able to kh_get_* afterwards.\n\n> +\n> +int initialized_hashmap_pointer_not_null() {\n> +  kh_str_t *hashmap = kh_init_str();\n> +\n> +  if(hashmap != NULL){\n> +    free(hashmap);\n> +    return 1;\n> +  }\n> +  return 0;\n> +}\n\nI don't think this is a useful test. It would be extremely weird for the\nreturn value to be NULL in regular usage.\n\n> +int put_key_into_hashmap_once_succeeds() {\n> +  int ret, value;\n> +  khint_t pos;\n> +  kh_str_t *hashmap = kh_init_str();\n> +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n> +  if(!ret)\n> +    return 0;\n> +  return pos != 0;\n> +}\n\nI don't think it makes sense to only check the return value, we should\ncheck that it had the intended effect - that we can read back the value.\nThere is a later test that does this, so I'd prefer we remove this.\n\nComment on the test framework, not the test: I think it makes sense to\nassert that \"ret\" is nonzero, but this \"if..return\" way of asserting\nmakes it impossible to tell what has broken. It also adds a lot of\ncognitive overhead when trying to parse the assertion. I'll echo Phillip\nWood's comments and say that I think this is definitely not a framework\nI want to use.\n\n> +int put_same_key_into_hashmap_twice_fails() {\n> +  int first_return_value, second_return_value, value;\n> +  khint_t pos;\n> +  kh_str_t *hashmap = kh_init_str();\n> +  kh_put_str(hashmap, \"test_key\", &first_return_value);\n> +  kh_put_str(hashmap, \"test_key\", &second_return_value);\n> +  return !second_return_value;\n> +}\n\nThis test seems reasonable.\n\n> +int put_value_into_hashmap_once_succeeds() {\n> +  int ret, value = 14;\n> +  khint_t pos;\n> +  kh_str_t *hashmap = kh_init_str();\n> +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n> +  if (!ret)\n> +    return 0;\n> +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n> +  kh_value(hashmap, pos) = &value;\n> +  return *kh_value(hashmap, pos) == value;\n> +}\n\nChecking the inserted value makes sense to me I don't really understand\nkhash.h, but is this the recommended way to get values back? kh_value()\nlooks like just a small macro around an internal array, so we are just\nchecking that *&value == value. I would have expected that we would call\nkh_get to get \"pos\", then call kh_value on that.\n\nAlso, do we have to kh_put_str(\"some_key\") and then immediately set it\nagain with kh_key(pos)? That seems odd, and I don't see other callers\ndoing that all the time.\n\nSince this is the second time I see \"if (!ret)\", I should ask whether\nthis is meant as an assertion (checking the correctness of the code), or\nfor flow control (the code might return either 0 or 1, and I need to\nhandle both). If it is the former, I think it's unnecessary to\nassert on it multiple times. The latter does not belong in a unit test -\nwe should already _know_ what we want to be returned. Btw, it is a huge\nflaw in the test framework that both look exactly the same, so I'll say\nagain that I really don't like it.\n\n> +int put_same_value_into_hashmap_twice_fails() {\n> +  int first_return_value, second_return_value, value = 14;\n> +  khint_t pos;\n> +  kh_str_t *hashmap = kh_init_str();\n> +  pos = kh_put_str(hashmap, \"test_key\", &first_return_value);\n> +  if (!first_return_value)\n> +    return 0;\n> +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n> +  kh_value(hashmap, pos) = &value;\n> +  kh_put_str(hashmap, \"test_key\", &second_return_value);\n> +  return !second_return_value;\n> +}\n\nI don't see how this is different from the previous test that tested for\ncollisions.\n\n> +int get_existing_kv_from_hashmap_succeeds() {\n> +  int ret, value =14;\n> +  int expected = value;\n> +  khint_t pos;\n> +  kh_str_t *hashmap = kh_init_str();\n> +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n> +  if (!ret)\n> +    return 0;\n> +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n> +  kh_value(hashmap, pos) = &value;\n> +  return *kh_value(hashmap, kh_get_str(hashmap, \"test_key\")) == expected;\n> +}\n\nIsn't this identical to put_value_into_hashmap_once_succeeds()?\n\nAlso, this assertion is harder to read than necessary because we have\nboth \"expected\" and \"value\" - the different names suggest that they are\ndifferent, but they're really the same. Let's just use one of them.\n\n> +int get_nonexisting_kv_from_hashmap_fails() {\n> +  int value = 14;\n> +  khint_t pos;\n> +  kh_str_t *hashmap = kh_init_str();\n> +  return !kh_get_str(hashmap, \"test_key\");\n> +}\n\nLooks reasonable.\n\n> +int deletion_from_hashmap_sets_flag() {\n> +  int ret, value = 14;\n> +  khint_t pos;\n> +  kh_str_t *hashmap = kh_init_str();\n> +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n> +  if (!ret)\n> +    return 0;\n> +  if(!kh_exist(hashmap, pos))\n> +    return 0;\n> +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n> +  kh_value(hashmap, pos) = &value;\n> +  kh_del_str(hashmap, pos);\n> +  return !kh_exist(hashmap, pos);\n> +}\n\nWhat is the \"flag\" being set? If it is the khash-internal flag that\nmarks that an entry is deleted, that's an unimportant implementation\ndetail - the important thing is that the value is deleted. Let's just\nrename this to \"deletion_works\" or something.\n\n> +\n> +int deletion_from_hashmap_updates_size() {\n> +  int ret, value = 14, current_size;\n> +  khint_t pos;\n> +  kh_str_t *hashmap = kh_init_str();\n> +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n> +  if (!ret)\n> +    return 0;\n> +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n> +  kh_value(hashmap, pos) = &value;\n> +  current_size = hashmap->size;\n> +  kh_del_str(hashmap, pos);\n> +  return hashmap->size + 1 == current_size;\n> +}\n\nThe interface is kh_size(), so let's use that instead of referencing the\n.size member correctly.\n\n> +int deletion_from_hashmap_does_not_update_kv() {\n> +  int ret, value = 14, current_size;\n> +  khint_t pos;\n> +  kh_str_t *hashmap = kh_init_str();\n> +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n> +  if (!ret)\n> +    return 0;\n> +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n> +  kh_value(hashmap, pos) = &value;\n> +  kh_del_str(hashmap, pos);\n> +  return !strcmp(kh_key(hashmap, pos),\"test_key\");\n> +}\n\nInteresting! If we kh_del_(pos) but read it back with kh_key(), we can\nstill read it back. This has some implications on how khash should be\nused - we should be extremely careful if we pass around a returned value\nfrom kh_get_ or kh_put_ because we don't know if it's deleted or not.\nInstead, we should prefer kh_get_(). Very useful demonstration.\n\n> +int update_value_after_deletion_succeeds() {\n> +  int ret, value1 = 14, value2 = 15;\n> +  khint_t pos1, pos2;\n> +  kh_str_t *hashmap = kh_init_str();\n> +  // adding the kv to the hashmap\n> +  pos1 = kh_put_str(hashmap, \"test_key\", &ret);\n> +  if (!ret)\n> +    return 0;\n> +  kh_key(hashmap, pos1) = xstrdup(\"test_key\");\n> +  kh_value(hashmap, pos1) = &value1;\n> +  // delete the kv from the hashmap\n> +  kh_del_str(hashmap, pos1);\n> +  // adding the same key with different value\n> +  pos2 = kh_put_str(hashmap, \"test_key\", &ret);\n> +  if (!ret)\n> +    return 0;\n> +  kh_key(hashmap, pos2) = xstrdup(\"test_key\");\n> +  kh_value(hashmap, pos2) = &value2;\n> +  // check if the value is different\n> +  return *kh_value(hashmap, kh_get_str(hashmap, \"test_key\")) == value2;\n> +}\n\nLooks reasonable.\n\n> +int hashmap_size_matches_number_of_added_elements() {\n> +  int ret, value1 = 14, value2 = 15, value3 = 16;\n> +  khint_t pos1, pos2, pos3;\n> +  kh_str_t *hashmap = kh_init_str();\n> +  pos1 = kh_put_str(hashmap, \"test_key1\", &ret);\n> +  if (!ret)\n> +    return 0;\n> +  kh_key(hashmap, pos1) = xstrdup(\"test_key1\");\n> +  kh_value(hashmap, pos1) = &value1;\n> +  pos2 = kh_put_str(hashmap, \"test_key2\", &ret);\n> +  if (!ret)\n> +    return 0;\n> +  kh_key(hashmap, pos2) = xstrdup(\"test_key2\");\n> +  kh_value(hashmap, pos2) = &value2;\n> +  pos3 = kh_put_str(hashmap, \"test_key3\", &ret);\n> +  if (!ret)\n> +    return 0;\n> +  kh_key(hashmap, pos3) = xstrdup(\"test_key3\");\n> +  kh_value(hashmap, pos3) = &value3;\n> +  return kh_size(hashmap) == 3;\n> +}\n\nLooks reasonable.\n\nI would also exercise kh_end, kh_begin, kh_clear, kh_foreach and\nkh_foreach_value\n\nkh_destroy might be useful to test too, in case there are oddities\nthere, but it's probably not that interesting\n\nkh_n_buckets doesn't seem useful to test - it looks too specific to the\nimpkementation. kh_resize looks like an implementation detail that we\nshouldn't test either.\n"},{"id":"479039","messageId":"kl6l5y75rc8u.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59814","inReplyTo":"20230627132026.3204186-1-siddhartth@google.com","subject":"Re: [RFC PATCH v2] khash_test.c : add unit tests","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-06-30T01:05:05Z","receivedAt":"2023-06-30T01:05:22Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Some comments on mechanical issues...\n\nIt seems that you dropped the other participants from your reply.\n\n  https://git-scm.com/docs/MyFirstContribution#v2-git-send-email\n\nDo CC them as it helps them keep track of discussions they were in. I\nthink they will not mind if you send a new message to this thread, CC\nthem, and explain that you didn't mean to omit them.\n\nSiddharth Singh <siddhartth@google.com> writes:\n\n> Signed-off-by: Siddharth Singh <siddhartth@google.com>\n> ---\n> Since v1\n> - rewrote the code keeping coding style guidelines in mind.\n> - refactored tests to improve their implementation..\n> - added a few more tests that cause collisions in the hashmap.\n> 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. \n> 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.\n> 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.\n> I'm also very new to writing unit tests in C and git codebase in general, so I'll appreciate constructive feedback and discussion..\n\nDo rewrap the line to 80 characters, otherwise it is quite hard to read\non most text-based clients. You can preview your patches with \"git\nsend-email --annotate\" and see if they are a reasonable column width.\n\n> diff --git a/Makefile b/Makefile\n> index 660c72d72e..d3ad2fa23c 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3847,11 +3847,13 @@ $(UNIT_TEST_RUNNER): t/runtests.c\n>  \n>  UNIT_TEST_PROGS += t/unit-tests/unit-test-t\n>  UNIT_TEST_PROGS += t/unit-tests/strbuf-t\n> +UNIT_TEST_PROGS += t/unit-tests/khash-t\n>  \n>  $(UNIT_TEST_PROGS): $(CTAP_OBJS) $(LIBS)\n>  \t$(QUIET)mkdir -p t/unit-tests\n>  \t$(QUIET_CC)$(CC) -It -o t/unit-tests/unit-test-t t/unit-test.c $(CTAP_OBJS)\n>  \t$(QUIET_CC)$(CC) -It -o t/unit-tests/strbuf-t t/strbuf-test.c -DSKIP_COMMON_MAIN common-main.c $(CTAP_OBJS) $(LIBS)\n\nMany reviewers like to apply the patches for review. It's typically\nassumed that patches are based on 'master', but this looks like it is\nnot. That's okay, though you should call it out. If it is based on\nsomething not in Junio's tree (and thus a reviewer can't apply your\npatch), it would be appreciated if you share this commit via a Git fork,\ne.g. GitHub.\n\n> +/*\n> +  These tests are for base assumptions; if they fails, library is broken\n\nMissing *.\n\n> +int hash_string_succeeds() {\n> +  const char *str = \"test_string\";\n\nWe use tabs, not spaces. I think \"make style\" catches this.\n\n> +int initialized_hashmap_pointer_not_null() {\n> +  kh_str_t *hashmap = kh_init_str();\n> +\n> +  if(hashmap != NULL){\n\n\"if \", not \"if\". \"make style\" will catch this.\n\n> +int update_value_after_deletion_succeeds() {\n> +  int ret, value1 = 14, value2 = 15;\n> +  khint_t pos1, pos2;\n> +  kh_str_t *hashmap = kh_init_str();\n> +  // adding the kv to the hashmap\n\nComments use /* */, not //.\n"},{"id":"479278","messageId":"ZKgpgnU7WrMCTp/C@google.com","threadId":"59814","inReplyTo":"kl6l8rc1rcsj.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [RFC PATCH v2] khash_test.c : add unit tests","fromName":"Siddharth Singh","fromEmail":"siddhartth@google.com","sentAt":"2023-07-07T15:04:34Z","receivedAt":"2023-07-07T15:04:46Z","isPatch":true,"sender":{"key":"siddhartth@google.com","avatar":"https://avatars.githubusercontent.com/u/68241942?v=4"},"body":"On Thu, Jun 29, 2023 at 05:53:16PM -0700, Glen Choo wrote:\n> Siddharth Singh <siddhartth@google.com> writes:\n> \n> > - rewrote the code keeping coding style guidelines in mind.\n> \n> I noticed several style issues in the patch. I'll avoid commenting on\n> those here so that we can focus on the other, big questions.\n> \n> > I'm also very new to writing unit tests in C and git codebase in general, so I'll appreciate constructive feedback and discussion..\n> \n> Okay. I think this is a good chance for the ML to decide on what sorts\n> of unit test practices we want too.\n> \n> > What are the unit test cases to include in khash.h?\n> > What are the other types of tests that would be useful for khash.h?\n> \n> For a third party library, I mostly agree with upthread commenters that\n> testing the correctness of the _library_ code isn't that valuable on its\n> own, but unit tests are also useful for demonstrating how the code\n> should be used. In a lot of projects, unit tests are a pseudo-official\n> demonstration of how an interface should be used, and that's sometimes\n> better than seeing the code in production, because unit tests are\n> typically a lot simpler and easier to reason about.\n> \n> As an aside, this is something that a unit test framework will do _much\n> better_ than test-tool, because it makes it easy to read and write these\n> idtiomatic demos without worrying about how the test-tool executable\n> will be used.\n> \n> I think that if we consider these two factors together (correctness and\n> developer education), the fact that we don't have great documentation of\n> khash.h in-tree, and that the external docs aren't that comprehensive\n> either, I think it would be useful to exercise each of the khash\n> 'functions' to show how they work.\n> \n> As Taylor mentioned upthread, I think the most valuable thing to test is\n> the way our code uses khash.h (not the library code itself). IOW:\n> \n>   KHASH_INIT(oid_set, struct object_id, int, 0, oidhash_by_value, oideq_by_value)\n>   KHASH_INIT(oid_map, struct object_id, void *, 1, oidhash_by_value, oideq_by_value)\n>   KHASH_INIT(oid_pos, struct object_id, int, 1, oidhash_by_value, oideq_by_value)\n> \n> So we would focus on oidhash_by_value and oideq_by_value in particular,\n> since that's provided by us. It would be very useful to verify that\n> these functions are doing exactly what we think they should be doing,\n> and we could do this with _every_ khash function to really explore the\n> API surface.\n> \n> In fact... I suspect that they _not_ doing what we think they should,\n> or at the very least, might be displaying some very unintuitive\n> behavior. object_id contains both the hash value _and_ the hash\n> algorithm that generated it. If we have the same hash value, but\n> generated by different algorithms, are they considered the same equal or\n> not? To me, the intuitive answer is \"no\", but oidhash_by_value() only\n> considers the hash value and ignores the algorithm altogether! I think\n> that testing collisions between SHA1 and SHA256 values would be very\n> useful.\n> \n> > +/*\n> > +  These tests are for base assumptions; if they fails, library is broken\n> > +*/\n> > +int hash_string_succeeds() {\n> > +  const char *str = \"test_string\";\n> > +  khint_t value = __ac_X31_hash_string(str);\n> > +  return value == 0xf1a6305e;\n> > +}\n> > +\n> > +int hash_string_with_non_alphabets_succeeds() {\n> > +  const char *str = \"test_string #2\";\n> > +  khint_t value = __ac_X31_hash_string(str);\n> > +  return value == 0xfa970771;\n> > +}\n> \n> For a unit test, it is a best practice to test the external interface\n> that callers are depending on, and to avoid implementation\n> details/internals. If we do the latter, we end up with brittle tests\n> that break on non-important changes. E.g. we definitely should not be\n> testing the exact value of the hash function. The khash library\n> maintainer could decide to change the hash function, and we should not\n> be broken by that.\n> \n> For khash.h, it appears that that anything prepended with \"__\" is meant\n> to be internal, and everything else is external. E.g.\n> __ac_X31_hash_string is certainly not meant to be used directly by end\n> users., especially since there's a human-friendly macro for it:\n> \n>   #define kh_str_hash_func(key) __ac_X31_hash_string(key)\n> \n> However, because something is external, doesn't mean we need to be\n> testing its implementation - we still shouldn't be testing the\n> return value of kh_str_hash_func(). We may want to test certain\n> _properties_ of the external function (e.g. does it provide good\n> collision resistance?), but in this case, I think we can trust that what\n> the library author wrote is a good default.\n> \n> > +KHASH_INIT(str, const char *, int *, 1, kh_str_hash_func, kh_str_hash_equal);\n> \n> As mentioned earlier, I think testing oid_[set|map|pos] would be much\n> more useful, and we should try to demonstrate each of the\n> macros/functions and how they interact, e.g. if I kh_put_*, I should\n> expect to be able to kh_get_* afterwards.\n> \n> > +\n> > +int initialized_hashmap_pointer_not_null() {\n> > +  kh_str_t *hashmap = kh_init_str();\n> > +\n> > +  if(hashmap != NULL){\n> > +    free(hashmap);\n> > +    return 1;\n> > +  }\n> > +  return 0;\n> > +}\n> \n> I don't think this is a useful test. It would be extremely weird for the\n> return value to be NULL in regular usage.\n\nWhile it is indeed uncommon for the return value of kh_init_str() to be null in\nregular usage, there could be certain cases where the hashmap pointer might be\nnull. One possible scenario is if there is an error during memory allocation for\nthe hashmap. This can happen if the system runs out of memory or if there are\nother memory-related issues. In such cases, the kh_init_str() function might\nreturn a null pointer to indicate the failure to allocate memory for the hashmap.\n\n>\n> > +int put_key_into_hashmap_once_succeeds() {\n> > +  int ret, value;\n> > +  khint_t pos;\n> > +  kh_str_t *hashmap = kh_init_str();\n> > +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n> > +  if(!ret)\n> > +    return 0;\n> > +  return pos != 0;\n> > +}\n> \n> I don't think it makes sense to only check the return value, we should\n> check that it had the intended effect - that we can read back the value.\n> There is a later test that does this, so I'd prefer we remove this.\n> \n> Comment on the test framework, not the test: I think it makes sense to\n> assert that \"ret\" is nonzero, but this \"if..return\" way of asserting\n> makes it impossible to tell what has broken. It also adds a lot of\n> cognitive overhead when trying to parse the assertion. I'll echo Phillip\n> Wood's comments and say that I think this is definitely not a framework\n> I want to use.\n> \n> > +int put_same_key_into_hashmap_twice_fails() {\n> > +  int first_return_value, second_return_value, value;\n> > +  khint_t pos;\n> > +  kh_str_t *hashmap = kh_init_str();\n> > +  kh_put_str(hashmap, \"test_key\", &first_return_value);\n> > +  kh_put_str(hashmap, \"test_key\", &second_return_value);\n> > +  return !second_return_value;\n> > +}\n> \n> This test seems reasonable.\n> \n> > +int put_value_into_hashmap_once_succeeds() {\n> > +  int ret, value = 14;\n> > +  khint_t pos;\n> > +  kh_str_t *hashmap = kh_init_str();\n> > +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n> > +  if (!ret)\n> > +    return 0;\n> > +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n> > +  kh_value(hashmap, pos) = &value;\n> > +  return *kh_value(hashmap, pos) == value;\n> > +}\n> \n> Checking the inserted value makes sense to me I don't really understand\n> khash.h, but is this the recommended way to get values back? kh_value()\n> looks like just a small macro around an internal array, so we are just\n> checking that *&value == value. I would have expected that we would call\n> kh_get to get \"pos\", then call kh_value on that.\n> \n> Also, do we have to kh_put_str(\"some_key\") and then immediately set it\n> again with kh_key(pos)? That seems odd, and I don't see other callers\n> doing that all the time.\n> \n> Since this is the second time I see \"if (!ret)\", I should ask whether\n> this is meant as an assertion (checking the correctness of the code), or\n> for flow control (the code might return either 0 or 1, and I need to\n> handle both). If it is the former, I think it's unnecessary to\n> assert on it multiple times. The latter does not belong in a unit test -\n> we should already _know_ what we want to be returned. Btw, it is a huge\n> flaw in the test framework that both look exactly the same, so I'll say\n> again that I really don't like it.\n> \n\nRegarding the usage of kh_value(), I saw this part of code[1] This suggests\nthat it may be the recommended approach in this context.\n\nFor kh_put_str, if i understand correctly, we first allocate a bucket then\nthen set it using kh_key, here's a use case[2] that does it similarly.\n\nRegarding the usage of if (!ret), it seems to serve as an assertion or error\nchecking mechanism. If the return value ret is 0, it likely indicates an\nerror condition. It's worth mentioning that the similarity between the\nassertion and flow control statements could be a flaw in the test framework,\nas you pointed out.\n\n> > +int put_same_value_into_hashmap_twice_fails() {\n> > +  int first_return_value, second_return_value, value = 14;\n> > +  khint_t pos;\n> > +  kh_str_t *hashmap = kh_init_str();\n> > +  pos = kh_put_str(hashmap, \"test_key\", &first_return_value);\n> > +  if (!first_return_value)\n> > +    return 0;\n> > +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n> > +  kh_value(hashmap, pos) = &value;\n> > +  kh_put_str(hashmap, \"test_key\", &second_return_value);\n> > +  return !second_return_value;\n> > +}\n> \n> I don't see how this is different from the previous test that tested for\n> collisions.\n\nI also attempt to assign a value to the hashmap here, but this will not be\nsuccessful due to a collision. The intent of this test was different, however\n \n> > +int get_existing_kv_from_hashmap_succeeds() {\n> > +  int ret, value =14;\n> > +  int expected = value;\n> > +  khint_t pos;\n> > +  kh_str_t *hashmap = kh_init_str();\n> > +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n> > +  if (!ret)\n> > +    return 0;\n> > +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n> > +  kh_value(hashmap, pos) = &value;\n> > +  return *kh_value(hashmap, kh_get_str(hashmap, \"test_key\")) == expected;\n> > +}\n> \n> Isn't this identical to put_value_into_hashmap_once_succeeds()?\n> \n> Also, this assertion is harder to read than necessary because we have\n> both \"expected\" and \"value\" - the different names suggest that they are\n> different, but they're really the same. Let's just use one of them.\n> \n\nI agree they both end up same implementation wise.\n\n> > +int get_nonexisting_kv_from_hashmap_fails() {\n> > +  int value = 14;\n> > +  khint_t pos;\n> > +  kh_str_t *hashmap = kh_init_str();\n> > +  return !kh_get_str(hashmap, \"test_key\");\n> > +}\n> \n> Looks reasonable.\n> \n> > +int deletion_from_hashmap_sets_flag() {\n> > +  int ret, value = 14;\n> > +  khint_t pos;\n> > +  kh_str_t *hashmap = kh_init_str();\n> > +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n> > +  if (!ret)\n> > +    return 0;\n> > +  if(!kh_exist(hashmap, pos))\n> > +    return 0;\n> > +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n> > +  kh_value(hashmap, pos) = &value;\n> > +  kh_del_str(hashmap, pos);\n> > +  return !kh_exist(hashmap, pos);\n> > +}\n> \n> What is the \"flag\" being set? If it is the khash-internal flag that\n> marks that an entry is deleted, that's an unimportant implementation\n> detail - the important thing is that the value is deleted. Let's just\n> rename this to \"deletion_works\" or something.\n> \nOka, sounds good.\n> > +\n> > +int deletion_from_hashmap_updates_size() {\n> > +  int ret, value = 14, current_size;\n> > +  khint_t pos;\n> > +  kh_str_t *hashmap = kh_init_str();\n> > +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n> > +  if (!ret)\n> > +    return 0;\n> > +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n> > +  kh_value(hashmap, pos) = &value;\n> > +  current_size = hashmap->size;\n> > +  kh_del_str(hashmap, pos);\n> > +  return hashmap->size + 1 == current_size;\n> > +}\n> \n> The interface is kh_size(), so let's use that instead of referencing the\n> .size member correctly.\n\nOkay.\n\n> > +int deletion_from_hashmap_does_not_update_kv() {\n> > +  int ret, value = 14, current_size;\n> > +  khint_t pos;\n> > +  kh_str_t *hashmap = kh_init_str();\n> > +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n> > +  if (!ret)\n> > +    return 0;\n> > +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n> > +  kh_value(hashmap, pos) = &value;\n> > +  kh_del_str(hashmap, pos);\n> > +  return !strcmp(kh_key(hashmap, pos),\"test_key\");\n> > +}\n> \n> Interesting! If we kh_del_(pos) but read it back with kh_key(), we can\n> still read it back. This has some implications on how khash should be\n> used - we should be extremely careful if we pass around a returned value\n> from kh_get_ or kh_put_ because we don't know if it's deleted or not.\n> Instead, we should prefer kh_get_(). Very useful demonstration.\n> \nYes, we are still able to get the value but to truly know if something\nwas deleted or not, we need to use kh_exist.\n\n> > +int update_value_after_deletion_succeeds() {\n> > +  int ret, value1 = 14, value2 = 15;\n> > +  khint_t pos1, pos2;\n> > +  kh_str_t *hashmap = kh_init_str();\n> > +  // adding the kv to the hashmap\n> > +  pos1 = kh_put_str(hashmap, \"test_key\", &ret);\n> > +  if (!ret)\n> > +    return 0;\n> > +  kh_key(hashmap, pos1) = xstrdup(\"test_key\");\n> > +  kh_value(hashmap, pos1) = &value1;\n> > +  // delete the kv from the hashmap\n> > +  kh_del_str(hashmap, pos1);\n> > +  // adding the same key with different value\n> > +  pos2 = kh_put_str(hashmap, \"test_key\", &ret);\n> > +  if (!ret)\n> > +    return 0;\n> > +  kh_key(hashmap, pos2) = xstrdup(\"test_key\");\n> > +  kh_value(hashmap, pos2) = &value2;\n> > +  // check if the value is different\n> > +  return *kh_value(hashmap, kh_get_str(hashmap, \"test_key\")) == value2;\n> > +}\n> \n> Looks reasonable.\n> \n> > +int hashmap_size_matches_number_of_added_elements() {\n> > +  int ret, value1 = 14, value2 = 15, value3 = 16;\n> > +  khint_t pos1, pos2, pos3;\n> > +  kh_str_t *hashmap = kh_init_str();\n> > +  pos1 = kh_put_str(hashmap, \"test_key1\", &ret);\n> > +  if (!ret)\n> > +    return 0;\n> > +  kh_key(hashmap, pos1) = xstrdup(\"test_key1\");\n> > +  kh_value(hashmap, pos1) = &value1;\n> > +  pos2 = kh_put_str(hashmap, \"test_key2\", &ret);\n> > +  if (!ret)\n> > +    return 0;\n> > +  kh_key(hashmap, pos2) = xstrdup(\"test_key2\");\n> > +  kh_value(hashmap, pos2) = &value2;\n> > +  pos3 = kh_put_str(hashmap, \"test_key3\", &ret);\n> > +  if (!ret)\n> > +    return 0;\n> > +  kh_key(hashmap, pos3) = xstrdup(\"test_key3\");\n> > +  kh_value(hashmap, pos3) = &value3;\n> > +  return kh_size(hashmap) == 3;\n> > +}\n> \n> Looks reasonable.\n> \n> I would also exercise kh_end, kh_begin, kh_clear, kh_foreach and\n> kh_foreach_value\n> \n> kh_destroy might be useful to test too, in case there are oddities\n> there, but it's probably not that interesting\n> \n> kh_n_buckets doesn't seem useful to test - it looks too specific to the\n> impkementation. kh_resize looks like an implementation detail that we\n> shouldn't test either.\n>\n\n"},{"id":"479295","messageId":"kl6lv8evpgs0.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59814","inReplyTo":"ZKgpgnU7WrMCTp/C@google.com","subject":"Re: [RFC PATCH v2] khash_test.c : add unit tests","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-07-07T21:12:47Z","receivedAt":"2023-07-07T21:13:20Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Siddharth Singh <siddhartth@google.com> writes:\n\n>> > +int initialized_hashmap_pointer_not_null() {\n>> > +  kh_str_t *hashmap = kh_init_str();\n>> > +\n>> > +  if(hashmap != NULL){\n>> > +    free(hashmap);\n>> > +    return 1;\n>> > +  }\n>> > +  return 0;\n>> > +}\n>> \n>> I don't think this is a useful test. It would be extremely weird for the\n>> return value to be NULL in regular usage.\n>\n> While it is indeed uncommon for the return value of kh_init_str() to be null in\n> regular usage, there could be certain cases where the hashmap pointer might be\n> null. One possible scenario is if there is an error during memory allocation for\n> the hashmap. This can happen if the system runs out of memory or if there are\n> other memory-related issues. In such cases, the kh_init_str() function might\n> return a null pointer to indicate the failure to allocate memory for the hashmap.\n\nAh, yes, that is what I expected. By \"regular usage\", I meant the case\nwhere the allocation succeeds. Nevertheless, I don't think this test\ndemonstrates or asserts anything that isn't easily observed in other\ntests, though I am open to be proven wrong.\n\n>> > +int put_value_into_hashmap_once_succeeds() {\n>> > +  int ret, value = 14;\n>> > +  khint_t pos;\n>> > +  kh_str_t *hashmap = kh_init_str();\n>> > +  pos = kh_put_str(hashmap, \"test_key\", &ret);\n>> > +  if (!ret)\n>> > +    return 0;\n>> > +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n>> > +  kh_value(hashmap, pos) = &value;\n>> > +  return *kh_value(hashmap, pos) == value;\n>> > +}\n>> Also, do we have to kh_put_str(\"some_key\") and then immediately set it\n>> again with kh_key(pos)? That seems odd, and I don't see other callers\n>> doing that all the time.\n>\n> Regarding the usage of kh_value(), I saw this part of code[1] This suggests\n> that it may be the recommended approach in this context.\n\nWhat 'context' were you thinking of when you say \"recommended approach\nin this context\"? (Did you mean kh_put_str() vs kh_put_oid_*()?) I\ndidn't see such an approach in most kh_put_*() call sites. I couldn't\nfind the link you're referencing, but I assume it is the following lines\nin delta-islands.c:\n\n\tint hash_ret;\n\tkhiter_t pos = kh_put_str(remote_islands, island_name, &hash_ret);\n\n\tif (hash_ret) {\n\t\tkh_key(remote_islands, pos) = xstrdup(island_name);\n\t\tkh_value(remote_islands, pos) = xcalloc(1, sizeof(struct remote_island));\n\t}\n\nIn which case, the use case (as I understand it at least) seems quite\ndifferent:\n\n- When kh_put_str() 'returns' hash_ret = 0, it means there's already a\n  matching entry and we should do nothing.\n\n- Otherwise, kh_put_str() actually creates a new matching entry, and now\n  we want to allocate memory to store the entry, so we overwrite the key\n  with \"kh_key() = xstrdup()\". We could have avoided this by doing\n  something like:\n\n    char *key = xstrdup(island_name)\n    khiter_t pos = kh_put_str(remote_islands, key, &hash_ret);\n\n    if (hash_ret) {\n      kh_value(remote_islands, pos) = xcalloc(1, sizeof(struct remote_island));\n    } else {\n      free(key);\n    }\n\n  but allocations are expensive, so we should avoid allocating before we\n  know it's needed.\n\n>> > +int put_same_value_into_hashmap_twice_fails() {\n>> > +  int first_return_value, second_return_value, value = 14;\n>> > +  khint_t pos;\n>> > +  kh_str_t *hashmap = kh_init_str();\n>> > +  pos = kh_put_str(hashmap, \"test_key\", &first_return_value);\n>> > +  if (!first_return_value)\n>> > +    return 0;\n>> > +  kh_key(hashmap, pos) = xstrdup(\"test_key\");\n>> > +  kh_value(hashmap, pos) = &value;\n>> > +  kh_put_str(hashmap, \"test_key\", &second_return_value);\n>> > +  return !second_return_value;\n>> > +}\n>> \n>> I don't see how this is different from the previous test that tested for\n>> collisions.\n>\n> I also attempt to assign a value to the hashmap here, but this will not be\n> successful due to a collision. The intent of this test was different, however\n\nHm, what was the different intent? Is it to test this slightly different\ncase where the value is assigned?\n\nMy reasoning is:\n\n- Would assigning a value result in different behavior?\n- Would a reasonable user expect that it would result in different\n  behavior?\n\nIf the answer to both of those is 'no' (which I believe it is), then it\nis not worth testing.\n"}]}