diff mbox series

[RFC] app/test: add support for skipping tests

Message ID 20230817105851.501947-1-bruce.richardson@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [RFC] app/test: add support for skipping tests |

Checks

Context Check Description
ci/loongarch-compilation success Compilation OK
ci/checkpatch warning coding style issues
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS

Commit Message

Bruce Richardson Aug. 17, 2023, 10:58 a.m. UTC 
  When called from automated tools, like meson test, it is often useful to
skip tests in a test suite, without having to alter the test build. To
do so, we add support for DPDK_TEST_SKIP environment variable, where one
can provide a comma-separated list of tests. When the test binary is
called to run one of the tests on the list via either cmdline parameter
or environment variable (as done with meson test), the test will not
actually be run, but will be reported skipped.

Example run:
  $ DPDK_TEST_SKIP=dump_devargs,dump_ring meson test --suite=debug-tests
  ...
  1/9 DPDK:debug-tests / dump_devargs             SKIP            1.11s
  2/9 DPDK:debug-tests / dump_log_types           OK              1.06s
  3/9 DPDK:debug-tests / dump_malloc_heaps        OK              1.11s
  4/9 DPDK:debug-tests / dump_malloc_stats        OK              1.07s
  5/9 DPDK:debug-tests / dump_mempool             OK              1.11s
  6/9 DPDK:debug-tests / dump_memzone             OK              1.06s
  7/9 DPDK:debug-tests / dump_physmem             OK              1.13s
  8/9 DPDK:debug-tests / dump_ring                SKIP            1.04s
  9/9 DPDK:debug-tests / dump_struct_sizes        OK              1.10s

  Ok:                 7
  Expected Fail:      0
  Fail:               0
  Unexpected Pass:    0
  Skipped:            2
  Timeout:            0

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Thomas Monjalon Aug. 29, 2023, 9:34 a.m. UTC | #1 
jeudi 17 août 2023, Bruce Richardson:
> When called from automated tools, like meson test, it is often useful to
> skip tests in a test suite, without having to alter the test build. To
> do so, we add support for DPDK_TEST_SKIP environment variable, where one
> can provide a comma-separated list of tests. When the test binary is
> called to run one of the tests on the list via either cmdline parameter
> or environment variable (as done with meson test), the test will not
> actually be run, but will be reported skipped.
> 
> Example run:
>   $ DPDK_TEST_SKIP=dump_devargs,dump_ring meson test --suite=debug-tests
>   ...
>   1/9 DPDK:debug-tests / dump_devargs             SKIP            1.11s
>   2/9 DPDK:debug-tests / dump_log_types           OK              1.06s
>   3/9 DPDK:debug-tests / dump_malloc_heaps        OK              1.11s
>   4/9 DPDK:debug-tests / dump_malloc_stats        OK              1.07s
>   5/9 DPDK:debug-tests / dump_mempool             OK              1.11s
>   6/9 DPDK:debug-tests / dump_memzone             OK              1.06s
>   7/9 DPDK:debug-tests / dump_physmem             OK              1.13s
>   8/9 DPDK:debug-tests / dump_ring                SKIP            1.04s
>   9/9 DPDK:debug-tests / dump_struct_sizes        OK              1.10s
> 
>   Ok:                 7
>   Expected Fail:      0
>   Fail:               0
>   Unexpected Pass:    0
>   Skipped:            2
>   Timeout:            0
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Looks to be a good addition.

Acked-by: Thomas Monjalon <thomas@monjalon.net>
Aaron Conole Aug. 29, 2023, 1:27 p.m. UTC | #2 
Bruce Richardson <bruce.richardson@intel.com> writes:

> When called from automated tools, like meson test, it is often useful to
> skip tests in a test suite, without having to alter the test build. To
> do so, we add support for DPDK_TEST_SKIP environment variable, where one
> can provide a comma-separated list of tests. When the test binary is
> called to run one of the tests on the list via either cmdline parameter
> or environment variable (as done with meson test), the test will not
> actually be run, but will be reported skipped.
>
> Example run:
>   $ DPDK_TEST_SKIP=dump_devargs,dump_ring meson test --suite=debug-tests
>   ...
>   1/9 DPDK:debug-tests / dump_devargs             SKIP            1.11s
>   2/9 DPDK:debug-tests / dump_log_types           OK              1.06s
>   3/9 DPDK:debug-tests / dump_malloc_heaps        OK              1.11s
>   4/9 DPDK:debug-tests / dump_malloc_stats        OK              1.07s
>   5/9 DPDK:debug-tests / dump_mempool             OK              1.11s
>   6/9 DPDK:debug-tests / dump_memzone             OK              1.06s
>   7/9 DPDK:debug-tests / dump_physmem             OK              1.13s
>   8/9 DPDK:debug-tests / dump_ring                SKIP            1.04s
>   9/9 DPDK:debug-tests / dump_struct_sizes        OK              1.10s
>
>   Ok:                 7
>   Expected Fail:      0
>   Fail:               0
>   Unexpected Pass:    0
>   Skipped:            2
>   Timeout:            0
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

