Re: [LTP] [PATCH v4] syscalls/statx09: Add new test

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

[-- Attachment #1.1: Type: text/plain, Size: 3027 bytes --]

Hi!

I also think that support for an array of functions is needed to cover 
all scenarios and cleanup the code a little bit. The real problem with 
tcases is that sometimes we are doing what we might do with multiple 
functions, but using an approach which is expecting struct and some sort 
of "filtering" in .test_all function.

And in some cases, where one particular testcase differs by a statement 
from an another, struct needs a flag to filter out the specific 
testcase. This would be easy to handle with two different functions.

Also the output message sometimes is stored into the struct, in order to 
show the correct TPASS/TFAIL message we need, according with the tcase. 
And this is probably an overengineering solution, since that would be 
handled well using multiple testcases functions, testing different 
scenarios and using different output messages.

Also simple tests, such as input arguments unit tests, would benefit 
from array of tests functions, since we can split tcases into multiple 
functions and make code more readable.

To sum up things, I think that having support for an array of test 
functions can cleanup code in many tests and make them easier to 
read/maintain. tcases can still do well sometimes, but adding the 
support for an array of functions can improve the LTP framework and so 
the way we are testing the kernel.

Andrea

On 2/7/22 12:55, Petr Vorel wrote:
> Hi all,
>
>> Hi!
>>>> +static void run(unsigned int i)
>>>> +{
>>>> +	tcases[i].tfunc();
>>>> +}
>>> OT: we may lack something in the API, when function like this need to be
>>> defined.
>> See:
>> https://lists.linux.it/pipermail/ltp/2017-October/005829.html
>> https://lists.linux.it/pipermail/ltp/2017-July/005132.html
> https://lore.kernel.org/ltp/860483630.25581747.1507017497043.JavaMail.zimbra@redhat.com/
> https://lore.kernel.org/ltp/20170727081437.27995-1-chrubis@suse.cz/
>
> Very nice that you remember your old work :) (we didn't have patchwork back then).
>
> Now I remember it - you already implemented it in 5 years old RFC, Jan didn't
> see a value and that's why it haven't been merged.
>
> Yes, Jan is right that it complicates code a bit, but even if you replace this
> code:
>
> statx09.c
> static struct test_cases {
>      void (*tfunc)(void);
> } tcases[] = {
>      {&test_flagged},
>      {&test_unflagged},
> };
>
> static void run(unsigned int i)
> {
>      tcases[i].tfunc();
> }
>
> with .test_all where you have the switch it still kind of boilerplate. Thus I
> agree with cyrils argument:
>
> https://lore.kernel.org/ltp/20171003125958.GB11692@rei/
>
> 	"aiming to avoid the need to have a switch () in each testcase that
> 	implements a similar tests but cannot be easily data driven (as we do
> 	for most of tests that loop over an array of structures describing the
> 	test data)"
>
> Thus, not sure if we want to rething the implementation, but I'd be for adding
> the support (sure doc and docparse adoption would need to be added but that's
> obvious).
>
> Kind regards,
> Petr
>

[-- Attachment #1.2: Type: text/html, Size: 4814 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp