ENHANCEMENT Ensure test DB is flushed on either DDL or transaction-disabled tests

Fixes #8182

src/Dev/SapphireTest.php

@@ -78,6 +78,14 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly
      */
     protected $usesDatabase = null;
 
+    /**
+     * This test will cleanup its state via transactions.
+     * If set to false a full schema is forced between tests, but at a performance cost.
+     *
+     * @var bool
+     */
+    protected $usesTransactions = true;
+
     /**
      * @var bool
      */
@@ -228,6 +236,14 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly
         return $this->usesDatabase;
     }
 
+    /**
+     * @return bool
+     */
+    public function getUsesTransactions()
+    {
+        return $this->usesTransactions;
+    }
+
     /**
      * @return array
      */
@@ -1188,7 +1204,7 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly
         if (strpos($fixtureFilePath, ':') !== false) {
             return ModuleResourceLoader::singleton()->resolvePath($fixtureFilePath);
         }
-        
+
         // Support fixture paths relative to the test class, rather than relative to webroot
         // String checking is faster than file_exists() calls.
         $resolvedPath = realpath($this->getCurrentAbsolutePath() . '/' . $fixtureFilePath);

src/Dev/State/FixtureTestState.php

@@ -26,25 +26,31 @@ class FixtureTestState implements TestState
      */
     public function setUp(SapphireTest $test)
     {
-        if ($this->testNeedsDB($test)) {
+        if (!$this->testNeedsDB($test)) {
-            $tmpDB = $test::tempDB();
+            return;
-            if (!$tmpDB->isUsed()) {
+        }
-                $tmpDB->build();
+        $tmpDB = $test::tempDB();
-            }
+        if (!$tmpDB->isUsed()) {
-            DataObject::singleton()->flushCache();
+            $tmpDB->build();
+        }
+        DataObject::singleton()->flushCache();
 
-            if (!$tmpDB->hasStarted()) {
+        // Ensure DB is built and populated
-                foreach ($test->getRequireDefaultRecordsFrom() as $className) {
+        if (!$tmpDB->hasStarted()) {
-                    $instance = singleton($className);
+            foreach ($test->getRequireDefaultRecordsFrom() as $className) {
-                    if (method_exists($instance, 'requireDefaultRecords')) {
+                $instance = singleton($className);
-                        $instance->requireDefaultRecords();
+                if (method_exists($instance, 'requireDefaultRecords')) {
-                    }
+                    $instance->requireDefaultRecords();
-                    if (method_exists($instance, 'augmentDefaultRecords')) {
+                }
-                        $instance->augmentDefaultRecords();
+                if (method_exists($instance, 'augmentDefaultRecords')) {
-                    }
+                    $instance->augmentDefaultRecords();
                 }
-                $this->loadFixtures($test);
             }
+            $this->loadFixtures($test);
+        }
+
+        // Begin transactions if enabled
+        if ($test->getUsesTransactions()) {
             $tmpDB->startTransaction();
         }
     }
@@ -56,9 +62,21 @@ class FixtureTestState implements TestState
      */
     public function tearDown(SapphireTest $test)
     {
-        if ($this->testNeedsDB($test)) {
+        if (!$this->testNeedsDB($test)) {
-            $test::tempDB()->rollbackTransaction();
+            return;
         }
+
+        // For transactional states, rollback if possible
+        if ($test->getUsesTransactions()) {
+            $success = $test::tempDB()->rollbackTransaction();
+            if ($success) {
+                return;
+            }
+        }
+
+        // Force reset if transaction failed, or disabled
+        $test::tempDB()->kill();
+        $this->resetFixtureFactory(get_class($test));
     }
 
     /**
@@ -68,7 +86,7 @@ class FixtureTestState implements TestState
      */
     public function setUpOnce($class)
     {
-        $this->fixtureFactories[strtolower($class)] = Injector::inst()->create(FixtureFactory::class);
+        $this->resetFixtureFactory($class);
     }
 
     /**
@@ -220,4 +238,14 @@ class FixtureTestState implements TestState
 
         return false;
     }
+
+    /**
+     * Bootstrap a clean fixture factory for the given class
+     *
+     * @param string $class
+     */
+    protected function resetFixtureFactory($class)
+    {
+        $this->fixtureFactories[strtolower($class)] = Injector::inst()->create(FixtureFactory::class);
+    }
 }

src/ORM/Connect/Database.php

@@ -654,13 +654,17 @@ abstract class Database
      *
      * @param string|boolean $savepoint Name of savepoint, or leave empty to rollback
      * to last savepoint
+     * @return bool|null Boolean is returned if success state is known, or null if
+     * unknown. Note: For error checking purposes null should not be treated as error.
      */
     abstract public function transactionRollback($savepoint = false);
 
     /**
      * Commit everything inside this transaction so far
      *
-     * @param boolean $chain
+     * @param bool $chain
+     * @return bool|null Boolean is returned if success state is known, or null if
+     * unknown. Note: For error checking purposes null should not be treated as error.
      */
     abstract public function transactionEnd($chain = false);
 
@@ -669,7 +673,7 @@ abstract class Database
      * which is different from table- or row-level locking.
      * See {@link getLock()} for details.
      *
-     * @return boolean Flag indicating that locking is available
+     * @return bool Flag indicating that locking is available
      */
     public function supportsLocks()
     {
@@ -681,7 +685,7 @@ abstract class Database
      * See {@link supportsLocks()} to check if locking is generally supported.
      *
      * @param string $name Name of the lock
-     * @return boolean
+     * @return bool
      */
     public function canLock($name)
     {
@@ -703,7 +707,7 @@ abstract class Database
      *
      * @param string $name Name of lock
      * @param integer $timeout Timeout in seconds
-     * @return boolean
+     * @return bool
      */
     public function getLock($name, $timeout = 5)
     {
@@ -715,7 +719,7 @@ abstract class Database
      * (if the execution aborts (e.g. due to an error) all locks are automatically released).
      *
      * @param string $name Name of the lock
-     * @return boolean Flag indicating whether the lock was successfully released
+     * @return bool Flag indicating whether the lock was successfully released
      */
     public function releaseLock($name)
     {
@@ -756,7 +760,7 @@ abstract class Database
      * Determine if the database with the specified name exists
      *
      * @param string $name Name of the database to check for
-     * @return boolean Flag indicating whether this database exists
+     * @return bool Flag indicating whether this database exists
      */
     public function databaseExists($name)
     {
@@ -778,12 +782,12 @@ abstract class Database
      * database if it doesn't exist in the current schema.
      *
      * @param string $name Name of the database
-     * @param boolean $create Flag indicating whether the database should be created
+     * @param bool $create Flag indicating whether the database should be created
      * if it doesn't exist. If $create is false and the database doesn't exist
      * then an error will be raised
-     * @param int|boolean $errorLevel The level of error reporting to enable for the query, or false if no error
+     * @param int|bool $errorLevel The level of error reporting to enable for the query, or false if no error
      * should be raised
-     * @return boolean Flag indicating success
+     * @return bool Flag indicating success
      */
     public function selectDatabase($name, $create = false, $errorLevel = E_USER_ERROR)
     {

src/ORM/Connect/MySQLDatabase.php

@@ -329,25 +329,73 @@ class MySQLDatabase extends Database
 
     public function transactionRollback($savepoint = false)
     {
+        // Named transaction
         if ($savepoint) {
             $this->query('ROLLBACK TO ' . $savepoint);
-        } else {
+            return true;
-            --$this->transactionNesting;
-            if ($this->transactionNesting > 0) {
-                $this->transactionRollback('NESTEDTRANSACTION' . $this->transactionNesting);
-            } else {
-                $this->query('ROLLBACK');
-            }
         }
+
+        // Fail if transaction isn't available
+        if (!$this->transactionNesting) {
+            return false;
+        }
+        --$this->transactionNesting;
+        if ($this->transactionNesting > 0) {
+            $this->transactionRollback('NESTEDTRANSACTION' . $this->transactionNesting);
+        } else {
+            $this->query('ROLLBACK');
+        }
+        return true;
     }
 
     public function transactionEnd($chain = false)
     {
+        // Fail if transaction isn't available
+        if (!$this->transactionNesting) {
+            return false;
+        }
         --$this->transactionNesting;
         if ($this->transactionNesting <= 0) {
             $this->transactionNesting = 0;
             $this->query('COMMIT AND ' . ($chain ? '' : 'NO ') . 'CHAIN');
         }
+        return true;
+    }
+
+    /**
+     * In error condition, set transactionNesting to zero
+     */
+    protected function discardTransactions()
+    {
+        $this->transactionNesting = 0;
+    }
+
+    public function query($sql, $errorLevel = E_USER_ERROR)
+    {
+        $this->inspectQuery($sql);
+        return parent::query($sql, $errorLevel);
+    }
+
+    public function preparedQuery($sql, $parameters, $errorLevel = E_USER_ERROR)
+    {
+        $this->inspectQuery($sql);
+        return parent::preparedQuery($sql, $parameters, $errorLevel);
+    }
+
+    /**
+     * Inspect a SQL query prior to execution
+     *
+     * @param string $sql
+     */
+    protected function inspectQuery($sql)
+    {
+        // Any DDL discards transactions.
+        // See https://dev.mysql.com/doc/internals/en/transactions-notes-on-ddl-and-normal-transaction.html
+        // on why we need to be over-eager
+        $isDDL = $this->getConnector()->isQueryDDL($sql);
+        if ($isDDL) {
+            $this->discardTransactions();
+        }
     }
 
     public function comparisonClause(

src/ORM/Connect/TempDatabase.php

@@ -102,15 +102,28 @@ class TempDatabase
 
     /**
      * Rollback a transaction (or trash all data if the DB doesn't support databases
+     *
+     * @return bool True if successfully rolled back, false otherwise. On error the DB is
+     * killed and must be re-created. Note that calling rollbackTransaction() when there
+     * is no transaction is counted as a failure, and will kill the DB.
      */
     public function rollbackTransaction()
     {
-        if (static::getConn()->supportsTransactions()) {
+        $success = $this->hasStarted() && static::getConn()->supportsTransactions();
-            static::getConn()->transactionRollback();
+        if ($success) {
-        } else {
+            try {
-            $this->hasStarted = false;
+                // Explicit false = gnostic error from transactionRollback
-            static::clearAllData();
+                if (static::getConn()->transactionRollback() === false) {
+                    $success = false;
+                }
+            } catch (DatabaseException $ex) {
+                $success = false;
+            }
         }
+        if (!$success) {
+            static::kill();
+        }
+        return $success;
     }
 
     /**
@@ -118,6 +131,8 @@ class TempDatabase
      */
     public function kill()
     {
+        $this->hasStarted = false;
+
         // Delete our temporary database
         if (!$this->isUsed()) {
             return;