Re: [PATCH] drm/client: Convert to VISIBLE_IF_KUNIT

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 2061 bytes --]

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.

Also, this deviates from the existing practice we had for selftests and
EXPORT_SYMBOL_FOR_TESTS_ONLY

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]