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

[Chris Bieneman via llvm-dev]

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@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
>