[patch/rfc] KFAIL gdb.c++/annota2.exp watch triggered on a.x

[patch/rfc] KFAIL gdb.c++/annota2.exp watch triggered on a.x

[David Carlton]

Here's a patch to KFAIL the test "watch triggered on a.x" in
gdb.c++/annota2.exp, corresponding to PR breakpoints/38.  The test
fails consistently on all the configurations that Michael tests.

This is an obvious candidate for KFAILing.  My only question is when
it should currently be expected to pass.  The PR indicates that it
might pass on some platforms; on the other hand, it fails reliably
on i686-pc-linux-gnu.  So what I did was change the two existing
instances of

  { pass "watch triggered on a.x" }

to

  { setup_kfail "gdb/38" "i686*-*-*"
    pass "watch triggered on a.x" }

but I made the new KFAIL case unconditional.  That seemed to be the
safest thing: that way, any FAIL or KPASS message is interesting.

I'm also a little worried that there are two pass regexps listed
whereas I'm only listing one fail regexp; it seems plausible to me
that I should add a second fail regexp.  But I don't want to do that
without seeing such a pattern in the wild.

I'll commit this on Wednesday unless somebody disagrees with how I'm
handling when to {K,}{FAIL,PASS} or unless somebody turns up more
KFAIL regexps.

David Carlton
carlton@math.stanford.edu

2003-01-03  David Carlton  <carlton@math.stanford.edu>

	* gdb.c++/annota2.exp: KFAIL "watch triggered on a.x".
	Add copyright year 2003.

Index: annota2.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/annota2.exp,v
retrieving revision 1.8
diff -u -p -r1.8 annota2.exp
--- annota2.exp	20 Dec 2002 18:37:15 -0000	1.8
+++ annota2.exp	3 Jan 2003 21:18:08 -0000
@@ -1,4 +1,4 @@
-#   Copyright 1999, 2000, 2001, 2002
+#   Copyright 1999, 2000, 2001, 2002, 2003
 #   Free Software Foundation, Inc.
 
 # This program is free software; you can redistribute it and/or modify
