llvm dev - Sep 2016 - [RFC] Require PRs for XFAILing tests

On 28 September 2016 at 10:08, Chris Bieneman via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> I cannot think of any situation where a universally failing test
> should be in-tree unless it is a bug that someone is expecting to fix.
It seems moderately common to mark something XFAIL temporarily to get
the bots green while then going ahead to fix the issue.  Your proposal
would add extra overhead to that flow by requiring a PR as well.  This
has value when it turns out that fix can't happen in the short term for 
any reason.  I don't have a feel for how common that is, although I'm
sure it does happen.
I think the overhead is worth the added value, but then I'm a process 
kind of guy.


On 28 September 2016 at 10:28, Renato Golin via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> We already have an unwritten rule to create PRs for XFAILs, and we
> normally don't XFAIL lightly (I don't, at least). But creating one
PR
> for every existing XFAIL may end up as a long list of never looked
> PRs. :)
As opposed to the other ~9000 open PRs?  At least they would be tracked.
--paulr

I think there are a few interesting things that could follow from solidifying a
policy requiring PRs for XFAILs.

First and foremost bugs can have way more context than than you would often find
in a test case comment. That would make it a lot easier to audit XFAILs in the
future and help keep the number of XFAILs to a minimum. I think this is
important because many of our XFAILs are really old, and I’m not convinced that
we shouldn’t just be deleting some of these tests.

For example, 2008-12-14-StrideAndSigned.ll and 7 other tests were marked with
“XFAIL: *” in 2009, and the commit message doesn’t really explain what was going
on:
> commit 789558db70d9513a017c11c5be30945839fdff1c
> Author: Nick Lewycky <nicholas at mxc.ca>
> Date:   Tue Jan 13 09:18:58 2009 +0000
> 
>     Wind SCEV back in time, to Nov 18th. This 'fixes' PR3275,
PR3294, PR3295,
>     PR3296 and PR3302.
>     
>     
>     git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk at 62160
91177308-0d34-0410-b5e6-96231b3b80d8

Requiring a PR doesn’t necessarily fix this problem because you could list the
wrong PR, or even just a generic stub PR that didn’t add meaningful value, but I
think our community is pretty good at keeping people honest via code reviews.

I also think that eventually we could extend LIT to optionally query the bug
database to perform automated audits to keep track of referenced bugs and
produce reports about the referenced bug reports.

I also think the process of instituting this policy would require an audit of
existing XFAILs which will allow us to evaluate the value provided by our
current XFAILing tests and we can consider solutions to handle some of these
cases better. That could be removing tests that are universally XFAILing with no
expected change coming, or migrating tests to REQUIRES or UNSUPPORTED instead of
XFAIL.

-Chris
> On Sep 28, 2016, at 11:58 AM, Robinson, Paul <paul.robinson at
sony.com> wrote:
> 
> On 28 September 2016 at 10:08, Chris Bieneman via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>> I cannot think of any situation where a universally failing test
>> should be in-tree unless it is a bug that someone is expecting to fix.
> 
> It seems moderately common to mark something XFAIL temporarily to get
> the bots green while then going ahead to fix the issue.  Your proposal
> would add extra overhead to that flow by requiring a PR as well.  This
> has value when it turns out that fix can't happen in the short term for
> any reason.  I don't have a feel for how common that is, although
I'm
> sure it does happen.
> I think the overhead is worth the added value, but then I'm a process 
> kind of guy.
> 
> 
> On 28 September 2016 at 10:28, Renato Golin via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>> We already have an unwritten rule to create PRs for XFAILs, and we
>> normally don't XFAIL lightly (I don't, at least). But creating
one PR
>> for every existing XFAIL may end up as a long list of never looked
>> PRs. :)
> 
> As opposed to the other ~9000 open PRs?  At least they would be tracked.
> --paulr
>

