On Mon, 03 Jun 2024, Josh Steadmon <steadmon@xxxxxxxxxx> wrote: > 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. Agreed about style guide/best practices doc. > 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.] I talked about this in private with Christian, and we came to the same conclusion that having independent state would better. But seeing the original test-example-decorate, it would be a bit more boiler plate to produce the exact same checks, without relying on previous state. And seeing the lack of convention (written guideline) about independent state vs dependent, I decided to stick to having the tests rely on previous state, similar to the original, and see the mailing list response about what should be done. Thanks.
