After discussion with chx on #drupal, we found that a good majority of Database tests are wrapping their test in try-catch statements that are no longer necessary with the Drupal error handler.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Status: Active » Needs review
FileSize
56.04 KB

Initial pass removing all try-catch statements. Tested locally and worked perfectly on database tests: 616 passes, 0 fails, and 0 exceptions.

Dave Reid’s picture

Second pass. Line whitespace changes, try-catch removal, and brings the tests in DatabaseInsertTestCase in line with each other. I feel like with these changes that the tests are running a little bit faster.

Dave Reid’s picture

In my completely un-scientific test-running stopwatch performance testing, the patch sped the Database tests up by about 5 seconds (with about a total running time of 220 seconds). :)

Dave Reid’s picture

chx wanted to know why I've removed the try-catch statements from all the database tests. We are currently testing the error handler in common.test which throws the following errors in system_test.module to make sure they're caught during testing:

/**
 * Menu callback; trigger an exception to test the exception handler.
 */
function system_test_trigger_exception() {
  define('SIMPLETEST_DONT_COLLECT_ERRORS', TRUE);
  throw new Exception("Drupal is awesome");
}

/**
 * Menu callback; trigger an exception to test the exception handler.
 */
function system_test_trigger_pdo_exception() {
  define('SIMPLETEST_DONT_COLLECT_ERRORS', TRUE);
  db_query("SELECT * FROM bananas_are_awesome");
}

It is reasonable that if we're testing the error handler here, we don't need all the try-catch statements in the database tests anymore.

chx’s picture

Status: Needs review » Reviewed & tested by the community

I knew why you were removing them -- I suggested removing them, actually -- I rather asked, why did you not add tests for this but there you are. FYI I also asked Dave how much the speedup is in percentage and it's like 2% so not that big of a deal but still.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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