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

[Lucas De Marchi]

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
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
2) Place it in end of the file with the #include otherwise

Lucas De Marchi