On Wed, Apr 27, 2022 at 11:06 AM Daniel Latypov dlatypov@google.com wrote:
On Tue, Apr 26, 2022 at 8:56 PM David Gow davidgow@google.com wrote:
static size_t kunit_suite_counter = 1;
-static void kunit_print_suite_end(struct kunit_suite *suite) +static void kunit_print_suite_end(struct kunit_suite *suite, int init_err)
A part of me feels that it'd be nicer to have the init_err be part of struct kunit_suite, and have kunit_suite_has_succeeded() take it into account. It could go either way, though -- WDYT?
Yeah, passing it around as a parameter felt a bit icky. But I think adding it in as a field feels worse.
Personally, I don't have a problem with having it as a field (other than the memory usage, which shouldn't be so much as to cause problems). It seems that the suite result is logically part of the suite, and given that status_comment is in struct kunit_suite and there's a kunit_status field in kunit_case, having it as a field in the suite seems the most logically consistent thing to do.
Another thought: perhaps have this function take a `kunit_status` parameter instead? Moving the ?: expression below out into the caller isn't that bad, imo.
It still doesn't solve the fact that kunit_suite_has_succeeded() no longer tells you if a suite has succeeded or not. If we stick with this (with the conditional either here or in the caller), I think we should at least rename kunit_suite_has_succeded() to something like kunit_suite_subtests_status().
That being said, I do prefer passing a 'kunit_status' around to an 'int'.
{
enum kunit_status status =init_err ? KUNIT_FAILURE : kunit_suite_has_succeeded(suite);