Re: [PATCH v2 13/33] drm/client: Add some tests for drm_connector_pick_cmdline_mode()

[Maxime Ripard]

On Fri, Sep 23, 2022 at 12:30:09PM +0200, Thomas Zimmermann wrote:
> Am 23.09.22 um 11:26 schrieb Javier Martinez Canillas:
> > On 9/23/22 11:15, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 22.09.22 um 16:25 schrieb Maxime Ripard:
> > > > drm_connector_pick_cmdline_mode() is in charge of finding a proper
> > > > drm_display_mode from the definition we got in the video= command line
> > > > argument.
> > > > 
> > > > Let's add some unit tests to make sure we're not getting any regressions
> > > > there.
> > > > 
> > > > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> > > > index bbc535cc50dd..d553e793e673 100644
> > > > --- a/drivers/gpu/drm/drm_client_modeset.c
> > > > +++ b/drivers/gpu/drm/drm_client_modeset.c
> > > > @@ -1237,3 +1237,7 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode)
> > > >    	return ret;
> > > >    }
> > > >    EXPORT_SYMBOL(drm_client_modeset_dpms);
> > > > +
> > > > +#ifdef CONFIG_DRM_KUNIT_TEST
> > > > +#include "tests/drm_client_modeset_test.c"
> > > > +#endif
> > > 
> > > I strongly dislike this style of including source files in each other.
> > > It's a recipe for all kind of build errors. Can you do something else?
> > > 
> > 
> > This seems to be the convention used to test static functions and what's
> > documented in the Kunit docs: https://kunit.dev/third_party/kernel/docs/tips.html#testing-static-functions
> 
> That document says "...one option is to conditionally #include the test
> file...". This doesn't sound like a strong requirement.

No, but this is the only option documented, which still indicates a very
strong preference.

> > I agree with you that's not ideal but I think that consistency with how
> > it is done by other subsystems is also important.
> > > As the tested function is an internal interface, maybe export a wrapper
> > > if tests are enabled, like this:
> > > 
> > > #ifdef CONFIG_DRM_KUNIT_TEST
> > > struct drm_display_mode *
> > > drm_connector_pick_cmdline_mode_kunit(drm_conenctor)
> > > {
> > >     return drm_connector_pick_cmdline_mode(connector)
> > > }
> > > EXPORT_SYMBOL(drm_connector_pick_cmdline_mode_kunit)
> > > #endif
> > > 
> > > The wrapper's declaration can be located in the kunit test file.

And I'm afraid this just doesn't scale. If we start testing more and
more static functions, do we really want to have that wrapper for each
of them?

> > But that's also not nice since we are artificially exposing these only
> > to allow the static functions to be called from unit tests. And would
> > be a different approach than the one used by all other subsystems...
> > 
> 
> There's the problem of interference between the source files when building
> the code. It's also not the same source code after including the test file.
> At a minimum, including the tests' source file further includes more files.
> <kunit/tests.h> also includes quite a few of Linux header files.
> 
> IMHO the current convention (if any) is far from optimal and we should
> consider breaking it.

I mean... this is a discussion about theoretical issues. If there is
indeed some regular build errors on this, then sure, we can change it.

I'm confident that will affect pretty much every one using kunit equally
though, so I'm fairly sure the documentation itself will have changed.

But right now we're discussing an alternative because of a problem we
never experienced. I don't see the point of breaking the consistency
provided by the documentation for something not backed by any actual
problem.

Maxime

Attachment: signature.asc
Description: PGP signature