From fbfd454d6557038ed9e152946ecfd55277ddd70c Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 18 Jun 2018 14:44:31 +1200 Subject: [PATCH] ENHANCEMENT Ensure test DB is flushed on either DDL or transaction-disabled tests Fixes #8182 --- src/Dev/SapphireTest.php | 18 +++++++- src/Dev/State/FixtureTestState.php | 66 +++++++++++++++++++++--------- src/ORM/Connect/Database.php | 22 ++++++---- src/ORM/Connect/MySQLDatabase.php | 62 ++++++++++++++++++++++++---- src/ORM/Connect/TempDatabase.php | 25 ++++++++--- 5 files changed, 152 insertions(+), 41 deletions(-) diff --git a/src/Dev/SapphireTest.php b/src/Dev/SapphireTest.php index c4c39ddd4..988f7f087 100644 --- a/src/Dev/SapphireTest.php +++ b/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); diff --git a/src/Dev/State/FixtureTestState.php b/src/Dev/State/FixtureTestState.php index ccbe67f1a..0272ebfc4 100644 --- a/src/Dev/State/FixtureTestState.php +++ b/src/Dev/State/FixtureTestState.php @@ -26,25 +26,31 @@ class FixtureTestState implements TestState */ public function setUp(SapphireTest $test) { - if ($this->testNeedsDB($test)) { - $tmpDB = $test::tempDB(); - if (!$tmpDB->isUsed()) { - $tmpDB->build(); - } - DataObject::singleton()->flushCache(); + if (!$this->testNeedsDB($test)) { + return; + } + $tmpDB = $test::tempDB(); + if (!$tmpDB->isUsed()) { + $tmpDB->build(); + } + DataObject::singleton()->flushCache(); - if (!$tmpDB->hasStarted()) { - foreach ($test->getRequireDefaultRecordsFrom() as $className) { - $instance = singleton($className); - if (method_exists($instance, 'requireDefaultRecords')) { - $instance->requireDefaultRecords(); - } - if (method_exists($instance, 'augmentDefaultRecords')) { - $instance->augmentDefaultRecords(); - } + // Ensure DB is built and populated + if (!$tmpDB->hasStarted()) { + foreach ($test->getRequireDefaultRecordsFrom() as $className) { + $instance = singleton($className); + if (method_exists($instance, 'requireDefaultRecords')) { + $instance->requireDefaultRecords(); + } + 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)) { - $test::tempDB()->rollbackTransaction(); + if (!$this->testNeedsDB($test)) { + 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); + } } diff --git a/src/ORM/Connect/Database.php b/src/ORM/Connect/Database.php index 3e2910522..add4d6948 100644 --- a/src/ORM/Connect/Database.php +++ b/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) { diff --git a/src/ORM/Connect/MySQLDatabase.php b/src/ORM/Connect/MySQLDatabase.php index fa3672db5..59c0c2b6c 100644 --- a/src/ORM/Connect/MySQLDatabase.php +++ b/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 { - --$this->transactionNesting; - if ($this->transactionNesting > 0) { - $this->transactionRollback('NESTEDTRANSACTION' . $this->transactionNesting); - } else { - $this->query('ROLLBACK'); - } + return true; } + + // 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( diff --git a/src/ORM/Connect/TempDatabase.php b/src/ORM/Connect/TempDatabase.php index 832acd606..b1fbecddf 100644 --- a/src/ORM/Connect/TempDatabase.php +++ b/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()) { - static::getConn()->transactionRollback(); - } else { - $this->hasStarted = false; - static::clearAllData(); + $success = $this->hasStarted() && static::getConn()->supportsTransactions(); + if ($success) { + try { + // Explicit false = gnostic error from transactionRollback + 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;