[kvm-unit-tests PATCH] lib: Do not report failures when test passes

[kvm-unit-tests PATCH] lib: Do not report failures when test passes

[nadav.amit]

From: Nadav Amit <nadav.amit@gmail.com>

Currently, if a test is expected to fail, but surprisingly it passes,
the test is considered as "failing". This means that running on old
KVM-unit-tests on new KVM versions can falsely report failures.

Fix it and simplify the logic.

Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
 lib/report.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/report.c b/lib/report.c
index ca9b4fd..8c8db14 100644
--- a/lib/report.c
+++ b/lib/report.c
@@ -96,10 +96,12 @@ static void va_report(const char *msg_fmt,
 	puts("\n");
 	if (skip)
 		skipped++;
-	else if (xfail && !pass)
-		xfailures++;
-	else if (xfail || !pass)
-		failures++;
+	else if (!pass) {
+		if (xfail)
+			xfailures++;
+		else
+			failures++;
+	}
 
 	spin_unlock(&lock);
 }
-- 
2.17.1

Re: [kvm-unit-tests PATCH] lib: Do not report failures when test passes

[Andrew Jones]

On Tue, Apr 16, 2019 at 10:18:11PM -0700, nadav.amit@gmail.com wrote:
> From: Nadav Amit <nadav.amit@gmail.com>
> 
> Currently, if a test is expected to fail, but surprisingly it passes,
> the test is considered as "failing".

It's not "failing", it's failing. If a test is expected to pass then
it shouldn't be getting reported with report_xfail().

If you know of an xfail test that is passing, and believe it should
be passing now, then please send a patch changing the report_xfail()
to report().

> This means that running on old
> KVM-unit-tests on new KVM versions can falsely report failures.

Why would one want to run old kvm-unit-tests on new kvm? We primarily
care about latest kvm-unit-tests running on latest kvm, but there are
cases where we want to run latest kvm-unit-tests on old kvm, like
when bisecting something with a new test. In some cases that can be
risky, though, if a test case can trigger host crashes. For those cases
we have the errata framework.

Thanks,
drew

> 
> Fix it and simplify the logic.
> 
> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> ---
>  lib/report.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/report.c b/lib/report.c
> index ca9b4fd..8c8db14 100644
> --- a/lib/report.c
> +++ b/lib/report.c
> @@ -96,10 +96,12 @@ static void va_report(const char *msg_fmt,
>  	puts("\n");
>  	if (skip)
>  		skipped++;
> -	else if (xfail && !pass)
> -		xfailures++;
> -	else if (xfail || !pass)
> -		failures++;
> +	else if (!pass) {
> +		if (xfail)
> +			xfailures++;
> +		else
> +			failures++;
> +	}
>  
>  	spin_unlock(&lock);
>  }
> -- 
> 2.17.1
> 

Re: [kvm-unit-tests PATCH] lib: Do not report failures when test passes

[Nadav Amit]

> On Apr 16, 2019, at 11:57 PM, Andrew Jones <drjones@redhat.com> wrote:
> 
> On Tue, Apr 16, 2019 at 10:18:11PM -0700, nadav.amit@gmail.com wrote:
>> From: Nadav Amit <nadav.amit@gmail.com>
>> 
>> Currently, if a test is expected to fail, but surprisingly it passes,
>> the test is considered as "failing".
> 
> It's not "failing", it's failing. If a test is expected to pass then
> it shouldn't be getting reported with report_xfail().

I find this terminology confusing. For instance, there are some tests which
are probabilistic (e.g., test_sti_nmi) - let’s assume you expect one to fail
and it passes, would you say that you encountered a failure?

> Why would one want to run old kvm-unit-tests on new kvm?

I can think of a couple of reasons, but I am not going to argue too much.

Re: [kvm-unit-tests PATCH] lib: Do not report failures when test passes

[Andrew Jones]

