[PATCH 0/4] Enable more unit tests

[PATCH 0/4] Enable more unit tests

[Stephen Hemminger]

Several of the unit tests were not listed in any test suite
and therefore were rarely run. When running build this was reported as
errors:
 WARNING: Test "alarm_autotest" is not defined in any test suite
 WARNING: Test "cksum_perf_autotest" is not defined in any test suite

Put the alarm, cksum, and timer tests into the appropriate test suite.
More tests should be updated (later).

Stephen Hemminger (4):
  test: update alarm test
  test: run cksum tests as part of perf test suite
  test: make red test part of fast suite
  test: run timer secondary tests as part of fast suite

 app/test/test_alarm.c           | 53 +++++++++++++--------------------
 app/test/test_cksum_perf.c      |  3 +-
 app/test/test_red.c             |  2 +-
 app/test/test_timer_secondary.c |  2 +-
 4 files changed, 23 insertions(+), 37 deletions(-)

-- 
2.43.0

[PATCH 1/4] test: update alarm test

[Stephen Hemminger]

This test should be using the TEST_ASSERT macros, and can be
run as part of the fast test suite now.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_alarm.c | 53 ++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 33 deletions(-)

diff --git a/app/test/test_alarm.c b/app/test/test_alarm.c
index 70e97a3109..4ba8aa1af2 100644
--- a/app/test/test_alarm.c
+++ b/app/test/test_alarm.c
@@ -10,7 +10,8 @@
 
 #include "test.h"
 
-#ifndef RTE_EXEC_ENV_WINDOWS
+#define US_PER_SEC 1000000
+
 static volatile int flag;
 
 static void
@@ -19,46 +20,32 @@ test_alarm_callback(void *cb_arg)
 	flag = 1;
 	printf("Callback setting flag - OK. [cb_arg = %p]\n", cb_arg);
 }
-#endif
 
 static int
 test_alarm(void)
 {
-#ifdef RTE_EXEC_ENV_FREEBSD
-	printf("The alarm API is not supported on FreeBSD\n");
-	return 0;
-#endif
+	int ret;
+
+	ret = rte_eal_alarm_set(0, test_alarm_callback, NULL);
+	TEST_ASSERT_FAIL(ret, "should not be succeed with 0 us value");
+
+	ret = rte_eal_alarm_set(UINT64_MAX - 1, test_alarm_callback, NULL);
+	TEST_ASSERT_FAIL(ret, "should not be succeed with (UINT64_MAX-1) us value");
+
+	ret = rte_eal_alarm_set(10, NULL, NULL);
+	TEST_ASSERT_FAIL(ret, "should not succeed with null callback parameter");
 
-#ifndef RTE_EXEC_ENV_WINDOWS
-	/* check if it will fail to set alarm with wrong us value */
-	printf("check if it will fail to set alarm with wrong ms values\n");
-	if (rte_eal_alarm_set(0, test_alarm_callback,
-						NULL) >= 0) {
-		printf("should not be successful with 0 us value\n");
-		return -1;
-	}
-	if (rte_eal_alarm_set(UINT64_MAX - 1, test_alarm_callback,
-						NULL) >= 0) {
-		printf("should not be successful with (UINT64_MAX-1) us value\n");
-		return -1;
-	}
-#endif
+	ret = rte_eal_alarm_cancel(NULL, NULL);
+	TEST_ASSERT_FAIL(ret, "should not succeed to remove alarm with null callback parameter");
 
-	/* check if it will fail to set alarm with null callback parameter */
-	printf("check if it will fail to set alarm with null callback parameter\n");
-	if (rte_eal_alarm_set(10 /* ms */, NULL, NULL) >= 0) {
-		printf("should not be successful to set alarm with null callback parameter\n");
-		return -1;
-	}
+	ret = rte_eal_alarm_set(US_PER_SEC, test_alarm_callback, NULL);
+	TEST_ASSERT_SUCCESS(ret, "could not set an alarm");
 
-	/* check if it will fail to remove alarm with null callback parameter */
-	printf("check if it will fail to remove alarm with null callback parameter\n");
-	if (rte_eal_alarm_cancel(NULL, NULL) == 0) {
-		printf("should not be successful to remove alarm with null callback parameter");
-		return -1;
-	}
+	ret = rte_eal_alarm_cancel(test_alarm_callback, NULL);
+	/* return is the number of the alarm set (or 0 if none or -1 if error) */
+	TEST_ASSERT(ret > 0, "could not cancel an alarm: %d", ret);
 
 	return 0;
 }
 
