[PATCH] app/test: don't count skipped tests as executed

[PATCH] app/test: don't count skipped tests as executed

[Bruce Richardson]

The logic around skipped tests is a little confusing in the unit test
runner.
* Any explicitly disabled tests are counted as skipped but not
  executed.
* Any tests that return TEST_SKIPPED are counted as both skipped and
  executed, using the same statistics counters.

This makes the stats very strange and hard to correlate, since the
totals don't add up.  One would expect that SKIPPED + EXECUTED +
UNSUPPORTED == TOTAL, and that PASSED + FAILED == EXECUTED.

To achieve this, mark any tests returning TEST_SKIPPED, or ENOTSUP as
not having executed.

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

diff --git a/app/test/test.c b/app/test/test.c
index bfa9ea52e3..7b882a59de 100644
--- a/app/test/test.c
+++ b/app/test/test.c
@@ -375,11 +375,13 @@ unit_test_suite_runner(struct unit_test_suite *suite)
 
 			if (test_success == TEST_SUCCESS)
 				suite->succeeded++;
-			else if (test_success == TEST_SKIPPED)
+			else if (test_success == TEST_SKIPPED) {
 				suite->skipped++;
-			else if (test_success == -ENOTSUP)
+				suite->executed--;
+			} else if (test_success == -ENOTSUP) {
 				suite->unsupported++;
-			else
+				suite->executed--;
+			} else
 				suite->failed++;
 		} else if (test_success == -ENOTSUP) {
 			suite->unsupported++;
-- 
2.39.2

RE: [EXT] [PATCH] app/test: don't count skipped tests as executed

[Akhil Goyal]

> Subject: [EXT] [PATCH] app/test: don't count skipped tests as executed
> The logic around skipped tests is a little confusing in the unit test
> runner.
> * Any explicitly disabled tests are counted as skipped but not
>   executed.
> * Any tests that return TEST_SKIPPED are counted as both skipped and
>   executed, using the same statistics counters.
> 
> This makes the stats very strange and hard to correlate, since the
> totals don't add up.  One would expect that SKIPPED + EXECUTED +
> UNSUPPORTED == TOTAL, and that PASSED + FAILED == EXECUTED.
> 
> To achieve this, mark any tests returning TEST_SKIPPED, or ENOTSUP as
> not having executed.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Acked-by: Akhil Goyal <gakhil@marvell.com>

Yes this makes sense.
One would say executed should count the unsupported cases as well.
But I think this makes sense to not include them in executed cases.
This would give better correlation.
Can we backport this as well?

Re: [EXT] [PATCH] app/test: don't count skipped tests as executed

[Bruce Richardson]

On Tue, Mar 05, 2024 at 02:36:27PM +0000, Akhil Goyal wrote:
> > Subject: [EXT] [PATCH] app/test: don't count skipped tests as executed
> > The logic around skipped tests is a little confusing in the unit test
> > runner.
> > * Any explicitly disabled tests are counted as skipped but not
> >   executed.
> > * Any tests that return TEST_SKIPPED are counted as both skipped and
> >   executed, using the same statistics counters.
> > 
> > This makes the stats very strange and hard to correlate, since the
> > totals don't add up.  One would expect that SKIPPED + EXECUTED +
> > UNSUPPORTED == TOTAL, and that PASSED + FAILED == EXECUTED.
> > 
> > To achieve this, mark any tests returning TEST_SKIPPED, or ENOTSUP as
> > not having executed.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> Acked-by: Akhil Goyal <gakhil@marvell.com>
Cc: stable@dpdk.org

> 
> Yes this makes sense.
> One would say executed should count the unsupported cases as well.
> But I think this makes sense to not include them in executed cases.

It's a good question and there are arguments either way. I'd say that no
test should return ENOTSUP now, and that such tests should return
TEST_SKIPPED. For now, I think it's best to treat them the same.

> This would give better correlation.
> Can we backport this as well?
> 

If LTS maintainers want it, sure. Adding stable on CC.

RE: [PATCH] app/test: don't count skipped tests as executed

[Power, Ciara]

Hi Bruce,

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Monday, November 13, 2023 3:06 PM
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>
> Subject: [PATCH] app/test: don't count skipped tests as executed
> 
> The logic around skipped tests is a little confusing in the unit test runner.
> * Any explicitly disabled tests are counted as skipped but not
>   executed.
> * Any tests that return TEST_SKIPPED are counted as both skipped and
>   executed, using the same statistics counters.
> 
> This makes the stats very strange and hard to correlate, since the totals don't add
> up.  One would expect that SKIPPED + EXECUTED + UNSUPPORTED == TOTAL,
> and that PASSED + FAILED == EXECUTED.
> 
> To achieve this, mark any tests returning TEST_SKIPPED, or ENOTSUP as not
> having executed.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  app/test/test.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test/test.c b/app/test/test.c index bfa9ea52e3..7b882a59de
> 100644
> --- a/app/test/test.c
> +++ b/app/test/test.c
> @@ -375,11 +375,13 @@ unit_test_suite_runner(struct unit_test_suite *suite)
> 
>  			if (test_success == TEST_SUCCESS)
>  				suite->succeeded++;
> -			else if (test_success == TEST_SKIPPED)
> +			else if (test_success == TEST_SKIPPED) {
>  				suite->skipped++;
> -			else if (test_success == -ENOTSUP)
> +				suite->executed--;
> +			} else if (test_success == -ENOTSUP) {
>  				suite->unsupported++;
> -			else
> +				suite->executed--;
> +			} else
>  				suite->failed++;
>  		} else if (test_success == -ENOTSUP) {
>  			suite->unsupported++;
> --
> 2.39.2

Makes sense - probably something I should have spotted way back when reworking some of the test framework for sub-testsuites.
Thanks

Acked-by: Ciara Power <ciara.power@intel.com>

Re: [PATCH] app/test: don't count skipped tests as executed

[Tyler Retzlaff]

On Mon, Nov 13, 2023 at 03:05:33PM +0000, Bruce Richardson wrote:
> The logic around skipped tests is a little confusing in the unit test
> runner.
> * Any explicitly disabled tests are counted as skipped but not
>   executed.
> * Any tests that return TEST_SKIPPED are counted as both skipped and
>   executed, using the same statistics counters.
> 
> This makes the stats very strange and hard to correlate, since the
> totals don't add up.  One would expect that SKIPPED + EXECUTED +
> UNSUPPORTED == TOTAL, and that PASSED + FAILED == EXECUTED.
> 
> To achieve this, mark any tests returning TEST_SKIPPED, or ENOTSUP as
> not having executed.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

Clearly something that was skipped didn't get executed. Solid change.

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Re: [EXT] [PATCH] app/test: don't count skipped tests as executed

[Thomas Monjalon]

05/03/2024 16:11, Bruce Richardson:
> On Tue, Mar 05, 2024 at 02:36:27PM +0000, Akhil Goyal wrote:
> > > Subject: [EXT] [PATCH] app/test: don't count skipped tests as executed
> > > The logic around skipped tests is a little confusing in the unit test
> > > runner.
> > > * Any explicitly disabled tests are counted as skipped but not
> > >   executed.
> > > * Any tests that return TEST_SKIPPED are counted as both skipped and
> > >   executed, using the same statistics counters.
> > > 
> > > This makes the stats very strange and hard to correlate, since the
> > > totals don't add up.  One would expect that SKIPPED + EXECUTED +
> > > UNSUPPORTED == TOTAL, and that PASSED + FAILED == EXECUTED.
> > > 
> > > To achieve this, mark any tests returning TEST_SKIPPED, or ENOTSUP as
> > > not having executed.
> > > 
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > 
> > Acked-by: Akhil Goyal <gakhil@marvell.com>
> Cc: stable@dpdk.org
> 
> > 
> > Yes this makes sense.
> > One would say executed should count the unsupported cases as well.
> > But I think this makes sense to not include them in executed cases.
> 
> It's a good question and there are arguments either way. I'd say that no
> test should return ENOTSUP now, and that such tests should return
> TEST_SKIPPED. For now, I think it's best to treat them the same.
> 
> > This would give better correlation.
> > Can we backport this as well?
> > 
> 
> If LTS maintainers want it, sure. Adding stable on CC.

Applied, thanks.