Re: [Intel-xe] [PATCH 1/7] drm/xe: Add basic unit tests for rtp

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 02.04.2023 03:34, Lucas De Marchi wrote:
> On Sat, Apr 01, 2023 at 08:26:24PM +0200, Michał Winiarski wrote:
>> On Sat, Apr 01, 2023 at 01:54:59AM -0700, Lucas De Marchi wrote:
>>> On Fri, Mar 31, 2023 at 08:34:21AM -0600, Lucas De Marchi wrote:
>>> > On Mon, Mar 27, 2023 at 07:56:01PM +0200, Michał Winiarski wrote:
>>> > > On Tue, Mar 21, 2023 at 03:05:21PM -0700, Lucas De Marchi wrote:
>>> > > > +EXPORT_SYMBOL(xe_rtp_process_incompat_types);
>>> > > > diff --git a/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>>> b/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>>> > > > new file mode 100644
>>> > > > index 000000000000..4a12aad3f759
>>> > > > --- /dev/null
>>> > > > +++ b/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>>> > >
>>> > > There's no need to have this as a separate file.
>>> > > We can move the contents of tests/xe_rtp.c into
>>> tests/xe_rtp_test.c and
>>> > > include tests/xe_rtp_test.c directly in xe_rtp.c
>>> >
>>> > this is not how the tests are currently structured.
>>> >
>>> > xe_rtp.c includes tests/xe_rtp.c. The latter can be seen as "an
>>> > extension of the that file, exporting whatever entrypoint is needed
>>> > for the real test". They are then part of the xe.ko module.
>>> >
>>> > tests/xe_rtp_test.c is the one in the separate test module, and
>>> contains
>>> > the integration with kunit.
>>>
>>> but it's possible to keep everythin in the separate module and not need
>>> the include of tests/xe_rtp.c. I did that on v2.
>>>
>>> Lucas De Marchi
>>
>> From my perspective - the way that the test is implemented carries an
>> additional information.
>> 1) Test is a separate module:
>> Means that we're testing the API exported by the module (that's used by
>> other modules).
>> 2) Test is a file that's a part of a module:
>> Means that we're testing non-static API used internally by the module.
>> 3) Test is a file that's included at the end of a file:
>> Means that we're testing static functions.
>>
>> Forcing everything to be a module doesn't really serve any purpose.
>> It just makes things harder to follow, as it removes this useful
>> additional bit of information.
> 
> I strongly disagree here.  Basing the test location on how much a
> function is exposed is a recipe for having to move the tests from one
> place to another. There are always refactors that make function change
> from non-static -> static, static -> non-static, and to a smaller degree
> exporting it.
> 
> In the end the code will just be inconsistent with the rules you
> outlined.
> 
> Separating everything to a separate module means the test code doesn't
> bloat the xe.ko module. I do think we should group all the tests into a
> single xe_test.ko, but that is a different topic.
> 
> See v2 of this patch series where I move *all* of the rtp tests to the
> separate module and simply use EXPORT_SYMBOL_IF_KUNIT() to export the 2

but having to export static functions simply don't scale, it's fine if
all you test needs is 2 functions, but to have full coverage we might
end with exporting all static functions (and to some extend bloating
xe.ko anyway, both in code and binary)

there is already BKM for testing static functions [1]
so why are we trying to force different (and IMO worst) model ?

[1]
https://docs.kernel.org/dev-tools/kunit/usage.html#testing-static-functions

> functions I need (In this v1 I was using
> EXPORT_SYMBOL_NS_GPL(..., XE_KUNIT) to make sure the tests were in a
> separate namespace, but then I noticed kunit itself has a recommended
> way by using its macro).
> 
> This makes the rule very simple to be followed:
> 
> 1) Place everything you can in the separate test module

but why? as Michal pointed out, this works best for testing already
exported API functions, to test internal static functions, this would be
unnecessary complicated solution

> 2) Place it in end of the file with the #include otherwise

this should be our primary BKM for mock/fake/sw/self/kunit/unit(*) test.

(*) pick one

note that in addition to easy maintenance (it doesn't require writing,
building, distributing and loading separate modules), this single test
file already has access to all required compilation unit static
functions that we want to test.

also from IGT point of view, this would mean that in case of
adding/removing some subtests all we need is just update test filter
rather then updating list of modules to load/unload.

and I wouldn't count this final #include as .ko bloating factor as it
would be primary used in UML build, where we don't care, and for the
target builds this could be easily compiled out.

thanks,
Michal

> 
> Lucas De Marchi