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. 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.
Best regards Thomas
Also, this deviates from the existing practice we had for selftests and EXPORT_SYMBOL_FOR_TESTS_ONLY Maxime
-- 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
