app/test: don't count skipped tests as executed
Checks
Commit Message
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(-)
Comments
> 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?
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.
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>
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>
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.
@@ -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++;