-REGISTER_TEST_COMMAND(alarm_autotest, test_alarm);
+REGISTER_FAST_TEST(alarm_autotest, true, true, test_alarm);
-- 
2.43.0

Re: [PATCH 1/4] test: update alarm test

[Bruce Richardson]

On Thu, Jul 18, 2024 at 12:07:12PM -0700, Stephen Hemminger wrote:
> This test should be using the TEST_ASSERT macros, and can be
> run as part of the fast test suite now.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  app/test/test_alarm.c | 53 ++++++++++++++++---------------------------
>  1 file changed, 20 insertions(+), 33 deletions(-)
> 
> diff --git a/app/test/test_alarm.c b/app/test/test_alarm.c
> index 70e97a3109..4ba8aa1af2 100644
> --- a/app/test/test_alarm.c
> +++ b/app/test/test_alarm.c
> @@ -10,7 +10,8 @@
>  
>  #include "test.h"
>  
> -#ifndef RTE_EXEC_ENV_WINDOWS
> +#define US_PER_SEC 1000000
> +
>  static volatile int flag;
>  
>  static void
> @@ -19,46 +20,32 @@ test_alarm_callback(void *cb_arg)
>  	flag = 1;
>  	printf("Callback setting flag - OK. [cb_arg = %p]\n", cb_arg);
>  }
> -#endif
>  
>  static int
>  test_alarm(void)
>  {
> -#ifdef RTE_EXEC_ENV_FREEBSD
> -	printf("The alarm API is not supported on FreeBSD\n");
> -	return 0;
> -#endif
> +	int ret;
> +
> +	ret = rte_eal_alarm_set(0, test_alarm_callback, NULL);
> +	TEST_ASSERT_FAIL(ret, "should not be succeed with 0 us value");
> +
> +	ret = rte_eal_alarm_set(UINT64_MAX - 1, test_alarm_callback, NULL);
> +	TEST_ASSERT_FAIL(ret, "should not be succeed with (UINT64_MAX-1) us value");
> +
> +	ret = rte_eal_alarm_set(10, NULL, NULL);
> +	TEST_ASSERT_FAIL(ret, "should not succeed with null callback parameter");
>  

+1 to use of TEST_ASSERT_FAIL, the test is a lot cleaner now.
However, I think we still need the #ifdefs in it if some of it doesn't work
on Windows/BSD.

/Bruce

Re: [PATCH 1/4] test: update alarm test

[Stephen Hemminger]

On Fri, 19 Jul 2024 11:17:43 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Thu, Jul 18, 2024 at 12:07:12PM -0700, Stephen Hemminger wrote:
> > This test should be using the TEST_ASSERT macros, and can be
> > run as part of the fast test suite now.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  app/test/test_alarm.c | 53 ++++++++++++++++---------------------------
> >  1 file changed, 20 insertions(+), 33 deletions(-)
> > 
> > diff --git a/app/test/test_alarm.c b/app/test/test_alarm.c
> > index 70e97a3109..4ba8aa1af2 100644
> > --- a/app/test/test_alarm.c
> > +++ b/app/test/test_alarm.c
> > @@ -10,7 +10,8 @@
> >  
> >  #include "test.h"
> >  
> > -#ifndef RTE_EXEC_ENV_WINDOWS
> > +#define US_PER_SEC 1000000
> > +
> >  static volatile int flag;
> >  
> >  static void
> > @@ -19,46 +20,32 @@ test_alarm_callback(void *cb_arg)
> >  	flag = 1;
> >  	printf("Callback setting flag - OK. [cb_arg = %p]\n", cb_arg);
> >  }
> > -#endif
> >  
> >  static int
> >  test_alarm(void)
> >  {
> > -#ifdef RTE_EXEC_ENV_FREEBSD
> > -	printf("The alarm API is not supported on FreeBSD\n");
> > -	return 0;
> > -#endif
> > +	int ret;
> > +
> > +	ret = rte_eal_alarm_set(0, test_alarm_callback, NULL);
> > +	TEST_ASSERT_FAIL(ret, "should not be succeed with 0 us value");
> > +
> > +	ret = rte_eal_alarm_set(UINT64_MAX - 1, test_alarm_callback, NULL);
> > +	TEST_ASSERT_FAIL(ret, "should not be succeed with (UINT64_MAX-1) us value");
> > +
> > +	ret = rte_eal_alarm_set(10, NULL, NULL);
> > +	TEST_ASSERT_FAIL(ret, "should not succeed with null callback parameter");
> >    
> 
> +1 to use of TEST_ASSERT_FAIL, the test is a lot cleaner now.
> However, I think we still need the #ifdefs in it if some of it doesn't work
> on Windows/BSD.
> 
> /Bruce

It looks like alarm API's exist on Windows (see lib/eal/windows/eal_alarm.c)
therefore should be tested but aren't being now.

Re: [PATCH 1/4] test: update alarm test

[David Marchand]

On Thu, Jul 18, 2024 at 9:11 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> This test should be using the TEST_ASSERT macros, and can be
> run as part of the fast test suite now.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

[...]

> +       ret = rte_eal_alarm_cancel(test_alarm_callback, NULL);
> +       /* return is the number of the alarm set (or 0 if none or -1 if error) */
> +       TEST_ASSERT(ret > 0, "could not cancel an alarm: %d", ret);
>
>         return 0;
>  }
>
> -REGISTER_TEST_COMMAND(alarm_autotest, test_alarm);
> +REGISTER_FAST_TEST(alarm_autotest, true, true, test_alarm);

