Catch notices, warnings, errors and fatal errors from the tested side

I was thinking on doing something with hook_watchdog (implementing it, getting the original db_prefix in user agent and then re-issuing a watchdog for the original DB) but that would only cover errors that have a watchdog -- drupal_error_handler has one but there are many errors that don't. So, +1 for this solution.

I made a few changes:

• Added simple doxygen comment.
• Changed method name to assertDrupalErrors since get implies that it returns something. (at least that is my interpretation)
• Change the class to messages error instead of messages errors which doesn't work.

Any comments?

Moving.

When I first read 'assertDrupalErrors()' I thought the function should be called 'assertNoDrupalErrors()'. I thought we wanted to assert that there are _no_ Drupal errors.

This is a useful little function and can probably be called at a million places ... It might actually be better to _always_ fail on these errors unless you specify that you expect errors. Not 100% sure about this one, but it looks better than having to sprinkle a ton of tests with this function?

I'm not sure I would want to always fail since many times the tests attempt to break things. Having both assertDrupalErrors and assertNoDrupalErrors could be handy, but instead of outputting the errors as failures simply make one pass or fail based on the method that is called.

I would vote for changing it to that. Any comments?

I have re-implemented the idea in this patch.

To test this code you can use the following test function.

function testAssertErrors() {
  $this->drupalGet('node/add/page');
  $this->drupalPost('node/add/page', array(), t('Save'));

  $this->assertDrupalErrors();
  $this->assertDrupalErrors(array(t('Title field is required.')));
  $this->assertNoDrupalErrors();
}

Toggle which one of the first two requests is called and check the assertion messages.

I think this looks good. It's probably a good idea to check if any of the existing tests could take advantage of these calls. That would validate the suggested API. Ideally, it would simplify some of the existing tests.

I guess before I got through all the tests and implement this new method it would be good to decide if we want to make this work for messages in general.

Either have two different sets of methods one for errors and one for messages, make the two methods work for both with a flag, or make them always look at either.

The only code that would need to change to make any of that work would be:

$this->elements->xpath("//div[@class='messages error']");

Many of the tests check for messages using assertText. Allowing this function to be able to check the messages would make that check slightly more sure, but at the same time it doesn't allow for messages to be placed on each of the individual asserts.

For example:

$this->assertText(t('The block has been created.'), t('Box successfully created.'));

Instead that would become apart of a single summary message.

Any comments?

but at the same time it doesn't allow for messages to be placed on each of the individual asserts.

I don't understand what you mean with this. The way that sentence is constructed confuses me. Can you try to rephrase? Thanks.

If the new function is used you pass an array of messages to check for and it creates one assertion message. As apposed to:

$this->assertText('text1', t('message1'));
$this->assertText('text2', t('message2'));
$this->assertText('text3', t('message3'));
$this->assertText('text4', t('message4'));
$this->assertText('text5', t('message5'));

would be replaced with:

assertDrupalErrors(array('text1', 'text2', 'text3', 'text4', 'text5'));

So you would never see message1, message2, etc.

I think that is fair as long the error message that is automatically generated is helpful.

Also as of #8 do we want to make this work for message (not errors) as well?

I think we want to make this work for messages (not errors) as well.

Alright I thought about the usefulness of the methods for a while and came up with this.

If we decide this is a good set then I will implement them in all the tests.

I don't know if something like this has been discussed earlier, but what about if we output some data inserted in the HTML output when a simpletest User-Agent is used. The data can be used by tests, which currently only receive the HTML output. The data could be: messages, errors, the current Drupal path (to check if forms redirect properly), the current user, ..., and other that is not possible or is hard to extract from the HTML output. For example:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
  "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en" dir="ltr">
  <head>
    <title>..</title>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
    ...
    ...
    <simpletest>
      <messages>
        <message type="error">The specified passwords do not match.</message>
        <message type="error">The e-mail address ... is not valid..</message>
      </messages>
      
      <path>user/1/edit</path> 
       
      <user>1</user>
    </simpletest>
    ...
  </head>

  <body class="sidebar-left">
  </body>
</html>  