On Wed, Apr 17, 2019 at 12:10:54AM -0700, Nadav Amit wrote:
> > On Apr 16, 2019, at 11:57 PM, Andrew Jones <drjones@redhat.com> wrote:
> > 
> > On Tue, Apr 16, 2019 at 10:18:11PM -0700, nadav.amit@gmail.com wrote:
> >> From: Nadav Amit <nadav.amit@gmail.com>
> >> 
> >> Currently, if a test is expected to fail, but surprisingly it passes,
> >> the test is considered as "failing".
> > 
> > It's not "failing", it's failing. If a test is expected to pass then
> > it shouldn't be getting reported with report_xfail().
> 
> I find this terminology confusing. For instance, there are some tests which
> are probabilistic (e.g., test_sti_nmi) - let’s assume you expect one to fail
> and it passes, would you say that you encountered a failure?

When testing something probabilistic you should take enough samples
that you can check for the expected frequency of the event.

xfail means the test is expected to fail. If it doesn't fail then
the software under test changed since the writing of the test. While
it's possible that something got fixed (turning an xfail to a pass),
it's also possible that the xfail started passing because something
got even more broken then before. The big, fat FAIL alerts testers
to it.

> 
> > Why would one want to run old kvm-unit-tests on new kvm?
> 
> I can think of a couple of reasons, but I am not going to argue too much.
> 

I'm not arguing either. I'm curious. I can't really see why one would
want to test with old kvm-unit-tests unless bisecting a problem with
new kvm-unit-tests. And, if new kvm-unit-tests brings in some undesired
dependency, then we should discuss removing it, rather than avoid the
new versions. (That's just a hypothetical if, because I can't think of
any new dependencies we've added.)

Thanks,
drew

Re: [kvm-unit-tests PATCH] lib: Do not report failures when test passes

[Paolo Bonzini]

On 17/04/19 09:10, Nadav Amit wrote:
>> It's not "failing", it's failing. If a test is expected to pass then
>> it shouldn't be getting reported with report_xfail().
> I find this terminology confusing. For instance, there are some tests which
> are probabilistic (e.g., test_sti_nmi) - let’s assume you expect one to fail
> and it passes, would you say that you encountered a failure?
> 

Yes. :)  Probabilistic tests should be changed so that the probability
of an incorrect result is very, very small.  XPASS are for known bugs or
known virtualization holes, not for probabilistic tests.  Basically, all
they do is spare you from having to invert the result of the test, so
that you can write

	// i actually isn't zero
	report_xfail("i should be zero", true, i == 0);

instead of

	report_xfail("i should be zero, but isn't", i != 0);

XPASS tests are a pleasant kind of failure, but still a surprise that
should be inspected.

There are several testsuite harnesses that fail on XPASS (dejagnu and
meson, for example), and others that succeed on XPASS (for example
"prove", the original TAP client).  In other cases again it's
customizable, for example PyTest ignores both XFAIL and XPASS results,
but it does have an "xfail_strict" option where XPASS will cause the
test suite to fail.

Paolo

Re: [kvm-unit-tests PATCH] lib: Do not report failures when test passes

[Nadav Amit]

> On Apr 17, 2019, at 5:23 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> On 17/04/19 09:10, Nadav Amit wrote:
>>> It's not "failing", it's failing. If a test is expected to pass then
>>> it shouldn't be getting reported with report_xfail().
>> I find this terminology confusing. For instance, there are some tests which
>> are probabilistic (e.g., test_sti_nmi) - let’s assume you expect one to fail
>> and it passes, would you say that you encountered a failure?
> 
> Yes. :)  Probabilistic tests should be changed so that the probability
> of an incorrect result is very, very small.  XPASS are for known bugs or
> known virtualization holes, not for probabilistic tests.  Basically, all
> they do is spare you from having to invert the result of the test, so
> that you can write
> 
> 	// i actually isn't zero
> 	report_xfail("i should be zero", true, i == 0);
> 
> instead of
> 
> 	report_xfail("i should be zero, but isn't", i != 0);
> 
> XPASS tests are a pleasant kind of failure, but still a surprise that
> should be inspected.
> 
> There are several testsuite harnesses that fail on XPASS (dejagnu and
> meson, for example), and others that succeed on XPASS (for example
> "prove", the original TAP client).  In other cases again it's
> customizable, for example PyTest ignores both XFAIL and XPASS results,
> but it does have an "xfail_strict" option where XPASS will cause the
> test suite to fail.

Thanks you both (you and Andrew) for the detailed explanations. I actually
did not know XPASS/XFAIL are a common terminology.