@@ -194,9 +194,13 @@ gdb_expect {
 send_gdb "next\n"
 gdb_expect {
    -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032watchpoint 3\r\nWatchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nmain\r\n\032\032frame-args\r\n \\(\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*$srcfile\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n$decimal\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$" \
-	   { pass "watch triggered on a.x" }
+	   { setup_kfail "gdb/38" "i686*-*-*"
+             pass "watch triggered on a.x" }
    -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032watchpoint 3\r\n\.*atchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nmain\r\n\032\032frame-args\r\n \\(\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*$srcfile\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n$decimal\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n.*$gdb_prompt$" \
-	   { pass "watch triggered on a.x" }
+	   { setup_kfail "gdb/38" "i686*-*-*"
+             pass "watch triggered on a.x" }
+   -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$" \
+	   { kfail "gdb/38" "watch triggered on a.x" }
    -re ".*$gdb_prompt$"  { fail "watch triggered on a.x" }
    timeout    { fail "watch triggered on a.x (timeout)" }
 }

Re: [patch/rfc] KFAIL gdb.c++/annota2.exp watch triggered on a.x

[Daniel Jacobowitz]

On Fri, Jan 03, 2003 at 01:35:59PM -0800, David Carlton wrote:
> Here's a patch to KFAIL the test "watch triggered on a.x" in
> gdb.c++/annota2.exp, corresponding to PR breakpoints/38.  The test
> fails consistently on all the configurations that Michael tests.
> 
> This is an obvious candidate for KFAILing.  My only question is when
> it should currently be expected to pass.  The PR indicates that it
> might pass on some platforms; on the other hand, it fails reliably
> on i686-pc-linux-gnu.  So what I did was change the two existing
> instances of
> 
>   { pass "watch triggered on a.x" }
> 
> to
> 
>   { setup_kfail "gdb/38" "i686*-*-*"
>     pass "watch triggered on a.x" }
> 
> but I made the new KFAIL case unconditional.  That seemed to be the
> safest thing: that way, any FAIL or KPASS message is interesting.

May I recommend at the least "i?86"?  Also, I really don't see the
point of the kpass's; before doing this, you need to establish if those
patterns are acceptable results; if so, they are passes, period.


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] KFAIL gdb.c++/annota2.exp watch triggered on a.x

[David Carlton]

On Fri, 3 Jan 2003 16:39:20 -0500, Daniel Jacobowitz <drow@mvista.com> said:

> May I recommend at the least "i?86"?

That makes sense.

> Also, I really don't see the point of the kpass's; before doing
> this, you need to establish if those patterns are acceptable
> results; if so, they are passes, period.

Sorry, I should have explained my reasoning there.  My theory behind
that is that they're a reminder to people who fix bugs that they
should update the test suite.  If somebody fixes this bug a year from
now, doesn't know that there's a test case for the bug, and doesn't
pay attention to gdb.sum (just to the naked 'make check'), then that
person might easily forget to update the test suite.  (Especially
since the test case in question is in gdb.c++/annota2.exp, whereas the
bug doesn't involve either C++ or annotations!)

So it seems to me that, if the failure isn't reliable, then we should
leave the success case as a PASS, but if the failure is reliable, then
KPASS is slightly better.

David Carlton
carlton@math.stanford.edu

Re: [patch/rfc] KFAIL gdb.c++/annota2.exp watch triggered on a.x

[Daniel Jacobowitz]

On Fri, Jan 03, 2003 at 01:48:25PM -0800, David Carlton wrote:
> On Fri, 3 Jan 2003 16:39:20 -0500, Daniel Jacobowitz <drow@mvista.com> said:
> 
> > May I recommend at the least "i?86"?
> 
> That makes sense.
> 
> > Also, I really don't see the point of the kpass's; before doing
> > this, you need to establish if those patterns are acceptable
> > results; if so, they are passes, period.
> 
> Sorry, I should have explained my reasoning there.  My theory behind
> that is that they're a reminder to people who fix bugs that they
> should update the test suite.  If somebody fixes this bug a year from
> now, doesn't know that there's a test case for the bug, and doesn't
> pay attention to gdb.sum (just to the naked 'make check'), then that
> person might easily forget to update the test suite.  (Especially
> since the test case in question is in gdb.c++/annota2.exp, whereas the
> bug doesn't involve either C++ or annotations!)
> 
> So it seems to me that, if the failure isn't reliable, then we should
> leave the success case as a PASS, but if the failure is reliable, then
> KPASS is slightly better.

How do you envision them updating the testsuite?  Certainly not by
removing the KFAIL's pattern; that defeats the point of having a
regression test.  That's why I like Michael's approach of having a pass
pattern and a kfail pattern.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] KFAIL gdb.c++/annota2.exp watch triggered on a.x

[David Carlton]

On Fri, 3 Jan 2003 16:51:34 -0500, Daniel Jacobowitz <drow@mvista.com> said:

> How do you envision them updating the testsuite?  Certainly not by
> removing the KFAIL's pattern; that defeats the point of having a
> regression test.

That's actually exactly how I expect them to update the testsuite
(though they might want to keep the pattern around in a comment
somewhere, or even leave the pattern intact but replace the kfail by
fail plus a comment).  If a bug is claimed to be fixed but isn't
actually fixed, then that bug isn't a known failure any more, so it
should be FAILed until the PR is reopened.

Consider this scenario: we have a bug, with a test for it; that test
has a PASS pattern (either because people don't like KPASS or because
the bug is intermittent) and a KFAIL pattern.

Then programmer A, a habitual user of platform A, fixes the bug,
closes the PR, happily notices that the test has changed from KFAIL to
PASS, but leaves the KFAIL branch intact.

Except that it turns out that programmer A's fix was more specific to
platform A than he or she realized.  The bug is still present on
platform B; programmer B on platform B didn't notice that the bug was
allegedly fixed (maybe programmer B wasn't even working on GDB at the
time that the bug was fixed), but programmer B does notice that
there's a KFAILed test case corresponding to the bug.  So programmer B
assumes that the bug is, in fact, known, whereas from the point of
view of both programmer A and the GDB bug database, the bug is
supposed to have gone away.

On the other hand, if programmer A had deleted the KFAIL pattern, then
programmer B would see the test start FAILing, would say "hey, the
test suite says that GDB has an unknown bug here", and would then have
a reason to investigate the situation.

Removing the test entirely would the point of having a regression
test.  But removing patterns that handle casses that we don't expect
to occur should make the test more effective rather than less
effective: if we get surprising output from GDB, we want that to be
flagged as prominently as possible.

At least, that's my reasoning.

David Carlton
carlton@math.stanford.edu

Re: [patch/rfc] KFAIL gdb.c++/annota2.exp watch triggered on a.x

[Daniel Jacobowitz]

On Fri, Jan 03, 2003 at 02:14:25PM -0800, David Carlton wrote:
> On Fri, 3 Jan 2003 16:51:34 -0500, Daniel Jacobowitz <drow@mvista.com> said:
> 
> > How do you envision them updating the testsuite?  Certainly not by
> > removing the KFAIL's pattern; that defeats the point of having a
> > regression test.
> 
> That's actually exactly how I expect them to update the testsuite
> (though they might want to keep the pattern around in a comment
> somewhere, or even leave the pattern intact but replace the kfail by
> fail plus a comment).  If a bug is claimed to be fixed but isn't
> actually fixed, then that bug isn't a known failure any more, so it
> should be FAILed until the PR is reopened.
> 
> Consider this scenario: we have a bug, with a test for it; that test
> has a PASS pattern (either because people don't like KPASS or because
> the bug is intermittent) and a KFAIL pattern.

> Removing the test entirely would the point of having a regression
> test.  But removing patterns that handle casses that we don't expect
> to occur should make the test more effective rather than less
> effective: if we get surprising output from GDB, we want that to be
> flagged as prominently as possible.
> 
> At least, that's my reasoning.

OK; you're definitely right.  How about a compromise: we agree not to
remove kfail patterns in the testsuite, but instead replace them with
specific fail patterns and a commented out reference to the failure.
That makes life much simpler.

I still don't see what the point of the KPASS's is.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] KFAIL gdb.c++/annota2.exp watch triggered on a.x

[David Carlton]

On Fri, 3 Jan 2003 17:30:31 -0500, Daniel Jacobowitz <drow@mvista.com> said:

> How about a compromise: we agree not to remove kfail patterns in the
> testsuite, but instead replace them with specific fail patterns and
> a commented out reference to the failure.  That makes life much
> simpler.

That sounds to me like the best of both worlds: we get the output that
I want, while making it as easy as possible to stick the KFAILs back
in the test suite if the same bug crops up again.

> I still don't see what the point of the KPASS's is.

Honestly, there isn't much of a point to them.  They serve as a
reminder to get rid of KFAILs once you've fixed a bug (because KPASSes
will show up in the output of 'runtest' or 'make check'), and they
might be useful if you're interested in tracking down exactly under
what conditions a bug manifests itself.  But I don't feel strongly
about either of those issues; if nobody likes KPASS but me, then I
have no objection to using plain old PASS instead.

David Carlton
carlton@math.stanford.edu

Re: [patch/rfc] KFAIL gdb.c++/annota2.exp watch triggered on a.x

[David Carlton]

On 03 Jan 2003 13:35:59 -0800, David Carlton <carlton@math.Stanford.EDU> said:

> Here's a patch to KFAIL the test "watch triggered on a.x" in
> gdb.c++/annota2.exp, corresponding to PR breakpoints/38.  The test
> fails consistently on all the configurations that Michael tests.

...

> I'll commit this on Wednesday unless somebody disagrees with how I'm
> handling when to {K,}{FAIL,PASS} or unless somebody turns up more
> KFAIL regexps.

My use of KPASS didn't meet with the most appreciative of responses,
so I've committed the following: it adds the KFAIL regexp but doesn't
change the existing PASSes to KPASSes.

David Carlton
carlton@math.stanford.edu

2003-01-03  David Carlton  <carlton@math.stanford.edu>

	* gdb.c++/annota2.exp: KFAIL "watch triggered on a.x".
	Add copyright year 2003.

Index: annota2.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/annota2.exp,v
retrieving revision 1.8
diff -u -p -r1.8 annota2.exp
--- annota2.exp	20 Dec 2002 18:37:15 -0000	1.8
+++ annota2.exp	9 Jan 2003 17:04:19 -0000
@@ -1,4 +1,4 @@
-#   Copyright 1999, 2000, 2001, 2002
+#   Copyright 1999, 2000, 2001, 2002, 2003
 #   Free Software Foundation, Inc.
 
 # This program is free software; you can redistribute it and/or modify
@@ -197,6 +197,8 @@ gdb_expect {
 	   { pass "watch triggered on a.x" }
    -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032watchpoint 3\r\n\.*atchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nmain\r\n\032\032frame-args\r\n \\(\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*$srcfile\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n$decimal\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n.*$gdb_prompt$" \
 	   { pass "watch triggered on a.x" }
+   -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032frames-invalid\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$" \
+	   { kfail "gdb/38" "watch triggered on a.x" }
    -re ".*$gdb_prompt$"  { fail "watch triggered on a.x" }
    timeout    { fail "watch triggered on a.x (timeout)" }
 }

Re: [patch/rfc] KFAIL gdb.c++/annota2.exp watch triggered on a.x

[Michael Elizabeth Chastain]

One advantage of David's code is that when the bug gets fixed,
people see a more informative message for a while: "KPASS gdb/38"
rather than "PASS".

One disadvantage is that it adds more file editing steps to the
lifecycle.  David, what edits, exactly, are you envisioning
when "KPASS gdb/38" starts popping up?

You shouldn't delete this part:

  -re "known bad expression" {
    kfail "gdb/NNN" "test name"
  }

... because old bugs have a way of popping up again, especially
when additional configurations get tested.

Michael C

Re: [patch/rfc] KFAIL gdb.c++/annota2.exp watch triggered on a.x

[David Carlton]

On Fri, 3 Jan 2003 16:12:26 -0600, Michael Elizabeth Chastain <mec@shout.net> said:

> One advantage of David's code is that when the bug gets fixed,
> people see a more informative message for a while: "KPASS gdb/38"
> rather than "PASS".

Actually, I don't consider that an advantage at all: any KPASS seems
to me to demand an investigation, and should be eradicated.  So, when
somebody fixes this bug, I think that somebody should change the
KPASSes to plain old normal PASSes.  (As well as remove or comment out
the KFAIL branches, as I said in my response to Daniel that crossed
your e-mail.)

Basically, I see three different priorites of output messages (I'm
ignoring ERROR/UNSUPPORTED/etc. for now):

1) PASS: everything is peachy keen.

2) KFAIL/XFAIL: something doesn't work right, but at least we know
   that it doesn't work right.