> On Sep 28, 2016, at 1:01 PM, Chris Bieneman via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> I think there are a few interesting things that could follow from
solidifying a policy requiring PRs for XFAILs.
> 
> First and foremost bugs can have way more context than than you would often
find in a test case comment. That would make it a lot easier to audit XFAILs in
the future and help keep the number of XFAILs to a minimum. I think this is
important because many of our XFAILs are really old, and I’m not convinced that
we shouldn’t just be deleting some of these tests.
> 
> For example, 2008-12-14-StrideAndSigned.ll and 7 other tests were marked
with “XFAIL: *” in 2009, and the commit message doesn’t really explain what was
going on:
> 
>> commit 789558db70d9513a017c11c5be30945839fdff1c
>> Author: Nick Lewycky <nicholas at mxc.ca>
>> Date:   Tue Jan 13 09:18:58 2009 +0000
>> 
>>    Wind SCEV back in time, to Nov 18th. This 'fixes' PR3275,
PR3294, PR3295,
>>    PR3296 and PR3302.
>> 
>> 
>>    git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk at 62160
91177308-0d34-0410-b5e6-96231b3b80d8
> 
> 
> Requiring a PR doesn’t necessarily fix this problem because you could list
the wrong PR, or even just a generic stub PR that didn’t add meaningful value,
but I think our community is pretty good at keeping people honest via code
reviews.
Other important aspects:
- We can immediately start a specific and meaningful conversation about the
problem
- Progress/blockers in solving the problem can be documented if necessary
- It is actually easy to find the corresponding discussion to a problem (the
alternative of looking up the commit adding the XFAIL and then searching the
commits mailing list for threads on that commit is more trouble)

- Matthias

On Wed, Sep 28, 2016 at 11:58 AM Robinson, Paul via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On 28 September 2016 at 10:08, Chris Bieneman via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > I cannot think of any situation where a universally failing test
> > should be in-tree unless it is a bug that someone is expecting to fix.
>
> It seems moderately common to mark something XFAIL temporarily to get
> the bots green while then going ahead to fix the issue.  Your proposal
> would add extra overhead to that flow by requiring a PR as well.  This
> has value when it turns out that fix can't happen in the short term for
> any reason.  I don't have a feel for how common that is, although
I'm
> sure it does happen.
> I think the overhead is worth the added value, but then I'm a process
> kind of guy.
>
>
> On 28 September 2016 at 10:28, Renato Golin via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > We already have an unwritten rule to create PRs for XFAILs, and we
> > normally don't XFAIL lightly (I don't, at least). But creating
one PR
> > for every existing XFAIL may end up as a long list of never looked
> > PRs. :)
>
> As opposed to the other ~9000 open PRs?  At least they would be tracked.
>
I'd be inclined to agree (or at least voice the same concern) as Renato
here - as a project we don't really have very good bug hygiene, so adding
more bug filing doesn't seem to me like it'd give us much value.

Auditing existing XFAILs can be done today without filing bugs for them.

And we still always have the option to (& in many cases do) file bugs for
XFAILs to discuss them, etc.

But I don't feel strongly about it either way, so happy to leave the folks
who do to make the call/do the work.

- Dave

> --paulr
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160929/a74298f3/attachment.html>

> On Sep 29, 2016, at 7:52 AM, David Blaikie <dblaikie at gmail.com>
wrote:
> 
> 
> 
> On Wed, Sep 28, 2016 at 11:58 AM Robinson, Paul via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> On 28 September 2016 at 10:08, Chris Bieneman via llvm-dev
> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
> > I cannot think of any situation where a universally failing test
> > should be in-tree unless it is a bug that someone is expecting to fix.
> 
> It seems moderately common to mark something XFAIL temporarily to get
> the bots green while then going ahead to fix the issue.  Your proposal
> would add extra overhead to that flow by requiring a PR as well.  This
> has value when it turns out that fix can't happen in the short term for
> any reason.  I don't have a feel for how common that is, although
I'm
> sure it does happen.
> I think the overhead is worth the added value, but then I'm a process
> kind of guy.
> 
> 
> On 28 September 2016 at 10:28, Renato Golin via llvm-dev
> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
> > We already have an unwritten rule to create PRs for XFAILs, and we
> > normally don't XFAIL lightly (I don't, at least). But creating
one PR
> > for every existing XFAIL may end up as a long list of never looked
> > PRs. :)
> 
> As opposed to the other ~9000 open PRs?  At least they would be tracked.
> 
> I'd be inclined to agree (or at least voice the same concern) as Renato
here - as a project we don't really have very good bug hygiene, so adding
more bug filing doesn't seem to me like it'd give us much value.
I haven’t done a full audit, but we have 257 XFAILs in LLVM.

* 44 of those are vg_leak failures on TableGen tests which should be UNSUPPORTED
because we allow TableGen to leak for performance the same way we allow clang to
leak.
* 15 of them are vg_leak in the OCaml bindings. Someone familiar with OCaml
should chime in on it, but I suspect those too should be UNSUPPORTED
* 125 of them are universal failure (XFAIL: *). Many of these have been marked
this way for years. I suspect that if we take the time to go through these we
will likely find that many of these test cases either should be tracked by bugs,
or should be removed from the tree

