From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1210410E770 for ; Fri, 1 Sep 2023 10:08:50 +0000 (UTC) Date: Fri, 1 Sep 2023 12:08:45 +0200 From: Mauro Carvalho Chehab To: "Modem, Bhanuprakash" Message-ID: <20230901120845.71ddfe41@maurocar-mobl2> In-Reply-To: <4c6de075-f0b3-fae0-cf6c-48f177f3ced3@intel.com> References: <20230901051808.1369104-1-bhanuprakash.modem@intel.com> <20230901092019.44f35b7e@maurocar-mobl2> <4c6de075-f0b3-fae0-cf6c-48f177f3ced3@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [igt-dev] [i-g-t] scripts/test_list: Allow unrecognized field:values in testplan List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: igt-dev@lists.freedesktop.org Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On Fri, 1 Sep 2023 14:32:09 +0530 "Modem, Bhanuprakash" wrote: > Hi Mauro, > > On Fri-01-09-2023 12:50 pm, Mauro Carvalho Chehab wrote: > > On Fri, 1 Sep 2023 10:48:08 +0530 > > Bhanuprakash Modem wrote: > > > >> As non-Intel vendors also contributing to KMS tests, allow > >> them to re-use the existing testplan documentation along with > >> their own definitions of field:values pair in test config json. > >> > >> Instead of aborting, just throw a warning about this unrecognized > >> field:values. > >> > >> Example: > >> tests/kms_color.c:994: Warning: unrecognized field in tests/kms_test_config.json ==> Foo: bla > > > > IMO, there's no need to to that. I mean, if someone wants to add > > a new field, he can just patch tests/kms_test_config.json, adding the > > new "Foo" field at kms_test_config.json: > > Non-Intel people may not use kms_test_config.json since it is designed > for Intel supported (tests/kms_*.c & tests/intel/kms_*.c) tests > (Probably we may need to rename it to intel_kms_test_config.json). The infra is designed to be generic, and not Intel driver specific. I'm even sending patches to Kernel upstream to also use it: https://lore.kernel.org/linux-doc/cover.1693550658.git.mchehab@kernel.org/T/#t > So, non-Intel vendors are also allowed to include few kms tests > (tests/kms_*.c) in their own testplan. Also, we can't restrict them to > use the same testplan as kms_test_config.json. Hmm... so you're thinking about having a tests/kms_foo.c file that will be included on both: vendor_a/kms_test_config.json and: vendor_b/kms_test_config.json Yes, that's possible. We actually have one or two cases at i915 and xe for some tests that aren't driver-specific. As the i915 JSON config file has an extra field ("Feature"), I added it to Xe driver as well. This is used currently only for those generic tests that are used on both. > Example: > Suppose, some non-Intel vendor (xyz) created a xyz_kms_test_config.json > with the "Foo" field defined in it, and updated the documentation of > tests/kms_*.c accordingly. Since Intel's kms_test_config.json is also > using tests/kms_*.c and not aware of "Foo" can lead the failure. Currently, the developer who created/updated vendor_a/kms_test_config.json and tests/kms_foo.c will have a compilation error pointing to it. Such developer will then know that the new field also needs to be at vendor_b/kms_test_config.json and can submit a patch against it to solve the error. If we apply your patch, it means that it will generate a warning and someone from vendor_b will need later to identify what happened and copy the (possible) definition from vendor_a/kms_test_config.json to suppress the warning. It works, but it means an extra step. So, while I would prefer to not merge this change, I won't oppose on converting it into a warning. If you decide to go ahead and merge it, you can add my acked-by. > The problem statement is to have different testplans with their own set > of field:values and share the documentation. > > - Bhanu > > > > > diff --git a/tests/kms_test_config.json b/tests/kms_test_config.json > > index 9219ae4ebd9d..1dfca84dd73c 100644 > > --- a/tests/kms_test_config.json > > +++ b/tests/kms_test_config.json > > @@ -35,6 +35,7 @@ > > "description": "Defines the test category. Usually used at subtest level." > > } > > }, > > + "Foo" : { }, > > "Description" : { > > "_properties_": { > > "description": "Provides a description for the test/subtest." > > > > NOTE: It would probably make sense to add a description to it, to make > > clear what such "Foo" field means, in a similar way to the descriptions > > added to the other fields. > > > > As the fields are optional, this won't require any changes at the > > existing tests, and will provide an extra benefit that the meaning of > > the "Foo" field can be documented via _properties_/description field > > > Regards, > > Mauro > > > >> > >> Cc: Mauro Carvalho Chehab > >> Signed-off-by: Bhanuprakash Modem > >> --- > >> scripts/test_list.py | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/scripts/test_list.py b/scripts/test_list.py > >> index 0bcc96869..517c4d067 100755 > >> --- a/scripts/test_list.py > >> +++ b/scripts/test_list.py > >> @@ -1219,8 +1219,8 @@ class TestList: > >> continue > >> > >> file_line.rstrip(r"\n") > >> - sys.exit(f"{fname}:{file_ln + 1}: Error: unrecognized line. Need to add field at %s?\n\t==> %s" % > >> - (self.config_fname, file_line)) > >> + print(f"{'/'.join(fname.split('/')[-2:])}:{file_ln + 1}: Warning: unrecognized field in %s ==> %s" % > >> + ('/'.join(self.config_fname.split('/')[-2:]), file_line)) > >> > >> def show_subtests(self, sort_field): > >>