[PATCH] drm/client: Convert to VISIBLE_IF_KUNIT

[Maxime Ripard]

On Thu, Feb 02, 2023 at 02:05:14PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 02.02.23 um 13:35 schrieb Maxime Ripard:
> > Hi,
> > 
> > On Thu, Feb 02, 2023 at 01:22:01PM +0100, Thomas Zimmermann wrote:
> > > Am 02.02.23 um 12:03 schrieb Maxime Ripard:
> > > > Commit 8fc0380f6ba7 ("drm/client: Add some tests for
> > > > drm_connector_pick_cmdline_mode()") was meant to introduce unit tests
> > > > for the static drm_connector_pick_cmdline_mode() function.
> > > > 
> > > > In such a case, the kunit documentation recommended to import the tests
> > > > source file directly from the source file with the static function to
> > > > test.
> > > > 
> > > > While it was working, it's generally frowned upon. Fortunately, commit
> > > > 9c988fae6f6a ("kunit: add macro to allow conditionally exposing static
> > > > symbols to tests") introduced macros to easily deal with that case. We
> > > > can thus remove our include and use those macros instead.
> > > 
> > > I like that this include statements is going away.
> > 
> > Yeah, when I saw that it was now available, I remembered you really
> > didn't like it :)
> > 
> > > But changing symbol visibility for tests is likewise awkward.
> > > 
> > > Maybe i'm askin gtoo miuch for this simple patch, but can't we have a helper
> > > macro that generates an exported wrapper for Kunit tests? Something like
> > > this:
> > > 
> > > EXPORT_KUNIT_WRAPPER(struct drm_display_mode *\
> > > 			drm_connector_pick_cmdline_mode,
> > > 			struct drm_connector *connector);
> > > 
> > > which then generates something like this:
> > > 
> > > struct drm_display_mode * drm_connector_pick_cmdline_mode_kunit(
> > > 	struct drm_connector *connector)
> > > {
> > > 	return drm_connector_pick_cmdline_mode(connector);
> > > }
> > > 
> > > I know that the macro for generating this code is more complex than
> > > illustrated here. But this solution separates Kunit and functions cleanly.
> > > The static functions that are exported for Kunit testing still need to be
> > > declared in a header file. That could also be done via such a macro.
> > 
> > I mean, I guess we could do that, but what's the point? I don't really
> > get what that wrapper brings to the table.
> 
> The big benefit of the kunit wrapper is that we don't change the visibility
> or implementation of the tested code. The currently existing macros invite
> linker errors because symbol visibility now depends on whether Kunit it
> enabled.

Sure, it can happen, but saying that it encourages them is a stretch.
And fortunately, we have build bots to detect that.

The huge downside of the wrapper approach is that you're no longer
testing the function you want to test but something else, and you have
to trust that it's exactly the same thing.

And it's far from obvious that we're supposed to do that in the first
place, especially when everyone else is doing something else, and we're
doing it ourselves in a similar situation.

It's still fairly new in kunit, but if it was indeed creating any kind
of friction, I think EXPORT_SYMBOL_FOR_TESTS_ONLY would have been
reworked some time in the last 4 years.

> It's also not clear to me how Kunit knows the symbol. Is there a
> function declaration in the Kunit test's source code? If so, it might
> diverge from the implementation; with consequences.

apparmor has been using an internal header included in both the test
file and the implementation. That way, the compiler will make sure
there's no inconsistencies.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230209/cd4b9227/attachment.sig>