This test was removed from the fast-tests list as it was triggering
false failures in some CI.
ee00af60170b ("test: remove strict timing requirements some tests")


-- 
David Marchand

Re: [PATCH 1/4] test: update alarm test

[Stephen Hemminger]

On Tue, 6 Aug 2024 14:45:45 +0200
David Marchand <david.marchand@redhat.com> wrote:

> On Thu, Jul 18, 2024 at 9:11 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > This test should be using the TEST_ASSERT macros, and can be
> > run as part of the fast test suite now.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> 
> [...]
> 
> > +       ret = rte_eal_alarm_cancel(test_alarm_callback, NULL);
> > +       /* return is the number of the alarm set (or 0 if none or -1 if error) */
> > +       TEST_ASSERT(ret > 0, "could not cancel an alarm: %d", ret);
> >
> >         return 0;
> >  }
> >
> > -REGISTER_TEST_COMMAND(alarm_autotest, test_alarm);
> > +REGISTER_FAST_TEST(alarm_autotest, true, true, test_alarm);  
> 
> This test was removed from the fast-tests list as it was triggering
> false failures in some CI.
> ee00af60170b ("test: remove strict timing requirements some tests")
> 
> 

Maybe test can be fixed to use different time values.
Or like lcore tests, some yielding is needed.

Re: [PATCH 1/4] test: update alarm test

[Stephen Hemminger]

On Tue, 6 Aug 2024 14:45:45 +0200
David Marchand <david.marchand@redhat.com> wrote:

> On Thu, Jul 18, 2024 at 9:11 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > This test should be using the TEST_ASSERT macros, and can be
> > run as part of the fast test suite now.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> 
> [...]
> 
> > +       ret = rte_eal_alarm_cancel(test_alarm_callback, NULL);
> > +       /* return is the number of the alarm set (or 0 if none or -1 if error) */
> > +       TEST_ASSERT(ret > 0, "could not cancel an alarm: %d", ret);
> >
> >         return 0;
> >  }
> >
> > -REGISTER_TEST_COMMAND(alarm_autotest, test_alarm);
> > +REGISTER_FAST_TEST(alarm_autotest, true, true, test_alarm);  
> 
> This test was removed from the fast-tests list as it was triggering
> false failures in some CI.
> ee00af60170b ("test: remove strict timing requirements some tests")

I can see why that test failed (too short an alarm), but the whole test has lots
of other issues. Starting new series to address.

[PATCH 2/4] test: run cksum tests as part of perf test suite

[Stephen Hemminger]

The cksum tests would not get run since not part of one
of the test suites. Meson complains with:

 WARNING: Test "cksum_perf_autotest" is not defined in any test suite

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_cksum_perf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/app/test/test_cksum_perf.c b/app/test/test_cksum_perf.c
index 1f296cae34..0b919cd59f 100644
--- a/app/test/test_cksum_perf.c
+++ b/app/test/test_cksum_perf.c
@@ -113,5 +113,4 @@ test_cksum_perf(void)
 	return TEST_SUCCESS;
 }
 
-
-REGISTER_TEST_COMMAND(cksum_perf_autotest, test_cksum_perf);
+REGISTER_PERF_TEST(cksum_perf_autotest, test_cksum_perf);
-- 
2.43.0

