From 4749caa26053c14ca66f676e82558d1203d866c1 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Wed, 14 Dec 2022 22:51:10 -0800 Subject: [PATCH 01/10] CiviUnitTestCase - Remove ancient/unused method 'getDataSet()' --- tests/phpunit/CiviTest/CiviUnitTestCase.php | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 304456c6e784..4ec5c81e5e33 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -311,12 +311,6 @@ protected function getConnection() { } - /** - * Required implementation of abstract method. - */ - protected function getDataSet() { - } - /** * @param bool $perClass * @param null $object From 6301a2ebc788fe7c8d8a1552d558e9f3fbb3c350 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Wed, 14 Dec 2022 22:54:15 -0800 Subject: [PATCH 02/10] CiviUnitTestCase - Remove ancient/unused property '$_dbName' --- tests/phpunit/CiviTest/CiviUnitTestCase.php | 9 --------- 1 file changed, 9 deletions(-) diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 4ec5c81e5e33..ec409a222ee9 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -90,13 +90,6 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { */ protected $_dbconn; - /** - * The database name. - * - * @var string - */ - static protected $_dbName; - /** * API version in use. * @@ -241,8 +234,6 @@ public function __construct($name = NULL, array $data = [], $dataName = '') { // we need full error reporting error_reporting(E_ALL & ~E_NOTICE); - self::$_dbName = self::getDBName(); - // also load the class loader require_once 'CRM/Core/ClassLoader.php'; CRM_Core_ClassLoader::singleton()->register(); From 7ded0866c8f86366757149ab342b94d63190c8a5 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Wed, 14 Dec 2022 22:55:47 -0800 Subject: [PATCH 03/10] CiviUnitTestCase - Remove unused method requireDBReset --- tests/phpunit/CiviTest/CiviUnitTestCase.php | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index ec409a222ee9..4c438deef8d2 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -261,13 +261,6 @@ protected function runTest() { } } - /** - * @return bool - */ - public function requireDBReset() { - return $this->DBResetRequired; - } - /** * @return string */ @@ -318,7 +311,7 @@ protected static function _populateDB($perClass = FALSE, &$object = NULL) { $dbreset = TRUE; } else { - $dbreset = $object->requireDBReset(); + $dbreset = $object->DBResetRequired; } if (self::$populateOnce || !$dbreset) { From 6fe119af61c81aaf2ce327118ebbe76ff1d4896e Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Wed, 14 Dec 2022 22:58:30 -0800 Subject: [PATCH 04/10] CiviUnitTestCase - Remove unused property $populateOnce The property is initialized to FALSE. It seems that the idea was to let some classes override this, but it is never used. --- tests/phpunit/CiviTest/CiviUnitTestCase.php | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 4c438deef8d2..feec9a0a2388 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -110,20 +110,6 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { */ protected $tempDirs; - /** - * @var bool - * populateOnce allows to skip db resets in setUp - * - * WARNING! USE WITH CAUTION - IT'LL RENDER DATA DEPENDENCIES - * BETWEEN TESTS WHEN RUN IN SUITE. SUITABLE FOR LOCAL, LIMITED - * "CHECK RUNS" ONLY! - * - * IF POSSIBLE, USE $this->DBResetRequired = FALSE IN YOUR TEST CASE! - * - * @see http://forum.civicrm.org/index.php/topic,18065.0.html - */ - public static $populateOnce = FALSE; - /** * DBResetRequired allows skipping DB reset * in specific test case. If you still need @@ -314,10 +300,9 @@ protected static function _populateDB($perClass = FALSE, &$object = NULL) { $dbreset = $object->DBResetRequired; } - if (self::$populateOnce || !$dbreset) { + if (!$dbreset) { return FALSE; } - self::$populateOnce = NULL; Civi\Test::data()->populate(); From e626ac36ee6b1d89ac73f35b7717fe37f52902d0 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Wed, 14 Dec 2022 23:02:11 -0800 Subject: [PATCH 05/10] CiviUnitTestCase - Remove unused property $_dbconn --- tests/phpunit/CiviTest/CiviUnitTestCase.php | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index feec9a0a2388..603331a564b0 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -83,13 +83,6 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { */ private static $dbInit = FALSE; - /** - * Database connection. - * - * @var PHPUnit_Extensions_Database_DB_IDatabaseConnection - */ - protected $_dbconn; - /** * API version in use. * @@ -339,7 +332,7 @@ protected function setUp(): void { } // Get and save a connection to the database - $this->_dbconn = $this->getConnection(); + $this->getConnection(); /* NOTE: Side-effects! */ // reload database before each test // $this->_populateDB(); From 8978e6b53b62c37e285ad50f7769127aaa14508b Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Wed, 14 Dec 2022 23:04:05 -0800 Subject: [PATCH 06/10] CiviUnitTestCase - Inline misleading function 'getConnection' This function returns a connection that nobody uses. It has only one caller. It's mostly interesting because it can reset the database. Hiding it under the name `getConnection` is misleading. --- tests/phpunit/CiviTest/CiviUnitTestCase.php | 32 +++++---------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 603331a564b0..0660d25b774d 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -254,26 +254,6 @@ public static function getDBName() { return $dbName; } - /** - * Create database connection for this instance. - * - * Initialize the test database if it hasn't been initialized - * - */ - protected function getConnection() { - if (!self::$dbInit) { - $dbName = self::getDBName(); - - // install test database - echo PHP_EOL . "Installing {$dbName} database" . PHP_EOL; - - static::_populateDB(FALSE, $this); - - self::$dbInit = TRUE; - } - - } - /** * @param bool $perClass * @param null $object @@ -331,11 +311,13 @@ protected function setUp(): void { exit(1); } - // Get and save a connection to the database - $this->getConnection(); /* NOTE: Side-effects! */ - - // reload database before each test - // $this->_populateDB(); + if (!self::$dbInit) { + $dbName = self::getDBName(); + // install test database + echo PHP_EOL . "Installing {$dbName} database" . PHP_EOL; + static::_populateDB(FALSE, $this); + self::$dbInit = TRUE; + } // "initialize" CiviCRM to avoid problems when running single tests // FIXME: look at it closer in second stage From 168ff84b7ace7381d844fefa1e127e6603e29726 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Wed, 14 Dec 2022 23:13:02 -0800 Subject: [PATCH 07/10] CiviUnitTestCase - Move UF check In practice, this check is the first thing done by `setUpBeforeClass()`. Might as well move it there. Technically, this check is also called by way of `setUp()`, but it's redundant at that point. (Constants are constants...) --- tests/phpunit/CiviTest/CiviUnitTestCase.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 0660d25b774d..eafb871002d8 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -262,10 +262,6 @@ public static function getDBName() { * TRUE if the populate logic runs; FALSE if it is skipped */ protected static function _populateDB($perClass = FALSE, &$object = NULL) { - if (CIVICRM_UF !== 'UnitTests') { - throw new \RuntimeException("_populateDB requires CIVICRM_UF=UnitTests"); - } - if ($perClass || $object == NULL) { $dbreset = TRUE; } @@ -287,6 +283,10 @@ protected static function _populateDB($perClass = FALSE, &$object = NULL) { } public static function setUpBeforeClass(): void { + if (CIVICRM_UF !== 'UnitTests') { + throw new \RuntimeException("CiviUnitTestCase requires CIVICRM_UF=UnitTests"); + } + static::_populateDB(TRUE); // also set this global hack From dd6530f69fec27d079cd192b3e174d8691a12af7 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Thu, 15 Dec 2022 00:26:46 -0800 Subject: [PATCH 08/10] CiviUnitTestCase - Extract method 'buildEnvironment'. Future target for method-overrides. --- tests/phpunit/CiviTest/CiviUnitTestCase.php | 24 +++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index eafb871002d8..06762cc51ab3 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -273,15 +273,27 @@ protected static function _populateDB($perClass = FALSE, &$object = NULL) { return FALSE; } - Civi\Test::data()->populate(); - - // `CiviUnitTestCase` has replaced the baseline DB configuration. To coexist with other - // tests based on `CiviEnvBuilder`, we need to store a signature marking the current DB configuration. - (new \Civi\Test\CiviEnvBuilder())->callback(function(){}, 'CiviUnitTestCase')->apply(TRUE); - + \Civi\Test::asPreInstall([static::CLASS, 'buildEnvironment'])->apply(TRUE); return TRUE; } + /** + * Declare the environment that we wish to run in. + * + * TODO: The hope is to get this to align with `Civi\Test::headless()` and perhaps + * assimilate other steps from 'setUp()'. The method is reserved while we look + * for the right split. However, when we're a little further along on that, this + * should be made overrideable. + * + * @return \Civi\Test\CiviEnvBuilder + */ + final public static function buildEnvironment(): \Civi\Test\CiviEnvBuilder { + // Ideally: return Civi\Test::headless(); + $b = new \Civi\Test\CiviEnvBuilder(); + $b->callback([\Civi\Test::data(), 'populate']); + return $b; + } + public static function setUpBeforeClass(): void { if (CIVICRM_UF !== 'UnitTests') { throw new \RuntimeException("CiviUnitTestCase requires CIVICRM_UF=UnitTests"); From 8cfba744faab400d5cf72cc2fad328f787127214 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Thu, 15 Dec 2022 00:31:45 -0800 Subject: [PATCH 09/10] CiviUnitTestCase - Inline method '_populateDB()' At this point, it's just a bunch of verbosely written conditions that appear to be trueisms; * When called via `setUpBeforeClass()`, the `$perClass` flag is TRUE, so it does the reset. * When called via `setUp()`, the `$object` flag is non-null, so it does the reset. At this point, the actual reset bit is 1-line. Easier to just call that. --- tests/phpunit/CiviTest/CiviUnitTestCase.php | 27 ++------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 06762cc51ab3..99484fe82e76 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -254,29 +254,6 @@ public static function getDBName() { return $dbName; } - /** - * @param bool $perClass - * @param null $object - * - * @return bool - * TRUE if the populate logic runs; FALSE if it is skipped - */ - protected static function _populateDB($perClass = FALSE, &$object = NULL) { - if ($perClass || $object == NULL) { - $dbreset = TRUE; - } - else { - $dbreset = $object->DBResetRequired; - } - - if (!$dbreset) { - return FALSE; - } - - \Civi\Test::asPreInstall([static::CLASS, 'buildEnvironment'])->apply(TRUE); - return TRUE; - } - /** * Declare the environment that we wish to run in. * @@ -299,7 +276,7 @@ public static function setUpBeforeClass(): void { throw new \RuntimeException("CiviUnitTestCase requires CIVICRM_UF=UnitTests"); } - static::_populateDB(TRUE); + \Civi\Test::asPreInstall([static::CLASS, 'buildEnvironment'])->apply(TRUE); // also set this global hack $GLOBALS['_PEAR_ERRORSTACK_OVERRIDE_CALLBACK'] = []; @@ -327,7 +304,7 @@ protected function setUp(): void { $dbName = self::getDBName(); // install test database echo PHP_EOL . "Installing {$dbName} database" . PHP_EOL; - static::_populateDB(FALSE, $this); + \Civi\Test::asPreInstall([static::CLASS, 'buildEnvironment'])->apply(TRUE); self::$dbInit = TRUE; } From 394772b5f714cdaaef5a8ca5169b2ceed1aba579 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Thu, 15 Dec 2022 00:36:04 -0800 Subject: [PATCH 10/10] CiviUnitTestCase - Simplify console message. Remove unused method 'getDBName()'. --- tests/phpunit/CiviTest/CiviUnitTestCase.php | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 99484fe82e76..5f4f9462f93a 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -240,20 +240,6 @@ protected function runTest() { } } - /** - * @return string - */ - public static function getDBName() { - static $dbName = NULL; - if ($dbName === NULL) { - require_once "DB.php"; - $dsn = CRM_Utils_SQL::autoSwitchDSN(CIVICRM_DSN); - $dsninfo = DB::parseDSN($dsn); - $dbName = $dsninfo['database']; - } - return $dbName; - } - /** * Declare the environment that we wish to run in. * @@ -301,9 +287,7 @@ protected function setUp(): void { } if (!self::$dbInit) { - $dbName = self::getDBName(); - // install test database - echo PHP_EOL . "Installing {$dbName} database" . PHP_EOL; + fprintf(STDERR, "\nInstalling %s database\n", \Civi\Test::dsn('database')); \Civi\Test::asPreInstall([static::CLASS, 'buildEnvironment'])->apply(TRUE); self::$dbInit = TRUE; }