add allow_fail test attribute

[pwoolcoc]

This change allows the user to add an #[allow_fail] attribute to
tests that will cause the test to compile & run, but if the test fails
it will not cause the entire test run to fail. The test output will
show the failure, but in yellow instead of red, and also indicate that
it was an allowed failure.

Here is an example of the output: http://imgur.com/a/wt7ga

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Thanks for the PR! We’ll periodically check in on it to make sure that @GuillaumeGomez or someone else from the team reviews it soon.

[Mark-Simulacrum]

Can you give some summary as to what the use cases for this would be? I can't think of anything where a failure would be okay (but not should_fail) right now.

[GuillaumeGomez]

I don't see the point of this add actually... :-/

[pwoolcoc]

This is basically for the same reasons you would set allow_failures in travis-ci. Maybe you want to commit a test for a feature or bug you haven't finished yet. You could set this instead of ignore so that the test always gets compiled, but won't fail the whole run if it fails at runtime. Or maybe it is an integration test that hits an outside API and you don't want the entire run to fail if there is an intermittent network issue that could cause the test to fail. The test is still shown in the output as failed, but it doesn't stop the whole run and the user doesn't have to do any catch_unwind tricks. cc @Mark-Simulacrum

[Mark-Simulacrum]

Yeah, that helps understand the reasoning. I think I'm neutral on it then -- I don't think we really need it but would be fine having it.

[GuillaumeGomez]

I understand the reason but then: why not just use no_run? It'll compile and won't run, as you seem to need.

[pwoolcoc]

Well, no_run is just a rustdoc thing, it doesn't work for regular tests. And it is still helpful to know when a test succeeds, even if it's failure should be ignored.

[GuillaumeGomez]

Your change is included in rustdoc as well, but in there, it's pretty much useless. Then, for consistency, why not just add no_run into libsyntax instead of allow_fail?

[pwoolcoc]

I find allow_fail to be more useful because it has the same effect on the test suite as no_run, but still gives you information about the test result.

[GuillaumeGomez]

That's just completely the opposite of the purpose of a test. I really don't agree with you on this functionality. However, maybe some other people might be interested so let's request opinions.

cc @rust-lang/compiler
cc @rust-lang/core

[aturon]

@pwoolcoc I've wanted this functionality from time to time, and wouldn't be opposed to seeing it land. @alexcrichton, @brson, @nrc, I imagine you might have opinions also? I'm not really sure whose jurisdiction this falls under, tbh.

[alexcrichton]

This definitely falls under the category of "I wish we had custom test frameworks" to allow this level of customization, but I don't personally have many thoughts here. I think that the most appropriate owner "team-wise" nowadays is the dev-tools team, but in general libtest needs a lot of love not just in ad-hoc additions but also in overall direction.

I would not personally be opposed to landing this, although it would perhaps be nice to do it with a feature gate first.

[Mark-Simulacrum]

I think a feature gate is a good idea here, too.

[pnkfelix]

Am I right in inferring from the conversation that the reason that this #[allow_fail] is different from #[should_panic] is that an #[allow_fail] test will always pass regardless of whether its execution invokes panic or not, while #[should_panic] will not pass if its execution runs without invoking panic?

[Mark-Simulacrum]

Yes, that seems correct.

[pwoolcoc]

@pnkfelix you are mostly correct. a test marked allow_fail will still be reported as a failure in the test output, but it will not cause the test run as a whole to fail.

[nikomatsakis]

There is also, of course, the #[ignore] flag which could be used for this; one problem I've found with that is that it is very easy to forget to "un-ignore" the tests. I imagine a similar thing could happen here, where you don't realize that the test is only passing because it is marked as #[allow_fail].

Usually in this scenario what I do is to write the test so that it passes with the current behavior (e.g., by adding #[should_panic], or by asserting that the "expected value" is in fact the current result, even though it's not what we eventually want) and leave some comments. This way, when I actually fix the code, I can't forget to update the test, since it starts to fail until I correct the behavior.

I definitely think a feature-gate would be wise here, not sure how technically hard that would be to implement. I also think we might consider this as a "flag" to the existing #[ignore] attribute (e.g., #[ignore(execute=true)]) -- it feels conceptually similar, at least.

[steveklabnik]

one problem I've found with that is that it is very easy to forget to "un-ignore" the tests.

Some frameworks have a "pending" flag instead; what this does is, run the test, and on a failure, do nothing. On a success, it fails your build and says "hey you thought this was not a real test but it works"

[Mark-Simulacrum]

That seems very much like a "should-fail" flag; so in that respect we have a pending flag already.

[nagisa]

To me this sounds like a tag for spurious/wont-fix-just-yet failures. This is not comparable to #[should_fail] at all. #[ignore] seems like an alternative, but it really is not – it does not detect when the test begins passing again :)

My primary concern with this feature is that once you add this tag, it is very easy to forget to remove it later (similar applies to #[ignore]), but since this runs the test, it has an option to loudly remind you about unignoring this some how; not by eventually failing the test suite, though, as that would bring down the whole build altogether for no real reason.

Custom testing framework would be great.

[nikomatsakis]

This is not comparable to #[should_fail] at all

I agree that the primary purpose of #[should_panic] is to write negative tests. But I think it can easily be used for the purpose of writing "wont-fix-just-yet" tests. In particular, the idea is to write a test that tests for the current behavior (even if is not what you want in the long term), along with a comment explaining what the right behavior would be -- if the current behavior is to panic, you might then use #[should_panic]. This has the added bonus that you find out if (a) the test starts passing or (b) if the test starts failing in some new, different way.

[nrc]

This seems a reasonable thing to add to me, but should definitely be behind a feature gate. I also agree that libtest needs some direction (and some RFC discussion), rather than just ad hoc extensions, but this seems like something that would be useful to experiment with first.

[pwoolcoc]

Ok, I think this is in a good state.

r? @GuillaumeGomez

[GuillaumeGomez]

I just thought about something: should we add a warning saying that the #[allow_fail] test attribute should only be used on WIP code? That'd allow to make a difference between it and the other test attributes.

[pwoolcoc]

I'm not sure about that, only because not all the use cases for this are for WIP code. For example, I also use it for some integration tests that contact an external service that is not 100% available.

[GuillaumeGomez]

Can you at least display the result of the fail. For example:

some_test ... Success (result: Failure)
some_other_test ... Success (result: Success)

Or something along the line. Like that, even if it succeeds; you can get the underlying result.

[pwoolcoc]

It does show the test as "test allowed_to_fail ... FAILED (allowed)", but I'm certainly not married to that format: http://imgur.com/a/wt7ga

[GuillaumeGomez]

Oh my bad, for me it's good enough. As long as we have a notification, it's good for me. Then it's all good, thanks a lot @pwoolcoc!

@bors: r+

[bors]

📌 Commit 4154f89 has been approved by GuillaumeGomez

[bors]

⌛ Testing commit 4154f89 with merge 1cd9a7050028a955b47c27d4d2789ba9e0c9c9d1...

[arielb1]

@bors retry - prioritizing rollup

[jonhoo]

Does this have a issue tracking stabilization? /cc @GuillaumeGomez@arielb1

[GuillaumeGomez]

No. It should though. I'll open one.