Re: [PATCH 2/4] test: run cksum tests as part of perf test suite

[Bruce Richardson]

On Thu, Jul 18, 2024 at 12:07:13PM -0700, Stephen Hemminger wrote:
> The cksum tests would not get run since not part of one
> of the test suites. Meson complains with:
> 
>  WARNING: Test "cksum_perf_autotest" is not defined in any test suite
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  app/test/test_cksum_perf.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/app/test/test_cksum_perf.c b/app/test/test_cksum_perf.c
> index 1f296cae34..0b919cd59f 100644
> --- a/app/test/test_cksum_perf.c
> +++ b/app/test/test_cksum_perf.c
> @@ -113,5 +113,4 @@ test_cksum_perf(void)
>  	return TEST_SUCCESS;
>  }
>  
> -
> -REGISTER_TEST_COMMAND(cksum_perf_autotest, test_cksum_perf);
> +REGISTER_PERF_TEST(cksum_perf_autotest, test_cksum_perf);
> -- 
> 2.43.0
> 

[PATCH 3/4] test: make red test part of fast suite

[Stephen Hemminger]

The red tests were not run because not part of any suite.
Meson warning is:
 WARNING: Test "red_autotest" is not defined in any test suite

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_red.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/test_red.c b/app/test/test_red.c
index aa7538d51a..4bb17dce7a 100644
--- a/app/test/test_red.c
+++ b/app/test/test_red.c
@@ -1877,6 +1877,6 @@ test_red_all(void)
 
 #endif /* !RTE_EXEC_ENV_WINDOWS */
 
-REGISTER_TEST_COMMAND(red_autotest, test_red);
+REGISTER_FAST_TEST(red_autotest, true, true, test_red);
 REGISTER_PERF_TEST(red_perf, test_red_perf);
 REGISTER_PERF_TEST(red_all, test_red_all);
-- 
2.43.0

Re: [PATCH 3/4] test: make red test part of fast suite

[Bruce Richardson]

On Thu, Jul 18, 2024 at 12:07:14PM -0700, Stephen Hemminger wrote:
> The red tests were not run because not part of any suite.
> Meson warning is:
>  WARNING: Test "red_autotest" is not defined in any test suite
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  app/test/test_red.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/app/test/test_red.c b/app/test/test_red.c
> index aa7538d51a..4bb17dce7a 100644
> --- a/app/test/test_red.c
> +++ b/app/test/test_red.c
> @@ -1877,6 +1877,6 @@ test_red_all(void)
>  
>  #endif /* !RTE_EXEC_ENV_WINDOWS */
>  
> -REGISTER_TEST_COMMAND(red_autotest, test_red);
> +REGISTER_FAST_TEST(red_autotest, true, true, test_red);
>  REGISTER_PERF_TEST(red_perf, test_red_perf);
>  REGISTER_PERF_TEST(red_all, test_red_all);
> -- 
> 2.43.0
> 

Re: [PATCH 3/4] test: make red test part of fast suite

[David Marchand]

On Thu, Jul 18, 2024 at 9:11 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> The red tests were not run because not part of any suite.
> Meson warning is:
>  WARNING: Test "red_autotest" is not defined in any test suite
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

There is an open bz about this unit test.
https://bugs.dpdk.org/show_bug.cgi?id=826


-- 
David Marchand

Re: [PATCH 3/4] test: make red test part of fast suite

[Bruce Richardson]

On Tue, Aug 06, 2024 at 02:28:13PM +0200, David Marchand wrote:
> On Thu, Jul 18, 2024 at 9:11 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > The red tests were not run because not part of any suite.
> > Meson warning is:
> >  WARNING: Test "red_autotest" is not defined in any test suite
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> There is an open bz about this unit test.
> https://bugs.dpdk.org/show_bug.cgi?id=826
> 
Do we need a "broken tests" placeholder suite, or is it better to just
leave the broken tests unassigned and warning in meson?

/Bruce

Re: [PATCH 3/4] test: make red test part of fast suite

[Stephen Hemminger]

On Tue, 6 Aug 2024 14:28:13 +0200
David Marchand <david.marchand@redhat.com> wrote:

> On Thu, Jul 18, 2024 at 9:11 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > The red tests were not run because not part of any suite.
> > Meson warning is:
> >  WARNING: Test "red_autotest" is not defined in any test suite
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> 
> There is an open bz about this unit test.
> https://bugs.dpdk.org/show_bug.cgi?id=826
> 
> 

