On Thu, 13 Apr 2023 at 21:15, Michal Wajdeczko
<michal.wajdeczko@xxxxxxxxx> wrote:
>
>
>
> On 13.04.2023 08:31, David Gow wrote:
> > On Thu, 13 Apr 2023 at 04:28, Rae Moar <rmoar@xxxxxxxxxx> wrote:
> >>
> >> On Tue, Apr 11, 2023 at 12:01 PM Michal Wajdeczko
> >> <michal.wajdeczko@xxxxxxxxx> wrote:
> >>>
> >>> There is function to report status of either suite or test, but it
> >>> doesn't support parameterized subtests that have to log report on
> >>> its own. Extend it to also accept subtest level results to avoid
> >>> code duplication.
> >>>
> >>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> >>> Cc: David Gow <davidgow@xxxxxxxxxx>
> >>> ---
> >>> lib/kunit/test.c | 28 +++++++++++++++++-----------
> >>> 1 file changed, 17 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> >>> index 5679197b5f8a..692fce258c5b 100644
> >>> --- a/lib/kunit/test.c
> >>> +++ b/lib/kunit/test.c
> >>> @@ -154,8 +154,14 @@ static void kunit_print_suite_start(struct kunit_suite *suite)
> >>> kunit_suite_num_test_cases(suite));
> >>> }
> >>>
> >>> +enum kunit_test_or_suite {
> >>> + KUNIT_SUITE = 0,
> >>> + KUNIT_TEST,
> >>> + KUNIT_SUBTEST,
> >>> +};
> >>> +
> >>
> >> Hi Michal!
> >>
> >> Since KUnit's goal is to progress toward supporting arbitrary levels
> >> of testing, I like the idea of starting to adjust these helper
> >> functions to allow for greater levels of testing.
> >>
> >> However, I'm not sure about this kunit_test_or_suite enum. If our goal
> >> is to support an arbitrary number of levels of tests then this enum
> >> still limits us to a finite number of levels. However, if we only want
> >> to focus on supporting parameterized tests (which is our direct goal),
> >> this could be the right solution.
> >>
> >> Maybe instead we could use an integer denoting the test level instead?
> >> This would remove the limit but would also remove the nice names of
> >> the levels.
>
> we can use integer as param but at the same time we can also have define
> or anonymous enum as nice aliases to currently known/used levels:
>
> /* Currently supported test levels */
> enum {
> KUNIT_LEVEL_SUITE = 0,
> KUNIT_LEVEL_TEST,
> KUNIT_LEVEL_PARAMTEST,
> };
>
> /* Future levels are TBD */
> #define KUNIT_LEVEL_SUBTEST(n) (KUNIT_LEVEL_TEST + (n))
>
This sounds good to me!
> >>
> >> I'm curious what others opinions are on these ideas?
> >>
> >> A bit of a nit: if we do use this enum I wonder if we could clarify
> >> the name to be kunit_test_level as the current name of
> >> kunit_test_or_suite seems to indicate to me a binary of two options
> >> rather than three.
> >>
> >>> static void kunit_print_ok_not_ok(void *test_or_suite,
> >>> - bool is_test,
> >>> + enum kunit_test_or_suite is_test,
> >>
> >> Similar to above, I think the name of is_test could be clarified. It
> >> is currently a bit confusing to me as they are all tests. Maybe
> >> test_level?
>
> ok
>
> >
> > I agree with Rae that this is not the ideal long-term solution.
> >
> > We're basically encoding two things here:
> > - Are we dealing with a 'struct kunit_suite' or a 'struct kunit'?
> > - How nested the test is.
> >
> > Given KUnit originally only had a 2-level nesting (suite->test), and
> > now has 3-level nesting (always suite->test[->param]), this works, but
> > the KTAP format permits arbitrary nesting of tests, and we'll want to
> > have something like that in KUnit going forward. We don't have a
> > design for that yet, but it could conceivably allow nested suites,
> > thus breaking the rule that nesting level 0 is always a suite, and the
> > rest are all tests.
>
> I guess "We don't have a design for that yet" is a key here
Yeah: I wouldn't worry too much about what the future design would
bring, tbh. As long as we don't totally paint ourselves into a corner
(and I don't think anything in this patch is at risk of doing that),
then doing whatever is sensible for the way things are now works.
>
> >
> > So there's definitely a part of me that would prefer to pass those two
> > pieces of information in separate arguments, rather than relying on an
> > enum like this.
> >
> > That being said, this is all very fuzzy future plans, rather than
> > anything concrete, and this will all likely need reworking if we do
> > anything drastic anyway, so I'm not worried if we go with this for
> > now, and change it when we need to. I do think it's an improvement
> > over what we're doing currently.
> > (For example, another possible implementation for nested tests could
> > be to get rid of the distinction between tests and suites completely:
> > or at least have them share 'struct kunit', so this wouldn't need
> > passing in separately.)
>
> maybe the problem will go away once we just replace this untyped param:
>
> kunit_print_ok_not_ok(void *test_or_suite, ...
>
> with proper type:
>
> kunit_print_ok_not_ok(struct kunit *test, ...
>
> and treat NULL as indication that we just want to print raw results (as
> it looks function is not using any suite attributes directly, all input
> is passed explicitly and kunit_log() expects test only)
>
Yeah: I think that makes sense. Suite results are still handled
separately in debugfs as well.
Cheers,
-- David
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
