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

This may be an unpopular opinion (and I don’t have the full context on those
specific issues), but I believe that these are an abuse of XFAIL, and should
probably be written in terms of REQUIRES instead of XFAIL.

I believe XFAIL tests actually execute, and are just marked as expected failure.
If a test is not expected to ever succeed, we shouldn’t bother running it, which
is what the REQUIRES directives are for.

-Chris
> On Sep 28, 2016, at 10:12 AM, Krzysztof Parzyszek via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
> 
> On 9/28/2016 11:39 AM, Chris Bieneman via llvm-dev wrote:
>> I believe that any test that is marked XFAIL is a bug
> 
> I don't know if that's true.
> 
> test/CodeGen/Generic/MachineBranchProb.ll:
> 
> ; ARM & AArch64 run an extra SimplifyCFG which disrupts this test.
> ; XFAIL: arm,aarch64
> 
> ; Hexagon runs passes that renumber the basic blocks, causing this test
> ; to fail.
> ; XFAIL: hexagon
> 
> -Krzysztof
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
by The Linux Foundation
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On 28 September 2016 at 18:21, Chris Bieneman via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> This may be an unpopular opinion (and I don’t have the full context on
those specific issues), but I believe that these are an abuse of XFAIL, and
should probably be written in terms of REQUIRES instead of XFAIL.
Agreed.

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. :)

This could be one of the commit hooks that were proposed a while ago,
to demand PR numbers for commits that have XFAIL in their change logs
(only if the line is changed). But I'm always sceptical as to the
value of such hooks...

cheers,
--renato

On 9/28/2016 12:21 PM, Chris Bieneman wrote:
> This may be an unpopular opinion (and I don’t have the full context on
those specific issues), but I believe that these are an abuse of XFAIL, and
should probably be written in terms of REQUIRES instead of XFAIL.
>
> I believe XFAIL tests actually execute, and are just marked as expected
failure. If a test is not expected to ever succeed, we shouldn’t bother running
it, which is what the REQUIRES directives are for.
And the directive would require what specifically?

If anything, UNSUPPORTED may be better for this.

-Krzysztof

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

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