So bug reported but no action taken. Maybe time to turn off RED then :-)

Re: [PATCH 3/4] test: make red test part of fast suite

[David Marchand]

On Thu, Aug 8, 2024 at 6:18 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue, 6 Aug 2024 14:28:13 +0200
> David Marchand <david.marchand@redhat.com> wrote:
>
> > On Thu, Jul 18, 2024 at 9:11 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> > >
> > > The red tests were not run because not part of any suite.
> > > Meson warning is:
> > >  WARNING: Test "red_autotest" is not defined in any test suite
> > >
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >
> > There is an open bz about this unit test.
> > https://bugs.dpdk.org/show_bug.cgi?id=826
> >
> So bug reported but no action taken. Maybe time to turn off RED then :-)
>

That's an option :-) but we did not announce it in before v24.11 LTS,
so it's late.
Can we schedule this for next LTS?


-- 
David Marchand

[PATCH 4/4] test: run timer secondary tests as part of fast suite

[Stephen Hemminger]

The tests were not part of any suite so not run normally.
Meson warning is:
WARNING: Test "timer_secondary_autotest" is not defined in any test suite

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_timer_secondary.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/test_timer_secondary.c b/app/test/test_timer_secondary.c
index 4e220559b4..2f98c177cd 100644
--- a/app/test/test_timer_secondary.c
+++ b/app/test/test_timer_secondary.c
@@ -224,4 +224,4 @@ test_timer_secondary(void)
 
 #endif /* !RTE_EXEC_ENV_WINDOWS */
 
-REGISTER_TEST_COMMAND(timer_secondary_autotest, test_timer_secondary);
+REGISTER_FAST_TEST(timer_secondary_autotest, false, true, test_timer_secondary);
-- 
2.43.0

Re: [PATCH 4/4] test: run timer secondary tests as part of fast suite

[Bruce Richardson]

On Thu, Jul 18, 2024 at 12:07:15PM -0700, Stephen Hemminger wrote:
> The tests were not part of any suite so not run normally.
> Meson warning is:
> WARNING: Test "timer_secondary_autotest" is not defined in any test suite
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH 4/4] test: run timer secondary tests as part of fast suite

[David Marchand]

On Thu, Jul 18, 2024 at 9:11 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> The tests were not part of any suite so not run normally.
> Meson warning is:
> WARNING: Test "timer_secondary_autotest" is not defined in any test suite
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  app/test/test_timer_secondary.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/app/test/test_timer_secondary.c b/app/test/test_timer_secondary.c
> index 4e220559b4..2f98c177cd 100644
> --- a/app/test/test_timer_secondary.c
> +++ b/app/test/test_timer_secondary.c
> @@ -224,4 +224,4 @@ test_timer_secondary(void)
>
>  #endif /* !RTE_EXEC_ENV_WINDOWS */
>
> -REGISTER_TEST_COMMAND(timer_secondary_autotest, test_timer_secondary);
> +REGISTER_FAST_TEST(timer_secondary_autotest, false, true, test_timer_secondary);
> --
> 2.43.0
>

This test timed out in GHA.


-- 
David Marchand

Re: [PATCH 0/4] Enable more unit tests

[David Marchand]

On Thu, Jul 18, 2024 at 9:11 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> Several of the unit tests were not listed in any test suite
> and therefore were rarely run. When running build this was reported as
> errors:
>  WARNING: Test "alarm_autotest" is not defined in any test suite
>  WARNING: Test "cksum_perf_autotest" is not defined in any test suite
>
> Put the alarm, cksum, and timer tests into the appropriate test suite.
> More tests should be updated (later).
>
> Stephen Hemminger (4):
>   test: update alarm test
>   test: run cksum tests as part of perf test suite
>   test: make red test part of fast suite
>   test: run timer secondary tests as part of fast suite
>
>  app/test/test_alarm.c           | 53 +++++++++++++--------------------
>  app/test/test_cksum_perf.c      |  3 +-
>  app/test/test_red.c             |  2 +-
>  app/test/test_timer_secondary.c |  2 +-
>  4 files changed, 23 insertions(+), 37 deletions(-)

I merged the alarm test cleanups, which means patch 1 is superseded.

The red and the timer secondary tests can't be put in fast-tests.

I merged the cksum patch only and updated patchwork accordingly.

Thanks.

-- 
David Marchand