This way, assert functions does not need to deal with complex XPath expressions, and would appear an accurate version of $this->assertText(t('The configuration options have been saved.'), .. to assert if a message has been set.

I think this might be done in addition to a SimpleTest theme.

I have played with the idea and it is the only way to make the tests independent of themes, which they should be. Testing for the theme layer should be separated out.

I think this could fall under that. I haven't gotten around to making a SimpleTest theme, but I think that would be the best way to go about this and be the best way to make assertions without having to worry about HTML.

Things like:

$this->assertBlock($title);

could be possible without xpath (not that xpath is bad, quiet awesome in fact).

Things like this $errors = $this->elements->xpath("//div[@class='messages status'] | //div[@class='messages status']"); makes tests dependent of themes. A good approach could be to force Drupal to output a XML response when the simpletest user-agent is used and make the testing system consume XML, instead of the HTML output.

The XML response could look like this (and any other data we need to include in the XML):

<simpletest>
  <user>1</user>
  <path>node/add</path>
  <title>Create content</title>
  
  <messages>
  	<message type="error">...</message>
	<message type="warning">...</message>
  </messages>
  
  <theme name="garland">
	<theme-settings>
	  <logo></logo>
	  ...
	</theme-settings>
  </theme>
  
  <html>
    <![CDATA[ 
       <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
          <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en"> 
            ... 
             <title>Create content | localhost</title>
            ...   	 
         </html>
      ]]>
  </html>
</simpletest>

This way, tests can have access to data that is only available in the testing thread. Note that we still could have the HTML output, to parse forms fields, or whatever.

I don't like the idea of a simpletest theme. After all we are testing a global product (Drupal on Garland), and thus it makes sense to test the behavior of the theme layer too. Moreover, a special theme or a special output will increase the maintenance work.

However, I worked on a way to pull error messaging from the tested site by the way of HTTP headers. This could be applied to messages too. Attached a patch for this general idea.

@Damien: It's not only about error messages, we are in need to have access to other data that is only available in the separate testing thread (request) and that can not be parsed from a HTML output. The idea of HTTP header works well for error messages, but with other complex data it may not.

I am going to create other issue for this.

@dropcube, I'm not sure what you mean by "other complex data". We should not have a lot of data to fetch, and we could very well serialize more complex data. But I agree we need a separate issue for that.

@Damien: I mean any data that can be available to make accurate assertions and that was available during the test request.

I have created an issue: #266220: Exposing metadata to tests

Wait on #266220: Exposing metadata to tests to be decided.

Damien's patch in #18 is very useful, and should not be postponed in favor of vaporware IMO. Perhaps those headers should only be sent if the request came in with the simpletest user agent. Thats a small point though. These sorts of improvements make fixing tests much easier and therefore more people get involved.

We need a little more progress here to decide how this info gets reported back on the results page.

Agreed in the sense that these sort of improvements make testing easier. However, sending special HTTP headers the data can't be parsed using xpath or XML which are more complete solutions. Moreover, we are not only in need of this data, we will be requiring other more complex set of data.

Current fatal error mean all you get is a fatal error on the page which can be checked for one. Other errors are catched by the test if I am not mistaken.

@chx: no, other errors are not catch if they occur on the *tested* side. Only exception from the tester side are currently reported. Thus my proposal in #18.

Other than that, I agree we should try to parse fatal errors from the output, too (PHP error handler probably can't catch them).

@chx:
Yes, I agree with Damien, it's about the notices/warnings/errors on the 'tested' thread. Is this what you mean?

I'll like #304924: Extend drupal_error_handler to manage exceptions to get reviewed and go in first.

Here is an updated version of a previous work I did in #304924: Extend drupal_error_handler to manage exceptions. Based on #304924: Extend drupal_error_handler to manage exceptions, you should apply the other one first (the testbot may be angry).

subscribe

Updated the patch to follow #304924.

Now that #304924 is in, here is an updated patch.

Will assertions ever get very long? There is a limit to the length of http headers. Have a look at how Firephp handles this - http://www.firephp.org/Wiki/Reference/Protocol. They have groovy code to split objects across multiple headers and put them back together on the receiving side. Perhaps this is overkill here.

@moshe: as far as I know, there is nothing limiting the length of HTTP headers.

You will get a lot of notices when running the full suite with that patch.

#321828: maintenance_page not registered correctly in drupal_common_theme() fixes the Simpletest functionality test (that's a D6-7 bug, so treated independently).

Some more D7-specific fixes (Session, Drupal error handler tests) in that new patch.

This is one of the most important patches of the year. Finally we have good visibility into the tested side. This RTBC.

Attached is a screenshot which shows notices when running the simpletest test (Damien submitted a separate patch which fixes these).

Would be very nice to get some committer feedback on this.

Can we add a code comment to this line: + call_user_func_array(array(&$this, 'error'), unserialize(urldecode($matches[1])));. Thanks.

Added code comments.

Much better. Please also describe call_user_func_array(array(&$this, 'error') (e.g. what function is being called).

Here are the issues that need to get fixed to get back to 0 fails:

• #321828: maintenance_page not registered correctly in drupal_common_theme()
• #329223: Notices in ActionsConfigurationTestCase
• #329224: Notices in BlockTestCase
• #329226: Store file_test.module's values in variables rather than globals
• #329227: Notices in ImportOPMLTestCase

#329229: SimpleTest does not map PHP errors that occur during curl requests to exceptions. was a duplicate :)

Reviewed, tested, committed to CVS HEAD. Let's sprint towards fixing those exceptions! Thanks.

It doesn't work when $db_prefix != '' (it probably did prior to the checkin for #328719: Module list functionality test fails a few days ago). This is fixed by this small patch.

$GLOBALS['db_prefix'] contains "simpletest\d" on both the tester side and the tested side, but we only need to set headers on the tested side, so it is sufficient to check HTTP_USER_AGENT (on the tester side this error handler isn't used, because Simpletest provides its own error handler than is registered prior to changing $db_prefix).

It'd be nice to get Damien to look at c960657's fix. Whenever I see HTTP_USER_AGENT I always feel a bit dirty.

A couple things to check:
* Does running tests from the command line still work?
* check if you throw a notice on the test side (not through drupalPost|Get) and see if it reports as exception with patch applied (from boombatower)

The HTTP_USER_AGENT approach is also used in include/database/database.inc. But I agree that it is not pretty. This is being addressed in #329177: Get rid of ugly simpletest regexp.. See also #276008: Missing user agent triggers notice.

Ok. The last of the test failure fixes went in tonight. I'd like to commit c960657's patch to finish off this issue, but I would still like confirmation from someone who understands this code a bit better than I do that this is the right fix, as well as the items in #51 work.

@webchick: the fix is ok.

Since #328719: Module list functionality test fails, we prefix the db_prefix of the tested site with the db_prefix of the testing site.

ie. if you are running the test inside a Drupal installation with $db_prefix = "mysite_", tested sites will be $db_prefix = "mysite_simpletest4647244"

The test on the HTTP_USER_AGENT is exactly the same as in Database::openConnection(), so everything is good here.

We made some work with c960657 to abstract those check into a nice and clean simpletest_id() function in #329177. In the meantime, RTBC, and end that November-make-the-tests-pass-again-frenzy.

Ok, awesome. Thanks folks! :D

Committed to HEAD!

_drupal_report_error()

+    $assertion = array(
+      $message,
+      $type,
+      $caller
+    );
+    header('X-Drupal-Assertion-' . $number . ': ' . rawurlencode(serialize($assertion)));

drupal_web_test_case::curlHeaderCallback()

+      call_user_func_array(array(&$this, 'error'), unserialize(urldecode($matches[1])));

This code assumes, that the third argument for drupal_web_test_case::error(), $caller, is the function name. But actually error() requires an associative array with the keys 'function', 'file' and 'line'.

I discovered this with E_STRICT error reporting enabled.

This patch adds type hinting to error() and _assert() (this would have made debugging this error easier), and makes _drupal_report_error() pass the file name and line number as well. The Doxygen comment for error() is elaborated (based on the comment for _assert()).

(is it okay to reopen the issue for things like this, or should I rather file a new issue?)

Well, this is not the good issue. The broken code was introduced by #328781: Fix horrible things in the error reporting.

Also, if I'm not mistaken we have a test for this, so how come this was not spotted earlier?

Edit: I was mistaken: we have no test for this.

Ah, sorry, I'll post the patch over there instead.