Re: [dpdk-dev] [RFC] doc: add a guide to describe developing tests

[Aaron Conole <aconole@redhat.com>]

Hi Bruce,

Thanks for the review!  Responses below.

Bruce Richardson <bruce.richardson@intel.com> writes:

> On Wed, Jan 27, 2021 at 10:16:18AM -0500, Aaron Conole wrote:
>> The DPDK testing infrastructure includes a comprehensive set of
>> libraries, utilities, and CI integrations for developers to test
>> their code changes.  This isn't well documented, however.
>> 
>> Document the basics for adding a test suite to the infrastructure
>> and enabling that test suite for continuous integration platforms
>> so that newer developers can understand how to develop test suites
>> and test cases.
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>
> Thanks for starting this Aaron, this doc is well needed. Couple of comments
> below.
>
> /Bruce
>> ---
>> Submitting as RFC as I'm not sure if this should include making
>> changes to the github actions or travis yml, or the linux-build
>> ci script.
>> 
>>  doc/guides/contributing/index.rst   |   1 +
>>  doc/guides/contributing/testing.rst | 200 ++++++++++++++++++++++++++++
>>  2 files changed, 201 insertions(+)
>>  create mode 100644 doc/guides/contributing/testing.rst
>> 
> <snip>
>> +The second form is useful for a scripting environment, and is used by
>> +the DPDK meson build system.  This mode is invoked by assigning a
>> +specific test suite name to the environment variable `DPDK_TEST`
>> +before invoking the `dpdk-test` command, such as::
>> +
>> +  $ DPDK_TEST=version_autotest ./build/app/test/dpdk-test --no-huge
>> +  EAL: Detected 4 lcore(s)
>> +  EAL: Detected 1 NUMA nodes
>> + EAL: Static memory layout is selected, amount of reserved memory
>> can be adjusted with -m or --socket-mem
>> +  EAL: Multi-process socket /run/user/26934/dpdk/rte/mp_socket
>> +  EAL: Selected IOVA mode 'VA'
>> +  EAL: Probing VFIO support...
>> +  EAL: PCI device 0000:00:1f.6 on NUMA socket -1
>> +  EAL:   Invalid NUMA socket, default to 0
>> +  EAL:   probe driver: 8086:15d7 net_e1000_em
>> +  APP: HPET is not enabled, using TSC as default timer
>> +  RTE>>version_autotest
>> +  Version string: 'DPDK 20.02.0-rc0'
>> +  Test OK
>> +  RTE>>$
>> +
>> +The above shows running a specific test case.  On success, the return
>> +code will be '0', otherwise it will be set to some error value (such
>> +as '255').
>> +
>
> This reminds me that I have patch almost ready to submit to extend this
> support to allow passing the test name - or even multiple test names - on
> the commandline, not just through the environment. Will upstream it
> shortly, I hope. I think command-line is more usable for folks than the
> environment. Thoughts?

I agree, such a change would be really useful as a developer.  I have no
preference when it comes to the scripting side though (which is what
this section is about).  Either we set env in the script, or we pass as
command line.

>> +
> <snip>
>> +
>> +Designing a test
>> +----------------
>> +
>> +Test cases have multiple ways of indicating an error has occurred,
>> +in order to reflect failure state back to the runner.  Using the
>> +various methods of indicating errors can assist in not only validating
>> +the requisite functionality is working, but also to help debug when
>> +a change in environment or code has caused things to go wrong.
>> +
>> +The first way to indicate a generic error is by returning a test
>> +result failure, using the *TEST_FAILED* error code.  This is the most
>> +basic way of indicating that an error has occurred in a test routine.
>> +It isn't very informative to the user, so it should really be used in
>> +cases where the test has catastrophically failed.
>> +
>> +The preferred method of indicating an error is via the
>> +`RTE_TEST_ASSERT` family of macros, which will immediately return
>> +*TEST_FAILED* error condition, but will also log details about the
>> +failure.  The basic form is:
>> +
>> +.. code-block:: c
>> +
>> +   RTE_TEST_ASSERT(cond, msg, ...)
>> +
>> +In the above macro, *cond* is the condition to evaluate to **true**.
>> +Any generic condition can go here.  The *msg* parameter will be a
>> +message to display if *cond* evaluates to **false**.  Some specialized
>> +macros already exist.  See `lib/librte_eal/include/rte_test.h` for
>> +a list of pre-build test assertions.
>> +
>
> Maybe also mention TEST_SKIPPED return value.

Okay, I will add some description about it.