From: Aaron Conole <aconole@redhat.com>
To: Ciara Power <ciara.power@intel.com>
Cc: dev@dpdk.org, declan.doherty@intel.com, gakhil@marvell.com,
hemant.agrawal@nxp.com, anoobj@marvell.com, ruifeng.wang@arm.com,
asomalap@amd.com, ajit.khaparde@broadcom.com, g.singh@nxp.com,
Bruce Richardson <bruce.richardson@intel.com>
Subject: Re: [dpdk-dev] [PATCH v2 2/6] test: introduce parent testsuite format
Date: Mon, 05 Apr 2021 08:30:05 -0400 [thread overview]
Message-ID: <f7twntgrk1e.fsf@dhcp-25.97.bos.redhat.com> (raw)
In-Reply-To: <20210402142424.1353789-3-ciara.power@intel.com> (Ciara Power's message of "Fri, 2 Apr 2021 14:24:20 +0000")
Ciara Power <ciara.power@intel.com> writes:
> The current structure for unit testing only allows for running a
> test suite with nested test cases. This means all test cases for an
> autotest must be in one suite, which is not ideal.
> For example, in some cases we may want to run multiple lists of test
> cases that each require different setup, so should be in separate suites.
>
> The unit test suite struct is modified to hold a pointer to a list of
> sub-testsuite pointers, along with the list of testcases as before.
> Both should not be used at once, if there are sub-testsuite pointers,
> that takes precedence over testcases.
>
> Signed-off-by: Ciara Power <ciara.power@intel.com>
>
> ---
> v2:
> - Added macro to loop sub-testsuites.
> - Added sub-testsuite summary detail.
> ---
> app/test/test.c | 168 ++++++++++++++++++++++++++++++++++--------------
> app/test/test.h | 1 +
> 2 files changed, 122 insertions(+), 47 deletions(-)
>
> diff --git a/app/test/test.c b/app/test/test.c
> index a795cba1bb..e401de0fdf 100644
> --- a/app/test/test.c
> +++ b/app/test/test.c
> @@ -41,6 +41,11 @@ extern cmdline_parse_ctx_t main_ctx[];
> suite->unit_test_cases[iter].testcase; \
> iter++, case = suite->unit_test_cases[iter])
>
> +#define FOR_EACH_SUITE_TESTSUITE(iter, suite, sub_ts) \
> + for (iter = 0, sub_ts = suite->unit_test_suites[0]; \
> + suite->unit_test_suites[iter]->suite_name != NULL; \
> + iter++, sub_ts = suite->unit_test_suites[iter])
> +
> const char *prgname; /* to be set to argv[0] */
>
> static const char *recursive_call; /* used in linux for MP and other tests */
> @@ -214,21 +219,46 @@ main(int argc, char **argv)
>
> static void
> unit_test_suite_count_tcs_on_setup_fail(struct unit_test_suite *suite,
> - int test_success)
> + int test_success, unsigned int *sub_ts_failed,
> + unsigned int *sub_ts_skipped, unsigned int *sub_ts_total)
> {
> struct unit_test_case tc;
> -
> - FOR_EACH_SUITE_TESTCASE(suite->total, suite, tc) {
> - if (!tc.enabled || test_success == TEST_SKIPPED)
> - suite->skipped++;
> - else
> - suite->failed++;
> + struct unit_test_suite *ts;
> + int i;
> +
> + if (suite->unit_test_suites) {
> + FOR_EACH_SUITE_TESTSUITE(i, suite, ts) {
> + unit_test_suite_count_tcs_on_setup_fail(
> + ts, test_success, sub_ts_failed,
> + sub_ts_skipped, sub_ts_total);
> + suite->total += ts->total;
> + suite->failed += ts->failed;
> + suite->skipped += ts->skipped;
> + if (ts->failed)
> + (*sub_ts_failed)++;
> + else
> + (*sub_ts_skipped)++;
> + (*sub_ts_total)++;
> + }
> + } else {
> + FOR_EACH_SUITE_TESTCASE(suite->total, suite, tc) {
> + if (!tc.enabled || test_success == TEST_SKIPPED)
> + suite->skipped++;
> + else
> + suite->failed++;
> + }
> }
> }
>
> static void
> unit_test_suite_reset_counts(struct unit_test_suite *suite)
> {
> + struct unit_test_suite *ts;
> + int i;
> +
> + if (suite->unit_test_suites)
> + FOR_EACH_SUITE_TESTSUITE(i, suite, ts)
> + unit_test_suite_reset_counts(ts);
> suite->total = 0;
> suite->executed = 0;
> suite->succeeded = 0;
> @@ -240,9 +270,12 @@ unit_test_suite_reset_counts(struct unit_test_suite *suite)
> int
> unit_test_suite_runner(struct unit_test_suite *suite)
> {
> - int test_success;
> + int test_success, i, ret;
> const char *status;
> struct unit_test_case tc;
> + struct unit_test_suite *ts;
> + unsigned int sub_ts_succeeded = 0, sub_ts_failed = 0;
> + unsigned int sub_ts_skipped = 0, sub_ts_total = 0;
>
> unit_test_suite_reset_counts(suite);
>
> @@ -259,70 +292,111 @@ unit_test_suite_runner(struct unit_test_suite *suite)
> * mark them as failed/skipped
> */
> unit_test_suite_count_tcs_on_setup_fail(suite,
> - test_success);
> + test_success, &sub_ts_failed,
> + &sub_ts_skipped, &sub_ts_total);
> goto suite_summary;
> }
> }
>
> printf(" + ------------------------------------------------------- +\n");
>
> - FOR_EACH_SUITE_TESTCASE(suite->total, suite, tc) {
> - if (!tc.enabled) {
> - suite->skipped++;
> - continue;
> - } else {
> - suite->executed++;
> + if (suite->unit_test_suites) {
> + FOR_EACH_SUITE_TESTSUITE(i, suite, ts) {
> + ret = unit_test_suite_runner(ts);
> + if (ret == TEST_SUCCESS)
> + sub_ts_succeeded++;
> + else if (ret == TEST_SKIPPED)
> + sub_ts_skipped++;
> + else
> + sub_ts_failed++;
> + sub_ts_total++;
> }
> + } else {
> + FOR_EACH_SUITE_TESTCASE(suite->total, suite, tc) {
> + if (!tc.enabled) {
> + suite->skipped++;
> + continue;
> + } else {
> + suite->executed++;
> + }
> +
> + /* run test case setup */
> + if (tc.setup)
> + test_success = tc.setup();
> + else
> + test_success = TEST_SUCCESS;
> +
> + if (test_success == TEST_SUCCESS) {
> + /* run the test case */
> + test_success = tc.testcase();
> + if (test_success == TEST_SUCCESS)
> + suite->succeeded++;
> + else if (test_success == TEST_SKIPPED)
> + suite->skipped++;
> + else if (test_success == -ENOTSUP)
> + suite->unsupported++;
> + else
> + suite->failed++;
> + } else if (test_success == -ENOTSUP) {
> + suite->unsupported++;
> + } else {
> + suite->failed++;
> + }
>
> - /* run test case setup */
> - if (tc.setup)
> - test_success = tc.setup();
> - else
> - test_success = TEST_SUCCESS;
> + /* run the test case teardown */
> + if (tc.teardown)
> + tc.teardown();
>
> - if (test_success == TEST_SUCCESS) {
> - /* run the test case */
> - test_success = tc.testcase();
> if (test_success == TEST_SUCCESS)
> - suite->succeeded++;
> + status = "succeeded";
> else if (test_success == TEST_SKIPPED)
> - suite->skipped++;
> + status = "skipped";
> else if (test_success == -ENOTSUP)
> - suite->unsupported++;
> + status = "unsupported";
> else
> - suite->failed++;
> - } else if (test_success == -ENOTSUP) {
> - suite->unsupported++;
> - } else {
> - suite->failed++;
> - }
> + status = "failed";
>
> - /* run the test case teardown */
> - if (tc.teardown)
> - tc.teardown();
> -
> - if (test_success == TEST_SUCCESS)
> - status = "succeeded";
> - else if (test_success == TEST_SKIPPED)
> - status = "skipped";
> - else if (test_success == -ENOTSUP)
> - status = "unsupported";
> - else
> - status = "failed";
> -
> - printf(" + TestCase [%2d] : %s %s\n", suite->total,
> - tc.name, status);
> + printf(" + TestCase [%2d] : %s %s\n", suite->total,
> + tc.name, status);
> + }
> }
>
> /* Run test suite teardown */
> if (suite->teardown)
> suite->teardown();
>
> + if (suite->unit_test_suites)
> + FOR_EACH_SUITE_TESTSUITE(i, suite, ts) {
> + suite->total += ts->total;
> + suite->succeeded += ts->succeeded;
> + suite->failed += ts->failed;
> + suite->skipped += ts->skipped;
> + suite->unsupported += ts->unsupported;
> + suite->executed += ts->executed;
> + }
> +
> goto suite_summary;
>
> suite_summary:
> printf(" + ------------------------------------------------------- +\n");
> printf(" + Test Suite Summary : %s\n", suite->suite_name);
> + printf(" + ------------------------------------------------------- +\n");
> +
> + if (suite->unit_test_suites) {
> + FOR_EACH_SUITE_TESTSUITE(i, suite, ts)
> + printf(" + %s : %d/%d passed, %d/%d skipped, "
> + "%d/%d failed, %d/%d unsupported\n",
> + ts->suite_name, ts->succeeded, ts->total,
> + ts->skipped, ts->total, ts->failed, ts->total,
> + ts->unsupported, ts->total);
> + printf(" + ------------------------------------------------------- +\n");
> + printf(" + Sub Testsuites Total : %2d\n", sub_ts_total);
> + printf(" + Sub Testsuites Skipped : %2d\n", sub_ts_skipped);
> + printf(" + Sub Testsuites Passed : %2d\n", sub_ts_succeeded);
> + printf(" + Sub Testsuites Failed : %2d\n", sub_ts_failed);
> + printf(" + ------------------------------------------------------- +\n");
> + }
> +
> printf(" + Tests Total : %2d\n", suite->total);
> printf(" + Tests Skipped : %2d\n", suite->skipped);
> printf(" + Tests Executed : %2d\n", suite->executed);
> diff --git a/app/test/test.h b/app/test/test.h
> index 10c7b496e6..e9bfc365e4 100644
> --- a/app/test/test.h
> +++ b/app/test/test.h
> @@ -144,6 +144,7 @@ struct unit_test_suite {
> unsigned int skipped;
> unsigned int failed;
> unsigned int unsupported;
> + struct unit_test_suite **unit_test_suites;
If it's only ever possible to either have test_suites or test_cases for
iterators, maybe it's best to put in an assert for that.
Either way, we probably don't need to
if (suite->unit_test_suites) {
...
} else {
...
}
It might be better to just look like:
FOR_EACH_SUITE_TESTSUITE(...) {
...
}
FOR_EACH_SUITE_TESTCASE(...) {
...
}
Code looks nicer, and we can also support a mix of suite/case (unless
there is a reason not to do that). WDYT? As the code exists, if I were
to write a new test suite with some sub-test suites, I might do:
overall_suite {
.unit_test_suite = {
{sub_suite1, ...},
{sub_suite2, ...}
},
.unit_test_cases = {
{test_case_1, ...},
{test_case_2, ...}
}
}
And then I would be surprised if they didn't run.
> struct unit_test_case unit_test_cases[];
> };
next prev parent reply other threads:[~2021-04-05 12:30 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-02 14:24 [dpdk-dev] [PATCH v2 0/6] test: refactor crypto unit test framework Ciara Power
2021-04-02 14:24 ` [dpdk-dev] [PATCH v2 1/6] app/test: refactor of unit test suite runner Ciara Power
2021-04-05 12:10 ` Aaron Conole
2021-04-02 14:24 ` [dpdk-dev] [PATCH v2 2/6] test: introduce parent testsuite format Ciara Power
2021-04-05 12:30 ` Aaron Conole [this message]
2021-04-02 14:24 ` [dpdk-dev] [PATCH v2 3/6] test/crypto: refactor to use sub-testsuites Ciara Power
2021-04-13 17:51 ` [dpdk-dev] [EXT] " Akhil Goyal
2021-04-13 18:17 ` Thomas Monjalon
2021-04-14 11:12 ` Doherty, Declan
2021-04-14 11:18 ` Akhil Goyal
2021-04-14 11:20 ` Thomas Monjalon
2021-04-14 11:22 ` Akhil Goyal
2021-04-02 14:24 ` [dpdk-dev] [PATCH v2 4/6] test/crypto: move testsuite params to header file Ciara Power
2021-04-02 14:24 ` [dpdk-dev] [PATCH v2 5/6] test/crypto: fix return value on test skipped Ciara Power
2021-04-13 17:07 ` [dpdk-dev] [EXT] " Akhil Goyal
2021-04-02 14:24 ` [dpdk-dev] [PATCH v2 6/6] test/crypto: dynamically build blockcipher suite Ciara Power
2021-04-06 1:34 ` [dpdk-dev] [PATCH v2 0/6] test: refactor crypto unit test framework Ruifeng Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f7twntgrk1e.fsf@dhcp-25.97.bos.redhat.com \
--to=aconole@redhat.com \
--cc=ajit.khaparde@broadcom.com \
--cc=anoobj@marvell.com \
--cc=asomalap@amd.com \
--cc=bruce.richardson@intel.com \
--cc=ciara.power@intel.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=g.singh@nxp.com \
--cc=gakhil@marvell.com \
--cc=hemant.agrawal@nxp.com \
--cc=ruifeng.wang@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).