From ec956a682dce2876322a3bc7c169a8eccec558ec Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Tue, 12 Jun 2018 22:35:45 +0100 Subject: [PATCH] API Moving tests to use transactions --- _config/tests.yml | 1 + src/Dev/FixtureFactory.php | 2 +- src/Dev/SapphireTest.php | 78 ++++++---- src/Dev/State/FixtureTestState.php | 229 ++++++++++++++++++++++++++++ src/Dev/State/SapphireTestState.php | 30 ++++ src/ORM/Connect/TempDatabase.php | 51 +++++++ src/ORM/DataObjectSchema.php | 38 ++++- 7 files changed, 393 insertions(+), 36 deletions(-) create mode 100644 src/Dev/State/FixtureTestState.php diff --git a/_config/tests.yml b/_config/tests.yml index bed956221..681d24290 100644 --- a/_config/tests.yml +++ b/_config/tests.yml @@ -8,6 +8,7 @@ SilverStripe\Core\Injector\Injector: globals: '%$SilverStripe\Dev\State\GlobalsTestState' extensions: '%$SilverStripe\Dev\State\ExtensionTestState' flushable: '%$SilverStripe\Dev\State\FlushableTestState' + fixtures: '%$SilverStripe\Dev\State\FixtureTestState' requirements: '%$SilverStripe\View\Dev\RequirementsTestState' ssviewer: '%$SilverStripe\View\Dev\SSViewerTestState' --- diff --git a/src/Dev/FixtureFactory.php b/src/Dev/FixtureFactory.php index 88a3fcc9f..9016ddb3b 100644 --- a/src/Dev/FixtureFactory.php +++ b/src/Dev/FixtureFactory.php @@ -41,7 +41,7 @@ class FixtureFactory protected $fixtures = array(); /** - * @var array Callbacks + * @var FixtureBlueprint[] Callbacks */ protected $blueprints = array(); diff --git a/src/Dev/SapphireTest.php b/src/Dev/SapphireTest.php index 3e6805c05..2e30f0c2e 100644 --- a/src/Dev/SapphireTest.php +++ b/src/Dev/SapphireTest.php @@ -23,6 +23,7 @@ use SilverStripe\Core\Manifest\ClassLoader; use SilverStripe\Dev\Constraint\SSListContains; use SilverStripe\Dev\Constraint\SSListContainsOnly; use SilverStripe\Dev\Constraint\SSListContainsOnlyMatchingItems; +use SilverStripe\Dev\State\FixtureTestState; use SilverStripe\Dev\State\SapphireTestState; use SilverStripe\Dev\State\TestState; use SilverStripe\i18n\i18n; @@ -148,7 +149,7 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly /** * State management container for SapphireTest * - * @var TestState + * @var SapphireTestState */ protected static $state = null; @@ -159,6 +160,17 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly */ protected static $tempDB = null; + /** + * @return TempDatabase + */ + public static function tempDB() + { + if (!static::$tempDB) { + static::$tempDB = TempDatabase::create(); + } + return static::$tempDB; + } + /** * Gets illegal extensions for this class * @@ -208,6 +220,22 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly return static::$fixture_file; } + /** + * @return bool + */ + public function getUsesDatabase() + { + return $this->usesDatabase; + } + + /** + * @return array + */ + public function getRequireDefaultRecordsFrom() + { + return $this->requireDefaultRecordsFrom; + } + /** * Setup the test. * Always sets up in order: @@ -254,31 +282,10 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly $fixtureFiles = $this->getFixturePaths(); - // Set up fixture if ($this->shouldSetupDatabaseForCurrentTest($fixtureFiles)) { - if (!static::$tempDB->isUsed()) { - static::$tempDB->build(); - } - - DataObject::singleton()->flushCache(); - - static::$tempDB->clearAllData(); - - foreach ($this->requireDefaultRecordsFrom as $className) { - $instance = singleton($className); - if (method_exists($instance, 'requireDefaultRecords')) { - $instance->requireDefaultRecords(); - } - if (method_exists($instance, 'augmentDefaultRecords')) { - $instance->augmentDefaultRecords(); - } - } - - foreach ($fixtureFiles as $fixtureFilePath) { - $fixture = YamlFixture::create($fixtureFilePath); - $fixture->writeInto($this->getFixtureFactory()); - } - + /** @var FixtureTestState $fixtureState */ + $fixtureState = static::$state->getStateByName('fixtures'); + $this->setFixtureFactory($fixtureState->getFixtureFactory(static::class)); $this->logInWithPermission('ADMIN'); } @@ -393,24 +400,29 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly } /** - * @return FixtureFactory + * @deprecated 4.0..5.0 + * @return FixtureFactory|false */ public function getFixtureFactory() { - if (!$this->fixtureFactory) { - $this->fixtureFactory = Injector::inst()->create(FixtureFactory::class); - } - return $this->fixtureFactory; + Deprecation::notice('5.0', __FUNCTION__ . ' is deprecated, use ' . FixtureTestState::class . ' instead'); + /** @var FixtureTestState $state */ + $state = static::$state->getStateByName('fixtures'); + return $state->getFixtureFactory(static::class); } /** * Sets a new fixture factory - * + * @deprecated 4.0..5.0 * @param FixtureFactory $factory * @return $this */ public function setFixtureFactory(FixtureFactory $factory) { + Deprecation::notice('5.0', __FUNCTION__ . ' is deprecated, use ' . FixtureTestState::class . ' instead'); + /** @var FixtureTestState $state */ + $state = static::$state->getStateByName('fixtures'); + $state->setFixtureFactory($factory, static::class); $this->fixtureFactory = $factory; return $this; } @@ -476,11 +488,13 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly * Load a YAML fixture file into the database. * Once loaded, you can use idFromFixture() and objFromFixture() to get items from the fixture. * Doesn't clear existing fixtures. + * @deprecated 4.0...5.0 * * @param string $fixtureFile The location of the .yml fixture file, relative to the site base dir */ public function loadFixture($fixtureFile) { + Deprecation::notice('5.0', __FUNCTION__ . ' is deprecated, use ' . FixtureTestState::class . ' instead'); $fixture = Injector::inst()->create(YamlFixture::class, $fixtureFile); $fixture->writeInto($this->getFixtureFactory()); } @@ -973,7 +987,7 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly // Register state static::$state = SapphireTestState::singleton(); // Register temp DB holder - static::$tempDB = TempDatabase::create(); + static::tempDB(); } /** diff --git a/src/Dev/State/FixtureTestState.php b/src/Dev/State/FixtureTestState.php new file mode 100644 index 000000000..b4987aef6 --- /dev/null +++ b/src/Dev/State/FixtureTestState.php @@ -0,0 +1,229 @@ +testNeedsDB($test)) { + $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(); + } + } + $this->loadFixtures($test); + } + $tmpDB->startTransaction(); + } + } + + /** + * Called on tear down + * + * @param SapphireTest $test + */ + public function tearDown(SapphireTest $test) + { + if ($this->testNeedsDB($test)) { + $test::tempDB()->rollbackTransaction(); + } + } + + /** + * Called once on setup + * + * @param string $class Class being setup + */ + public function setUpOnce($class) + { + $this->fixtureFactories[strtolower($class)] = Injector::inst()->create(FixtureFactory::class); + } + + /** + * Called once on tear down + * + * @param string $class Class being torn down + */ + public function tearDownOnce($class) + { + unset($this->fixtureFactories[strtolower($class)]); + $class::tempDB()->clearAllData(); + } + + /** + * @param string $class + * + * @return bool|FixtureFactory + */ + public function getFixtureFactory($class) + { + $testClass = strtolower($class); + if (array_key_exists($testClass, $this->fixtureFactories)) { + return $this->fixtureFactories[$testClass]; + } + return false; + } + + /** + * @param FixtureFactory $factory + * @param string $class + */ + public function setFixtureFactory(FixtureFactory $factory, $class) + { + $this->fixtureFactories[strtolower($class)] = $factory; + } + + /** + * @param array $fixtures + * + * @param SapphireTest $test + * + * @return array + */ + protected function getFixturePaths($fixtures, SapphireTest $test) + { + return array_map(function ($fixtureFilePath) use ($test) { + return $this->resolveFixturePath($fixtureFilePath, $test); + }, $fixtures); + } + + /** + * @param SapphireTest $test + */ + protected function loadFixtures(SapphireTest $test) + { + $fixtures = $test::get_fixture_file(); + $fixtures = is_array($fixtures) ? $fixtures : [$fixtures]; + $paths = $this->getFixturePaths($fixtures, $test); + foreach ($paths as $fixtureFile) { + $this->loadFixture($fixtureFile, $test); + } + } + + /** + * @param string $fixtureFile + * @param SapphireTest $test + */ + protected function loadFixture($fixtureFile, SapphireTest $test) + { + /** @var YamlFixture $fixture */ + $fixture = Injector::inst()->create(YamlFixture::class, $fixtureFile); + $fixture->writeInto($this->getFixtureFactory(get_class($test))); + } + + /** + * Map a fixture path to a physical file + * + * @param string $fixtureFilePath + * @param SapphireTest $test + * + * @return string + */ + protected function resolveFixturePath($fixtureFilePath, SapphireTest $test) + { + // Support fixture paths relative to the test class, rather than relative to webroot + // String checking is faster than file_exists() calls. + $isRelativeToFile + = (strpos($fixtureFilePath, '/') === false) + || preg_match('/^(\.){1,2}/', $fixtureFilePath); + + if ($isRelativeToFile) { + $resolvedPath = realpath($this->getTestAbsolutePath($test) . '/' . $fixtureFilePath); + if ($resolvedPath) { + return $resolvedPath; + } + } + + // Check if file exists relative to base dir + $resolvedPath = realpath(Director::baseFolder() . '/' . $fixtureFilePath); + if ($resolvedPath) { + return $resolvedPath; + } + + return $fixtureFilePath; + } + + /** + * Useful for writing unit tests without hardcoding folder structures. + * + * @param SapphireTest $test + * + * @return string Absolute path to current class. + */ + protected function getTestAbsolutePath(SapphireTest $test) + { + $filename = ClassLoader::inst()->getItemPath(get_class($test)); + if (!$filename) { + throw new LogicException('getItemPath returned null for ' . static::class + . '. Try adding flush=1 to the test run.'); + } + return dirname($filename); + } + + /** + * @param SapphireTest $test + * + * @return bool + */ + protected function testNeedsDB(SapphireTest $test) + { + $annotations = $test->getAnnotations(); + + // annotation explicitly disables the DB + if (array_key_exists('useDatabase', $annotations['method']) + && $annotations['method']['useDatabase'][0] === 'false') { + return false; + } + + // annotation explicitly enables the DB + if (array_key_exists('useDatabase', $annotations['method']) + && $annotations['method']['useDatabase'][0] !== 'false') { + return true; + } + + // test class explicitly enables DB + if ($test->getUsesDatabase()) { + return true; + } + + // presence of fixture file implicitly enables DB + $fixtures = $test::get_fixture_file(); + if (!empty($fixtures)) { + return true; + } + + return false; + } +} diff --git a/src/Dev/State/SapphireTestState.php b/src/Dev/State/SapphireTestState.php index 3571a8006..f8dbcd3a4 100644 --- a/src/Dev/State/SapphireTestState.php +++ b/src/Dev/State/SapphireTestState.php @@ -22,6 +22,36 @@ class SapphireTestState implements TestState return $this->states; } + /** + * @param string $name + * + * @return bool|TestState + */ + public function getStateByName($name) + { + $states = $this->getStates(); + if (array_key_exists($name, $states)) { + return $states[$name]; + } + return false; + } + + /** + * @param string $class + * + * @return bool|TestState + */ + public function getStateByClass($class) + { + $lClass = strtolower($class); + foreach ($this->getStates() as $state) { + if ($lClass === strtolower(get_class($state))) { + return $state; + } + } + return false; + } + /** * @param TestState[] $states * @return $this diff --git a/src/ORM/Connect/TempDatabase.php b/src/ORM/Connect/TempDatabase.php index c41f98f78..832acd606 100644 --- a/src/ORM/Connect/TempDatabase.php +++ b/src/ORM/Connect/TempDatabase.php @@ -23,6 +23,11 @@ class TempDatabase */ protected $name = null; + /** + * @var bool If a transaction has been started + */ + protected $hasStarted = false; + /** * Create a new temp database * @@ -68,6 +73,46 @@ class TempDatabase return $this->isDBTemp($selected); } + /** + * @return bool + */ + public function hasStarted() + { + return $this->hasStarted; + } + + /** + * @return bool + */ + public function supportsTransactions() + { + return static::getConn()->supportsTransactions(); + } + + /** + * Start a transaction for easy rollback after tests + */ + public function startTransaction() + { + $this->hasStarted = true; + if (static::getConn()->supportsTransactions()) { + static::getConn()->transactionStart(); + } + } + + /** + * Rollback a transaction (or trash all data if the DB doesn't support databases + */ + public function rollbackTransaction() + { + if (static::getConn()->supportsTransactions()) { + static::getConn()->transactionRollback(); + } else { + $this->hasStarted = false; + static::clearAllData(); + } + } + /** * Destroy the current temp database */ @@ -102,6 +147,7 @@ class TempDatabase */ public function clearAllData() { + $this->hasStarted = false; if (!$this->isUsed()) { return; } @@ -183,6 +229,11 @@ class TempDatabase */ public function resetDBSchema(array $extraDataObjects = []) { + // pgsql doesn't allow schema updates inside transactions + // so we need to rollback any transactions before commencing a schema reset + if ($this->hasStarted()) { + $this->rollbackTransaction(); + } if (!$this->isUsed()) { return; } diff --git a/src/ORM/DataObjectSchema.php b/src/ORM/DataObjectSchema.php index 4a13d968f..e14821615 100644 --- a/src/ORM/DataObjectSchema.php +++ b/src/ORM/DataObjectSchema.php @@ -100,6 +100,7 @@ class DataObjectSchema * @param string $class Class name (not a table). * @param string $field Name of field that belongs to this class (or a parent class) * @param string $tablePrefix Optional prefix for table (alias) + * * @return string The SQL identifier string for the corresponding column for this field */ public function sqlColumnForField($class, $field, $tablePrefix = null) @@ -118,6 +119,7 @@ class DataObjectSchema * the name that would be used if this table did exist. * * @param string $class + * * @return string Returns the table name, or null if there is no table */ public function tableName($class) @@ -135,6 +137,7 @@ class DataObjectSchema * passed class. * * @param string|object $class + * * @return string * @throws InvalidArgumentException */ @@ -155,6 +158,7 @@ class DataObjectSchema * Get the base table * * @param string|object $class + * * @return string */ public function baseDataTable($class) @@ -185,6 +189,7 @@ class DataObjectSchema * - UNINHERITED Limit to only this table * - DB_ONLY Exclude virtual fields (such as composite fields), and only include fields with a db column. * - INCLUDE_CLASS Prefix the field specification with the class name in RecordClass.Column(spec) format. + * * @return array List of fields, where the key is the field name and the value is the field specification. */ public function fieldSpecs($classOrInstance, $options = 0) @@ -235,6 +240,7 @@ class DataObjectSchema * - UNINHERITED Limit to only this table * - DB_ONLY Exclude virtual fields (such as composite fields), and only include fields with a db column. * - INCLUDE_CLASS Prefix the field specification with the class name in RecordClass.Column(spec) format. + * * @return string|null Field will be a string in FieldClass(args) format, or * RecordClass.FieldClass(args) format if using INCLUDE_CLASS. Will be null if no field is found. */ @@ -248,6 +254,7 @@ class DataObjectSchema * Find the class for the given table * * @param string $table + * * @return string|null The FQN of the class, or null if not found */ public function tableClass($table) @@ -303,6 +310,7 @@ class DataObjectSchema * See dev/build errors for details in case of table name violation. * * @param string $class + * * @return string */ protected function buildTableName($class) @@ -334,6 +342,7 @@ class DataObjectSchema * * @param string $class Class name to query from * @param bool $aggregated Include fields in entire hierarchy, rather than just on this table + * * @return array Map of fieldname to specification, similiar to {@link DataObject::$db}. */ public function databaseFields($class, $aggregated = true) @@ -360,6 +369,7 @@ class DataObjectSchema * @param string $class Class name to query from * @param string $field Field name * @param bool $aggregated Include fields in entire hierarchy, rather than just on this table + * * @return string|null Field specification, or null if not a field */ public function databaseField($class, $field, $aggregated = true) @@ -392,6 +402,7 @@ class DataObjectSchema * Check if the given class has a table * * @param string $class + * * @return bool */ public function classHasTable($class) @@ -415,6 +426,7 @@ class DataObjectSchema * * @param string $class Name of class to check * @param bool $aggregated Include fields in entire hierarchy, rather than just on this table + * * @return array List of composite fields and their class spec */ public function compositeFields($class, $aggregated = true) @@ -442,6 +454,7 @@ class DataObjectSchema * @param string $class Class name to query from * @param string $field Field name * @param bool $aggregated Include fields in entire hierarchy, rather than just on this table + * * @return string|null Field specification, or null if not a field */ public function compositeField($class, $field, $aggregated = true) @@ -535,6 +548,7 @@ class DataObjectSchema * Get "default" database indexable field types * * @param string $class + * * @return array */ protected function cacheDefaultDatabaseIndexes($class) @@ -559,6 +573,7 @@ class DataObjectSchema * Look for custom indexes declared on the class * * @param string $class + * * @return array * @throws InvalidArgumentException If an index already exists on the class * @throws InvalidArgumentException If a custom index format is not valid @@ -637,6 +652,7 @@ class DataObjectSchema * Parses a specified column into a sort field and direction * * @param string $column String to parse containing the column name + * * @return array Resolved table and column. */ protected function parseSortColumn($column) @@ -659,6 +675,7 @@ class DataObjectSchema * * @param string $candidateClass * @param string $fieldName + * * @return string */ public function tableForField($candidateClass, $fieldName) @@ -677,6 +694,7 @@ class DataObjectSchema * * @param string $candidateClass * @param string $fieldName + * * @return string */ public function classForField($candidateClass, $fieldName) @@ -720,8 +738,10 @@ class DataObjectSchema * If the class name is 'ManyManyThroughList' then this is the name of the * has_many relation. * ) + * * @param string $class Name of class to get component for * @param string $component The component name + * * @return array|null */ public function manyManyComponent($class, $component) @@ -768,6 +788,7 @@ class DataObjectSchema * @param string $parentClass Name of class * @param string $component Name of relation on class * @param string $specification specification for this belongs_many_many + * * @return array Array with child class and relation name */ protected function parseBelongsManyManyComponent($parentClass, $component, $specification) @@ -802,7 +823,7 @@ class DataObjectSchema // Return relatios return [ 'childClass' => $childClass, - 'relationName' => $relationName + 'relationName' => $relationName, ]; } @@ -811,6 +832,7 @@ class DataObjectSchema * * @param string $class * @param string $component + * * @return array|null */ public function manyManyExtraFieldsForComponent($class, $component) @@ -845,6 +867,7 @@ class DataObjectSchema * @param string $component * @param bool $classOnly If this is TRUE, than any has_many relationships in the form * "ClassName.Field" will have the field data stripped off. It defaults to TRUE. + * * @return string|null */ public function hasManyComponent($class, $component, $classOnly = true) @@ -868,6 +891,7 @@ class DataObjectSchema * * @param string $class * @param string $component + * * @return string|null */ public function hasOneComponent($class, $component) @@ -890,6 +914,7 @@ class DataObjectSchema * @param string $component * @param bool $classOnly If this is TRUE, than any has_many relationships in the * form "ClassName.Field" will have the field data stripped off. It defaults to TRUE. + * * @return string|null */ public function belongsToComponent($class, $component, $classOnly = true) @@ -912,8 +937,10 @@ class DataObjectSchema * Check class for any unary component * * Alias for hasOneComponent() ?: belongsToComponent() + * * @param string $class * @param string $component + * * @return string|null */ public function unaryComponent($class, $component) @@ -926,6 +953,7 @@ class DataObjectSchema * @param string $parentClass Parent class name * @param string $component ManyMany name * @param string|array $specification Declaration of many_many relation type + * * @return array */ protected function parseManyManyComponent($parentClass, $component, $specification) @@ -975,6 +1003,7 @@ class DataObjectSchema * * @param string $childClass * @param string $parentClass + * * @return string|null */ protected function getManyManyInverseRelationship($childClass, $parentClass) @@ -992,10 +1021,10 @@ class DataObjectSchema if (is_array($manyManySpec)) { $toClass = $this->hasOneComponent($manyManySpec['through'], $manyManySpec['to']); if ($toClass === $parentClass) { - return $inverseComponentName; + return $inverseComponentName; + } } } - } return null; } @@ -1011,6 +1040,7 @@ class DataObjectSchema * remote object. * @param string $type the join type - either 'has_many' or 'belongs_to' * @param boolean $polymorphic Flag set to true if the remote join field is polymorphic. + * * @return string * @throws Exception */ @@ -1091,6 +1121,7 @@ class DataObjectSchema * @param string $joinClass Class for the joined table * @param array $specification Complete many_many specification * @param string $key Name of key to check ('from' or 'to') + * * @return string Class that matches the given relation * @throws InvalidArgumentException */ @@ -1152,6 +1183,7 @@ class DataObjectSchema * @param string $parentClass Name of parent class * @param string $component Name of many_many component * @param array $specification Complete many_many specification + * * @return string Name of join class */ protected function checkManyManyJoinClass($parentClass, $component, $specification)