The idea looks fine to me, although I would really like it if we could
do a command like argument instead of env variable (but that's just a
suggestion to color the shed).

Just minor nit stuff below.

>  app/test/test.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/app/test/test.c b/app/test/test.c
> index fb073ff795..3569c36049 100644
> --- a/app/test/test.c
> +++ b/app/test/test.c
> @@ -193,6 +193,20 @@ main(int argc, char **argv)
>  
>  	if (test_count > 0) {
>  		char buf[1024];
> +		char *dpdk_test_skip = getenv("DPDK_TEST_SKIP");
> +		char *skip_tests[128] = {0};
> +		size_t n_skip_tests = 0;
> +
> +		if (dpdk_test_skip != NULL && strlen(dpdk_test_skip) > 0){
> +			char *dpdk_test_skip_cp = strdup(dpdk_test_skip);
> +			if (dpdk_test_skip_cp == NULL) {
> +				ret = -1;
> +				goto out;
> +			}
> +			dpdk_test_skip = dpdk_test_skip_cp;
> +			n_skip_tests = rte_strsplit(dpdk_test_skip, strlen(dpdk_test_skip),
> +					skip_tests, RTE_DIM(skip_tests), ',');

We probably should check for n_skip_tests == -1 - although it shouldn't
ever happen for this code.  If it ever did happen, we'd walk right off
the for() loop below.  Actually, it looks like that error condition for
rte_strsplit isn't documented.

> +		}
>  
>  		cl = cmdline_new(main_ctx, "RTE>>", 0, 1);
>  		if (cl == NULL) {
> @@ -201,6 +215,15 @@ main(int argc, char **argv)
>  		}
>  
>  		for (i = 0; i < test_count; i++) {
> +			/* check if test is to be skipped */
> +			for (size_t j = 0; j < n_skip_tests; j++) {

We might want to check j < MIN(n_skip_tests, RTE_DIM(skip_tests)) to
prevent a rogue env variable that makes rte_strsplit fail in the future.

> +				if (strcmp(tests[i], skip_tests[j]) == 0) {
> +					fprintf(stderr, "Skipping %s [DPDK_TEST_SKIP]\n", tests[i]);
> +					ret = TEST_SKIPPED;
> +					goto end_of_cmd;
> +				}
> +			}
> +
>  			snprintf(buf, sizeof(buf), "%s\n", tests[i]);
>  			if (cmdline_parse_check(cl, buf) < 0) {
>  				printf("Error: invalid test command: '%s'\n", tests[i]);
> @@ -211,9 +234,13 @@ main(int argc, char **argv)
>  			} else
>  				ret = last_test_result;
>  
> +end_of_cmd:
>  			if (ret != 0)
>  				break;
>  		}
> +		if (n_skip_tests > 0)
> +			free(dpdk_test_skip);
> +
>  		cmdline_free(cl);
>  		goto out;
>  	} else {
Bruce Richardson Aug. 29, 2023, 1:39 p.m. UTC | #3 
On Tue, Aug 29, 2023 at 09:27:23AM -0400, Aaron Conole wrote:
> Bruce Richardson <bruce.richardson@intel.com> writes:
> 
> > When called from automated tools, like meson test, it is often useful to
> > skip tests in a test suite, without having to alter the test build. To
> > do so, we add support for DPDK_TEST_SKIP environment variable, where one
> > can provide a comma-separated list of tests. When the test binary is
> > called to run one of the tests on the list via either cmdline parameter
> > or environment variable (as done with meson test), the test will not
> > actually be run, but will be reported skipped.
> >
> > Example run:
> >   $ DPDK_TEST_SKIP=dump_devargs,dump_ring meson test --suite=debug-tests
> >   ...
> >   1/9 DPDK:debug-tests / dump_devargs             SKIP            1.11s
> >   2/9 DPDK:debug-tests / dump_log_types           OK              1.06s
> >   3/9 DPDK:debug-tests / dump_malloc_heaps        OK              1.11s
> >   4/9 DPDK:debug-tests / dump_malloc_stats        OK              1.07s
> >   5/9 DPDK:debug-tests / dump_mempool             OK              1.11s
> >   6/9 DPDK:debug-tests / dump_memzone             OK              1.06s
> >   7/9 DPDK:debug-tests / dump_physmem             OK              1.13s
> >   8/9 DPDK:debug-tests / dump_ring                SKIP            1.04s
> >   9/9 DPDK:debug-tests / dump_struct_sizes        OK              1.10s
> >
> >   Ok:                 7
> >   Expected Fail:      0
> >   Fail:               0
> >   Unexpected Pass:    0
> >   Skipped:            2
> >   Timeout:            0
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> 
> The idea looks fine to me, although I would really like it if we could
> do a command like argument instead of env variable (but that's just a
> suggestion to color the shed).
> 

Yes, I prefer cmdline args too, but I believed that for running test suites
having the tests to disable set in the environment was a better solution,
since it was easy to get at whatever way the tests were run via a
test-runner.

However, there is support for passing "--test-args" parameter to meson
test, which should allow passing this stuff via cmdline. If you prefer the
cmdline solution and if it doesn't have any gotchas like that, I can
probably look to rework this to use cmdline. I assume that any other test
runners in use would similarly be able to append addition cmdline args to
the test runs?

> Just minor nit stuff below.
> 
> >  app/test/test.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/app/test/test.c b/app/test/test.c
> > index fb073ff795..3569c36049 100644
> > --- a/app/test/test.c
> > +++ b/app/test/test.c
> > @@ -193,6 +193,20 @@ main(int argc, char **argv)
> >  
> >  	if (test_count > 0) {
> >  		char buf[1024];
> > +		char *dpdk_test_skip = getenv("DPDK_TEST_SKIP");
> > +		char *skip_tests[128] = {0};
> > +		size_t n_skip_tests = 0;
> > +
> > +		if (dpdk_test_skip != NULL && strlen(dpdk_test_skip) > 0){
> > +			char *dpdk_test_skip_cp = strdup(dpdk_test_skip);
> > +			if (dpdk_test_skip_cp == NULL) {
> > +				ret = -1;
> > +				goto out;
> > +			}
> > +			dpdk_test_skip = dpdk_test_skip_cp;
> > +			n_skip_tests = rte_strsplit(dpdk_test_skip, strlen(dpdk_test_skip),
> > +					skip_tests, RTE_DIM(skip_tests), ',');
> 
> We probably should check for n_skip_tests == -1 - although it shouldn't
> ever happen for this code.  If it ever did happen, we'd walk right off
> the for() loop below.  Actually, it looks like that error condition for
> rte_strsplit isn't documented.
> 

Can't happen for this code. The strsplit only returns -1 if the first or
third params are NULL, and the first param (dpdk_test_skip) is checked for
NULL before the call, and the third is a local array pointer in this case.

> > +		}
> >  
> >  		cl = cmdline_new(main_ctx, "RTE>>", 0, 1);
> >  		if (cl == NULL) {
> > @@ -201,6 +215,15 @@ main(int argc, char **argv)
> >  		}
> >  
> >  		for (i = 0; i < test_count; i++) {
> > +			/* check if test is to be skipped */
> > +			for (size_t j = 0; j < n_skip_tests; j++) {
> 
> We might want to check j < MIN(n_skip_tests, RTE_DIM(skip_tests)) to
> prevent a rogue env variable that makes rte_strsplit fail in the future.
> 

rte_strsplit cannot return a value > RTE_DIM(skip_tests). The main loop
only increments the return value (tok) at most once per loop, and at the
start of the loop checks for tok >= maxtokens.

> > +				if (strcmp(tests[i], skip_tests[j]) == 0) {
> > +					fprintf(stderr, "Skipping %s [DPDK_TEST_SKIP]\n", tests[i]);
> > +					ret = TEST_SKIPPED;
> > +					goto end_of_cmd;
> > +				}
> > +			}
> > +
<snip>

Thanks for the review.
/Bruce
diff mbox series

Patch

diff --git a/app/test/test.c b/app/test/test.c
index fb073ff795..3569c36049 100644
--- a/app/test/test.c
+++ b/app/test/test.c
@@ -193,6 +193,20 @@  main(int argc, char **argv)
 
 	if (test_count > 0) {
 		char buf[1024];
+		char *dpdk_test_skip = getenv("DPDK_TEST_SKIP");
+		char *skip_tests[128] = {0};
+		size_t n_skip_tests = 0;
+
+		if (dpdk_test_skip != NULL && strlen(dpdk_test_skip) > 0){
+			char *dpdk_test_skip_cp = strdup(dpdk_test_skip);
+			if (dpdk_test_skip_cp == NULL) {
+				ret = -1;
+				goto out;
+			}
+			dpdk_test_skip = dpdk_test_skip_cp;
+			n_skip_tests = rte_strsplit(dpdk_test_skip, strlen(dpdk_test_skip),
+					skip_tests, RTE_DIM(skip_tests), ',');
+		}
 
 		cl = cmdline_new(main_ctx, "RTE>>", 0, 1);
 		if (cl == NULL) {
@@ -201,6 +215,15 @@  main(int argc, char **argv)
 		}
 
 		for (i = 0; i < test_count; i++) {
+			/* check if test is to be skipped */
+			for (size_t j = 0; j < n_skip_tests; j++) {
+				if (strcmp(tests[i], skip_tests[j]) == 0) {
+					fprintf(stderr, "Skipping %s [DPDK_TEST_SKIP]\n", tests[i]);
+					ret = TEST_SKIPPED;
+					goto end_of_cmd;
+				}
+			}
+
 			snprintf(buf, sizeof(buf), "%s\n", tests[i]);
 			if (cmdline_parse_check(cl, buf) < 0) {
 				printf("Error: invalid test command: '%s'\n", tests[i]);
@@ -211,9 +234,13 @@  main(int argc, char **argv)
 			} else
 				ret = last_test_result;
 
+end_of_cmd:
 			if (ret != 0)
 				break;
 		}
+		if (n_skip_tests > 0)
+			free(dpdk_test_skip);
+
 		cmdline_free(cl);
 		goto out;
 	} else {