Re: [dpdk-dev] [PATCH 05/13] app/test: introduce librte_security tests

[Thomas Monjalon <thomas@monjalon.net>]

02/04/2020 21:49, Lukasz Wojciechowski:
> 
> W dniu 01.04.2020 o 19:51, Thomas Monjalon pisze:
> > 01/04/2020 19:09, Akhil Goyal:
> >> Hi Lukasz,
> >>
> >>> This patch introduces set of unit tests of librte_security API functions.
> >>> Tests are added to dpdk-test application and can be run with
> >>> "security_autotest" runtime command.
> >>>
> >>> This is the first patch in the series of patches as adding all test cases
> >>> for all API functions in a single patch would make it unreadable.
> >>>
> >>> This patch defines structure of the file and necessary test framework
> >>> initialization. It also contains first subset of unit tests for
> >>> rte_security_session_create API function.
> >>>
> >>> Structure of the tests file is following:
> >>> - macros for making tests more readable;
> >>> - mockup structures and functions for rte_security_ops;
> >>> - test suite and test cases setup and teardown functions;
> >>> - tests functions;
> >>> - declaration of testcases.
> >>>
> >>> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> >>> Change-Id: I3a4585f56ef1a75b2984fcea3a3e4a30e4c2d8a6
> >>> ---
> >> This patchset has a lot of repeated(for each API) tests just to check the input parameters to
> >> Rte_security APIs. I am not sure what value addition is done to separate out each API as a separate
> >> Negative Test. Instead a single case can be added to test all APIs with inappropriate arguments.
> >> We should add more positive cases with proper session parameters.
> >>
> >> Thomas,
> >> Do we allow these type of test cases in other modules?
> > I did not review these patches, but I think we can try to compare
> > with tests done on eventdev library.
> > It would be interesting to have an opinion from Declan.
> >
> > These rte_security tests look quite big.
> > However I don't know what is too big for test code?
> > Lukasz, please could you explain the initial motivation when writing
> > these tests? Are you especially interested in rte_security?
> > Or do you plan to reproduce this effort on other libraries?
> >
> >
> Hi Akhil and Thomas,
> 
> I've just started using dpdk this year and was writing few small apps to 
> get familiar with API.
> I noticed few things that can be fixed in the security lib and though 
> that they can be fixed easily, so I prepared patches.
> 
> After that I searched for some unit tests of the lib to add some tests 
> that cover my code. I always try to do so
> and when I found no tests for security lib I made some.
> 
> I don't know if you need them or is this the right place to put them, 
> but I did. And as I made the tests I tried to cover 100% code of 
> security lib.
> 
> Currently I don't have any plans to add or change anything in 
> rte_security. These patches are just a "collateral damage" to my process 
> of learning dpdk and a strong believe that if you can share some fixes 
> you should do it, because that the open source power.
> 
> So it is totaly up to you, what we can do with these patches. I would be 
> glad to work on them and change them the way you think they should look 
> like, e.g. I can squash all the parameters checks to a single negative 
> case as you, Akhil suggested. If you would like to see similar patches 
> in other libraries I can help with that in some spare time. I'm the new 
> guy here, so please guide me a bit ;)

Thanks a lot Lukasz.
In general we need more tests in DPDK.

I won't be able to review these patches myself,
so I hope many developers will review and guide you.

My advice is to review at least one or two other patchsets.
Hopefully someone will do the same for you.
If it does not work, please ping Akhil and me.
As a last resort, we could prioritize merging patches of
contributors helping others.