{"thread":{"id":"61556","subject":"[GSoC][PATCH] t/: migrate helper/test-example-decorate to the unit testing framework","startedAt":"2024-05-28T12:59:13Z","lastAt":"2024-06-03T21:09:06Z","messageCount":8,"participants":["Ghanshyam Thakkar","Junio C Hamano","Christian Couder","Josh Steadmon"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"495749","messageId":"20240528125837.31090-1-shyamthakkar001@gmail.com","threadId":"61556","inReplyTo":null,"subject":"[GSoC][PATCH] t/: migrate helper/test-example-decorate to the unit testing framework","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-05-28T12:58:25Z","receivedAt":"2024-05-28T12:59:13Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"helper/test-example-decorate.c along with t9004-example.sh provide\nan example of how to use the functions in decorate.h (which provides\na data structure that associates Git objects to void pointers) and\nalso test their output.\n\nMigrate them to the new unit testing framework for better debugging\nand runtime performance.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>\n---\n Makefile                          |  2 +-\n decorate.h                        |  2 +-\n t/helper/test-example-decorate.c  | 78 ------------------------------\n t/helper/test-tool.c              |  1 -\n t/helper/test-tool.h              |  1 -\n t/t9004-example.sh                | 12 -----\n t/unit-tests/t-example-decorate.c | 80 +++++++++++++++++++++++++++++++\n 7 files changed, 82 insertions(+), 94 deletions(-)\n delete mode 100644 t/helper/test-example-decorate.c\n delete mode 100755 t/t9004-example.sh\n create mode 100644 t/unit-tests/t-example-decorate.c\n\ndiff --git a/Makefile b/Makefile\nindex 8f4432ae57..43663fe528 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -793,7 +793,6 @@ TEST_BUILTINS_OBJS += test-dump-fsmonitor.o\n TEST_BUILTINS_OBJS += test-dump-split-index.o\n TEST_BUILTINS_OBJS += test-dump-untracked-cache.o\n TEST_BUILTINS_OBJS += test-env-helper.o\n-TEST_BUILTINS_OBJS += test-example-decorate.o\n TEST_BUILTINS_OBJS += test-example-tap.o\n TEST_BUILTINS_OBJS += test-find-pack.o\n TEST_BUILTINS_OBJS += test-fsmonitor-client.o\n@@ -1335,6 +1334,7 @@ THIRD_PARTY_SOURCES += sha1collisiondetection/%\n THIRD_PARTY_SOURCES += sha1dc/%\n \n UNIT_TEST_PROGRAMS += t-ctype\n+UNIT_TEST_PROGRAMS += t-example-decorate\n UNIT_TEST_PROGRAMS += t-mem-pool\n UNIT_TEST_PROGRAMS += t-prio-queue\n UNIT_TEST_PROGRAMS += t-strbuf\ndiff --git a/decorate.h b/decorate.h\nindex cdeb17c9df..08af658d34 100644\n--- a/decorate.h\n+++ b/decorate.h\n@@ -3,7 +3,7 @@\n \n /*\n  * A data structure that associates Git objects to void pointers. See\n- * t/helper/test-example-decorate.c for a demonstration of how to use these\n+ * t/unit-tests/t-example-decorate.c for a demonstration of how to use these\n  * functions.\n  */\n \ndiff --git a/t/helper/test-example-decorate.c b/t/helper/test-example-decorate.c\ndeleted file mode 100644\nindex 8f59f6be4c..0000000000\n--- a/t/helper/test-example-decorate.c\n+++ /dev/null\n@@ -1,78 +0,0 @@\n-#include \"test-tool.h\"\n-#include \"git-compat-util.h\"\n-#include \"object.h\"\n-#include \"decorate.h\"\n-#include \"repository.h\"\n-\n-int cmd__example_decorate(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tstruct decoration n;\n-\tstruct object_id one_oid = { {1} };\n-\tstruct object_id two_oid = { {2} };\n-\tstruct object_id three_oid = { {3} };\n-\tstruct object *one, *two, *three;\n-\n-\tint decoration_a, decoration_b;\n-\n-\tvoid *ret;\n-\n-\tint i, objects_noticed = 0;\n-\n-\t/*\n-\t * The struct must be zero-initialized.\n-\t */\n-\tmemset(&n, 0, sizeof(n));\n-\n-\t/*\n-\t * Add 2 objects, one with a non-NULL decoration and one with a NULL\n-\t * decoration.\n-\t */\n-\tone = lookup_unknown_object(the_repository, &one_oid);\n-\ttwo = lookup_unknown_object(the_repository, &two_oid);\n-\tret = add_decoration(&n, one, &decoration_a);\n-\tif (ret)\n-\t\tBUG(\"when adding a brand-new object, NULL should be returned\");\n-\tret = add_decoration(&n, two, NULL);\n-\tif (ret)\n-\t\tBUG(\"when adding a brand-new object, NULL should be returned\");\n-\n-\t/*\n-\t * When re-adding an already existing object, the old decoration is\n-\t * returned.\n-\t */\n-\tret = add_decoration(&n, one, NULL);\n-\tif (ret != &decoration_a)\n-\t\tBUG(\"when readding an already existing object, existing decoration should be returned\");\n-\tret = add_decoration(&n, two, &decoration_b);\n-\tif (ret)\n-\t\tBUG(\"when readding an already existing object, existing decoration should be returned\");\n-\n-\t/*\n-\t * Lookup returns the added declarations, or NULL if the object was\n-\t * never added.\n-\t */\n-\tret = lookup_decoration(&n, one);\n-\tif (ret)\n-\t\tBUG(\"lookup should return added declaration\");\n-\tret = lookup_decoration(&n, two);\n-\tif (ret != &decoration_b)\n-\t\tBUG(\"lookup should return added declaration\");\n-\tthree = lookup_unknown_object(the_repository, &three_oid);\n-\tret = lookup_decoration(&n, three);\n-\tif (ret)\n-\t\tBUG(\"lookup for unknown object should return NULL\");\n-\n-\t/*\n-\t * The user can also loop through all entries.\n-\t */\n-\tfor (i = 0; i < n.size; i++) {\n-\t\tif (n.entries[i].base)\n-\t\t\tobjects_noticed++;\n-\t}\n-\tif (objects_noticed != 2)\n-\t\tBUG(\"should have 2 objects\");\n-\n-\tclear_decoration(&n, NULL);\n-\n-\treturn 0;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex f6fd0fe491..2d82515f56 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -29,7 +29,6 @@ static struct test_cmd cmds[] = {\n \t{ \"dump-split-index\", cmd__dump_split_index },\n \t{ \"dump-untracked-cache\", cmd__dump_untracked_cache },\n \t{ \"env-helper\", cmd__env_helper },\n-\t{ \"example-decorate\", cmd__example_decorate },\n \t{ \"example-tap\", cmd__example_tap },\n \t{ \"find-pack\", cmd__find_pack },\n \t{ \"fsmonitor-client\", cmd__fsmonitor_client },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 868f33453c..bc334183c3 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -23,7 +23,6 @@ int cmd__dump_split_index(int argc, const char **argv);\n int cmd__dump_untracked_cache(int argc, const char **argv);\n int cmd__dump_reftable(int argc, const char **argv);\n int cmd__env_helper(int argc, const char **argv);\n-int cmd__example_decorate(int argc, const char **argv);\n int cmd__example_tap(int argc, const char **argv);\n int cmd__find_pack(int argc, const char **argv);\n int cmd__fsmonitor_client(int argc, const char **argv);\ndiff --git a/t/t9004-example.sh b/t/t9004-example.sh\ndeleted file mode 100755\nindex 590aab0304..0000000000\n--- a/t/t9004-example.sh\n+++ /dev/null\n@@ -1,12 +0,0 @@\n-#!/bin/sh\n-\n-test_description='check that example code compiles and runs'\n-\n-TEST_PASSES_SANITIZE_LEAK=true\n-. ./test-lib.sh\n-\n-test_expect_success 'decorate' '\n-\ttest-tool example-decorate\n-'\n-\n-test_done\ndiff --git a/t/unit-tests/t-example-decorate.c b/t/unit-tests/t-example-decorate.c\nnew file mode 100644\nindex 0000000000..3c856a8cf2\n--- /dev/null\n+++ b/t/unit-tests/t-example-decorate.c\n@@ -0,0 +1,80 @@\n+#include \"test-lib.h\"\n+#include \"object.h\"\n+#include \"decorate.h\"\n+#include \"repository.h\"\n+\n+struct test_vars {\n+\tstruct object *one, *two, *three;\n+\tstruct decoration n;\n+\tint decoration_a, decoration_b;\n+};\n+\n+static void t_add(struct test_vars *vars)\n+{\n+\tvoid *ret = add_decoration(&vars->n, vars->one, &vars->decoration_a);\n+\n+\tif (!check(ret == NULL))\n+\t\ttest_msg(\"when adding a brand-new object, NULL should be returned\");\n+\tret = add_decoration(&vars->n, vars->two, NULL);\n+\tif (!check(ret == NULL))\n+\t\ttest_msg(\"when adding a brand-new object, NULL should be returned\");\n+}\n+\n+static void t_readd(struct test_vars *vars)\n+{\n+\tvoid *ret = add_decoration(&vars->n, vars->one, NULL);\n+\n+\tif (!check(ret == &vars->decoration_a))\n+\t\ttest_msg(\"when readding an already existing object, existing decoration should be returned\");\n+\tret = add_decoration(&vars->n, vars->two, &vars->decoration_b);\n+\tif (!check(ret == NULL))\n+\t\ttest_msg(\"when readding an already existing object, existing decoration should be returned\");\n+}\n+\n+static void t_lookup(struct test_vars *vars)\n+{\n+\tvoid *ret = lookup_decoration(&vars->n, vars->one);\n+\n+\tif (!check(ret == NULL))\n+\t\ttest_msg(\"lookup should return added declaration\");\n+\tret = lookup_decoration(&vars->n, vars->two);\n+\tif (!check(ret == &vars->decoration_b))\n+\t\ttest_msg(\"lookup should return added declaration\");\n+\tret = lookup_decoration(&vars->n, vars->three);\n+\tif (!check(ret == NULL))\n+\t\ttest_msg(\"lookup for unknown object should return NULL\");\n+}\n+\n+static void t_loop(struct test_vars *vars)\n+{\n+\tint i, objects_noticed = 0;\n+\n+\tfor (i = 0; i < vars->n.size; i++) {\n+\t\tif (vars->n.entries[i].base)\n+\t\t\tobjects_noticed++;\n+\t}\n+\tif (!check_int(objects_noticed, ==, 2))\n+\t\ttest_msg(\"should have 2 objects\");\n+}\n+\n+int cmd_main(int argc UNUSED, const char **argv UNUSED)\n+{\n+\tstruct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };\n+\tstruct test_vars vars = { 0 };\n+\n+\tvars.one = lookup_unknown_object(the_repository, &one_oid);\n+\tvars.two = lookup_unknown_object(the_repository, &two_oid);\n+\tvars.three = lookup_unknown_object(the_repository, &three_oid);\n+\n+\tTEST(t_add(&vars),\n+\t     \"Add 2 objects, one with a non-NULL decoration and one with a NULL decoration.\");\n+\tTEST(t_readd(&vars),\n+\t     \"When re-adding an already existing object, the old decoration is returned.\");\n+\tTEST(t_lookup(&vars),\n+\t     \"Lookup returns the added declarations, or NULL if the object was never added.\");\n+\tTEST(t_loop(&vars), \"The user can also loop through all entries.\");\n+\n+\tclear_decoration(&vars.n, NULL);\n+\n+\treturn test_done();\n+}\n-- \n2.45.1\n\n"},{"id":"495862","messageId":"xmqq8qzsuwh1.fsf@gitster.g","threadId":"61556","inReplyTo":"20240528125837.31090-1-shyamthakkar001@gmail.com","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-example-decorate to the unit testing framework","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-29T21:41:46Z","receivedAt":"2024-05-29T21:41:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n\n> +struct test_vars {\n> +\tstruct object *one, *two, *three;\n> +\tstruct decoration n;\n> +\tint decoration_a, decoration_b;\n> +};\n> +\n> +static void t_add(struct test_vars *vars)\n> +{\n> +\tvoid *ret = add_decoration(&vars->n, vars->one, &vars->decoration_a);\n> +\n> +\tif (!check(ret == NULL))\n> +\t\ttest_msg(\"when adding a brand-new object, NULL should be returned\");\n> +\tret = add_decoration(&vars->n, vars->two, NULL);\n> +\tif (!check(ret == NULL))\n> +\t\ttest_msg(\"when adding a brand-new object, NULL should be returned\");\n> +}\n> +\n> +static void t_readd(struct test_vars *vars)\n> +{\n> +\tvoid *ret = add_decoration(&vars->n, vars->one, NULL);\n> +\n> +\tif (!check(ret == &vars->decoration_a))\n> +\t\ttest_msg(\"when readding an already existing object, existing decoration should be returned\");\n> +\tret = add_decoration(&vars->n, vars->two, &vars->decoration_b);\n> +\tif (!check(ret == NULL))\n> +\t\ttest_msg(\"when readding an already existing object, existing decoration should be returned\");\n> +}\n> +\n> +static void t_lookup(struct test_vars *vars)\n> +{\n> +\tvoid *ret = lookup_decoration(&vars->n, vars->one);\n> +\n> +\tif (!check(ret == NULL))\n> +\t\ttest_msg(\"lookup should return added declaration\");\n> +\tret = lookup_decoration(&vars->n, vars->two);\n> +\tif (!check(ret == &vars->decoration_b))\n> +\t\ttest_msg(\"lookup should return added declaration\");\n> +\tret = lookup_decoration(&vars->n, vars->three);\n> +\tif (!check(ret == NULL))\n> +\t\ttest_msg(\"lookup for unknown object should return NULL\");\n> +}\n> +\n> +static void t_loop(struct test_vars *vars)\n> +{\n> +\tint i, objects_noticed = 0;\n> +\n> +\tfor (i = 0; i < vars->n.size; i++) {\n> +\t\tif (vars->n.entries[i].base)\n> +\t\t\tobjects_noticed++;\n> +\t}\n> +\tif (!check_int(objects_noticed, ==, 2))\n> +\t\ttest_msg(\"should have 2 objects\");\n> +}\n> +\n> +int cmd_main(int argc UNUSED, const char **argv UNUSED)\n> +{\n> +\tstruct object_id one_oid = { { 1 } }, two_oid = { { 2 } }, three_oid = { { 3 } };\n> +\tstruct test_vars vars = { 0 };\n> +\n> +\tvars.one = lookup_unknown_object(the_repository, &one_oid);\n> +\tvars.two = lookup_unknown_object(the_repository, &two_oid);\n> +\tvars.three = lookup_unknown_object(the_repository, &three_oid);\n> +\n> +\tTEST(t_add(&vars),\n> +\t     \"Add 2 objects, one with a non-NULL decoration and one with a NULL decoration.\");\n> +\tTEST(t_readd(&vars),\n> +\t     \"When re-adding an already existing object, the old decoration is returned.\");\n> +\tTEST(t_lookup(&vars),\n> +\t     \"Lookup returns the added declarations, or NULL if the object was never added.\");\n> +\tTEST(t_loop(&vars), \"The user can also loop through all entries.\");\n\nThese tests as a whole look like a faithful copy of the original\ndone by cmd__example_decorate().\n\nI do not understand the criteria used to split them into the four\nseparate helper functions.  It is not like they can be reused or\nreordered---for example, t_readd() must be done after t_add() has\nbeen done.\n\nWhat benefit are you trying to get out of these split?  IOW, what\nare we gaining by having four separate helper functions, instead of\ntesting all of these same things in a single helper function t_all\nwith something like\n\n\tTEST(t_all(&vars), \"Do all decorate tests.\");\n\nin cmd_main()?  If there is a concrete benefit of having larger\nnumber of smaller tests, would it make the result even better if we\nsplit t_add() further into t_add_one() that adds one with deco_a and\nt_add_two() that adds two with NULL?  The other helpers can of\ncourse be further split into individual pieces the same way.  What\nere the criteria used to decide where to stop and use these four?\n\nThanks.\n\n\n\n"},{"id":"495891","messageId":"CAP8UFD1YVyZj-uGfGXp6UxMfj3kZC5XXNed-5s-jj=ROx4URnA@mail.gmail.com","threadId":"61556","inReplyTo":"xmqq8qzsuwh1.fsf@gitster.g","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-example-decorate to the unit testing framework","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-30T06:55:34Z","receivedAt":"2024-05-30T06:55:48Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, May 29, 2024 at 11:41 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n\n> > +     TEST(t_add(&vars),\n> > +          \"Add 2 objects, one with a non-NULL decoration and one with a NULL decoration.\");\n> > +     TEST(t_readd(&vars),\n> > +          \"When re-adding an already existing object, the old decoration is returned.\");\n> > +     TEST(t_lookup(&vars),\n> > +          \"Lookup returns the added declarations, or NULL if the object was never added.\");\n> > +     TEST(t_loop(&vars), \"The user can also loop through all entries.\");\n>\n> These tests as a whole look like a faithful copy of the original\n> done by cmd__example_decorate().\n>\n> I do not understand the criteria used to split them into the four\n> separate helper functions.  It is not like they can be reused or\n> reordered---for example, t_readd() must be done after t_add() has\n> been done.\n>\n> What benefit are you trying to get out of these split?  IOW, what\n> are we gaining by having four separate helper functions, instead of\n> testing all of these same things in a single helper function t_all\n> with something like\n>\n>         TEST(t_all(&vars), \"Do all decorate tests.\");\n>\n> in cmd_main()?  If there is a concrete benefit of having larger\n> number of smaller tests, would it make the result even better if we\n> split t_add() further into t_add_one() that adds one with deco_a and\n> t_add_two() that adds two with NULL?  The other helpers can of\n> course be further split into individual pieces the same way.  What\n> ere the criteria used to decide where to stop and use these four?\n\nThe original code has some kind of \"sections\" (or paragraphs)\nseparated using comments like:\n\n      /*\n       * Add 2 objects, one with a non-NULL decoration and one with a NULL\n       * decoration.\n       */\n\nor:\n\n      /*\n       * When re-adding an already existing object, the old decoration is\n       * returned.\n       */\n\nI think it makes sense to separate the code using functions matching\nthese \"sections\" and to reuse each comment in the TEST() macro that\ncalls the corresponding function. If this patch is rerolled for some\nreason, I think it would be a good idea to mention this in the commit\nmessage though.\n"},{"id":"495912","messageId":"tubjmjeczh6iigem32ulffvt2ucpygbm4frsr3jsps5tv2i7v5@ly3wge23zn6f","threadId":"61556","inReplyTo":"CAP8UFD1YVyZj-uGfGXp6UxMfj3kZC5XXNed-5s-jj=ROx4URnA@mail.gmail.com","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-example-decorate to the unit testing framework","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-05-30T08:39:09Z","receivedAt":"2024-05-30T08:39:12Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Thu, 30 May 2024, Christian Couder <christian.couder@gmail.com> wrote:\n> On Wed, May 29, 2024 at 11:41 PM Junio C Hamano <gitster@pobox.com> wrote:\n> > Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n> \n> > > +     TEST(t_add(&vars),\n> > > +          \"Add 2 objects, one with a non-NULL decoration and one with a NULL decoration.\");\n> > > +     TEST(t_readd(&vars),\n> > > +          \"When re-adding an already existing object, the old decoration is returned.\");\n> > > +     TEST(t_lookup(&vars),\n> > > +          \"Lookup returns the added declarations, or NULL if the object was never added.\");\n> > > +     TEST(t_loop(&vars), \"The user can also loop through all entries.\");\n> >\n> > These tests as a whole look like a faithful copy of the original\n> > done by cmd__example_decorate().\n> >\n> > I do not understand the criteria used to split them into the four\n> > separate helper functions.  It is not like they can be reused or\n> > reordered---for example, t_readd() must be done after t_add() has\n> > been done.\n> >\n> > What benefit are you trying to get out of these split?  IOW, what\n> > are we gaining by having four separate helper functions, instead of\n> > testing all of these same things in a single helper function t_all\n> > with something like\n> >\n> >         TEST(t_all(&vars), \"Do all decorate tests.\");\n> >\n\nIn addition to what Christian said, doing it all in one function would\nprovide no context as is. i.e. when we do it in a single function,\n\n*** unit-tests/bin/t-example-decorate ***\n# check \"objects_noticed == 1\" failed at t/unit-tests/t-example-decorate.c:46\n#    left: 2\n#   right: 1\n# should have 2 objects\nnot ok 1 - All decorate tests\n1..1\nmake[1]: *** [Makefile:78: unit-tests/bin/t-example-decorate] Error 1\n\nvs separated\n\n*** unit-tests/bin/t-example-decorate ***\nok 1 - Add 2 objects, one with a non-NULL decoration and one with a NULL decoration.\nok 2 - When re-adding an already existing object, the old decoration is returned.\nok 3 - Lookup returns the added declarations, or NULL if the object was never added.\n# check \"objects_noticed == 1\" failed at t/unit-tests/t-example-decorate.c:56\n#    left: 2\n#   right: 1\n# should have 2 objects\nnot ok 4 - The user can also loop through all entries.\n1..4\nmake[1]: *** [Makefile:78: unit-tests/bin/t-example-decorate] Error 1\n\nThe latter provides much more context (we almost don't have to open\nt-example-decorate.c file itself in some cases to know what failed)\nthan the former. Now, of course we can add more test_msg()s to the\nformer to improve, but I feel that this approach of splitting them\nprovides and improves the information provided on stdout _without_\nadding any of my own test_msg()s. And I think that this is a good\nmiddleground between cluttering the stdout vs providing very little\ncontext while also remaining a faithful copy of the original.\n\n> > in cmd_main()?  If there is a concrete benefit of having larger\n> > number of smaller tests, would it make the result even better if we\n> > split t_add() further into t_add_one() that adds one with deco_a and\n> > t_add_two() that adds two with NULL?  The other helpers can of\n> > course be further split into individual pieces the same way.  What\n> > ere the criteria used to decide where to stop and use these four?\n> \n> The original code has some kind of \"sections\" (or paragraphs)\n> separated using comments like:\n> \n>       /*\n>        * Add 2 objects, one with a non-NULL decoration and one with a NULL\n>        * decoration.\n>        */\n> \n> or:\n> \n>       /*\n>        * When re-adding an already existing object, the old decoration is\n>        * returned.\n>        */\n> \n> I think it makes sense to separate the code using functions matching\n> these \"sections\" and to reuse each comment in the TEST() macro that\n> calls the corresponding function. If this patch is rerolled for some\n> reason, I think it would be a good idea to mention this in the commit\n> message though.\n\nI agree about the commit message.\n\nThanks.\n"},{"id":"495961","messageId":"xmqqjzjbqoqc.fsf@gitster.g","threadId":"61556","inReplyTo":"tubjmjeczh6iigem32ulffvt2ucpygbm4frsr3jsps5tv2i7v5@ly3wge23zn6f","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-example-decorate to the unit testing framework","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-30T15:54:51Z","receivedAt":"2024-05-30T15:54:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n\n> The latter provides much more context (we almost don't have to open\n> t-example-decorate.c file itself in some cases to know what failed)\n> than the former. Now, of course we can add more test_msg()s to the\n> former to improve, but I feel that this approach of splitting them\n> provides and improves the information provided on stdout _without_\n> adding any of my own test_msg()s. And I think that this is a good\n> middleground between cluttering the stdout vs providing very little\n> context while also remaining a faithful copy of the original.\n\nIf so, why stop at having four, each of which has more than one step\nthat could further be split?  What's the downside?\n\n    Note: Here in this review, I am not necessarily suggesting the\n    tests in this patch to be further split into greater number of\n    smaller helper functions.  I am primarily interested in finding\n    out what the unit test framework can further do to help unit\n    tests written using it (i.e., like this patch).  If using\n    finer-grained tests gives you better diagnosis, but if it is too\n    cumbersome to separate the tests out further, is it because the\n    framework is inadequate in some way?  How can we improve it?\n\nThanks.\n"},{"id":"496166","messageId":"uplnglu2texnwzwf4fnu6kkbpg46nfxwuhum6pzwlgqwsqksg4@xgxy4mayyrcp","threadId":"61556","inReplyTo":"xmqqjzjbqoqc.fsf@gitster.g","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-example-decorate to the unit testing framework","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-06-03T17:51:16Z","receivedAt":"2024-06-03T17:51:20Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Thu, 30 May 2024, Junio C Hamano <gitster@pobox.com> wrote:\n> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n> \n> > The latter provides much more context (we almost don't have to open\n> > t-example-decorate.c file itself in some cases to know what failed)\n> > than the former. Now, of course we can add more test_msg()s to the\n> > former to improve, but I feel that this approach of splitting them\n> > provides and improves the information provided on stdout _without_\n> > adding any of my own test_msg()s. And I think that this is a good\n> > middleground between cluttering the stdout vs providing very little\n> > context while also remaining a faithful copy of the original.\n> \n> If so, why stop at having four, each of which has more than one step\n> that could further be split?  What's the downside?\n> \n>     Note: Here in this review, I am not necessarily suggesting the\n>     tests in this patch to be further split into greater number of\n>     smaller helper functions.  I am primarily interested in finding\n>     out what the unit test framework can further do to help unit\n>     tests written using it (i.e., like this patch).  If using\n>     finer-grained tests gives you better diagnosis, but if it is too\n>     cumbersome to separate the tests out further, is it because the\n>     framework is inadequate in some way?  How can we improve it?\n\nIt's not that the framework is inadequate in its current state\n(for this test). As Christian said, in the original\ntest-example-decorate.c, the tests were divided into four sections \nby a space and comments like:\n\t/*\n\t * Add 2 objects, one with a non-NULL decoration and one with a NULL\n\t * decoration.\n\t */\n\nSo, I also made those four sections in the form of those functions and\nthe comments became the test description. I definitely don't see any\ndownside in further dividing where it makes sense. For example, the\nfirst test can be split into two, one which adds an object with non-NULL\ndecoration and one with NULL (I think you mentioned this). And the third\ntest can split to test lookup for a known object vs an unknown object.\nBesides these I don't see where we can split.\n\nThanks.\n"},{"id":"496170","messageId":"zeenwui37wk5ascgqw7kl6si7oyebn6kojidpevxuy2q4e45r4@sdxjxwn4657s","threadId":"61556","inReplyTo":"xmqqjzjbqoqc.fsf@gitster.g","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-example-decorate to the unit testing framework","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-06-03T18:53:08Z","receivedAt":"2024-06-03T18:53:15Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.05.30 08:54, Junio C Hamano wrote:\n> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n> \n> > The latter provides much more context (we almost don't have to open\n> > t-example-decorate.c file itself in some cases to know what failed)\n> > than the former. Now, of course we can add more test_msg()s to the\n> > former to improve, but I feel that this approach of splitting them\n> > provides and improves the information provided on stdout _without_\n> > adding any of my own test_msg()s. And I think that this is a good\n> > middleground between cluttering the stdout vs providing very little\n> > context while also remaining a faithful copy of the original.\n> \n> If so, why stop at having four, each of which has more than one step\n> that could further be split?  What's the downside?\n> \n>     Note: Here in this review, I am not necessarily suggesting the\n>     tests in this patch to be further split into greater number of\n>     smaller helper functions.  I am primarily interested in finding\n>     out what the unit test framework can further do to help unit\n>     tests written using it (i.e., like this patch).  If using\n>     finer-grained tests gives you better diagnosis, but if it is too\n>     cumbersome to separate the tests out further, is it because the\n>     framework is inadequate in some way?  How can we improve it?\n\nI'll try not to speak for anyone else here, but I think the test\nframework isn't causing much friction here in the decision of how to\nsplit the tests. [However, neither is it providing much guidance. At\nsome point we should review the unit tests and see if we can extract a\nhelpful style guide or best practices doc.] The setup for the cases is\nminimal and done through the main function.\n\nI think the current split is reasonable as a first patch, as it mirrors\nthe organization of the original test and makes it easier for reviewers\nto verify that it tests the same behaviors. If further simplification or\nreorganization is needed, I would like to see that as a separate patch\non top of the more straightforward conversion.\n\nThe only part that bothers me a bit (and this is really more of a\ncomplaint about the framework than the patch itself) is the carryover of\nstate between the different TEST() cases. We can't skip t_add and expect\nthe other test cases to still pass, unfortunately. However, I don't\nthink this patch needs to worry about that, since the framework doesn't\nrestrict persistent state. [And we certainly don't restrict persistent\nstate in the shell tests either.]\n\n> Thanks.\n> \n"},{"id":"496184","messageId":"dsdg4jeoog2awxtry64joaxt4dawwq3ajmm4pksy733vwbsvp7@fgkp7d76pp46","threadId":"61556","inReplyTo":"zeenwui37wk5ascgqw7kl6si7oyebn6kojidpevxuy2q4e45r4@sdxjxwn4657s","subject":"Re: [GSoC][PATCH] t/: migrate helper/test-example-decorate to the unit testing framework","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-06-03T21:09:01Z","receivedAt":"2024-06-03T21:09:06Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Mon, 03 Jun 2024, Josh Steadmon <steadmon@google.com> wrote:\n> On 2024.05.30 08:54, Junio C Hamano wrote:\n> > Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:\n> > \n> > > The latter provides much more context (we almost don't have to open\n> > > t-example-decorate.c file itself in some cases to know what failed)\n> > > than the former. Now, of course we can add more test_msg()s to the\n> > > former to improve, but I feel that this approach of splitting them\n> > > provides and improves the information provided on stdout _without_\n> > > adding any of my own test_msg()s. And I think that this is a good\n> > > middleground between cluttering the stdout vs providing very little\n> > > context while also remaining a faithful copy of the original.\n> > \n> > If so, why stop at having four, each of which has more than one step\n> > that could further be split?  What's the downside?\n> > \n> >     Note: Here in this review, I am not necessarily suggesting the\n> >     tests in this patch to be further split into greater number of\n> >     smaller helper functions.  I am primarily interested in finding\n> >     out what the unit test framework can further do to help unit\n> >     tests written using it (i.e., like this patch).  If using\n> >     finer-grained tests gives you better diagnosis, but if it is too\n> >     cumbersome to separate the tests out further, is it because the\n> >     framework is inadequate in some way?  How can we improve it?\n> \n> I'll try not to speak for anyone else here, but I think the test\n> framework isn't causing much friction here in the decision of how to\n> split the tests. [However, neither is it providing much guidance. At\n> some point we should review the unit tests and see if we can extract a\n> helpful style guide or best practices doc.] The setup for the cases is\n> minimal and done through the main function.\n\nAgreed about style guide/best practices doc.\n\n> I think the current split is reasonable as a first patch, as it mirrors\n> the organization of the original test and makes it easier for reviewers\n> to verify that it tests the same behaviors. If further simplification or\n> reorganization is needed, I would like to see that as a separate patch\n> on top of the more straightforward conversion.\n> \n> The only part that bothers me a bit (and this is really more of a\n> complaint about the framework than the patch itself) is the carryover of\n> state between the different TEST() cases. We can't skip t_add and expect\n> the other test cases to still pass, unfortunately. However, I don't\n> think this patch needs to worry about that, since the framework doesn't\n> restrict persistent state. [And we certainly don't restrict persistent\n> state in the shell tests either.]\n\nI talked about this in private with Christian, and we came to the\nsame conclusion that having independent state would better. But seeing the\noriginal test-example-decorate, it would be a bit more boiler plate to\nproduce the exact same checks, without relying on previous state. And\nseeing the lack of convention (written guideline) about independent\nstate vs dependent, I decided to stick to having the tests rely on\nprevious state, similar to the original, and see the mailing list\nresponse about what should be done.\n\nThanks.\n"}]}