Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220426181925.3940286-1-dlatypov@google.com> <20220426181925.3940286-2-dlatypov@google.com>
 <CABVgOSkqymYzwaQ68AdEC5yake9VT8HkQmqbyi+9-bg1Jk1UAQ@mail.gmail.com> <CAGS_qxrowBiRBBhNmo+RyQSR6NQphkzx1k3HZ7KqXUNDZFZDzw@mail.gmail.com>
In-Reply-To: <CAGS_qxrowBiRBBhNmo+RyQSR6NQphkzx1k3HZ7KqXUNDZFZDzw@mail.gmail.com>
From:   David Gow <davidgow@google.com>
Date:   Fri, 29 Apr 2022 14:01:40 +0800
Message-ID: <CABVgOSnb0CveWUqbB8aSYu53uRmi+H9Dim3mYbyXi+eQo=y4ww@mail.gmail.com>
Subject: Re: [PATCH 2/3] kunit: add ability to specify suite-level init and
 exit functions
To:     Daniel Latypov <dlatypov@google.com>
Cc:     Brendan Higgins <brendanhiggins@google.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        KUnit Development <kunit-dev@googlegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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);
> > > +