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

[Karthik B S <karthik.b.s@intel.com>]

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
>>>>