You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@collab.net> on 2009/03/10 14:06:29 UTC

Test XFail/Skip policy (was: svn commit: r36439 - trunk/subversion/tests/cmdline)

> Isn't the whole point of the test suite to identify bugs? So, if you
> unconditionally Skip a test that fails on some platforms, it'll never
> get fixed because it'll never get run, and no-one ever reviews skipped
> tetss. Might as well remove the test then, what's the point?
> 
> Or maybe fix the bug, who knows.
> 
> -- Brane

I could really use some clarification on this, myself.

As Brane rightly says, the point of the test suite is to identify bugs.  Now
obviously, bugs come in various shapes and sizes, different complexities and
severities.  Some bugs we can live with for now; some we need to fix
immediately.

Our test suite provides a way to flag our level of concern about various
bugs, too, I think.  I'm just not sure we're effectively taking advantage of
it.  The following is the set of rules I've carried in my head about our
test decorators:

   Skip() should be reserved for test cases that don't make sense in a given
   scenario.  This is *not* a conditional XFail() -- it's like a "N/A" (not
   applicable) marker, and only that.  (I suppose it could also be used
   without condition for test cases which themselves aren't completely well-
   formed, but I'd be in favor of an alias BadTest() or somesuch to disam-
   biguate that case.)

   XFail() is for tests that fail for known reasons when those known reasons
   are deemed to be of *shrug*-level severity.  You'd like the test to pass,
   but you wouldn't hold up a release for the bug that's causing it not to.

Now, I don't always obey these nicely defined rules myself.  When adding a
new test, it's pretty common for me (and others) to write the test, XFail()
it (without considering the bug's severity), and commit.  Then later, we fix
the bug, remove the XFail(), and commit the fix.  As routine as this has
become, I think this undermines the effectiveness of our test suite.  I
*should* (in my opinion) be applying the rules I listed above when composing
that regression test, which means possibly adding a test *not* marked as
XFail() -- even when I know it's going to fail -- if the bug's severity
dictates as much.  What if, despite my best intentions to fix the bug
immediately, I get tied up elsewhere after composing the test but before
fixing the bug?  Do any of us routinely survey the XFail()ing tests to see
if any particularly horrid bugs are "expected"?  I know I don't.  A failing
test -- and the public outrage that accompanies it -- are infinitely louder
than an XFail()ing test.

On the flip side, I'm also aware of the "Cry Wolf" effect that can happen if
the test suite is always failing.  It benefits no one if we grow calloused
to the notifications of our early alert system to the point of ignoring it.

Am I off-base here?  Is there a better policy summary that someone could
whip up (and add to hacking.html)?

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1303205

Re: Test XFail/Skip policy

Posted by Greg Stein <gs...@gmail.com>.
On Wed, Mar 11, 2009 at 01:28, Branko Čibej <br...@xbc.nu> wrote:
> Stefan Sperling wrote:
>...
>> And maybe s/WIMP/WIP/g ?
>> Or what does the M in WIMP stand for?
>> Work In Much Progress?
>
> Does it have to stand for anything? I like meaningful not-an-acronyms,
> and we can have flamewars over it, like for "svn blame". :-D

It may be that Stefan doesn't know what "wimp" means...
  http://www.answers.com/wimp


hehe....

Cheers,
-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1305603


Re: Test XFail/Skip policy

Posted by Branko Cibej <br...@xbc.nu>.
Stefan Sperling wrote:
> On Wed, Mar 11, 2009 at 01:00:14AM +0100, Greg Stein wrote:
>   
>> Reviewed. Looks great!
>>     
>
> Same here.
>
> I'd suggest s/behavioiur/behaviour/g
> (The u is optional if you prefer american spelling.)
>   

It would be behavior then :) The second i is less than optional unless
you prefer typos ... yep, my writing is notoriously typoid.

> And maybe s/WIMP/WIP/g ?
> Or what does the M in WIMP stand for?
> Work In Much Progress?
>   

Does it have to stand for anything? I like meaningful not-an-acronyms,
and we can have flamewars over it, like for "svn blame". :-D

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1305445

Re: Test XFail/Skip policy

Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 11, 2009 at 01:00:14AM +0100, Greg Stein wrote:
> Reviewed. Looks great!

Same here.

I'd suggest s/behavioiur/behaviour/g
(The u is optional if you prefer american spelling.)

And maybe s/WIMP/WIP/g ?
Or what does the M in WIMP stand for?
Work In Much Progress?

Stefan