3) FAIL/KPASS/XPASS: either something doesn't work right that we
   weren't aware of, or something does work right when we think it
   shouldn't.

And, in the mythical utopia where the GDB testsuite has been carefully
audited to have KFAILs added where appropriate, messages in category 3
are grounds for immediate investigation, whereas messages in
categories 1 and 2 aren't grounds for immediate investigation.

So, when bugs have been fixed, I don't want to see KPASS (because that
gives the misleading impression that we think it hasn't been fixed) or
KFAIL (because that gives the misleading impression that we're aware
that the bug hasn't, in fact, been fixed after all).

David Carlton
carlton@math.stanford.edu

Re: [patch/rfc] KFAIL gdb.c++/annota2.exp watch triggered on a.x

[Michael Elizabeth Chastain]

Daniel J says:
> OK; you're definitely right.  How about a compromise: we agree not to
> remove kfail patterns in the testsuite, but instead replace them with
> specific fail patterns and a commented out reference to the failure.
> That makes life much simpler.

That sounds good to me.

Before the bug fix:

  -re "known bad pattern" {
    kfail "gdb/38" "test name"
  }

After the bug fix:

  -re "known bad pattern" {
    # this was pr gdb/38
    fail "test name"
  }

If somebody leaves a KFAIL pointing to a closed bug, and the KFAIL
fires, then eventually somebody will notice that the KFAIL points to
a closed bug -- which means that the bug has re-appeared.  The bug ID#
then provides valuable history.

If somebody applies the 'bug fix' too soon, or it doesn't work in
all cases, then the person investigating it will see the comment
about "gdb 38" in the test script source pretty quickly.

Michael C