Re: [PATCH RFC 2/2] drm: add documentation for drm_buddy_test kUnit test

[Rae Moar <rmoar@google.com>]

On Fri, Sep 1, 2023 at 3:11 AM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
>
> Hi Rae,
>
> Em Thu, 13 Jul 2023 17:31:19 -0400
> Rae Moar <rmoar@google.com> escreveu:
>
> > On Wed, Jul 12, 2023 at 10:29 AM Mauro Carvalho Chehab <mchehab@kernel.org>
> > wrote:
> >
> > > As an example for the new documentation tool, add a documentation
> > > for drm_buddy_test.
> > >
> > > I opted to place this on a completely different directory, in order
> > > to make easier to test the feature with:
> > >
> > >         $ make SPHINXDIRS="tests" htmldocs
> > >
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > ---
> > >
> > > To avoid mailbombing on a large number of people, only mailing lists were
> > > C/C on the cover.
> > > See [PATCH RFC 0/2] at:
> > > https://lore.kernel.org/all/cover.1689171160.git.mchehab@kernel.org/
> > >
> > >  Documentation/index.rst                |  2 +-
> > >  Documentation/tests/index.rst          |  6 ++++++
> > >  Documentation/tests/kunit.rst          |  5 +++++
> > >  drivers/gpu/drm/tests/drm_buddy_test.c | 12 ++++++++++++
> > >  4 files changed, 24 insertions(+), 1 deletion(-)
> > >  create mode 100644 Documentation/tests/index.rst
> > >  create mode 100644 Documentation/tests/kunit.rst
> > >
> > > diff --git a/Documentation/index.rst b/Documentation/index.rst
> > > index 9dfdc826618c..80a6ce14a61a 100644
> > > --- a/Documentation/index.rst
> > > +++ b/Documentation/index.rst
> > > @@ -60,7 +60,7 @@ Various other manuals with useful information for all
> > > kernel developers.
> > >     fault-injection/index
> > >     livepatch/index
> > >     rust/index
> > > -
> > > +   test/index
> > >
> > >  User-oriented documentation
> > >  ===========================
> > > diff --git a/Documentation/tests/index.rst b/Documentation/tests/index.rst
> > > new file mode 100644
> > > index 000000000000..bfc39eb5c0aa
> > > --- /dev/null
> > > +++ b/Documentation/tests/index.rst
> > > @@ -0,0 +1,6 @@
> > > +========================
> > > +Kunit documentation test
> > > +========================
> > > +
> > > +.. toctree::
> > > +   kunit
> > > diff --git a/Documentation/tests/kunit.rst b/Documentation/tests/kunit.rst
> > > new file mode 100644
> > > index 000000000000..6ffc151988a0
> > > --- /dev/null
> > > +++ b/Documentation/tests/kunit.rst
> > > @@ -0,0 +1,5 @@
> > > +Kunit tests
> > > +-----------
> > > +
> > > +.. include-test:: drivers/gpu/drm/tests/drm_buddy_test.c
> > > +
> > > diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c
> > > b/drivers/gpu/drm/tests/drm_buddy_test.c
> > > index 09ee6f6af896..dd6c5afd6cd6 100644
> > > --- a/drivers/gpu/drm/tests/drm_buddy_test.c
> > > +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
> > > @@ -737,6 +737,18 @@ static int drm_buddy_suite_init(struct kunit_suite
> > > *suite)
> > >         return 0;
> > >  }
> > >
> > > +/**
> > > + * KTEST_SUITE: set of tests for drm buddy alloc
> > > + * Scope: drm subsystem
> > > + * Mega feature: drm
> > > + * Feature: buddy_alloc
> > > + *
> > > + * KTEST_TEST: drm_test_buddy_alloc_%s
> > > + * Description: Run DRM buddy allocation %arg[1] test
> > > + *
> > > + * arg[1].values: limit, range, optimistic, smoke, pathological
> > > + */
> >
> >
> > Hi!
> >
> > This is such a cool patch series. I just have a few comments related to the
> > output.
>
> Thank you for your comments! Sorry for not answering earlier. I took some
> vacations and this series ended sleeping over other tasks on my
> todo list.
>
> Also, before sending another version, I wanted to have the test_list.py
> changes to make it generic enough to be merged on IGT, to avoid having
> a fork of it. Those got merged today.

Hi Mauro!

No worries at all!

>
> > In the html output the tests are listed as:
> > ktest@drm_buddy_test@…
> >
> > I wonder if instead of using the file name of “drm_buddy_test” this could
> > possibly be the suite name, “drm_buddy”, as this is what users will call
> > when using kunit.py to run the tests. Although "drm_buddy_test" is also the
> > module name so I don't mind it too much. But in the future the file name
> > and module name are not guaranteed to be the same for other tests.
> >
> > Most preferably, there would be a reference to the kunit suite name, file
> > name, and the module name.
>
> I guess it shouldn't be hard to do such change in a way that it won't
> affect its usage on IGT. We need to define what would be the best
> pattern. As this can be used for both kunit and selftests, I would
> place kunit at the beginning.
>
> Currently, we're using:
>
>         kunit@<base file name without .c>@<test_name>
>
> Some possible patterns would be:
>
>         kunit@<base file name without .c>@<suite name>@<test_name>
>         kunit@<subsystem>@<base file name without .c>@<suite name>@<test_name>
>         kunit@<subsystem>@<suite name>@<test_name>
>
> Would do you think it would work best?

How possible is it to separate out the file and suite name as headers?
I think that this could reduce some of the repetition.

If we are already separating documentation pages by subsystem, a
potential format could be:

File: <base file>

<kunit_or_kselftest> suite: <suite name>
List of Tests:
<test name>
<test name>
...

What do you think?

>
> > This may be difficult to implement as these can all differ. I am currently
> > working on the KUnit Attribute framework which saves the module name and I
> > am thinking about also saving the file path as a future attribute. This
> > could be a helpful framework for the KUnit tests specifically.
> >
> > I am not sure how easy it would be to access c objects/functions using this
> > system.
>
> I would prefer not. C language allows lots of flexibility with macros,
> making it hard to write a parser to read those C objects from the source.
> We have this at kernel-doc. As one of the people that did some work there,
> I can say that that several tricks are needed to keep this working.
>
> On the other hand, it should be easy to use the TestList class from
> test_list.py at kunit.py.
>
> So, kunit.py could use the data that came from the documentation
> directly.
>

Got it. So it is possible to get some of this info. Thanks!

> > Finally, I was wondering if it is the intention to put a list of all kunit
> > tests that use this new feature into tests/kunit.rst or would this be
> > broken up in some way
>
> IMO, it makes sense to break this per subsystem, and have an auto-generated
> index.rst pointing to the entire set of documents.
>
> We're already storing the subsystem at the documentation macros, so, IMO,
> it should shouldn't be hard to implement it.
>
> Regards,
> Mauro

I think breaking this up by subsystems sounds like a good idea,
especially since we still have them documented already.

Thanks for your responses!
-Rae

>