DPDK patches and discussions
 help / color / mirror / Atom feed
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
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[];
>  };


  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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git