Hi 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" +#endifI 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.
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.
Best regards Thomas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
