Add "expected fail" functionality to simpletest

Just to reiterate, I have written the functionality, Barry fixed bugs and added tests (awesome!) the API is $this->expectedFail()->assert...

I am wondering why this is important. why not use inversions of functions (assertTrue => assertFalse, assertEqual => assertNotEqual)

Still not sure why we don't just keep the tests in the issue as discussed numerous times. One of the major reasons being to keep the level of chaos and upkeep work down.

Having all these expectedFail() tests seems like it is just asking for tests to be forgotten and not sure why it is useful. Just put the tests in the issues..........

If we go through with this as it seems I will get over-ruled I think it would be nice to have the reporting being able to distinguish between expected failures and regular assertions.

@boombatower #4: That was my initial reaction as well. The advantages we gain here are:

1. We have a list *in core* of the bugs we need to fix, rather than scattered all over the issue queue.
2. Drupal has bugs and always will have bugs. If we try and wait until every single bug that has a test to go with it is patched, Drupal 7 will never be released. :P Especially after everyone's addicted to tests post-testing party. ;)
3. As Barry mentions, we can document the expected behaviour of code that is not working, and tell if it suddenly things have changed.

@bjaspan: I was interested in discussing it before I spent time reviewing something that I didn't even like the idea of. I see the feature is implemented which is nice.

Minor:

remove double space from line 273 of simpletest.test.