Re: Test XFail/Skip policy

Posted by Greg Stein <gs...@gmail.com>.
Reviewed. Looks great!

On Wed, Mar 11, 2009 at 00:21, Branko Cibej <br...@xbc.nu> wrote:
> Branko Čibej wrote:
>> Stefan Sperling wrote:
>>
>>> On Tue, Mar 10, 2009 at 04:07:15PM +0100, Branko Cibej wrote:
>>>
>>>
>>>> The thing I'd find useful is adding an optional comment to XFail and
>>>> Skip; so for this test, you could Xfail(foo, reason="yeah we know its
>>>> broken, this is issue #bla, foo@ is working on it, don't panic")
>>>>
>>>>
>>> Yeah, that would do!
>>>
>>>
>>
>> Guess what -- that's a bitesize (for me). :) And an opportunity to
>> contribute some code, not just blab, after a long time. I'm on it.
>>
>
> r36475. Please review. We can adjust terminology.
>
> This introduces the concept of a "work in progress" test; It's similar
> to an XPASS, except that neither pass nor fail is treated as a test
> failure. The test results are summarized separately so that they don't
> get lost in the noise, and there's a separate kind of "work-in-progress"
> description distinct from the test name.
>
> I adjusted the patch-tests and one XFAILing fs-test to this new concept.
>
> -- Brane
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1305230
>

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1305338


Re: Test XFail/Skip policy

Posted by Branko Cibej <br...@xbc.nu>.
Branko Čibej wrote:
> Stefan Sperling wrote:
>   
>> On Tue, Mar 10, 2009 at 04:07:15PM +0100, Branko Cibej wrote:
>>   
>>     
>>> The thing I'd find useful is adding an optional comment to XFail and
>>> Skip; so for this test, you could Xfail(foo, reason="yeah we know its
>>> broken, this is issue #bla, foo@ is working on it, don't panic")
>>>     
>>>       
>> Yeah, that would do!
>>   
>>     
>
> Guess what -- that's a bitesize (for me). :) And an opportunity to
> contribute some code, not just blab, after a long time. I'm on it.
>   

r36475. Please review. We can adjust terminology.

This introduces the concept of a "work in progress" test; It's similar
to an XPASS, except that neither pass nor fail is treated as a test
failure. The test results are summarized separately so that they don't
get lost in the noise, and there's a separate kind of "work-in-progress"
description distinct from the test name.

I adjusted the patch-tests and one XFAILing fs-test to this new concept.

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1305230


RE: Test XFail/Skip policy

Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Bert Huijben wrote on Tue, 10 Mar 2009 at 16:32 +0100:
> Currently XFail is a must-fail and breaks the buildbots if it doesn't fail
> (XPass error).
> 

If that's a problem, we could make the buildbots stop considering XPass an 
error[1] --- i.e., make them only color a run red if it had any FAIL, not 
if it had either a FAIL or an XPASS.

Daniel



[1] "an error" == "makes the buildbot 'red' and prints 'FAILURE' in the
output of 'make check'"

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1303861

Re: Test XFail/Skip policy

Posted by Greg Stein <gs...@gmail.com>.
On Tue, Mar 10, 2009 at 17:17, Branko Cibej <br...@xbc.nu> wrote:
> Bert Huijben wrote:
>> If the build fails here at work, it must be fixed ASAP.. not half a week later.
>> (These patch tests started failing last Saturday; the moment the branch was merged to trunk)
>>
>
> I don't see a reason why work should be any different than this project.

Because this is a VOLUNTEER project.

People don't have a ton of time to work on this stuff. They don't have
a paycheck on the line.

> So I'm not happy that things have been failing for so long.
>
>>>  We're not reinventing the wheel WRT test results here; we inherited the
>>> PASS/FAIL/XPASS/XFAIL from older systems that have a long history of
>>> using them.
>>>
>>
>> This all assumes somebody investigates problems until they are a stable PASS or a stable FAIL, which is not the case here.
>>
>> I think Arfrever can use some help to get these tests in either an Pass or an XFail. Until that point is reached the test code doesn't belong on trunk. At least not in a way that it can fail the buildbots.
>>
>
> I agree -- like Greg said, revert it or fix it. :)

Reverting it would be dumb. Fixing it is: skip the test.

When somebody gets some time to work on the tests, *then* it can be
moved to something better.

I forget who suggested it, but an alias for Skip() would be nice:

  NeedsWork(...)

It could be reported separately, too, and we make sure it hits zero
before release. But it doesn't cause buildbots to light up.

Cheers,
-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1303860


Re: Test XFail/Skip policy

Posted by Branko Cibej <br...@xbc.nu>.
Bert Huijben wrote:
> If the build fails here at work, it must be fixed ASAP.. not half a week later.
> (These patch tests started failing last Saturday; the moment the branch was merged to trunk)
>   

I don't see a reason why work should be any different than this project.
So I'm not happy that things have been failing for so long.

>>  We're not reinventing the wheel WRT test results here; we inherited the
>> PASS/FAIL/XPASS/XFAIL from older systems that have a long history of
>> using them.
>>     
>
> This all assumes somebody investigates problems until they are a stable PASS or a stable FAIL, which is not the case here. 
>
> I think Arfrever can use some help to get these tests in either an Pass or an XFail. Until that point is reached the test code doesn't belong on trunk. At least not in a way that it can fail the buildbots.
>   

I agree -- like Greg said, revert it or fix it. :)

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1303819