From there, many of the test cases are XFAIL on features where they really
should be UNSUPPORTED. I suspect that if we do a full audit of the XFAILs we
would likely find <100 which should actually be XFAIL, and tracking those
seems valuable to me.
> 
> Auditing existing XFAILs can be done today without filing bugs for them.
Yes, we can audit them today. Making bugs required for them makes it easier to
audit them because there will (in theory) be a bug describing the justification
for the XFAIL and what the underlying issue is. Digging up the reason why an
XFAIL was added in 2009 is a little bit challenging today if there isn’t a PR
associated with it (or a really good comment or commit message).

-Chris
> 
> And we still always have the option to (& in many cases do) file bugs
for XFAILs to discuss them, etc.
> 
> But I don't feel strongly about it either way, so happy to leave the
folks who do to make the call/do the work.
> 
> - Dave
>  
> --paulr
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160929/653ac085/attachment.html>

On 28 September 2016 at 19:58, Robinson, Paul via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> On 28 September 2016 at 10:08, Chris Bieneman via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>> I cannot think of any situation where a universally failing test
>> should be in-tree unless it is a bug that someone is expecting to fix.
>
> It seems moderately common to mark something XFAIL temporarily to get
> the bots green while then going ahead to fix the issue.  Your proposal
> would add extra overhead to that flow by requiring a PR as well.  This
> has value when it turns out that fix can't happen in the short term for
> any reason.  I don't have a feel for how common that is, although
I'm
> sure it does happen.
> I think the overhead is worth the added value, but then I'm a process
> kind of guy.
I'm not saying I _like_ this solution, but if that were an issue we
could always have an open issue e.g. "PRNNNN: Some tests are marked
XFAIL but only have this generic PR listed as the reason", for use in
these "quick fix" cases. It would also be easy to track if these
"quick fixes" didn't happen shortly.

Alex

> -----Original Message-----
> From: Alex Bradbury [mailto:asb at asbradbury.org]
> Sent: Saturday, October 01, 2016 1:06 PM
> To: Robinson, Paul
> Cc: Renato Golin; Chris Bieneman; llvm-dev at lists.llvm.org
> Subject: Re: [llvm-dev] [RFC] Require PRs for XFAILing tests
> 
> On 28 September 2016 at 19:58, Robinson, Paul via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > On 28 September 2016 at 10:08, Chris Bieneman via llvm-dev
> > <llvm-dev at lists.llvm.org> wrote:
> >> I cannot think of any situation where a universally failing test
> >> should be in-tree unless it is a bug that someone is expecting to
fix.
> >
> > It seems moderately common to mark something XFAIL temporarily to get
> > the bots green while then going ahead to fix the issue.  Your proposal
> > would add extra overhead to that flow by requiring a PR as well.  This
> > has value when it turns out that fix can't happen in the short
term for
> > any reason.  I don't have a feel for how common that is, although
I'm
> > sure it does happen.
> > I think the overhead is worth the added value, but then I'm a
process
> > kind of guy.
> 
> I'm not saying I _like_ this solution, but if that were an issue we
> could always have an open issue e.g. "PRNNNN: Some tests are marked
> XFAIL but only have this generic PR listed as the reason", for use in
> these "quick fix" cases. It would also be easy to track if these
> "quick fixes" didn't happen shortly.
As David Blaikie mentioned, our bug hygiene is not really that good.
It would be easy to find the set of tests citing the generic PR, but
somebody would have to take it upon themselves to go looking for them.
By the time that happened, the kinds of details we'd want to see in a
bug would be just as missing as if we had no XFAIL-to-PR link at all.

Conversely, requiring short-term XFAILs to have their own PR means
that if somebody fixed the test and forgot to close the PR, that
dangling PR would be easy to recognize as something that could be
summarily closed if anybody decided to go look at all the XFAIL-linked
PRs.  This scenario leaves an open PR kicking around, O the horror,
but we have not lost any useful information.

Now, I think it would be a great and useful thing for somebody to take
on the role of PR Czar, to do that kind of sanitization of the bugs,
but I don't see it happening as an ongoing role.  Therefore I prefer
a process that is a bit more tedious but doesn't lose information
over a simpler but lossy process.
--paulr
> 
> Alex