diff mbox series

[kvm-unit-tests,v9,02/31] report: Add known failure reporting option

Message ID 20240504122841.1177683-3-npiggin@gmail.com (mailing list archive)
State Handled Elsewhere
Headers show
Series powerpc improvements | expand

Commit Message

Nicholas Piggin May 4, 2024, 12:28 p.m. UTC 
There are times we would like to test a function that is known to fail
in some conditions due to a bug in implementation (QEMU, KVM, or even
hardware). It would be nice to count these as known failures and not
report a summary failure.

xfail is not the same thing, xfail means failure is required and a pass
causes the test to fail. So add kfail for known failures.

Mark the failing ppc64 h_cede_tm and spapr_vpa tests as kfail.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/libcflat.h |  2 ++
 lib/report.c   | 33 +++++++++++++++++++++++++--------
 2 files changed, 27 insertions(+), 8 deletions(-)

Comments

Thomas Huth May 6, 2024, 7:25 a.m. UTC | #1 
On 04/05/2024 14.28, Nicholas Piggin wrote:
> There are times we would like to test a function that is known to fail
> in some conditions due to a bug in implementation (QEMU, KVM, or even
> hardware). It would be nice to count these as known failures and not
> report a summary failure.
> 
> xfail is not the same thing, xfail means failure is required and a pass
> causes the test to fail. So add kfail for known failures.

Actually, I wonder whether that's not rather a bug in report_xfail() 
instead. Currently, when you call report_xfail(true, ...), the result is 
*always* counted as a failure, either as an expected failure (if the test 
really failed), or as a normal failure (if the test succeeded). What's the 
point of counting a successful test as a failure??

Andrew, you've originally introduced report_xfail in commit a5af7b8a67e, 
could you please comment on this?

IMHO we should rather do something like this instead:

