On Thu, 30 May 2024, Christian Couder <christian.couder@xxxxxxxxx> wrote: > On Wed, May 29, 2024 at 11:41 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Ghanshyam Thakkar <shyamthakkar001@xxxxxxxxx> writes: > > > > + TEST(t_add(&vars), > > > + "Add 2 objects, one with a non-NULL decoration and one with a NULL decoration."); > > > + TEST(t_readd(&vars), > > > + "When re-adding an already existing object, the old decoration is returned."); > > > + TEST(t_lookup(&vars), > > > + "Lookup returns the added declarations, or NULL if the object was never added."); > > > + TEST(t_loop(&vars), "The user can also loop through all entries."); > > > > These tests as a whole look like a faithful copy of the original > > done by cmd__example_decorate(). > > > > I do not understand the criteria used to split them into the four > > separate helper functions. It is not like they can be reused or > > reordered---for example, t_readd() must be done after t_add() has > > been done. > > > > What benefit are you trying to get out of these split? IOW, what > > are we gaining by having four separate helper functions, instead of > > testing all of these same things in a single helper function t_all > > with something like > > > > TEST(t_all(&vars), "Do all decorate tests."); > > In addition to what Christian said, doing it all in one function would provide no context as is. i.e. when we do it in a single function, *** unit-tests/bin/t-example-decorate *** # check "objects_noticed == 1" failed at t/unit-tests/t-example-decorate.c:46 # left: 2 # right: 1 # should have 2 objects not ok 1 - All decorate tests 1..1 make[1]: *** [Makefile:78: unit-tests/bin/t-example-decorate] Error 1 vs separated *** unit-tests/bin/t-example-decorate *** ok 1 - Add 2 objects, one with a non-NULL decoration and one with a NULL decoration. ok 2 - When re-adding an already existing object, the old decoration is returned. ok 3 - Lookup returns the added declarations, or NULL if the object was never added. # check "objects_noticed == 1" failed at t/unit-tests/t-example-decorate.c:56 # left: 2 # right: 1 # should have 2 objects not ok 4 - The user can also loop through all entries. 1..4 make[1]: *** [Makefile:78: unit-tests/bin/t-example-decorate] Error 1 The latter provides much more context (we almost don't have to open t-example-decorate.c file itself in some cases to know what failed) than the former. Now, of course we can add more test_msg()s to the former to improve, but I feel that this approach of splitting them provides and improves the information provided on stdout _without_ adding any of my own test_msg()s. And I think that this is a good middleground between cluttering the stdout vs providing very little context while also remaining a faithful copy of the original. > > in cmd_main()? If there is a concrete benefit of having larger > > number of smaller tests, would it make the result even better if we > > split t_add() further into t_add_one() that adds one with deco_a and > > t_add_two() that adds two with NULL? The other helpers can of > > course be further split into individual pieces the same way. What > > ere the criteria used to decide where to stop and use these four? > > The original code has some kind of "sections" (or paragraphs) > separated using comments like: > > /* > * Add 2 objects, one with a non-NULL decoration and one with a NULL > * decoration. > */ > > or: > > /* > * When re-adding an already existing object, the old decoration is > * returned. > */ > > I think it makes sense to separate the code using functions matching > these "sections" and to reuse each comment in the TEST() macro that > calls the corresponding function. If this patch is rerolled for some > reason, I think it would be a good idea to mention this in the commit > message though. I agree about the commit message. Thanks.