RE: Test XFail/Skip policy

Posted by Bert Huijben <rh...@sharpsvn.net>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@xbc.nu]
> Sent: dinsdag 10 maart 2009 16:41
> To: C. Michael Pilato
> Cc: Bert Huijben; dev@subversion.tigris.org
> Subject: Re: Test XFail/Skip policy
> 
> C. Michael Pilato wrote:
> > Bert Huijben wrote:
> >
> >>> -----Original Message-----
> >>> From: Branko Cibej [mailto:brane@xbc.nu]
> >>> Sent: dinsdag 10 maart 2009 16:27
> >>> To: Branko Cibej; dev@subversion.tigris.org
> >>> Subject: Re: Test XFail/Skip policy
> >>>
> >>> Stefan Sperling wrote:
> >>>
> >>>> On Tue, Mar 10, 2009 at 04:07:15PM +0100, Branko Cibej wrote:
> >>>>
> >>>>
> >>>>> The thing I'd find useful is adding an optional comment to XFail and
> >>>>> Skip; so for this test, you could Xfail(foo, reason="yeah we know its
> >>>>> broken, this is issue #bla, foo@ is working on it, don't panic")
> >>>>>
> >>>>>
> >>>> Yeah, that would do!
> >>>>
> >>>>
> >>> Guess what -- that's a bitesize (for me). :) And an opportunity to
> >>> contribute some code, not just blab, after a long time. I'm on it.
> >>>
> >> I think we need a separate marker for might fail, and must fail.
> >>
> >> Currently XFail is a must-fail and breaks the buildbots if it doesn't fail
> >> (XPass error).
> >>
> >
> > I assert that there is no such thing as "might fail".  If you know exactly
> > what situations cause a test to fail, then test those conditions and claim
> > that the test must fail when those conditions are true (XFail).  If you
> > *don't* know what situations cause a test to fail, that's a bug and needs to
> > be flagged as such with an unexpected failure.
> >
> 
> I agree. You (Bert) have also not yet explained why you think it's good
> for tests to fail in a way that our buildbots don't notice -- is it just
> about not receiving a bunch of failed-test mails?

The big problem is that a failed mail with 5 errors doesn't stand out between hundreds of mails per day with 4 errors.

If there is at least one error http://www.mobsol.be/buildbot/waterfall and http://crest.ics.uci.edu/buildbot/waterfall are red. So the alarm effect is 0 if this status is kept for days. (We started failing Saturday).


If the build fails here at work, it must be fixed ASAP.. not half a week later.
(These patch tests started failing last Saturday; the moment the branch was merged to trunk)

>  We're not reinventing the wheel WRT test results here; we inherited the
> PASS/FAIL/XPASS/XFAIL from older systems that have a long history of
> using them.

This all assumes somebody investigates problems until they are a stable PASS or a stable FAIL, which is not the case here. 

I think Arfrever can use some help to get these tests in either an Pass or an XFail. Until that point is reached the test code doesn't belong on trunk. At least not in a way that it can fail the buildbots.

	Bert

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1303784


Re: Test XFail/Skip policy

Posted by Branko Cibej <br...@xbc.nu>.
C. Michael Pilato wrote:
> Bert Huijben wrote:
>   
>>> -----Original Message-----
>>> From: Branko Cibej [mailto:brane@xbc.nu]
>>> Sent: dinsdag 10 maart 2009 16:27
>>> To: Branko Cibej; dev@subversion.tigris.org
>>> Subject: Re: Test XFail/Skip policy
>>>
>>> Stefan Sperling wrote:
>>>       
>>>> On Tue, Mar 10, 2009 at 04:07:15PM +0100, Branko Cibej wrote:
>>>>
>>>>         
>>>>> The thing I'd find useful is adding an optional comment to XFail and
>>>>> Skip; so for this test, you could Xfail(foo, reason="yeah we know its
>>>>> broken, this is issue #bla, foo@ is working on it, don't panic")
>>>>>
>>>>>           
>>>> Yeah, that would do!
>>>>
>>>>         
>>> Guess what -- that's a bitesize (for me). :) And an opportunity to
>>> contribute some code, not just blab, after a long time. I'm on it.
>>>       
>> I think we need a separate marker for might fail, and must fail.
>>
>> Currently XFail is a must-fail and breaks the buildbots if it doesn't fail
>> (XPass error).
>>     
>
> I assert that there is no such thing as "might fail".  If you know exactly
> what situations cause a test to fail, then test those conditions and claim
> that the test must fail when those conditions are true (XFail).  If you
> *don't* know what situations cause a test to fail, that's a bug and needs to
> be flagged as such with an unexpected failure.
>   

I agree. You (Bert) have also not yet explained why you think it's good
for tests to fail in a way that our buildbots don't notice -- is it just
about not receiving a bunch of failed-test mails?

 We're not reinventing the wheel WRT test results here; we inherited the
PASS/FAIL/XPASS/XFAIL from older systems that have a long history of
using them.

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1303561

Re: Test XFail/Skip policy

Posted by "C. Michael Pilato" <cm...@collab.net>.
Bert Huijben wrote:
>> -----Original Message-----
>> From: Branko Cibej [mailto:brane@xbc.nu]
>> Sent: dinsdag 10 maart 2009 16:27
>> To: Branko Cibej; dev@subversion.tigris.org
>> Subject: Re: Test XFail/Skip policy
>>
>> Stefan Sperling wrote:
>>> On Tue, Mar 10, 2009 at 04:07:15PM +0100, Branko Cibej wrote:
>>>
>>>> The thing I'd find useful is adding an optional comment to XFail and
>>>> Skip; so for this test, you could Xfail(foo, reason="yeah we know its
>>>> broken, this is issue #bla, foo@ is working on it, don't panic")
>>>>
>>> Yeah, that would do!
>>>
>> Guess what -- that's a bitesize (for me). :) And an opportunity to
>> contribute some code, not just blab, after a long time. I'm on it.
> 
> I think we need a separate marker for might fail, and must fail.
> 
> Currently XFail is a must-fail and breaks the buildbots if it doesn't fail
> (XPass error).

I assert that there is no such thing as "might fail".  If you know exactly
what situations cause a test to fail, then test those conditions and claim
that the test must fail when those conditions are true (XFail).  If you
*don't* know what situations cause a test to fail, that's a bug and needs to
be flagged as such with an unexpected failure.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1303560

RE: Test XFail/Skip policy

Posted by Bert Huijben <rh...@sharpsvn.net>.
> -----Original Message-----
> From: Branko Cibej [mailto:brane@xbc.nu]
> Sent: dinsdag 10 maart 2009 16:27
> To: Branko Cibej; dev@subversion.tigris.org
> Subject: Re: Test XFail/Skip policy
> 
> Stefan Sperling wrote:
> > On Tue, Mar 10, 2009 at 04:07:15PM +0100, Branko Cibej wrote:
> >
> >> The thing I'd find useful is adding an optional comment to XFail and
> >> Skip; so for this test, you could Xfail(foo, reason="yeah we know its
> >> broken, this is issue #bla, foo@ is working on it, don't panic")
> >>
> >
> > Yeah, that would do!
> >
> 
> Guess what -- that's a bitesize (for me). :) And an opportunity to
> contribute some code, not just blab, after a long time. I'm on it.

I think we need a separate marker for might fail, and must fail.

Currently XFail is a must-fail and breaks the buildbots if it doesn't fail
(XPass error).

Skip just skips the test, so that is not a real solution to fixing a test.

	Bert

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1303534

Re: Test XFail/Skip policy

Posted by Branko Cibej <br...@xbc.nu>.
Stefan Sperling wrote:
> On Tue, Mar 10, 2009 at 04:07:15PM +0100, Branko Cibej wrote:
>   
>> The thing I'd find useful is adding an optional comment to XFail and
>> Skip; so for this test, you could Xfail(foo, reason="yeah we know its
>> broken, this is issue #bla, foo@ is working on it, don't panic")
>>     
>
> Yeah, that would do!
>   

Guess what -- that's a bitesize (for me). :) And an opportunity to
contribute some code, not just blab, after a long time. I'm on it.

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1303524

Re: Test XFail/Skip policy

Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 10, 2009 at 04:07:15PM +0100, Branko Cibej wrote:
> The thing I'd find useful is adding an optional comment to XFail and
> Skip; so for this test, you could Xfail(foo, reason="yeah we know its
> broken, this is issue #bla, foo@ is working on it, don't panic")

Yeah, that would do!

Stefan

Re: Test XFail/Skip policy

Posted by Branko Cibej <br...@xbc.nu>.
Stefan Sperling wrote:
> On Tue, Mar 10, 2009 at 10:06:29AM -0400, C. Michael Pilato wrote:
>   
>>> Isn't the whole point of the test suite to identify bugs? So, if you
>>> unconditionally Skip a test that fails on some platforms, it'll never
>>> get fixed because it'll never get run, and no-one ever reviews skipped
>>> tetss. Might as well remove the test then, what's the point?
>>>
>>> Or maybe fix the bug, who knows.
>>>
>>> -- Brane
>>>       
>> I could really use some clarification on this, myself.
>>
>> As Brane rightly says, the point of the test suite is to identify bugs.  Now
>> obviously, bugs come in various shapes and sizes, different complexities and
>> severities.  Some bugs we can live with for now; some we need to fix
>> immediately.
>>
>> Our test suite provides a way to flag our level of concern about various
>> bugs, too, I think.  I'm just not sure we're effectively taking advantage of
>> it.  The following is the set of rules I've carried in my head about our
>> test decorators:
>>
>>    Skip() should be reserved for test cases that don't make sense in a given
>>    scenario.  This is *not* a conditional XFail() -- it's like a "N/A" (not
>>    applicable) marker, and only that.  (I suppose it could also be used
>>    without condition for test cases which themselves aren't completely well-
>>    formed, but I'd be in favor of an alias BadTest() or somesuch to disam-
>>    biguate that case.)
>>
>>    XFail() is for tests that fail for known reasons when those known reasons
>>    are deemed to be of *shrug*-level severity.  You'd like the test to pass,
>>    but you wouldn't hold up a release for the bug that's causing it not to.
>>
>> Now, I don't always obey these nicely defined rules myself.  When adding a
>> new test, it's pretty common for me (and others) to write the test, XFail()
>> it (without considering the bug's severity), and commit.  Then later, we fix
>> the bug, remove the XFail(), and commit the fix.  As routine as this has
>> become, I think this undermines the effectiveness of our test suite.  I
>> *should* (in my opinion) be applying the rules I listed above when composing
>> that regression test, which means possibly adding a test *not* marked as
>> XFail() -- even when I know it's going to fail -- if the bug's severity
>> dictates as much.  What if, despite my best intentions to fix the bug
>> immediately, I get tied up elsewhere after composing the test but before
>> fixing the bug?  Do any of us routinely survey the XFail()ing tests to see
>> if any particularly horrid bugs are "expected"?  I know I don't.  A failing
>> test -- and the public outrage that accompanies it -- are infinitely louder
>> than an XFail()ing test.
>>
>> On the flip side, I'm also aware of the "Cry Wolf" effect that can happen if
>> the test suite is always failing.  It benefits no one if we grow calloused
>> to the notifications of our early alert system to the point of ignoring it.
>>
>> Am I off-base here?  Is there a better policy summary that someone could
>> whip up (and add to hacking.html)?
>>     
>
> I'd map categories like this:
>
> default  # expected to pass, alert me when it fails
> XFAIL    # known to fail, alert me if this suddenly passes again
> SKIP     # cannot run this test in this situation
>
> Now, for the scenario at stake, do we need another test category?
> A test that is known to fail, but should not be silenced like XFAIL
> or SKIP do? What about KNOWN?
>
> KNOWN # known failure, unconditionally alert me anytime this test is run,
>       # either the test is broken or there's a bug somewhere, somebody
>       # should look at this asap, but don't hold the buildbots on it,
>       # and if you suddenly see this in your local WC, you likely didn't
>       # break it
>
> While not perfect, this would convey more information than XFAIL/SKIP.
> Maybe there should also be an attribute or comment that indicates
> who is trying to fix the problem, or point to an issue number,
> so that others can enquire about the problem's status.
>
> Would something like this be useful?
>   

Not really. The definition of XFAIL is exactly "this test is expected to
fail (sometimes)". What's the point of having an XFAIL that doesn't get
flagged?

The "don't hold the buildbots on it" bit ... sorry, I can't see a
rational reason for this. The purpose of the buildbots is to test our
code. Causing buildbots to ignore known errors is ... counterproductive?

The thing I'd find useful is adding an optional comment to XFail and
Skip; so for this test, you could Xfail(foo, reason="yeah we know its
broken, this is issue #bla, foo@ is working on it, don't panic")

Or perhaps just an optional list of issue numbers -- after all, if you
want to have a failing test on trunk and aren't prepared to roll back
whatever change caused it to fail, you can at least take time to report
an issue.

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1303470

Re: Test XFail/Skip policy (was: svn commit: r36439 - trunk/subversion/tests/cmdline)

Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 10, 2009 at 10:06:29AM -0400, C. Michael Pilato wrote:
> > Isn't the whole point of the test suite to identify bugs? So, if you
> > unconditionally Skip a test that fails on some platforms, it'll never
> > get fixed because it'll never get run, and no-one ever reviews skipped
> > tetss. Might as well remove the test then, what's the point?
> > 
> > Or maybe fix the bug, who knows.
> > 
> > -- Brane
> 
> I could really use some clarification on this, myself.
> 
> As Brane rightly says, the point of the test suite is to identify bugs.  Now
> obviously, bugs come in various shapes and sizes, different complexities and
> severities.  Some bugs we can live with for now; some we need to fix
> immediately.
> 
> Our test suite provides a way to flag our level of concern about various
> bugs, too, I think.  I'm just not sure we're effectively taking advantage of
> it.  The following is the set of rules I've carried in my head about our
> test decorators:
> 
>    Skip() should be reserved for test cases that don't make sense in a given
>    scenario.  This is *not* a conditional XFail() -- it's like a "N/A" (not
>    applicable) marker, and only that.  (I suppose it could also be used
>    without condition for test cases which themselves aren't completely well-
>    formed, but I'd be in favor of an alias BadTest() or somesuch to disam-
>    biguate that case.)
> 
>    XFail() is for tests that fail for known reasons when those known reasons
>    are deemed to be of *shrug*-level severity.  You'd like the test to pass,
>    but you wouldn't hold up a release for the bug that's causing it not to.
> 
> Now, I don't always obey these nicely defined rules myself.  When adding a
> new test, it's pretty common for me (and others) to write the test, XFail()
> it (without considering the bug's severity), and commit.  Then later, we fix
> the bug, remove the XFail(), and commit the fix.  As routine as this has
> become, I think this undermines the effectiveness of our test suite.  I
> *should* (in my opinion) be applying the rules I listed above when composing
> that regression test, which means possibly adding a test *not* marked as
> XFail() -- even when I know it's going to fail -- if the bug's severity
> dictates as much.  What if, despite my best intentions to fix the bug
> immediately, I get tied up elsewhere after composing the test but before
> fixing the bug?  Do any of us routinely survey the XFail()ing tests to see
> if any particularly horrid bugs are "expected"?  I know I don't.  A failing
> test -- and the public outrage that accompanies it -- are infinitely louder
> than an XFail()ing test.
> 
> On the flip side, I'm also aware of the "Cry Wolf" effect that can happen if
> the test suite is always failing.  It benefits no one if we grow calloused
> to the notifications of our early alert system to the point of ignoring it.
> 
> Am I off-base here?  Is there a better policy summary that someone could
> whip up (and add to hacking.html)?

I'd map categories like this:

default  # expected to pass, alert me when it fails
XFAIL    # known to fail, alert me if this suddenly passes again
SKIP     # cannot run this test in this situation

Now, for the scenario at stake, do we need another test category?
A test that is known to fail, but should not be silenced like XFAIL
or SKIP do? What about KNOWN?

KNOWN # known failure, unconditionally alert me anytime this test is run,
      # either the test is broken or there's a bug somewhere, somebody
      # should look at this asap, but don't hold the buildbots on it,
      # and if you suddenly see this in your local WC, you likely didn't
      # break it

While not perfect, this would convey more information than XFAIL/SKIP.
Maybe there should also be an attribute or comment that indicates
who is trying to fix the problem, or point to an issue number,
so that others can enquire about the problem's status.

Would something like this be useful?

Stefan