function  testExpectedFailures() { if ($this->inCURL()) {

All messages in assertions should be t()ed.

example:

$this->expectedFail()->assertTrue(FALSE, 'Expected failure');
$this->expectedFail()->assertTrue(TRUE, 'Unexpected pass');

should have t() around 'Expected failure'...

First line of test function above shouldn't have if() on the same line.

Would be nice if we could get one of these patches that remove all the extra white-spacing would get in, as that is in several patches which will all need to be re-rolled once one of these gets in.

i would suggest requiring a issue number as the argument to expectedFail() , pointing honest folks to track the reason why it fails. Then you could build a hyper-link in test report to issues.

Although I reviewed patch, it's not obvious if run-tests.sh will return error code 1 only on failures. That would help me measure a regression....on errors anyway ;)

While it's a good feature to have, I've got concerns about how it will affect core workflow.

By attaching tests to issues, we have a clear way to document which issues have tests written for them, what the approach of the test is etc. If the test is in core, then there's not an easy way to do the same thing, since it'll no longer be a patch (and may well be committed from a different issue), tests don't show up on api.drupal.org etc. etc. So you'd have to actually go into the test file and find the line rather than just reading it straight on the issue.

The issue which prompted this patch is #276267: Fix the minimum cache lifetime feature - and is a pretty good example I think.

There's perfectly decent bug report for this issue over at #277448: Cache system isn't doing CACHE_TEMPORARY like documented - despite that test failing in core for weeks and weeks, only three people have commented on it. The test is written to follow the documentation, but it's not clear that the documentation is correct - so potentially the code is right and the test is wrong. I've not spent much time on that issue to know which is the case, but iif not, we could very easily come up against the same thing somewhere else. If the test does indeed need changing along with the code comments, then it's much harder to do this when it's already in core. So we'd need to be 100% sure that each test with expectedFail() correctly documents the broken code before committing it.

Also, dlhubler is right in pointing out that we need an issue id hammered into expectedFail() - so people know where to look (ideally with a nice link too) - this might actually help prevent duplicates sometimes, but without it would be very nasty. So I'm marking to needs work until that's in there. Additionally we desperately need a way to document bugs with tests, with or without this patch, see #280972: Add a new project issue status option: active (reproducible) - you can mess with the status and call things patch (code needs work) if the only thing in the patch file is the test, but that'll be even less accurate with this in.

Like this?

I still don't like this idea and agree with catch.

By the way: tzhe bugfix attached to an issue will still contain a test fix which removes the expectedFail. The latest patch links to the issue. That actually drives people to the issue, every time you check the test results you get a link.

I really like the idea. Having issues documented in the core itself will probably help to avoid sleeping on them too long. I will review the patch tomorrow.

Except not all issues can be documented in core. There's critical usability, javascript, PHP version bugs, even simple string issues all the time, which in some cases are much more release critical than what could be a fairly minor bug which happens to have a test - but we can't write simpletests for those (well maybe the typos, but that would be silly). There's also over 500 bugs against 7.x at the moment, and while an 'active (steps to reproduce)' status might help to filter them a bit, the problems with fragmenting things between core and the issue queue seems to have been lost in this conversation, or turned into a feature maybe. Are we going to start leaving notes on crappy interfaces that we need to fix them before release too?

IMO, there is a real problem in Drupal community with perfectionism. This patch recognizes that there will be bugs and that sometimes the tests land before the fixes do. +1 from me. KDE project acknowledges this reality, as well as many others. Anyway, we can rip this out later if we feel it is doing more harm than good. Instead of "being worried", just try it out.

The point that not all issues can be documented in core is not relevant IMO. This helps those that can.

Yes, I think a convention or a requirement to specify an issue number is a good idea. @chx's implementation is great except the URL for the issue should be constructed from a variable so it can point to own issue tracker. That can come later if need be.

OK, I've given this some thought and it's fine with me, but let's just be very, very careful not to commit tests with false positives.

Can we work on getting #250047: Rework the SimpleTest results interface and clean-up of backend code reviewed/committed first and then re-roll this.

I agree with bjaspan in #19.

How will this patch deal with tests that fail on postgres but not on mysql (we have some at the moment). Or tests which only fail without clean urls enabled (afaik we have some of those too).

Let's get that added then.

@catch - a naive approach says not to use expected fail for those failures - then we are no worse off than today. if a developer really wants to use it, he certainly can certainly declare 'expected fail' after checking those conditions.

Just want to chime in and say I would like to get this in.

I welcome proposed solutions for dealing with the "human" solution once we get the technical solution in.

Can you hear the desperate meowing of the dying kittens? Under the pretense of adding a simple test case for expected failures, this patch reorganized the whole of simpletest.test. Here is the kitten safe version with the asked for changes.

Fixing status.

Debug crap removed.

Patch review, round 1. I'm getting sleepy.

+  var $_results = array('#pass' => 0, '#fail' => 0, '#exception' => 0, '#unexpected pass' => 0, '#expected fail' => 0);
...
+    $test_results = array('#pass' => 0, '#fail' => 0, '#exception' => 0, '#unexpected pass' => 0, '#expected fail' => 0);

Let's get that conforming to coding standards please, with one on each line. I realize that you didn't make it this way, but you did highlight that it's really hard to read. ;)

+  var $expectedFail = FALSE;

All the rest of the variables here are prefixed with a _ to indicate they are private. This one likely should too.

+   * The next assert is expected to fail.

assertion, not assert.

+   * Use $this->expectedFail()->assert... to mark the assert as an
+   * expected fail.  If your test reveals a bug then use this function.

I would like to see this documentation fleshed out quite a bit more. Why would I ever do this? What's the benefits of putting in known failing tests? Give me usage example in code?

Basically, let's capture the points in this issue in favour of this patch in PHPDoc so that others don't need to find this URL to understand it.

+   * @param $issue

Need a newline above this to separate it from the description.

+   *   Either the nid of the issue that covers this bug on drupal.org or
+   *   an absolute URL pointing to the issue in any bugtracker.

nid => node ID
bugtracker => bug tracker

+  protected function expectedFail($issue = NULL) {

I'm not sure about the name of this function. All of our other test functions are verbThingBeingVerbed(), like assertFail(). I think this should be called expectFail() to be inline with that.

I tried Googling various testing frameworks to see what they do and had very mixed results, the most o_0 one being public void testForExpectedExceptionWithAnnotation(). ;)

Incidentally, http://fsunit.googlecode.com/svn-history/r104/trunk/SourceCode/FsUnit.Te... also has an expectPass() function. Not sure why. Something we should consider here?

+      $issue = l(t('a known bug'), is_numeric($issue) ? "http://drupal.org/node/". $issue : $issue);
+      $this->expectedFail = t('This is expected to fail due to !issue. ', array('!issue' => $issue));

The t() should be re-writen with the link in it like:

$issue = // logic to figure out the url
t('This is expected to fail due to <a href="!issue">a known bug</a> ' ...)

Not that I ever forsee anyone actually translating these, but it's best practice.

"http://drupal.org/node/". $issue also has a concatenation bug (spaces should be on either side of the period) but would be more succinctly written as "http://drupal.org/node/$issue". If you want to keep concatenation, 'http://drupal.org/node/' should have single quotes around it since it is not doing variable interpolation.

+        'length' => 17,

17? "unexpected pass" looks like only 15 to mine eyes.

         'description' => t('Message status. Core understands pass, fail, exception.'),

Needs updating to reflect this patch.

I also notice that there's no update function. Is that because we're doing HEAD => HEAD? What about people who have the existing SimpleTest module installed in contrib?

    */
+
   function testWebTestRunner() {

Bug. This will cause the PHPDoc not to compute correctly in API module.

Also, as a general, NON-nit-picky thing, I'd feel a lot better about committing this if there was some code to go along with it to confirm it works. A new test case for simpletest.test, perhaps?

Edit Ignore me. :P This test exists, it's just later in the patch than I had got to yet.

There. A bit of meow, but really little -- I added a new private function instead of adding the new statuses everywhere.

I always leave in debug.

As for $warning_passes business that would be solved by my house-cleaning patch, to be split off today/tomorrow as it would simply report Pass, Warning, Fail. Which I think it cleaner than having that parameter.

Would prefer waiting for that and re-rolling this. (as the house-cleaning will touch on a number of areas this tends to edit, but obviously that doesn't necessarily have to hold this back)

Otherwise after quick review looks fine.

Subscribing.

Here's a completely untested re-roll.

subscribing

Tested the latest patch and I am getting two new failures in simpletest tests. They are the final two assertions, and their text is very cryptic (please make it less cryptic). For example - "Found assertion {"This is expected to fail. Expected fail.", "Other", "Pass", "simpletest.test", "SimpleTestTestCase->testExpectedFails()"}."

I think the failures are an error but I am not 100% sure.

Haven't read the whole thread, but wanted to point out that we *need* a way to say "This test is going to throw some known exceptions, and that's okay", or there's no way to have automated tests for things like #391264: Update XML parse errors make installation fail. We need to feed bogus XML to update.module make sure it gracefully handles the exception. But, it's going to throw an exception, and *any* exception, even if you catch it and do something graceful and nice, is currently considered a test failure. :(

I think this is D8 at this point, sorry. For D7 we'll have to live with the fact that we can't do this technique.

The patch in #41 should probably be a separate issue, or merged into the 'expected fail' functionality of the previous patches. This patch may be needed to properly write tests for #1171436: Assert that modules passed into setUp are properly enabled..

This is no longer a feature but a necessary task if done by driver.

Reverting per discussions with alexpott. We will handle this other ways.

Closing as outdated. If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.

Thanks!