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

From: Javier Martinez Canillas <javierm@redhat.com>
To: Thomas Zimmermann <tzimmermann@suse.de>,
	Maxime Ripard <maxime@cerno.tech>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>,
	Ben Skeggs <bskeggs@redhat.com>, David Airlie <airlied@linux.ie>,
	Maxime Ripard <mripard@kernel.org>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	Emma Anholt <emma@anholt.net>, Karol Herbst <kherbst@redhat.com>,
	Samuel Holland <samuel@sholland.org>,
	Jani Nikula <jani.nikula@linux.intel.com>,
	Daniel Vetter <daniel@ffwll.ch>, Lyude Paul <lyude@redhat.com>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	Chen-Yu Tsai <wens@csie.org>
Cc: "Dom Cobley" <dom@raspberrypi.com>,
	"Dave Stevenson" <dave.stevenson@raspberrypi.com>,
	"Phil Elwell" <phil@raspberrypi.com>,
	nouveau@lists.freedesktop.org, intel-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	"Mateusz Kwiatkowski" <kfyatek+publicgit@gmail.com>,
	"Hans de Goede" <hdegoede@redhat.com>,
	"Noralf Trønnes" <noralf@tronnes.org>,
	"Geert Uytterhoeven" <geert@linux-m68k.org>,
	linux-sunxi@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 13/33] drm/client: Add some tests for drm_connector_pick_cmdline_mode()
Date: Fri, 23 Sep 2022 13:01:14 +0200	[thread overview]
Message-ID: <4bef4296-b32f-e6c9-c8d0-591e2945e0d4@redhat.com> (raw)
In-Reply-To: <93969920-b5ed-ff15-48d4-02e2f9c23505@suse.de>

On 9/23/22 12:30, Thomas Zimmermann wrote:

[...]

>>>> +
>>>> +#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.
>

That's true.

>>
>> 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.
>>>
>>
>> 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.
>

Yes, I agree with that. But probably we should be explicit about the
wrapper export symbols to access static functions pattern in the KUnit
docs so other subsystems could do the same and it becomes a convention ?

> Best regards
> Thomas
> 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat