[PATCH 1/1] Added tests to handle error string conditions in xe_sysfs_scheduler.c

[Kamil Konieczny]

Hi Piatkowski,,
On 2025-03-27 at 08:15:35 +0000, Piatkowski, Dominik Karol wrote:
> Hi Sobin,
> 
> > -----Original Message-----
> > From: Thomas, Sobin <sobin.thomas at intel.com>
> > Sent: Wednesday, March 26, 2025 7:03 PM
> > To: igt-dev at lists.freedesktop.org
> > Cc: Konieczny, Kamil <kamil.konieczny at intel.com>; Piatkowski, Dominik Karol
> > <dominik.karol.piatkowski at intel.com>
> > Subject: [PATCH 1/1] Added tests to handle error string conditions in
> > xe_sysfs_scheduler.c
> 
> Just like in cover letter, please fix the subject for future submissions.
> 
> > 
> > From: "Thomas, Sobin" <sobin.thomas at intel.com>
> 
> "From" field shouldn't be here.
> 
> The patch is missing commit description. Please fix it.

Well, sometimes you can use 'From:' - what is important it should be
the same as s-o-b below.

> 
> > 
> > Signed-off-by: Thomas, Sobin <sobin.thomas at intel.com>
> 
> I believe the formatting we use for name and surname inside tags is "Sobin Thomas".
> 

There should be no ',' chars, imho just set it up globally:

git config --global user.name "FIRST_NAME LAST_NAME"

or if your culture uses LAST_NAME in first place:

git config --global user.name "LAST_NAME FIRST_NAME"

Up to you, once set please keep it.

Regards,
Kamil

> > ---
> >  tests/intel/xe_sysfs_scheduler.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/tests/intel/xe_sysfs_scheduler.c b/tests/intel/xe_sysfs_scheduler.c
> > index 4fc764f82..a0eaad5ab 100644
> > --- a/tests/intel/xe_sysfs_scheduler.c
> > +++ b/tests/intel/xe_sysfs_scheduler.c
> > @@ -10,6 +10,11 @@
> >   * Sub-category: SysMan tests
> >   * Functionality: scheduler control interface
> >   *
> > + * SUBTEST: %s-invalid-string
> > + * Description: Test to check if %s arg[1] schedule parameter checks for
> > + * min max values.
> 
> You need to indent this line, or build will fail.
> 
> > + * Test category: Negative string test
> > + *
> >   * SUBTEST: %s-invalid
> >   * Description: Test to check if %s arg[1] schedule parameter rejects any
> >   *		unrepresentable intervals.
> > @@ -121,6 +126,23 @@ static void test_min_max(int xe, int engine, const
> > char **property,
> >  	igt_assert_eq(set, default_max);
> >  }
> > 
> > +static void test_invalid_string(int xe, int engine, const char **property,
> > +                         uint16_t class, int gt)
> 
> Please use tabs to indent code where possible (unless it's less than one tab worth of spaces after tabs for correct alignment).
> 
> > +{
> > +	unsigned int saved, set;
> > +	char invalid_input[] = "999abc"; //Intentionally passing wrong string
> 
> I think this comment is redundant.
> 
> > +
> > +	for(int i=0;i<3;i++) {
> 
> There are missing spaces inside this statement. Please format it.
> 
> > +		igt_assert(igt_sysfs_scanf(engine,property[i],"%u",&saved) ==
> > 1);
> 
> Same here.
> 
> > +		igt_info("Initial %s:%u\n",property[i],saved);
> 
> And here.
> 
> > +		igt_sysfs_printf(engine, property[i], "%s", invalid_input);
> > +		igt_sysfs_scanf(engine,property[i],"%u",&set);
> 
> And here.
> 
> > +		igt_assert_eq(set,saved);
> 
> And here.
> 
> > +		igt_info("Property %s correctly rejected input
> > %s\n",property[i],invalid_input);
> 
> And here.
> 
> Also, it would be a good idea to check return values of igt_sysfs_printf/igt_sysfs_scanf, for example checking if igt_sysfs_printf failed (returned negative value) - as I assume we expect it to fail. If second igt_sysfs_scanf fails, and we won't know about it, we will deal with uninitialized values, and that will be a headache of its own.
> 
> Thanks,
> Dominik Karol
> 
> > +	}
> > +}
> > +
> > +
> >  #define MAX_GTS 8
> >  igt_main
> >  {
> > @@ -130,6 +152,7 @@ igt_main
> >  	} tests[] = {
> >  		{ "invalid", test_invalid },
> >  		{ "min-max", test_min_max },
> > +		{ "invalid-string", test_invalid_string },
> >  		{ }
> >  	};
> > 
> > --
> > 2.34.1
>