Fix Simpletest integration

Attached patch

- fixes requirements
- uses already known __FILE__ path info to exclude code_coverage files from recording
- makes ?code_coverage=1 work again

However, SimpleTest integration does not work yet. Nothing is recorded.

Attached patch makes SimpleTest code coverage work :)

Incorporating latest findings from #741174: Deal with Xdebug inconsistencies

Now this starts to make sense! :)

...and works ;)

@Berdir: Running tests works for me. Obviously, you need to put the following line to the top of bootstrap.inc though:

require_once DRUPAL_ROOT . '/sites/all/modules/code_coverage/code_coverage.xdebug.inc';

On the patch:

+++ b/code_coverage.module
@@ -135,3 +131,11 @@ function code_coverage_result_clear() {
+function code_coverage_form_simpletest_result_form_alter(&$form, &$form_state) {

Missing PHPDoc.

+++ b/code_coverage.module
@@ -135,3 +131,11 @@ function code_coverage_result_clear() {
+  // @todo: Find better way to get test_id.
+  $test_id = arg(5);

The test ID might be contained in the $form structure as #property - needs to be checked.

+++ b/code_coverage.module
@@ -135,3 +131,11 @@ function code_coverage_result_clear() {
+  code_coverage_process($test_id);
+  drupal_set_message(t('View the !coverage_report_link for the test run.', array('!coverage_report_link' => l(t('code coverage report'), 'coverage/' . $test_id))));

code_coverage_process() should have a TRUE|FALSE return value, so we do not output this message if no coverage results are available.

+++ b/code_coverage.xdebug.inc
@@ -7,32 +7,25 @@
+/**
+ * Global flag indicating that this file was loaded from bootstrap.inc.
+ */
+$code_coverage_installed = TRUE;

I moved this flag into xdebug.inc, as it should be safe to assume that whenever xdebug.inc is loaded, the core patch has been applied. Overall, however, I'm not sure whether we need this flag. Should be discussed in a follow-up issue.

+++ b/code_coverage.xdebug.inc
@@ -7,32 +7,25 @@
     // Generage coverage set ID and provide link to results.
-    $coverage_set = mt_rand(100000, 1000000000);
...
+    $coverage_set = mt_rand(1000000, 1000000000);

Changed the random value to not conflict with Simpletest db prefixes first, but afterwards decided to implement Simpletest integration differently. Either way, could use a minimal comment to explain the range.

+++ b/code_coverage.xdebug.inc
@@ -7,32 +7,25 @@
-  xdebug_start_code_coverage();
+  xdebug_start_code_coverage(XDEBUG_CC_DEAD_CODE | XDEBUG_CC_UNUSED);

Apparently, the only documentation about these bit-wise OR'ed flags exists in some slides. Would be helpful if we'd quickly explain them and their effect in an inline comment. Also, the old code/patch mistakenly used || instead of |, so only the value of the first flag was passed, instead of both being OR'ed together.

+++ b/code_coverage.xdebug.inc
@@ -42,62 +35,91 @@ if (!empty($_GET['code_coverage']) || strpos($_SERVER['HTTP_USER_AGENT'], 'simpl
+  try {
...
+  }
+  catch (Exception $e) {
+    if (error_displayable()) {
+      print '<h1>Uncaught exception thrown in shutdown function.</h1>';
+      print '<p>' . _drupal_render_exception_safe($e) . '</p><hr />';
+    }
   }

I'm not sure why we have this try/catch block. The new code should always work and I think it only requires a db-level bootstrap.

I'd like to remove the try/catch block in favor of ensuring/enforcing at least a DRUPAL_BOOTSTRAP_DATABASE in this shutdown function. Some functions that required a full bootstrap have already been eliminated; it should be possible to make it fully db-level bootstrap compliant.

That's also something we could verify in a test.

+++ b/code_coverage.xdebug.inc
@@ -42,62 +35,91 @@ if (!empty($_GET['code_coverage']) || strpos($_SERVER['HTTP_USER_AGENT'], 'simpl
+    $db_options = array();
+
+    // Check whether coverage set maps to a simpletest database prefix, and if
+    // it does, replace it with the test_id.
+    if (preg_match('@^(simpletest\d+)@', $coverage_set, $matches)) {
+      $connection_info = Database::getConnectionInfo('default');
+      unset($connection_info['default']['prefix']);
+      Database::addConnectionInfo('default', 'code_coverage_original', $connection_info['default']);
+      $db_options['target'] = 'code_coverage_original';

This db connection target munging could use some more comments, I think.

+++ b/code_coverage.xdebug.inc
@@ -42,62 +35,91 @@ if (!empty($_GET['code_coverage']) || strpos($_SERVER['HTTP_USER_AGENT'], 'simpl
+      $coverage_set = $matches[1];
+
+      $test_id = db_query('SELECT test_id FROM {simpletest_test_id} WHERE last_prefix = :prefix', array(
+        ':prefix' => $coverage_set,

Let's use $matches[1] as query argument. $coverage_set will be overwritten anyway.

Performed all changes, except for removing the try/catch block. I'd like to simply remove it and move forward here, but not sure whether that's acceptable.

Removed the try/catch block. Let's improve that in a follow-up patch/issue.

The existing try/catch in _drupal_shutdown_function() is sufficient -- I forgot to mention that I also reverted the change that registered a custom shutdown function back to drupal_register_shutdown_function(). Hence, any exception thrown in our shutdown function will be catched by _drupal_shutdown_function().

I went ahead and committed #9 to master.

Next best step would be to commit a .patch file for bootstrap.inc, I think: #293298: Add a .patch file for core