Re: [PATCH i-g-t] tests/kms_addfb_basic: Split tests into subtest groups

["Sharma, Swati2" <swati2.sharma@intel.com>]

Hi Karthik,

Patch LGTM

Reviewed-by: Swati Sharma <swati2.sharma@intel.com>

On 20-03-2025 07:39 pm, Karthik B S wrote:
> Hi Kamil,
>
> On 3/20/2025 7:00 PM, Kamil Konieczny wrote:
>> Hi Karthik,
>> On 2025-03-14 at 15:13:57 +0530, Karthik B S wrote:
>>> Hi Kamil,
>>>
>>> On 3/13/2025 2:13 AM, Kamil Konieczny wrote:
>>>> Hi Karthik,
>>>> On 2025-03-12 at 16:00:08 +0530, Karthik B S wrote:
>>>>> Multiple subtest groups exist in kms_addfb_basic with individual
>>>>> igt_fixutre calls. Split these using igt_subtest_group so that any
>>>>> failure in the fixture of one of the individual subtest groups don't
>>>>> affect the results of subtests in other groups.
>>>>>
>>>>> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
>>>>> ---
>>>>>    tests/kms_addfb_basic.c | 15 ++++++++++-----
>>>>>    1 file changed, 10 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
>>>>> index e5ff9ea32..268e4e0e4 100644
>>>>> --- a/tests/kms_addfb_basic.c
>>>>> +++ b/tests/kms_addfb_basic.c
>>>>> @@ -989,15 +989,20 @@ igt_main
>>>>>            igt_require(has_addfb2_iface(fd));
>>>>>        }
>>>>> -    invalid_tests(fd);
>>>>> +    igt_subtest_group
>>>>> +        invalid_tests(fd);
>>>>> -    pitch_tests(fd);
>>>>> +    igt_subtest_group
>>>>> +        pitch_tests(fd);
>>>>> -    prop_tests(fd);
>>>>> +    igt_subtest_group
>>>>> +        prop_tests(fd);
>>>>> -    master_tests(fd);
>>>>> +    igt_subtest_group
>>>>> +        master_tests(fd);
>>>>> -    tiling_tests(fd);
>>>>> +    igt_subtest_group
>>>>> +        tiling_tests(fd);
>>>> I looked into those tests and imho this needs a major refactor,
>>>> for example there should not be any igt_assert() in a fixture.
>>>> Generally igt tests works ok for one fixture in begin, last one
>>>> at igt_main() end. When used in-between it becomes more
>>>> complicated to handle cleanup.
>>> Thank you for the review.
>>>
>>> I tried out by inducing an error using an igt_assert(0) within the
>>> igt_fixture of a subtest group and verified that the this change 
>>> handles the
>>> issue by not failing the subtests in other subtest groups. As agreed
>>> offline, will use this as a quick fix to handle failures in the current
>>> design and for the long run will check if there is a way these fb
>>> tests(kms_addfb_basic, kms_getfb) can be redesigned to be more robust.
>>>
>>> Thanks,
>>> Karthik.B.S
>> Yes, after a second reading this could be a first step
>> followed by refactoring later, so as this could improve
>> testing a little this is
>>
>> Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>
> Thank you for the rb. Will merge this and then see how overall test 
> can be redesigned for the long run.
>
> Thanks,
> Karthik.B.S
>>
>>>> Regards,
>>>> Kamil
>>>>
>>>>>        igt_subtest_group {
>>>>>            igt_fixture
>>>>> -- 
>>>>> 2.43.0
>>>>>