Geting drupal error messages from a page load in ST browser is very important for debugging tests. Now we can easily do this with xpath. This is initial suggestion what we can do. Suggestions?

Files: 
CommentFileSizeAuthor
#56 243532-follow-up-2.patch3.58 KBc960657
Unable to apply patch 243532-follow-up-2.patch View
#50 243532-follow-up-1.patch1.11 KBc960657
Failed: Failed to apply patch. View
#46 243532-simpletest-errors.patch6.5 KBDamien Tournoud
Failed: Failed to apply patch. View
#44 243532-simpletest-errors.patch6.43 KBDamien Tournoud
Failed: Failed to apply patch. View
#42 simpletest-errors.patch6.22 KBbeejeebus
Failed: Failed to apply patch. View
#39 Picture 1.png93.44 KBmoshe weitzman
#38 243532-simpletest-errors.patch6.22 KBDamien Tournoud
Failed: Failed to apply patch. View
#34 243532-simpletest-errors.patch2.57 KBDamien Tournoud
Failed: Failed to apply patch. View
#33 243532.patch2.89 KBDamien Tournoud
Failed: Failed to apply patch. View
#31 243532.patch3.02 KBDamien Tournoud
Failed: Failed to apply patch. View
#29 simpletest_report_errors.patch3.06 KBbeejeebus
Failed: Failed to apply patch. View
#18 243532-simpletest-error-reporting.patch1.94 KBDamien Tournoud
Failed: Failed to apply patch. View
#14 simpletest_assert_errors.patch3.63 KBboombatower
Failed: Failed to apply patch. View
#6 simpletest_assert_errors.patch2.14 KBboombatower
Passed: 7252 passes, 0 fails, 0 exceptions View
#2 STgeterrors.patch1.18 KBboombatower
Failed: Failed to apply patch. View
STgeterrors.patch1.09 KBRok Žlender
Failed: Failed to apply patch. View

Comments

chx’s picture

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.

boombatower’s picture

Status: Needs work » Needs review
FileSize
1.18 KB
Failed: Failed to apply patch. View

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?

boombatower’s picture

Project: SimpleTest » Drupal core
Version: » 7.x-dev
Component: Code » simpletest.module

Moving.

Dries’s picture

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?

boombatower’s picture

Status: Needs review » Needs work

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?

boombatower’s picture

Title: Parse drupal errors » SimpleTest provide assertions on error messages
Assigned: Rok Žlender » boombatower
Status: Needs work » Needs review
FileSize
2.14 KB
Passed: 7252 passes, 0 fails, 0 exceptions View

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.

Dries’s picture

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.

boombatower’s picture

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?

Dries’s picture

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.

boombatower’s picture

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.

Dries’s picture

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

boombatower’s picture

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

Dries’s picture

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

boombatower’s picture

FileSize
3.63 KB
Failed: Failed to apply patch. View

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.

dropcube’s picture

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.

boombatower’s picture

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

dropcube’s picture

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.

Damien Tournoud’s picture

FileSize
1.94 KB
Failed: Failed to apply patch. View

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.

dropcube’s picture

@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.

Damien Tournoud’s picture

@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.

dropcube’s picture

@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

boombatower’s picture

Status: Needs review » Postponed
moshe weitzman’s picture

Status: Postponed » Needs work

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.

dropcube’s picture

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.

chx’s picture

Title: SimpleTest provide assertions on error messages » SimpleTest catch fatal errors
Assigned: boombatower » chx
Category: feature » task

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.

Damien Tournoud’s picture

Title: SimpleTest catch fatal errors » Catch notices, warnings, errors and fatal errors from the tested side

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

dropcube’s picture

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

beejeebus’s picture

i'm not convinced either way about #266220: Exposing metadata to tests, but the patch at #18 or something similar would have saved me a chunk of time tonight.

what do we need to get this moving forward?

beejeebus’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
Failed: Failed to apply patch. View

updated patch at #18, implemented simpletest_get_severity_as_string($severity) .

running all tests, we get tests where everything passes *and* exceptions are thrown.

  • Import feeds from OPML functionality: 41 passes, 0 fails, 2 exceptions
  • Comment interface: 84 passes, 0 fails, 2 exceptions
  • Session tests: 9 passes, 0 fails, 2 exceptions
Damien Tournoud’s picture

Assigned: chx » Unassigned
Status: Needs review » Postponed

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

Damien Tournoud’s picture

Status: Postponed » Needs review
FileSize
3.02 KB
Failed: Failed to apply patch. View

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

CorniI’s picture

subscribe

Damien Tournoud’s picture

FileSize
2.89 KB
Failed: Failed to apply patch. View

Updated the patch to follow #304924.

Damien Tournoud’s picture

FileSize
2.57 KB
Failed: Failed to apply patch. View

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

moshe weitzman’s picture

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.

Damien Tournoud’s picture

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

Damien Tournoud’s picture

FileSize
6.22 KB
Failed: Failed to apply patch. View

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.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
93.44 KB

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

beejeebus’s picture

code looks good, patch applies cleanly.

until the file is committed, you'll need to manually get the includes/database/log.inc from that issue to run all tests.

ran all tests with the caveat above, all pass, plus 49 exceptions... 7065 passes, 0 fails, 49 exceptions.

moshe weitzman’s picture

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

beejeebus’s picture

FileSize
6.22 KB
Failed: Failed to apply patch. View

bump. rerolled patch to get rid of fuzz, no code changes. running all tests, i get:

7105 passes, 0 fails, and 51 exceptions

once this is in, we can chase down the exceptions.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

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

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
6.43 KB
Failed: Failed to apply patch. View

Added code comments.

Dries’s picture

Status: Needs review » Needs work

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

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
6.5 KB
Failed: Failed to apply patch. View
Damien Tournoud’s picture

Dries’s picture

Status: Needs review » Fixed

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

c960657’s picture

Status: Fixed » Needs review
FileSize
1.11 KB
Failed: Failed to apply patch. View

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

webchick’s picture

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)

c960657’s picture

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.

webchick’s picture

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.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

@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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, awesome. Thanks folks! :D

Committed to HEAD!

c960657’s picture

Status: Fixed » Needs review
FileSize
3.58 KB
Unable to apply patch 243532-follow-up-2.patch View

_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?)

Damien Tournoud’s picture

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.

c960657’s picture

Status: Needs review » Fixed

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

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.