From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 783DEA0524; Tue, 1 Jun 2021 15:11:44 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 031EA40689; Tue, 1 Jun 2021 15:11:44 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id CF86E40041 for ; Tue, 1 Jun 2021 15:11:42 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1622553102; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=tVhvtJbBpG8A6a4qNNbK4cg1QGuElVlbtxVdhuWUSR4=; b=C8algsUu1vlSOOryBLqX9j5m8AwSAUNtp4HhLbiSHe1ix2tA/ucUbkliCQuv91hD3UuSGf xpCY+AyK39UBl/0eet37Y9m4dUo3+uvps1mzf1nUBacebo0lxvAIFnDLZQqbdz3ZrEouRY hoC34z9Gv42218D1ghoSsiJ9Vc4MckY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-536-4jguOws2PRCEGY9t49Xogg-1; Tue, 01 Jun 2021 09:11:39 -0400 X-MC-Unique: 4jguOws2PRCEGY9t49Xogg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2E9C6107ACCD; Tue, 1 Jun 2021 13:11:38 +0000 (UTC) Received: from RHTPC1VM0NT (ovpn-116-19.rdu2.redhat.com [10.10.116.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0195260BD8; Tue, 1 Jun 2021 13:11:33 +0000 (UTC) From: Aaron Conole To: Ferruh Yigit Cc: dev@dpdk.org, David Marchand , Bruce Richardson , Ray Kinsella , "declan.doherty@intel.com" References: <20210210145559.4090684-1-aconole@redhat.com> <20210309155757.615536-1-aconole@redhat.com> <6f487e05-c40f-8445-247d-43f4dc3bf822@intel.com> Date: Tue, 01 Jun 2021 09:11:32 -0400 In-Reply-To: <6f487e05-c40f-8445-247d-43f4dc3bf822@intel.com> (Ferruh Yigit's message of "Mon, 31 May 2021 16:17:57 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=aconole@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain Subject: Re: [dpdk-dev] [PATCH v3] guides: add a guide for developing unit tests X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Ferruh Yigit writes: > On 3/9/2021 3:57 PM, 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. > > +1 to adding this long missing documentation, thanks. > >> >> Reviewed-by: David Marchand >> Signed-off-by: Aaron Conole >> --- >> v0->v1: Added information for TEST_SKIPPED and details about generating >> code coverage to help with ideas for writing unit test cases. >> v1->v2: Corrected some spelling, rephrased a bit after suggestions by >> Ray. >> v2->v3: Rewrite the meson build block, updated the copyright section, >> and change the title to be a bit nicer >> >> doc/guides/contributing/index.rst | 1 + >> doc/guides/contributing/testing.rst | 243 ++++++++++++++++++++++++++++ >> 2 files changed, 244 insertions(+) >> create mode 100644 doc/guides/contributing/testing.rst >> >> diff --git a/doc/guides/contributing/index.rst b/doc/guides/contributing/index.rst >> index 2fefd91931..41909d949b 100644 >> --- a/doc/guides/contributing/index.rst >> +++ b/doc/guides/contributing/index.rst >> @@ -14,6 +14,7 @@ Contributor's Guidelines >> abi_versioning >> documentation >> patches >> + testing >> vulnerability >> stable >> cheatsheet >> diff --git a/doc/guides/contributing/testing.rst b/doc/guides/contributing/testing.rst >> new file mode 100644 >> index 0000000000..0757d71ad0 >> --- /dev/null >> +++ b/doc/guides/contributing/testing.rst >> @@ -0,0 +1,243 @@ >> +.. SPDX-License-Identifier: BSD-3-Clause >> + Copyright 2021 The DPDK contributors >> + >> +DPDK Testing Guidelines >> +======================= >> + >> +This document outlines the guidelines for running and adding new >> +tests to the in-tree DPDK test suites. >> + > > I think both the section name (DPDK Testing Guidelines) and the file name > (testing.rst) is too broad comparing to what is described here. Makes sense to me. > What about using "DPDK Unit Test Guidelines", and 'unit_test.rst'? Sure - I will make this change. >> +The DPDK test suite model is loosely based on the xunit model, where >> +tests are grouped into test suites, and suites are run by runners. >> +For a basic overview, see the basic Wikipedia article on xunit: >> +`xUnit - Wikipedia `_. >> + >> + >> +Running a test >> +-------------- >> + >> +DPDK tests are run via the main test runner, the `dpdk-test` app. >> +The `dpdk-test` app is a command-line interface that facilitates >> +running various tests or test suites. >> + >> +There are two modes of operation. The first mode is as an interactive >> +command shell that allows launching specific test suites. This is >> +the default operating mode of `dpdk-test` and can be done by:: >> + >> + $ ./build/app/test/dpdk-test --dpdk-options-here >> + 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>> >> + >> +At the prompt, simply type the name of the test suite you wish to run >> +and it will execute. >> + >> +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 --dpdk-options-here >> + 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>>$ >> + > > According code, it is also possible to run the unit test by providing its name > as argument to 'dpdk-test', like bellow. And benefit of this is, it is possible > to provide multiple tests at once and run them all, which can't be done via > 'DPDK_TEST" env var. Should we document this too? Such functionality was added after I posted this series. I think it's a worthwhile addition. > ./build/app/test/dpdk-test version_autotest > EAL: Detected 96 lcore(s) > EAL: Detected 2 NUMA nodes > EAL: Detected static linkage of DPDK > EAL: Multi-process socket /var/run/dpdk/rte/mp_socket > EAL: Selected IOVA mode 'VA' > EAL: No free 2048 kB hugepages reported on node 0 > EAL: No free 2048 kB hugepages reported on node 1 > EAL: No available 2048 kB hugepages reported > APP: HPET is not enabled, using TSC as default timer > RTE>>version_autotest > Version string: 'DPDK 21.05.0' > Test OK > > > Another env variable it "DPDK_TEST_PARAMS" to provide more parameters, since > meson has args support for test exec, we are not using it in meson, but maybe > good to document it for the individual exec usage. Makes sense. > > Also there is a 'RTE_TEST_RECURSIVE' env var used, I am not clear why it is for, > anyone knows the history on it? > > >> +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'). >> + > > Also negative value > >> + >> +Running all tests >> +----------------- >> + > > What about "Running multiple test cases", instead of "all tests"? > > Btw, it is possible to run single unit test with meson, perhaps this can be > documented in previous section: > "meson test -C build version_autotest" > > Also another useful command is to list all test cases, may worth documenting > that too: > "meson test -C build --list" Agreed - I will add this. > > We have a legacy runner as well, 'autotest.py', most probably it is outdated > now, if we don't document it we may consider removing it completely > (./app/test/autotest*.py): > "python ./app/test/autotest.py ./build/app/test/dpdk-test build version" I guess it might be best to remove it. >> +In order to allow developers to quickly execute all the standard >> +internal tests without needing to remember or look up each test suite >> +name, the build system includes a standard way of executing the >> +default test suites. After building via `ninja`, the ``meson test`` >> +command will execute the standard tests and report errors. >> + >> +There are four groups of default test suites. The first group is >> +the **fast** test suite, which is the largest group of test cases. >> +These are the bulk of the unit tests to validate functional blocks. >> +The second group is the **perf** tests. These test suites can take >> +longer to run and do performance evaluations. The third group is >> +the **driver** test suite, which is mostly for special hardware >> +related testing (such as `cryptodev`). The last group are the >> +**debug** tests. These mostly are used to dump system information. >> + >> +The suites can be selected by adding the ``--suite`` option to the >> +``meson test`` command. Ex: ``meson test --suite fast-tests``:: >> + >> + $ meson test -C build --suite fast-tests >> + ninja: Entering directory `/home/aconole/git/dpdk/build' >> + [2543/2543] Linking target app/test/dpdk-test. >> + 1/60 DPDK:fast-tests / acl_autotest OK 3.17 s >> + 2/60 DPDK:fast-tests / bitops_autotest OK 0.22 s >> + 3/60 DPDK:fast-tests / byteorder_autotest OK 0.22 s >> + 4/60 DPDK:fast-tests / cmdline_autotest OK 0.28 s >> + 5/60 DPDK:fast-tests / common_autotest OK 0.57 s >> + 6/60 DPDK:fast-tests / cpuflags_autotest OK 0.27 s >> + ... >> + >> + >> +Adding test suites >> +------------------ >> + > > What about renaming 'test suites' to 'test cases', since 'test suites' is just a > group of test case and we can add single test case too. Okay. >> +To add a test suite to the DPDK test application, create a new test >> +file for that suite (ex: see *app/test/test_version.c* for the >> +``version_autotest`` test suite). There are two important functions >> +for interacting with the test harness: >> + >> + 1. REGISTER_TEST_COMMAND(command_name, function_to_execute) >> + Registers a test command with the name `command_name` and which >> + runs the function `function_to_execute` when `command_name` is >> + invoked. >> + >> + 2. unit_test_suite_runner(struct unit_test_suite \*) >> + Returns a runner for a full test suite object, which contains >> + a test suite name, setup, tear down, and vector of unit test >> + cases. >> + > > There are two different usage as "test suite" which is confusing I think, > > 1) First usage within dpdk-test app > > a) A test command can be running single test_case, like > 'version_autotest' -> 'test_version' > > b) Or it can be a group of test cases, like: Well 'link_bonding_autotest' is a "test-suite" > 'link_bonding_autotest' -> 'test_create_bonded_device', > 'test_create_bonded_device_with_invalid_params', > 'test_add_slave_to_bonded_device', > '...' > > The second one is called as unit_test_suite within the scope of the 'dpdk-test' > application, but at the end of the day it is a single command for test app. Sure, the distinction is more important for development rather than executing cases, so maybe I can make it more clear here. > First one has simple execution, but second one has setup/teardown as you already > described below. > > > 2) Second usage with meson, > > There is a next level of grouping in meson, as described in previous paragraph, > like "fast-test suite", "perf test suite" etc.., this is grouping of various > test commands. > > > I wonder if this two different "test suite" can be clarified more. Can we say > "meson test suite" to the meson one? Okay, that makes sense to me. >> +Each test suite has a setup and tear down function that runs at the >> +beginning and end of the test suite execution. Each unit test has >> +a similar function for test case setup and tear down. >> + >> +Test cases are added to the `.unit_test_cases` element of the unit >> +test suite structure. Ex: >> + >> +.. code-block:: c >> + :linenos: >> + >> + #include >> + >> + #include >> + #include >> + #include >> + #include >> + >> + #include "test.h" >> + >> + static int testsuite_setup(void) { return TEST_SUCCESS; } >> + static void testsuite_teardown(void) { } >> + >> + static int ut_setup(void) { return TEST_SUCCESS; } >> + static void ut_teardown(void) { } >> + >> + static int test_case_first(void) { return TEST_SUCCESS; } >> + >> + static struct unit_test_suite example_testsuite = { >> + .suite_name = "EXAMPLE TEST SUITE", >> + .setup = testsuite_setup, >> + .teardown = testsuite_teardown, >> + .unit_test_cases = { >> + TEST_CASE_ST(ut_setup, ut_teardown, test_case_first), >> + >> + TEST_CASES_END(), /**< NULL terminate unit test array */ >> + }, >> + }; >> + >> + static int example_tests() >> + { >> + return unit_test_suite_runner(&example_testsuite); >> + } >> + >> + REGISTER_TEST_COMMAND(example_autotest, example_tests); >> + >> +The above code block is a small example that can be used to create a >> +complete test suite with test case. >> + >> + >> +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 defined test assertions. >> + >> +Sometimes it is important to indicate that a test needs to be >> +skipped, either because the environment isn't able to support running >> +the test, or because some requisite functionality isn't available. The >> +test suite supports returning a result of `TEST_SKIPPED` during test >> +case setup, or during test case execution to indicate that the >> +preconditions of the test aren't available. Ex:: >> + >> + $ meson test -C build --suite fast-tests >> + ninja: Entering directory `/home/aconole/git/dpdk/build >> + [2543/2543] Linking target app/test/dpdk-test. >> + 1/60 DPDK:fast-tests / acl_autotest OK 3.17 s >> + 2/60 DPDK:fast-tests / bitops_autotest OK 0.22 s >> + 3/60 DPDK:fast-tests / byteorder_autotest OK 0.22 s >> + ... >> + 46/60 DPDK:fast-tests / ipsec_autotest SKIP 0.22 s >> + ... >> + >> + >> +Checking code coverage >> +---------------------- >> +The meson build system supports generating a code coverage report >> +via the `-Db_coverage=true` option, in conjunction with a package >> +like **lcov**, to generate an HTML code coverage report. Example:: >> + >> + $ meson setup build -Db_coverage=true >> + $ meson test -C build --suite fast-tests >> + $ ninja coverage-html -C build >> + >> +The above will generate an html report in the >> +`build/meson-logs/coveragereport/` directory that can be explored >> +for detailed code covered information. This can be used to assist >> +in test development. >> + >> + >> +Adding a suite to the default >> +----------------------------- >> + > > What is "the default"? I meant the meson test suite here. >> +Adding to one of the default tests involves editing the appropriate >> +meson build file `app/test/meson.build` and adding the command to >> +the correct test suite class. Once added, the new test suite will >> +be run as part of the appropriate class (fast, perf, driver, etc.). >> + > > Here "meson test suite" referenced as 'class', lets figure out the naming for it > and use it consistently. Yes, will do. >> +Some of these default test suites are run during continuous integration > > Same comment for "default test suites" usage. ACK >> +tests, making regression checking automatic for new patches submitted >> +to the project. >> > > Is there a rule to document how to decide to add (or not) a testcase to a meson > test suit? No - it's mostly up to developers and maintainers for now, although I would think the normal thing would be to add the case to the meson test suite. I will try to clarify it a bit more.