Re: [PATCH kvm-unit-tests v2 05/12] lib/report: allow test skipping
On Fri, Dec 18, 2015 at 12:18:19PM +0100, Radim Krčmář wrote:
> 2015-12-17 13:37-0600, Andrew Jones:
> > On Thu, Dec 17, 2015 at 01:30:23PM -0600, Andrew Jones wrote:
> >> On Thu, Dec 17, 2015 at 06:53:36PM +0100, Radim Krčmář wrote:
> >> > We can now explicitly mark a unit-test as skipped.
> >> > If all unit-tests were skipped, the whole test is reported as skipped as
> >> > well. This also includes the case where no tests were run, but still
> >> > ended with report_summary().
> >> >
> >> > When the whole test is skipped, ./run_tests.sh prints yellow "SKIP"
> >> > instead of green "PASS".
> >> >
> >> > Return value of 77 is used to please Autotools. I also renamed few
> >> > things in reporting code and chose to refactor a logic while at it.
> >> >
> >> > Signed-off-by: Radim Krčmář
> >> > ---
> >> > diff --git a/lib/report.c b/lib/report.c
> >> > @@ -43,25 +43,28 @@ void report_prefix_pop(void)
> >> > -void va_report_xfail(const char *msg_fmt, bool xfail, bool cond,
> >> > va_list va)
> >> > +static void va_report(const char *msg_fmt, bool pass, bool xfail, bool
> >> > skip,
> >> > +va_list va)
> >>
> >> Making this static disallows unit test writers to create their own
> >> variable arg report() wrapper functions. Perhaps to determine whether
> >> or not a skip is in order, e.g.
> >>
> >> xyz_report(msg, pass, ...)
> >> {
> >> va_list va;
> >> va_start(va, pass);
> >> if (xyz)
> >>va_report(msg, pass, false, false, va);
> >> else
> >>va_report(msg, false, false, true, va);
> >> va_end(va);
> >> }
> >
> > Hmm, while I still think we should avoid using static, to allow new
> > wrappers,
> > the wrapper I wrote here as an example wouldn't be necessary if
> > report_skip's
> > inputs were instead
>
> That breaks encapsulation -- if we ever want to change va_report(),
> we've just made our lives harder.
OK, let's make it static and extend the API.
>
> > void report_skip(const char *msg_fmt, bool pass, bool skip, ...)
> >
> > Why not do that?
>
> Yeah, some cases want to unconditionally skip, so we'd want to have
> both. I'll think of naming during lunch :)
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests v2 05/12] lib/report: allow test skipping
2015-12-17 13:37-0600, Andrew Jones:
> On Thu, Dec 17, 2015 at 01:30:23PM -0600, Andrew Jones wrote:
>> On Thu, Dec 17, 2015 at 06:53:36PM +0100, Radim Krčmář wrote:
>> > We can now explicitly mark a unit-test as skipped.
>> > If all unit-tests were skipped, the whole test is reported as skipped as
>> > well. This also includes the case where no tests were run, but still
>> > ended with report_summary().
>> >
>> > When the whole test is skipped, ./run_tests.sh prints yellow "SKIP"
>> > instead of green "PASS".
>> >
>> > Return value of 77 is used to please Autotools. I also renamed few
>> > things in reporting code and chose to refactor a logic while at it.
>> >
>> > Signed-off-by: Radim Krčmář
>> > ---
>> > diff --git a/lib/report.c b/lib/report.c
>> > @@ -43,25 +43,28 @@ void report_prefix_pop(void)
>> > -void va_report_xfail(const char *msg_fmt, bool xfail, bool cond, va_list
>> > va)
>> > +static void va_report(const char *msg_fmt, bool pass, bool xfail, bool
>> > skip,
>> > + va_list va)
>>
>> Making this static disallows unit test writers to create their own
>> variable arg report() wrapper functions. Perhaps to determine whether
>> or not a skip is in order, e.g.
>>
>> xyz_report(msg, pass, ...)
>> {
>> va_list va;
>> va_start(va, pass);
>> if (xyz)
>>va_report(msg, pass, false, false, va);
>> else
>>va_report(msg, false, false, true, va);
>> va_end(va);
>> }
>
> Hmm, while I still think we should avoid using static, to allow new wrappers,
> the wrapper I wrote here as an example wouldn't be necessary if report_skip's
> inputs were instead
That breaks encapsulation -- if we ever want to change va_report(),
we've just made our lives harder.
> void report_skip(const char *msg_fmt, bool pass, bool skip, ...)
>
> Why not do that?
Yeah, some cases want to unconditionally skip, so we'd want to have
both. I'll think of naming during lunch :)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH kvm-unit-tests v2 05/12] lib/report: allow test skipping
On Thu, Dec 17, 2015 at 01:30:23PM -0600, Andrew Jones wrote:
> On Thu, Dec 17, 2015 at 06:53:36PM +0100, Radim Krčmář wrote:
> > We can now explicitly mark a unit-test as skipped.
> > If all unit-tests were skipped, the whole test is reported as skipped as
> > well. This also includes the case where no tests were run, but still
> > ended with report_summary().
> >
> > When the whole test is skipped, ./run_tests.sh prints yellow "SKIP"
> > instead of green "PASS".
> >
> > Return value of 77 is used to please Autotools. I also renamed few
> > things in reporting code and chose to refactor a logic while at it.
> >
> > Signed-off-by: Radim Krčmář
> > ---
> > v2:
> > - turned skip into yellow SKIP [Drew]
> > - wrapped line at 80 characters [Drew]
> > - added static to va_report
> >
> > lib/libcflat.h | 1 +
> > lib/report.c | 44 ++--
> > scripts/run.bash | 12 +++-
> > 3 files changed, 38 insertions(+), 19 deletions(-)
> >
> > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > index 9747ccdbc9f1..070818354ee1 100644
> > --- a/lib/libcflat.h
> > +++ b/lib/libcflat.h
> > @@ -61,6 +61,7 @@ void report_prefix_push(const char *prefix);
> > void report_prefix_pop(void);
> > void report(const char *msg_fmt, bool pass, ...);
> > void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...);
> > +void report_skip(const char *msg_fmt, ...);
> > int report_summary(void);
> >
> > #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0]))
> > diff --git a/lib/report.c b/lib/report.c
> > index 35e664108a92..a47f2e00b529 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;
> > +static unsigned int tests, failures, xfailures, skipped;
> > static char prefixes[256];
> > static struct spinlock lock;
> >
> > @@ -43,25 +43,28 @@ void report_prefix_pop(void)
> > spin_unlock(&lock);
> > }
> >
> > -void va_report_xfail(const char *msg_fmt, bool xfail, bool cond, va_list
> > va)
> > +static void va_report(const char *msg_fmt, bool pass, bool xfail, bool
> > skip,
> > + va_list va)
>
> Making this static disallows unit test writers to create their own
> variable arg report() wrapper functions. Perhaps to determine whether
> or not a skip is in order, e.g.
>
> xyz_report(msg, pass, ...)
> {
> va_list va;
> va_start(va, pass);
> if (xyz)
>va_report(msg, pass, false, false, va);
> else
>va_report(msg, false, false, true, va);
> va_end(va);
> }
Hmm, while I still think we should avoid using static, to allow new wrappers,
the wrapper I wrote here as an example wouldn't be necessary if report_skip's
inputs were instead
void report_skip(const char *msg_fmt, bool pass, bool skip, ...)
Why not do that?
>
> > {
> > - char *pass = xfail ? "XPASS" : "PASS";
> > - char *fail = xfail ? "XFAIL" : "FAIL";
> > char buf[2000];
> > + char *prefix = skip ? "SKIP"
> > + : xfail ? (pass ? "XPASS" : "XFAIL")
> > + : (pass ? "PASS" : "FAIL");
> >
> > spin_lock(&lock);
> >
> > tests++;
> > - printf("%s: ", cond ? pass : fail);
> > + printf("%s: ", prefix);
> > puts(prefixes);
> > vsnprintf(buf, sizeof(buf), msg_fmt, va);
> > puts(buf);
> > puts("\n");
> > - if (xfail && cond)
> > - failures++;
> > - else if (xfail)
> > +
> > + if (skip)
> > + skipped++;
> > + else if (xfail && !pass)
> > xfailures++;
> > - else if (!cond)
> > + else if (xfail || !pass)
> > failures++;
> >
> > spin_unlock(&lock);
> > @@ -71,7 +74,7 @@ void report(const char *msg_fmt, bool pass, ...)
> > {
> > va_list va;
> > va_start(va, pass);
> > - va_report_xfail(msg_fmt, false, pass, va);
> > + va_report(msg_fmt, pass, false, false, va);
> > va_end(va);
> > }
> >
> > @@ -79,7 +82,15 @@ void report_xfail(const char *msg_fmt, bool xfail, bool
> > pass, ...)
> > {
> > va_list va;
> > va_start(va, pass);
> > - va_report_xfail(msg_fmt, xfail, pass, va);
> > + va_report(msg_fmt, pass, xfail, false, va);
> > + va_end(va);
> > +}
> > +
> > +void report_skip(const char *msg_fmt, ...)
> > +{
> > + va_list va;
> > + va_start(va, msg_fmt);
> > + va_report(msg_fmt, false, false, true, va);
> > va_end(va);
> > }
> >
> > @@ -89,9 +100,14 @@ int report_summary(void)
> >
> > printf("\nSUMMARY: %d tests, %d unexpected failures", tests, failures);
> > if (xfailures)
> > - printf(", %d expected failures\n", xfailures);
> > - else
> > - printf("\n");
> > + printf(", %d expected failures", xfailures);
> > + if (skipped)
> > + printf(", %d skipped", skipped);
> > + printf("\n");
> > +
> > + if (tests == skipped)
> > + return 77; /* blame AUTOTOOLS */
> > +
> > r
Re: [PATCH kvm-unit-tests v2 05/12] lib/report: allow test skipping
On Thu, Dec 17, 2015 at 06:53:36PM +0100, Radim Krčmář wrote:
> We can now explicitly mark a unit-test as skipped.
> If all unit-tests were skipped, the whole test is reported as skipped as
> well. This also includes the case where no tests were run, but still
> ended with report_summary().
>
> When the whole test is skipped, ./run_tests.sh prints yellow "SKIP"
> instead of green "PASS".
>
> Return value of 77 is used to please Autotools. I also renamed few
> things in reporting code and chose to refactor a logic while at it.
>
> Signed-off-by: Radim Krčmář
> ---
> v2:
> - turned skip into yellow SKIP [Drew]
> - wrapped line at 80 characters [Drew]
> - added static to va_report
>
> lib/libcflat.h | 1 +
> lib/report.c | 44 ++--
> scripts/run.bash | 12 +++-
> 3 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 9747ccdbc9f1..070818354ee1 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -61,6 +61,7 @@ void report_prefix_push(const char *prefix);
> void report_prefix_pop(void);
> void report(const char *msg_fmt, bool pass, ...);
> void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...);
> +void report_skip(const char *msg_fmt, ...);
> int report_summary(void);
>
> #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0]))
> diff --git a/lib/report.c b/lib/report.c
> index 35e664108a92..a47f2e00b529 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;
> +static unsigned int tests, failures, xfailures, skipped;
> static char prefixes[256];
> static struct spinlock lock;
>
> @@ -43,25 +43,28 @@ void report_prefix_pop(void)
> spin_unlock(&lock);
> }
>
> -void va_report_xfail(const char *msg_fmt, bool xfail, bool cond, va_list va)
> +static void va_report(const char *msg_fmt, bool pass, bool xfail, bool skip,
> + va_list va)
Making this static disallows unit test writers to create their own
variable arg report() wrapper functions. Perhaps to determine whether
or not a skip is in order, e.g.
xyz_report(msg, pass, ...)
{
va_list va;
va_start(va, pass);
if (xyz)
va_report(msg, pass, false, false, va);
else
va_report(msg, false, false, true, va);
va_end(va);
}
> {
> - char *pass = xfail ? "XPASS" : "PASS";
> - char *fail = xfail ? "XFAIL" : "FAIL";
> char buf[2000];
> + char *prefix = skip ? "SKIP"
> + : xfail ? (pass ? "XPASS" : "XFAIL")
> + : (pass ? "PASS" : "FAIL");
>
> spin_lock(&lock);
>
> tests++;
> - printf("%s: ", cond ? pass : fail);
> + printf("%s: ", prefix);
> puts(prefixes);
> vsnprintf(buf, sizeof(buf), msg_fmt, va);
> puts(buf);
> puts("\n");
> - if (xfail && cond)
> - failures++;
> - else if (xfail)
> +
> + if (skip)
> + skipped++;
> + else if (xfail && !pass)
> xfailures++;
> - else if (!cond)
> + else if (xfail || !pass)
> failures++;
>
> spin_unlock(&lock);
> @@ -71,7 +74,7 @@ void report(const char *msg_fmt, bool pass, ...)
> {
> va_list va;
> va_start(va, pass);
> - va_report_xfail(msg_fmt, false, pass, va);
> + va_report(msg_fmt, pass, false, false, va);
> va_end(va);
> }
>
> @@ -79,7 +82,15 @@ void report_xfail(const char *msg_fmt, bool xfail, bool
> pass, ...)
> {
> va_list va;
> va_start(va, pass);
> - va_report_xfail(msg_fmt, xfail, pass, va);
> + va_report(msg_fmt, pass, xfail, false, va);
> + va_end(va);
> +}
> +
> +void report_skip(const char *msg_fmt, ...)
> +{
> + va_list va;
> + va_start(va, msg_fmt);
> + va_report(msg_fmt, false, false, true, va);
> va_end(va);
> }
>
> @@ -89,9 +100,14 @@ int report_summary(void)
>
> printf("\nSUMMARY: %d tests, %d unexpected failures", tests, failures);
> if (xfailures)
> - printf(", %d expected failures\n", xfailures);
> - else
> - printf("\n");
> + printf(", %d expected failures", xfailures);
> + if (skipped)
> + printf(", %d skipped", skipped);
> + printf("\n");
> +
> + if (tests == skipped)
> + return 77; /* blame AUTOTOOLS */
> +
> return failures > 0 ? 1 : 0;
>
> spin_unlock(&lock);
> diff --git a/scripts/run.bash b/scripts/run.bash
> index 243586c6d2fc..b92611c29fbb 100644
> --- a/scripts/run.bash
> +++ b/scripts/run.bash
> @@ -46,11 +46,13 @@ function run()
> # Unit-tests' return value is shifted by one.
> ret=$(($? >> 1))
>
> -if [ $ret -eq 0 ]; then
> -echo -e "\e[32mPASS\e[0m $1"
> -else
> -echo -e "\e[31mFAIL\e[0m $1"
> -fi
> +case $ret in
> +0) echo -ne "\e[32mPASS\e[0m" ;;
> +
