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

Hello LLVM-Dev,

The other day as I was digging through lldb’s test suite I noticed they support
something kinda neat. In their python test harness, the attribute they use to
denote expected failures supports a parameter for specifying the bug number.
This got me thinking.

I believe that any test that is marked XFAIL is a bug, and we can use LIT to
enforce that. So I wrote a patch (https://reviews.llvm.org/D25035
<https://reviews.llvm.org/D25035>) to add a feature to LIT which would
support mapping XFAILs to PRs, and a flag to turn XFAILS without PRs into
failures.

My proposal is to add this feature to LIT (after proper code review, and I still
need to write tests for it), and then to annotate all our XFAILS with PRs. Once
all the PRs are annotated I think we should enable this behavior by default and
require PRs tracking all XFAILs.

Thoughts?
-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160928/f8848b9c/attachment.html>

> On Sep 28, 2016, at 9:39 AM, Chris Bieneman via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hello LLVM-Dev,
> 
> The other day as I was digging through lldb’s test suite I noticed they
support something kinda neat. In their python test harness, the attribute they
use to denote expected failures supports a parameter for specifying the bug
number. This got me thinking.
> 
> I believe that any test that is marked XFAIL is a bug, and we can use LIT
to enforce that. So I wrote a patch (https://reviews.llvm.org/D25035
<https://reviews.llvm.org/D25035>) to add a feature to LIT which would
support mapping XFAILs to PRs, and a flag to turn XFAILS without PRs into
failures.
> 
> My proposal is to add this feature to LIT (after proper code review, and I
still need to write tests for it), and then to annotate all our XFAILS with PRs.
Once all the PRs are annotated I think we should enable this behavior by default
and require PRs tracking all XFAILs.
> 
> Thoughts?
I think that’s a great idea!

I wonder if there won’t be annoying cases that won’t fit well though, and a fake
PR may be inserted just to please the system.
Have you survey the existing XFAIL test to see if it would be the case? (I can’t
imagine why but…)

Thanks,

— 
Mehdi

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160928/14464b5e/attachment.html>

I think one of the good things that would come out of this policy change is that
we would have to audit the existing XFAILs. The most common XFAIL directive from
my grep is "XFAIL: *”, which is the universal failure. 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.

The only situation that I’ve come up with where filing a PR for an XFAIL isn’t
reasonable would be for out-of-tree tests, although it is likely I’m not
thinking of some edge case. To support out-of-tree code I think that we could
adapt my patch to support multiple prefixes that denote different bug trackers.
That would allow downstream users to annotate their tests for their bug
trackers.

-Chris
> On Sep 28, 2016, at 9:48 AM, Mehdi Amini <mehdi.amini at apple.com>
wrote:
> 
>> 
>> On Sep 28, 2016, at 9:39 AM, Chris Bieneman via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>> 
>> Hello LLVM-Dev,
>> 
>> The other day as I was digging through lldb’s test suite I noticed they
support something kinda neat. In their python test harness, the attribute they
use to denote expected failures supports a parameter for specifying the bug
number. This got me thinking.
>> 
>> I believe that any test that is marked XFAIL is a bug, and we can use
LIT to enforce that. So I wrote a patch (https://reviews.llvm.org/D25035
<https://reviews.llvm.org/D25035>) to add a feature to LIT which would
support mapping XFAILs to PRs, and a flag to turn XFAILS without PRs into
failures.
>> 
>> My proposal is to add this feature to LIT (after proper code review,
and I still need to write tests for it), and then to annotate all our XFAILS
with PRs. Once all the PRs are annotated I think we should enable this behavior
by default and require PRs tracking all XFAILs.
>> 
>> Thoughts?
> 
> I think that’s a great idea!
> 
> I wonder if there won’t be annoying cases that won’t fit well though, and a
fake PR may be inserted just to please the system.
> Have you survey the existing XFAIL test to see if it would be the case? (I
can’t imagine why but…)
> 
> Thanks,
> 
> — 
> Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160928/061672a2/attachment.html>

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

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 Sep 2016, at 17:39, Chris Bieneman via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> My proposal is to add this feature to LIT (after proper code review, and I
still need to write tests for it), and then to annotate all our XFAILS with PRs.
Once all the PRs are annotated I think we should enable this behavior by default
and require PRs tracking all XFAILs.
This looks very useful for LLVM itself, but please consider downstream consumers
with this feature.  We have a few XFAIL’d tests in our downstream fork and run
llvm-lit in our CI setup.  We obviously can’t open an llvm.org bug for each one,
because they don’t relate to bugs in upstream LLVM.  Please ensure that we have
a mechanism for disabling this (or a different spelling of XFAIL for local
XFAILs).

David

> On Sep 29, 2016, at 2:52 AM, David Chisnall <David.Chisnall at
cl.cam.ac.uk> wrote:
> 
> On 28 Sep 2016, at 17:39, Chris Bieneman via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>> 
>> My proposal is to add this feature to LIT (after proper code review,
and I still need to write tests for it), and then to annotate all our XFAILS
with PRs. Once all the PRs are annotated I think we should enable this behavior
by default and require PRs tracking all XFAILs.
> 
> This looks very useful for LLVM itself, but please consider downstream
consumers with this feature.  We have a few XFAIL’d tests in our downstream fork
and run llvm-lit in our CI setup.  We obviously can’t open an llvm.org bug for
each one, because they don’t relate to bugs in upstream LLVM.  Please ensure
that we have a mechanism for disabling this (or a different spelling of XFAIL
for local XFAILs).
Absolutely. I expect to have two mechanisms for downstream users.

(1) The ability to specify your own bug tracker prefixes so that you can mark
out-of-tree XFAILs with your own bugs if you’d like
(2) The ability to disable forcing failure. Additionally Matthias Braun
suggested that I should add the ability to disable or add bug tracker URLs
per-test suite, which I’ll be doing as well.

-Chris
> 
> David
> 
>