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

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

[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>
---
 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), ',');
+		}
 
 		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 {
-- 
2.39.2

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

[Thomas Monjalon]

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>

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

[Aaron Conole]

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 {

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

[Bruce Richardson]

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

[PATCH v2] app/test: add support for skipping tests

[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>
Acked-by: Thomas Monjalon <thomas@monjalon.net>

---
V2: Check return value from rte_strsplit, just in case.
---
 app/test/test.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/app/test/test.c b/app/test/test.c
index fb073ff795..1143285af2 100644
--- a/app/test/test.c
+++ b/app/test/test.c
@@ -193,6 +193,25 @@ 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){
+			int split_ret;
+			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;
+			split_ret = rte_strsplit(dpdk_test_skip, strlen(dpdk_test_skip),
+					skip_tests, RTE_DIM(skip_tests), ',');
+			if (split_ret > 0)
+				n_skip_tests = split_ret;
+			else
+				free(dpdk_test_skip);
+		}
 
 		cl = cmdline_new(main_ctx, "RTE>>", 0, 1);
 		if (cl == NULL) {
@@ -201,6 +220,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 +239,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 {
-- 
2.39.2

[PATCH v3] app/test: add support for skipping tests

[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>
Acked-by: Thomas Monjalon <thomas@monjalon.net>

---
V3: Fix checkpatch whitespace issue

V2: Check return value from rte_strsplit, just in case.
---
 app/test/test.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/app/test/test.c b/app/test/test.c
index fb073ff795..bfa9ea52e3 100644
--- a/app/test/test.c
+++ b/app/test/test.c
@@ -193,6 +193,25 @@ 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) {
+			int split_ret;
+			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;
+			split_ret = rte_strsplit(dpdk_test_skip, strlen(dpdk_test_skip),
+					skip_tests, RTE_DIM(skip_tests), ',');
+			if (split_ret > 0)
+				n_skip_tests = split_ret;
+			else
+				free(dpdk_test_skip);
+		}
 
 		cl = cmdline_new(main_ctx, "RTE>>", 0, 1);
 		if (cl == NULL) {
@@ -201,6 +220,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 +239,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 {
-- 
2.39.2

Re: [PATCH v3] app/test: add support for skipping tests

[Bruce Richardson]

On Thu, Aug 31, 2023 at 11:18:06AM +0100, Bruce Richardson wrote:
> 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>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
+Tyler

I see this set is failing CI checks due to breaking Windows builds. The
issue seems to be the use of the "strdup" function. I notice in the log
library, that we have a "#define strdup _strdup" macro. Since strdup is
fairly common, widespread function, I think we should consider a more
general approach to it.

Tyler, looking for your input here: should we just globally define strdup
as _strdup for windows in DPDK? Alternatively, some googling indicates that
there is the "_CRT_NONSTDC_NO_DEPRECATE" define which could be used to
enable a whole range of POSIX functions. Should we, or could we, just set
this to ease porting of code over? I'd hate each of our C files to have a
bunch of duplicated #defines at the start to prefix standard unix functions
with "_"s.

Thoughts?
/Bruce

[PATCH v4] app/test: add support for skipping tests

[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>
Acked-by: Thomas Monjalon <thomas@monjalon.net>

---
V4: Add os shim header, to avoid issues with use of strdup on windows

V3: Fix checkpatch whitespace issue

V2: Check return value from rte_strsplit, just in case.
---
 app/test/test.c | 32 ++++++++++++++++++++++++++++++++
 app/test/test.h |  1 +
 2 files changed, 33 insertions(+)

diff --git a/app/test/test.c b/app/test/test.c
index fb073ff795..bfa9ea52e3 100644
--- a/app/test/test.c
+++ b/app/test/test.c
@@ -193,6 +193,25 @@ 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) {
+			int split_ret;
+			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;
+			split_ret = rte_strsplit(dpdk_test_skip, strlen(dpdk_test_skip),
+					skip_tests, RTE_DIM(skip_tests), ',');
+			if (split_ret > 0)
+				n_skip_tests = split_ret;
+			else
+				free(dpdk_test_skip);
+		}
 
 		cl = cmdline_new(main_ctx, "RTE>>", 0, 1);
 		if (cl == NULL) {
@@ -201,6 +220,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 +239,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 {
diff --git a/app/test/test.h b/app/test/test.h
index a91ded76af..eede583cf9 100644
--- a/app/test/test.h
+++ b/app/test/test.h
@@ -12,6 +12,7 @@
 
 #include <rte_hexdump.h>
 #include <rte_common.h>
+#include <rte_os_shim.h>
 
 #define TEST_SUCCESS EXIT_SUCCESS
 #define TEST_FAILED  -1
-- 
2.39.2

Re: [PATCH v4] app/test: add support for skipping tests

[Aaron Conole]

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>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>
> ---

Acked-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH v4] app/test: add support for skipping tests

[David Marchand]

On Mon, Sep 25, 2023 at 6:27 PM Aaron Conole <aconole@redhat.com> 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>
> > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> Acked-by: Aaron Conole <aconole@redhat.com>

Applied, thanks.


-- 
David Marchand

Re: [PATCH v4] app/test: add support for skipping tests

[Patrick Robb]

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]

Thanks, this should help greatly going forward in the community lab.

As it relates to our arm64 unit testing, I will give it a few days (or
longer if needed) for next branches to rebase off of main and then
re-enable arm64 unit testing with the eal_flags_file_prefix_autotest added
to the skipped list. David explained to me on slack that this patch would
not likely be a candidate for backporting, so of course LTS will be
excluded.

[-- Attachment #2: Type: text/html, Size: 486 bytes --]

Re: [PATCH v4] app/test: add support for skipping tests

[Aaron Conole]

Patrick Robb <probb@iol.unh.edu> writes:

> Thanks, this should help greatly going forward in the community lab.
>
> As it relates to our arm64 unit testing, I will give it a few days (or longer if needed) for next branches to rebase off of
> main and then re-enable arm64 unit testing with the eal_flags_file_prefix_autotest added to the skipped list. David
> explained to me on slack that this patch would not likely be a candidate for backporting, so of course LTS will be
> excluded. 

This is in testing area, and maybe it can be considered as an exception
if it allows for improved LTS testing.  CC'd Kevin.

Re: [PATCH v4] app/test: add support for skipping tests

[Patrick Robb]

[-- Attachment #1: Type: text/plain, Size: 1435 bytes --]

On Wed, Oct 4, 2023 at 11:13 AM Aaron Conole <aconole@redhat.com> wrote:

> Patrick Robb <probb@iol.unh.edu> writes:
>
> > Thanks, this should help greatly going forward in the community lab.
> >
> > As it relates to our arm64 unit testing, I will give it a few days (or
> longer if needed) for next branches to rebase off of
> > main and then re-enable arm64 unit testing with the
> eal_flags_file_prefix_autotest added to the skipped list. David
> > explained to me on slack that this patch would not likely be a candidate
> for backporting, so of course LTS will be
> > excluded.
>
> This is in testing area, and maybe it can be considered as an exception
> if it allows for improved LTS testing.  CC'd Kevin.
>
> Hello,

Yes, backporting would be ideal from a CI perspective because without it we
can't run arm64 testing on LTS tests. But I know there are other
considerations which also have to be weighed.

David also has a patch[1] which should resolve the underlying issue which
introduces the failures on the unit test we want to skip. If that patch is
accepted, and backported, fixing our original problem with unit testing on
our arm testbeds, that's another solution, at least for this specific unit
test issue.

It would still be nice to have this feature in case we need it otherwise.

[1]
https://patches.dpdk.org/project/dpdk/patch/20230821085806.3062613-4-david.marchand@redhat.com/

[-- Attachment #2: Type: text/html, Size: 2060 bytes --]

Re: [PATCH v4] app/test: add support for skipping tests

[Patrick Robb]

[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]

On Mon, Oct 9, 2023 at 4:03 PM Patrick Robb <probb@iol.unh.edu> wrote:

>
>> Hello,
>
> Yes, backporting would be ideal from a CI perspective because without it
> we can't run arm64 testing on LTS tests. But I know there are other
> considerations which also have to be weighed.
>
> David also has a patch[1] which should resolve the underlying issue which
> introduces the failures on the unit test we want to skip. If that patch is
> accepted, and backported, fixing our original problem with unit testing on
> our arm testbeds, that's another solution, at least for this specific unit
> test issue.
>
> It would still be nice to have this feature in case we need it otherwise.
>
> [1]
> https://patches.dpdk.org/project/dpdk/patch/20230821085806.3062613-4-david.marchand@redhat.com/
>
> Hi. just to close the loops on this, yes David's aforementioned patch did
resolve the unit test failure which was preventing us from running
fast-tests on our arm64 test beds. But, it is not (yet, at least)
backported for LTS releases.

Even if it were, having Bruce's patch here backported would mean the CI
testing approach could be common across releases in situations where
testcases have to be skipped.

Anyways, whether it's possible or "worth it" is ultimately down to the
community's bandwidth, but I didn't want to let the conversation lapse
without an update, and raising what the benefits would be.

In any case, thanks again Bruce for the rework, it's a great addition.

[-- Attachment #2: Type: text/html, Size: 2346 bytes --]

Re: [PATCH v4] app/test: add support for skipping tests

[Bruce Richardson]

+stable on CC, to allow it be considered for possible backport. It's a
change to the unit test app, so not affecting any ABI or any end-user app.

On Fri, Oct 20, 2023 at 11:02:07AM -0400, Patrick Robb wrote:
>    On Mon, Oct 9, 2023 at 4:03 PM Patrick Robb <[1]probb@iol.unh.edu>
>    wrote:
> 
>    Hello,
>    Yes, backporting would be ideal from a CI perspective because without
>    it we can't run arm64 testing on LTS tests. But I know there are other
>    considerations which also have to be weighed.
>    David also has a patch[1] which should resolve the underlying issue
>    which introduces the failures on the unit test we want to skip. If that
>    patch is accepted, and backported, fixing our original problem with
>    unit testing on our arm testbeds, that's another solution, at least for
>    this specific unit test issue.
>    It would still be nice to have this feature in case we need it
>    otherwise.
>    [1] [2]https://patches.dpdk.org/project/dpdk/patch/20230821085806.30626
>    13-4-david.marchand@redhat.com/
> 
>    Hi. just to close the loops on this, yes David's aforementioned patch
>    did resolve the unit test failure which was preventing us from running
>    fast-tests on our arm64 test beds. But, it is not (yet, at least)
>    backported for LTS releases.
>    Even if it were, having Bruce's patch here backported would mean the CI
>    testing approach could be common across releases in situations where
>    testcases have to be skipped.
>    Anyways, whether it's possible or "worth it" is ultimately down to the
>    community's bandwidth, but I didn't want to let the conversation lapse
>    without an update, and raising what the benefits would be.
>    In any case, thanks again Bruce for the rework, it's a great addition.
> 
> References
> 
>    1. mailto:probb@iol.unh.edu
>    2. https://patches.dpdk.org/project/dpdk/patch/20230821085806.3062613-4-david.marchand@redhat.com/