On Tue, Apr 26, 2022 at 8:56 PM David Gow <[email protected]> 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.
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.
>
>
> > {
> > + enum kunit_status status =
> > + init_err ? KUNIT_FAILURE : kunit_suite_has_succeeded(suite);
> > +
On Wed, Apr 27, 2022 at 11:06 AM Daniel Latypov <[email protected]> wrote:
>
> On Tue, Apr 26, 2022 at 8:56 PM David Gow <[email protected]> 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);
> > > +
On Fri, Apr 29, 2022 at 1:01 AM David Gow <[email protected]> wrote:
>
> On Wed, Apr 27, 2022 at 11:06 AM Daniel Latypov <[email protected]> wrote:
> >
> > On Tue, Apr 26, 2022 at 8:56 PM David Gow <[email protected]> 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
I forgot kunit_suite_has_succeeded() is called in the debugfs code.
So it looks like we need to track the init error in struct
kunit_suite, as you said.
It might be cleaner to just store a status in the suite eventually,
but I'll just add the int for now.
Sending a v2 series here:
https://lore.kernel.org/linux-kselftest/[email protected]
I also added on a new patch to fix the type confusion in the debugfs
code (using bool instead of enum kunit_status).