diff --git a/lib/report.c b/lib/report.c
--- a/lib/report.c
+++ b/lib/report.c
@@ -98,7 +98,7 @@ static void va_report(const char *msg_fmt,
                 skipped++;
         else if (xfail && !pass)
                 xfailures++;
-       else if (xfail || !pass)
+       else if (!xfail && !pass)
                 failures++;

         spin_unlock(&lock);

  Thomas
Andrew Jones May 6, 2024, 8:01 a.m. UTC | #2 
On Mon, May 06, 2024 at 09:25:37AM GMT, Thomas Huth wrote:
> On 04/05/2024 14.28, Nicholas Piggin wrote:
> > There are times we would like to test a function that is known to fail
> > in some conditions due to a bug in implementation (QEMU, KVM, or even
> > hardware). It would be nice to count these as known failures and not
> > report a summary failure.
> > 
> > xfail is not the same thing, xfail means failure is required and a pass
> > causes the test to fail. So add kfail for known failures.
> 
> Actually, I wonder whether that's not rather a bug in report_xfail()
> instead. Currently, when you call report_xfail(true, ...), the result is
> *always* counted as a failure, either as an expected failure (if the test
> really failed), or as a normal failure (if the test succeeded). What's the
> point of counting a successful test as a failure??
> 
> Andrew, you've originally introduced report_xfail in commit a5af7b8a67e,
> could you please comment on this?
> 

An expected failure passes when the test fails and fails when the test
passes, i.e.

  XFAIL == PASS (but separately accounted with 'xfailures')
  XPASS == FAIL

If we expect something to fail and it passes then this may be due to the
thing being fixed, so we should change the test to expect success, or
due to the test being written incorrectly for our expectations. Either
way, when an expected failure doesn't fail, it means our expectations are
wrong and we need to be alerted to that, hence a FAIL is reported.

Thanks,
drew

> IMHO we should rather do something like this instead:
> 
> diff --git a/lib/report.c b/lib/report.c
> --- a/lib/report.c
> +++ b/lib/report.c
> @@ -98,7 +98,7 @@ static void va_report(const char *msg_fmt,
>                 skipped++;
>         else if (xfail && !pass)
>                 xfailures++;
> -       else if (xfail || !pass)
> +       else if (!xfail && !pass)
>                 failures++;
> 
>         spin_unlock(&lock);
> 
>  Thomas
>
Thomas Huth May 6, 2024, 10:19 a.m. UTC | #3 
On 06/05/2024 10.01, Andrew Jones wrote:
> On Mon, May 06, 2024 at 09:25:37AM GMT, Thomas Huth wrote:
>> On 04/05/2024 14.28, Nicholas Piggin wrote:
>>> There are times we would like to test a function that is known to fail
>>> in some conditions due to a bug in implementation (QEMU, KVM, or even
>>> hardware). It would be nice to count these as known failures and not
>>> report a summary failure.
>>>
>>> xfail is not the same thing, xfail means failure is required and a pass
>>> causes the test to fail. So add kfail for known failures.
>>
>> Actually, I wonder whether that's not rather a bug in report_xfail()
>> instead. Currently, when you call report_xfail(true, ...), the result is
>> *always* counted as a failure, either as an expected failure (if the test
>> really failed), or as a normal failure (if the test succeeded). What's the
>> point of counting a successful test as a failure??
>>
>> Andrew, you've originally introduced report_xfail in commit a5af7b8a67e,
>> could you please comment on this?
>>
> 
> An expected failure passes when the test fails and fails when the test
> passes, i.e.
> 
>    XFAIL == PASS (but separately accounted with 'xfailures')
>    XPASS == FAIL
> 
> If we expect something to fail and it passes then this may be due to the
> thing being fixed, so we should change the test to expect success, or
> due to the test being written incorrectly for our expectations. Either
> way, when an expected failure doesn't fail, it means our expectations are
> wrong and we need to be alerted to that, hence a FAIL is reported.

Ok, so this was on purpose, indeed. Maybe we should add this information in 
a comment right in front of the function, so that others don't scratch their 
head, too?

Anyway, this patch here is fine then:
Reviewed-by: Thomas Huth <thuth@redhat.com>
diff mbox series

Patch

diff --git a/lib/libcflat.h b/lib/libcflat.h
index 700f43527..ae3c2c6d0 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -100,6 +100,8 @@  extern void report(bool pass, const char *msg_fmt, ...)
 		__attribute__((format(printf, 2, 3), nonnull(2)));
 extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
 		__attribute__((format(printf, 3, 4), nonnull(3)));
+extern void report_kfail(bool kfail, bool pass, const char *msg_fmt, ...)
+		__attribute__((format(printf, 3, 4), nonnull(3)));
 extern void report_abort(const char *msg_fmt, ...)
 					__attribute__((format(printf, 1, 2)))
 					__attribute__((noreturn));
diff --git a/lib/report.c b/lib/report.c
index 8e9bff5b8..7f3c4f059 100644
--- a/lib/report.c
+++ b/lib/report.c
@@ -13,7 +13,7 @@ 
 #include "libcflat.h"
 #include "asm/spinlock.h"
 
-static unsigned int tests, failures, xfailures, skipped;
+static unsigned int tests, failures, xfailures, kfailures, skipped;
 static char prefixes[256];
 static struct spinlock lock;
 
@@ -81,11 +81,12 @@  void report_prefix_pop(void)
 }
 
 static void va_report(const char *msg_fmt,
-		bool pass, bool xfail, bool skip, va_list va)
+		bool pass, bool xfail, bool kfail, bool skip, va_list va)
 {
 	const char *prefix = skip ? "SKIP"
 				  : xfail ? (pass ? "XPASS" : "XFAIL")
-					  : (pass ? "PASS"  : "FAIL");
+				          : kfail ? (pass ? "PASS" : "KFAIL")
+					          : (pass ? "PASS"  : "FAIL");
 
 	spin_lock(&lock);
 
@@ -98,6 +99,8 @@  static void va_report(const char *msg_fmt,
 		skipped++;
 	else if (xfail && !pass)
 		xfailures++;
+	else if (kfail && !pass)
+		kfailures++;
 	else if (xfail || !pass)
 		failures++;
 
@@ -108,7 +111,7 @@  void report(bool pass, const char *msg_fmt, ...)
 {
 	va_list va;
 	va_start(va, msg_fmt);
-	va_report(msg_fmt, pass, false, false, va);
+	va_report(msg_fmt, pass, false, false, false, va);
 	va_end(va);
 }
 
@@ -117,7 +120,7 @@  void report_pass(const char *msg_fmt, ...)
 	va_list va;
 
 	va_start(va, msg_fmt);
-	va_report(msg_fmt, true, false, false, va);
+	va_report(msg_fmt, true, false, false, false, va);
 	va_end(va);
 }
 
@@ -126,7 +129,7 @@  void report_fail(const char *msg_fmt, ...)
 	va_list va;
 
 	va_start(va, msg_fmt);
-	va_report(msg_fmt, false, false, false, va);
+	va_report(msg_fmt, false, false, false, false, va);
 	va_end(va);
 }
 
@@ -134,7 +137,19 @@  void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
 {
 	va_list va;
 	va_start(va, msg_fmt);
-	va_report(msg_fmt, pass, xfail, false, va);
+	va_report(msg_fmt, pass, xfail, false, false, va);
+	va_end(va);
+}
+
+/*
+ * kfail is known failure. If kfail is true then test will succeed
+ * regardless of pass.
+ */
+void report_kfail(bool kfail, bool pass, const char *msg_fmt, ...)
+{
+	va_list va;
+	va_start(va, msg_fmt);
+	va_report(msg_fmt, pass, false, kfail, false, va);
 	va_end(va);
 }
 
@@ -142,7 +157,7 @@  void report_skip(const char *msg_fmt, ...)
 {
 	va_list va;
 	va_start(va, msg_fmt);
-	va_report(msg_fmt, false, false, true, va);
+	va_report(msg_fmt, false, false, false, true, va);
 	va_end(va);
 }
 
@@ -168,6 +183,8 @@  int report_summary(void)
 	printf("SUMMARY: %d tests", tests);
 	if (failures)
 		printf(", %d unexpected failures", failures);
+	if (kfailures)
+		printf(", %d known failures", kfailures);
 	if (xfailures)
 		printf(", %d expected failures", xfailures);
 	if (skipped)