On Tue, Feb 11 2025, David Gow <davidgow@xxxxxxxxxx> wrote: > On Mon, 10 Feb 2025 at 19:57, Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> wrote: >> >> On Fri, Feb 07 2025, Tamir Duberstein <tamird@xxxxxxxxx> wrote: >> >> > On Fri, Feb 7, 2025 at 5:01 AM Rasmus Villemoes >> > <linux@xxxxxxxxxxxxxxxxxx> wrote: >> >> >> >> On Thu, Feb 06 2025, Tamir Duberstein <tamird@xxxxxxxxx> wrote: >> >> >> >> >> >> I'll have to see the actual code, of course. In general, I find reading >> >> code using those KUNIT macros quite hard, because I'm not familiar with >> >> those macros and when I try to look up what they do they turn out to be >> >> defined in terms of other KUNIT macros 10 levels deep. >> >> >> >> But that still leaves a few points. First, I really like that "388 test >> >> cases passed" tally or some other free-form summary (so that I can see >> >> that I properly hooked up, compiled, and ran a new testcase inside >> >> test_number(), so any kind of aggregation on those top-level test_* is >> >> too coarse). >> > >> > This one I'm not sure how to address. What you're calling test cases >> > here would typically be referred to as assertions, and I'm not aware >> > of a way to report a count of assertions. >> > >> >> I'm not sure that's accurate. >> >> The thing is, each of the current test() instances results in four >> different tests being done, which is roughly why we end up at the 4*97 >> == 388, but each of those tests has several assertions being done - >> depending on which variant of the test we're doing (i.e. the buffer >> length used or if we're passing it through kasprintf), we may do only >> some of those assertions, and we do an early return in case one of those >> assertions fail (because it wouldn't be safe to do the following >> assertions, and the test as such has failed already). So there are far >> more assertions than those 388. >> >> OTOH, that the number reported is 388 is more a consequence of the >> implementation than anything explicitly designed. I can certainly live >> with 388 being replaced by 97, i.e. that each current test() invocation >> would count as one KUNIT case, as that would still allow me to detect a >> PEBKAC when I've added a new test() instance and failed to actually run >> that. > > It'd be possible to split things up further into tests, at the cost of > it being a more extensive refactoring, if having the more granular > count tracked by KUnit were desired. I think the problem is that kunit is simply not a good framework to do these kinds of tests in, and certainly it's very hard to retrofit kunit after the fact. It'd also be possible to make > these more explicitly data driven via a parameterised test (so each > input/output pair is listed in an array, and automatically gets > converted to a KUnit subtest). So that "array of input/output" very much doesn't work for these specific tests: We really want the format string/varargs to be checked by the compiler, and besides, there's no way to store the necessary varargs and generate a call from those in an array. Moreover, we verify a lot more than just that the correct string is produced; it's also a matter of the right return value regardless of the passed buffer size, etc. That's also why is nigh impossible to simply change __test() into (another) macro that expands to something that defines an individual struct kunit_case, because the framework is really built around the notion that each case can be represented by a void function call and the name of the test is the stringification of the function name. So I don't mind the conversion to kunit if that really helps other people, as long as the basic functionality is still present and doesn't impede future extensions - and certainly I don't want to end up in a situation where somebody adds a new %p extension but cannot really add a test for it because kunit makes that hard. But I hope you all agree that it doesn't make much _sense_ to consider test_number() and test_string() and so on individual "test cases"; the atomic units of test being done in the printf suite is each invocation of the __test() function, with one specific format string/varargs combination. Rasmus
