[RFC kvm-unit-tests PATCH] lib/report: Return pass/fail result from report

[RFC kvm-unit-tests PATCH] lib/report: Return pass/fail result from report

[Andrew Jones]

A nice pattern to use in order to try and maintain parsable reports,
but also output unexpected values, is

    if (!report(value == expected_value, "my test")) {
        report_info("failure due to unexpected value (received %d, expected %d)",
                    value, expected_value);
    }

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/libcflat.h |  6 +++---
 lib/report.c   | 28 +++++++++++++++++++++-------
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/lib/libcflat.h b/lib/libcflat.h
index eec34c3f2710..b4110b9ec91b 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -97,11 +97,11 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
 extern void report_prefix_push(const char *prefix);
 extern void report_prefix_pop(void);
 extern void report_prefix_popn(int n);
-extern void report(bool pass, const char *msg_fmt, ...)
+extern bool 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, ...)
+extern bool 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, ...)
+extern bool 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)))
diff --git a/lib/report.c b/lib/report.c
index 0756e64e6f10..43c0102c1b0e 100644
--- a/lib/report.c
+++ b/lib/report.c
@@ -89,7 +89,7 @@ void report_prefix_popn(int n)
 	spin_unlock(&lock);
 }
 
-static void va_report(const char *msg_fmt,
+static bool va_report(const char *msg_fmt,
 		bool pass, bool xfail, bool kfail, bool skip, va_list va)
 {
 	const char *prefix = skip ? "SKIP"
@@ -114,14 +114,20 @@ static void va_report(const char *msg_fmt,
 		failures++;
 
 	spin_unlock(&lock);
+
+	return pass || xfail;
 }
 
-void report(bool pass, const char *msg_fmt, ...)
+bool report(bool pass, const char *msg_fmt, ...)
 {
 	va_list va;
+	bool ret;
+
 	va_start(va, msg_fmt);
-	va_report(msg_fmt, pass, false, false, false, va);
+	ret = va_report(msg_fmt, pass, false, false, false, va);
 	va_end(va);
+
+	return ret;
 }
 
 void report_pass(const char *msg_fmt, ...)
@@ -142,24 +148,32 @@ void report_fail(const char *msg_fmt, ...)
 	va_end(va);
 }
 
-void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
+bool report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
 {
+	bool ret;
+
 	va_list va;
 	va_start(va, msg_fmt);
-	va_report(msg_fmt, pass, xfail, false, false, va);
+	ret = va_report(msg_fmt, pass, xfail, false, false, va);
 	va_end(va);
+
+	return ret;
 }
 
 /*
  * 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, ...)
+bool report_kfail(bool kfail, bool pass, const char *msg_fmt, ...)
 {
+	bool ret;
+
 	va_list va;
 	va_start(va, msg_fmt);
-	va_report(msg_fmt, pass, false, kfail, false, va);
+	ret = va_report(msg_fmt, pass, false, kfail, false, va);
 	va_end(va);
+
+	return ret;
 }
 
 void report_skip(const char *msg_fmt, ...)
-- 
2.47.0

Re: [RFC kvm-unit-tests PATCH] lib/report: Return pass/fail result from report

[Alexandru Elisei]

Hi Drew,

On Wed, Oct 23, 2024 at 06:53:48PM +0200, Andrew Jones wrote:
> A nice pattern to use in order to try and maintain parsable reports,
> but also output unexpected values, is
> 
>     if (!report(value == expected_value, "my test")) {
>         report_info("failure due to unexpected value (received %d, expected %d)",
>                     value, expected_value);
>     }

This looks like a good idea to me, makes the usage of report() similar to
the kernel pattern of wrapping an if condition around WARN_ON():

	if (WARN_ON(condition)) {
		do_stuff()
	}

Plus, current users are not affected by the change so I see no reason not
to have the choice.

> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  lib/libcflat.h |  6 +++---
>  lib/report.c   | 28 +++++++++++++++++++++-------
>  2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index eec34c3f2710..b4110b9ec91b 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -97,11 +97,11 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
>  extern void report_prefix_push(const char *prefix);
>  extern void report_prefix_pop(void);
>  extern void report_prefix_popn(int n);
> -extern void report(bool pass, const char *msg_fmt, ...)
> +extern bool 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, ...)
> +extern bool 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, ...)
> +extern bool 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)))
> diff --git a/lib/report.c b/lib/report.c
> index 0756e64e6f10..43c0102c1b0e 100644
> --- a/lib/report.c
> +++ b/lib/report.c
> @@ -89,7 +89,7 @@ void report_prefix_popn(int n)
>  	spin_unlock(&lock);
>  }
>  
> -static void va_report(const char *msg_fmt,
> +static bool va_report(const char *msg_fmt,
>  		bool pass, bool xfail, bool kfail, bool skip, va_list va)
>  {
>  	const char *prefix = skip ? "SKIP"
> @@ -114,14 +114,20 @@ static void va_report(const char *msg_fmt,
>  		failures++;
>  
>  	spin_unlock(&lock);
> +
> +	return pass || xfail;

va_report() has 4 boolean parameters that the callers set. 'kfail' can be
ignored, because all it does is control which variable serves as the
accumulator for the failure.

I was thinking about the 'skip' parameter - report_skip() sets pass = xfail
= false, skip = true. Does it matter that va_report() returns false for
report_skip()? I don't think so (report_skip() returns void), just wanting
to make sure we've considered all the cases.  Sorry if this looks like
nitpicking.

Other than that, the patch looks good to me.

Thanks,
Alex

>  }
>  
> -void report(bool pass, const char *msg_fmt, ...)
> +bool report(bool pass, const char *msg_fmt, ...)
>  {
>  	va_list va;
> +	bool ret;
> +
>  	va_start(va, msg_fmt);
> -	va_report(msg_fmt, pass, false, false, false, va);
> +	ret = va_report(msg_fmt, pass, false, false, false, va);
>  	va_end(va);
> +
> +	return ret;
>  }
>  
>  void report_pass(const char *msg_fmt, ...)
> @@ -142,24 +148,32 @@ void report_fail(const char *msg_fmt, ...)
>  	va_end(va);
>  }
>  
> -void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
> +bool report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
>  {
> +	bool ret;
> +
>  	va_list va;
>  	va_start(va, msg_fmt);
> -	va_report(msg_fmt, pass, xfail, false, false, va);
> +	ret = va_report(msg_fmt, pass, xfail, false, false, va);
>  	va_end(va);
> +
> +	return ret;
>  }
>  
>  /*
>   * 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, ...)
> +bool report_kfail(bool kfail, bool pass, const char *msg_fmt, ...)
>  {
> +	bool ret;
> +
>  	va_list va;
>  	va_start(va, msg_fmt);
> -	va_report(msg_fmt, pass, false, kfail, false, va);
> +	ret = va_report(msg_fmt, pass, false, kfail, false, va);
>  	va_end(va);
> +
> +	return ret;
>  }
>  
>  void report_skip(const char *msg_fmt, ...)
> -- 
> 2.47.0
> 
> 

Re: [RFC kvm-unit-tests PATCH] lib/report: Return pass/fail result from report

[Andrew Jones]

On Fri, Oct 25, 2024 at 04:45:54PM +0100, Alexandru Elisei wrote:
> Hi Drew,
> 
> On Wed, Oct 23, 2024 at 06:53:48PM +0200, Andrew Jones wrote:
> > A nice pattern to use in order to try and maintain parsable reports,
> > but also output unexpected values, is
> > 
> >     if (!report(value == expected_value, "my test")) {
> >         report_info("failure due to unexpected value (received %d, expected %d)",
> >                     value, expected_value);
> >     }
> 
> This looks like a good idea to me, makes the usage of report() similar to
> the kernel pattern of wrapping an if condition around WARN_ON():
> 
> 	if (WARN_ON(condition)) {
> 		do_stuff()
> 	}
> 
> Plus, current users are not affected by the change so I see no reason not
> to have the choice.
> 
> > 
> > Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> > ---
> >  lib/libcflat.h |  6 +++---
> >  lib/report.c   | 28 +++++++++++++++++++++-------
> >  2 files changed, 24 insertions(+), 10 deletions(-)
> > 
> > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > index eec34c3f2710..b4110b9ec91b 100644
> > --- a/lib/libcflat.h
> > +++ b/lib/libcflat.h
> > @@ -97,11 +97,11 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
> >  extern void report_prefix_push(const char *prefix);
> >  extern void report_prefix_pop(void);
> >  extern void report_prefix_popn(int n);
> > -extern void report(bool pass, const char *msg_fmt, ...)
> > +extern bool 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, ...)
> > +extern bool 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, ...)
> > +extern bool 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)))
> > diff --git a/lib/report.c b/lib/report.c
> > index 0756e64e6f10..43c0102c1b0e 100644
> > --- a/lib/report.c
> > +++ b/lib/report.c
> > @@ -89,7 +89,7 @@ void report_prefix_popn(int n)
> >  	spin_unlock(&lock);
> >  }
> >  
> > -static void va_report(const char *msg_fmt,
> > +static bool va_report(const char *msg_fmt,
> >  		bool pass, bool xfail, bool kfail, bool skip, va_list va)
> >  {
> >  	const char *prefix = skip ? "SKIP"
> > @@ -114,14 +114,20 @@ static void va_report(const char *msg_fmt,
> >  		failures++;
> >  
> >  	spin_unlock(&lock);
> > +
> > +	return pass || xfail;
> 
> va_report() has 4 boolean parameters that the callers set. 'kfail' can be
> ignored, because all it does is control which variable serves as the
> accumulator for the failure.
> 
> I was thinking about the 'skip' parameter - report_skip() sets pass = xfail
> = false, skip = true. Does it matter that va_report() returns false for
> report_skip()? I don't think so (report_skip() returns void), just wanting
> to make sure we've considered all the cases.  Sorry if this looks like
> nitpicking.

I think I considered all the cases, but if you see something missing, then
I'm all ears.

> 
> Other than that, the patch looks good to me.

Thanks,
drew

Re: [RFC kvm-unit-tests PATCH] lib/report: Return pass/fail result from report

[Claudio Imbrenda]

On Wed, 23 Oct 2024 18:53:48 +0200
Andrew Jones <andrew.jones@linux.dev> wrote:

> A nice pattern to use in order to try and maintain parsable reports,
> but also output unexpected values, is
> 
>     if (!report(value == expected_value, "my test")) {
>         report_info("failure due to unexpected value (received %d, expected %d)",
>                     value, expected_value);
>     }

it would be cool if we could somehow do this with just one function
call or macro, but I can't really think of a reasonable way to do it.

this patch is a good step in that direction, though

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  lib/libcflat.h |  6 +++---
>  lib/report.c   | 28 +++++++++++++++++++++-------
>  2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index eec34c3f2710..b4110b9ec91b 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -97,11 +97,11 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
>  extern void report_prefix_push(const char *prefix);
>  extern void report_prefix_pop(void);
>  extern void report_prefix_popn(int n);
> -extern void report(bool pass, const char *msg_fmt, ...)
> +extern bool 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, ...)
> +extern bool 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, ...)
> +extern bool 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)))
> diff --git a/lib/report.c b/lib/report.c
> index 0756e64e6f10..43c0102c1b0e 100644
> --- a/lib/report.c
> +++ b/lib/report.c
> @@ -89,7 +89,7 @@ void report_prefix_popn(int n)
>  	spin_unlock(&lock);
>  }
>  
> -static void va_report(const char *msg_fmt,
> +static bool va_report(const char *msg_fmt,
>  		bool pass, bool xfail, bool kfail, bool skip, va_list va)
>  {
>  	const char *prefix = skip ? "SKIP"
> @@ -114,14 +114,20 @@ static void va_report(const char *msg_fmt,
>  		failures++;
>  
>  	spin_unlock(&lock);
> +
> +	return pass || xfail;
>  }
>  
> -void report(bool pass, const char *msg_fmt, ...)
> +bool report(bool pass, const char *msg_fmt, ...)
>  {
>  	va_list va;
> +	bool ret;
> +
>  	va_start(va, msg_fmt);
> -	va_report(msg_fmt, pass, false, false, false, va);
> +	ret = va_report(msg_fmt, pass, false, false, false, va);
>  	va_end(va);
> +
> +	return ret;
>  }
>  
>  void report_pass(const char *msg_fmt, ...)
> @@ -142,24 +148,32 @@ void report_fail(const char *msg_fmt, ...)
>  	va_end(va);
>  }
>  
> -void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
> +bool report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
>  {
> +	bool ret;
> +
>  	va_list va;
>  	va_start(va, msg_fmt);
> -	va_report(msg_fmt, pass, xfail, false, false, va);
> +	ret = va_report(msg_fmt, pass, xfail, false, false, va);
>  	va_end(va);
> +
> +	return ret;
>  }
>  
>  /*
>   * 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, ...)
> +bool report_kfail(bool kfail, bool pass, const char *msg_fmt, ...)
>  {
> +	bool ret;
> +
>  	va_list va;
>  	va_start(va, msg_fmt);
> -	va_report(msg_fmt, pass, false, kfail, false, va);
> +	ret = va_report(msg_fmt, pass, false, kfail, false, va);
>  	va_end(va);
> +
> +	return ret;
>  }
>  
>  void report_skip(const char *msg_fmt, ...)

Re: [RFC kvm-unit-tests PATCH] lib/report: Return pass/fail result from report

[Andrew Jones]

On Wed, Oct 23, 2024 at 06:53:48PM +0200, Andrew Jones wrote:
> A nice pattern to use in order to try and maintain parsable reports,
> but also output unexpected values, is
> 
>     if (!report(value == expected_value, "my test")) {
>         report_info("failure due to unexpected value (received %d, expected %d)",
>                     value, expected_value);
>     }
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  lib/libcflat.h |  6 +++---
>  lib/report.c   | 28 +++++++++++++++++++++-------
>  2 files changed, 24 insertions(+), 10 deletions(-)

Merged.

Thanks,
drew