On 2024.05.30 08:54, Junio C Hamano wrote: > Ghanshyam Thakkar <shyamthakkar001@xxxxxxxxx> writes: > > > 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. > > If so, why stop at having four, each of which has more than one step > that could further be split? What's the downside? > > Note: Here in this review, I am not necessarily suggesting the > tests in this patch to be further split into greater number of > smaller helper functions. I am primarily interested in finding > out what the unit test framework can further do to help unit > tests written using it (i.e., like this patch). If using > finer-grained tests gives you better diagnosis, but if it is too > cumbersome to separate the tests out further, is it because the > framework is inadequate in some way? How can we improve it? I'll try not to speak for anyone else here, but I think the test framework isn't causing much friction here in the decision of how to split the tests. [However, neither is it providing much guidance. At some point we should review the unit tests and see if we can extract a helpful style guide or best practices doc.] The setup for the cases is minimal and done through the main function. I think the current split is reasonable as a first patch, as it mirrors the organization of the original test and makes it easier for reviewers to verify that it tests the same behaviors. If further simplification or reorganization is needed, I would like to see that as a separate patch on top of the more straightforward conversion. The only part that bothers me a bit (and this is really more of a complaint about the framework than the patch itself) is the carryover of state between the different TEST() cases. We can't skip t_add and expect the other test cases to still pass, unfortunately. However, I don't think this patch needs to worry about that, since the framework doesn't restrict persistent state. [And we certainly don't restrict persistent state in the shell tests either.] > Thanks. >
