Re: [PATCH kvm-unit-tests v2 05/12] lib/report: allow test skipping

From: "Radim Krčmář" <rkrcmar@redhat.com>
To: Andrew Jones <drjones@redhat.com>
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH kvm-unit-tests v2 05/12] lib/report: allow test skipping
Date: Fri, 18 Dec 2015 12:18:19 +0100	[thread overview]
Message-ID: <20151218111818.GB21525@potion.brq.redhat.com> (raw)
In-Reply-To: <20151217193702.GF14168@hawk.localdomain>

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ář <rkrcmar@redhat.com>
>> > ---
>> > 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 :)