From 0f06287dc56436ab0f0748b48dcca0a421d330e1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Apr 2012 07:52:01 -0700 Subject: [PATCH] Allow PhabricatorEnv to be temporarily made mutable for unit tests Summary: Introduces a scope-guarded way to override the env config, for unit tests which are sensitive to config values. Test Plan: Ran unit tests. Reviewers: btrahan, vrana, jungejason Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2237 --- src/__phutil_library_map__.php | 1 + src/infrastructure/env/PhabricatorEnv.php | 242 +++++++++++++++--- .../env/PhabricatorScopedEnv.php | 72 ++++++ src/infrastructure/env/__init__.php | 1 + .../env/__tests__/PhabricatorEnvTestCase.php | 52 ++++ .../testing/testcase/PhabricatorTestCase.php | 17 +- .../testing/testcase/__init__.php | 1 + 7 files changed, 342 insertions(+), 44 deletions(-) create mode 100644 src/infrastructure/env/PhabricatorScopedEnv.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index dfbaedd6bb..631e4a71a3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -842,6 +842,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryType' => 'applications/repository/constants/repositorytype', 'PhabricatorS3FileStorageEngine' => 'applications/files/engine/s3', 'PhabricatorSQLPatchList' => 'infrastructure/setup/sql', + 'PhabricatorScopedEnv' => 'infrastructure/env', 'PhabricatorSearchAbstractDocument' => 'applications/search/index/abstractdocument', 'PhabricatorSearchAttachController' => 'applications/search/controller/attach', 'PhabricatorSearchBaseController' => 'applications/search/controller/base', diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php index cda86bb545..015b967816 100644 --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -17,58 +17,98 @@ */ /** - * @task uri URI Validation + * Manages the execution environment configuration, exposing APIs to read + * configuration settings and other similar values that are derived directly + * from configuration settings. + * + * + * = Reading Configuration = + * + * The primary role of this class is to provide an API for reading + * Phabricator configuration, @{method:getEnvConfig}: + * + * $value = PhabricatorEnv::getEnvConfig('some.key', $default); + * + * The class also handles some URI construction based on configuration, via + * the methods @{method:getURI}, @{method:getProductionURI}, + * @{method:getCDNURI}, and @{method:getDoclink}. + * + * For configuration which allows you to choose a class to be responsible for + * some functionality (e.g., which mail adapter to use to deliver email), + * @{method:newObjectFromConfig} provides a simple interface that validates + * the configured value. + * + * + * = Unit Test Support = + * + * In unit tests, you can use @{method:beginScopedEnv} to create a temporary, + * mutable environment. The method returns a scope guard object which restores + * the environment when it is destroyed. For example: + * + * public function testExample() { + * $env = PhabricatorEnv::beginScopedEnv(); + * $env->overrideEnv('some.key', 'new-value-for-this-test'); + * + * // Some test which depends on the value of 'some.key'. + * + * } + * + * Your changes will persist until the `$env` object leaves scope or is + * destroyed. + * + * You should //not// use this in normal code. + * + * + * @task read Reading Configuration + * @task uri URI Validation + * @task test Unit Test Support + * @task internal Internals */ final class PhabricatorEnv { + private static $env; + private static $stack = array(); - public static function setEnvConfig(array $config) { - self::$env = $config; - } +/* -( Reading Configuration )---------------------------------------------- */ + + + /** + * Get the current configuration setting for a given key. + * + * @task read + */ public static function getEnvConfig($key, $default = null) { + + // If we have environment overrides via beginScopedEnv(), check them for + // the key first. + if (self::$stack) { + foreach (array_reverse(self::$stack) as $override) { + if (array_key_exists($key, $override)) { + return $override[$key]; + } + } + } + return idx(self::$env, $key, $default); } - public static function newObjectFromConfig($key, $args = array()) { - $class = self::getEnvConfig($key); - $object = newv($class, $args); - $instanceof = idx(self::getRequiredClasses(), $key); - if (!($object instanceof $instanceof)) { - throw new Exception("Config setting '$key' must be an instance of ". - "'$instanceof', is '".get_class($object)."'."); - } - return $object; - } - - public static function getRequiredClasses() { - return array( - 'metamta.mail-adapter' => 'PhabricatorMailImplementationAdapter', - 'metamta.maniphest.reply-handler' => 'PhabricatorMailReplyHandler', - 'metamta.differential.reply-handler' => 'PhabricatorMailReplyHandler', - 'metamta.diffusion.reply-handler' => 'PhabricatorMailReplyHandler', - 'storage.engine-selector' => 'PhabricatorFileStorageEngineSelector', - 'search.engine-selector' => 'PhabricatorSearchEngineSelector', - 'differential.field-selector' => 'DifferentialFieldSelector', - 'maniphest.custom-task-extensions-class' => 'ManiphestTaskExtensions', - 'aphront.default-application-configuration-class' => - 'AphrontApplicationConfiguration', - 'controller.oauth-registration' => - 'PhabricatorOAuthRegistrationController', - 'mysql.implementation' => 'AphrontMySQLDatabaseConnectionBase', - 'differential.attach-task-class' => 'DifferentialTasksAttacher', - 'mysql.configuration-provider' => 'DatabaseConfigurationProvider', - ); - } - - public static function envConfigExists($key) { - return array_key_exists($key, self::$env); - } + /** + * Get the fully-qualified URI for a path. + * + * @task read + */ public static function getURI($path) { return rtrim(self::getEnvConfig('phabricator.base-uri'), '/').$path; } + + /** + * Get the fully-qualified production URI for a path. + * + * @task read + */ public static function getProductionURI($path) { // If we're passed a URI which already has a domain, simply return it // unmodified. In particular, files may have URIs which point to a CDN @@ -85,6 +125,12 @@ final class PhabricatorEnv { return rtrim($production_domain, '/').$path; } + + /** + * Get the fully-qualified production URI for a static resource path. + * + * @task read + */ public static function getCDNURI($path) { $alt = self::getEnvConfig('security.alternate-file-domain'); if (!$alt) { @@ -95,15 +141,70 @@ final class PhabricatorEnv { return (string)$uri; } - public static function getAllConfigKeys() { - return self::$env; - } + /** + * Get the fully-qualified production URI for a documentation resource. + * + * @task read + */ public static function getDoclink($resource) { return 'http://www.phabricator.com/docs/phabricator/'.$resource; } + /** + * Build a concrete object from a configuration key. + * + * @task read + */ + public static function newObjectFromConfig($key, $args = array()) { + $class = self::getEnvConfig($key); + $object = newv($class, $args); + $instanceof = idx(self::getRequiredClasses(), $key); + if (!($object instanceof $instanceof)) { + throw new Exception("Config setting '$key' must be an instance of ". + "'$instanceof', is '".get_class($object)."'."); + } + return $object; + } + + +/* -( Unit Test Support )-------------------------------------------------- */ + + + /** + * @task test + */ + public static function beginScopedEnv() { + return new PhabricatorScopedEnv(self::pushEnvironment()); + } + + + /** + * @task test + */ + private static function pushEnvironment() { + self::$stack[] = array(); + return last_key(self::$stack); + } + + + /** + * @task test + */ + public static function popEnvironment($key) { + $stack_key = last_key(self::$stack); + + array_pop(self::$stack); + + if ($stack_key !== $key) { + throw new Exception( + "Scoped environments were destroyed in a diffent order than they ". + "were initialized."); + } + } + + /* -( URI Validation )----------------------------------------------------- */ @@ -181,4 +282,63 @@ final class PhabricatorEnv { return true; } + +/* -( Internals )---------------------------------------------------------- */ + + + /** + * @task internal + */ + public static function setEnvConfig(array $config) { + self::$env = $config; + } + + + /** + * @task internal + */ + public static function getRequiredClasses() { + return array( + 'metamta.mail-adapter' => 'PhabricatorMailImplementationAdapter', + 'metamta.maniphest.reply-handler' => 'PhabricatorMailReplyHandler', + 'metamta.differential.reply-handler' => 'PhabricatorMailReplyHandler', + 'metamta.diffusion.reply-handler' => 'PhabricatorMailReplyHandler', + 'storage.engine-selector' => 'PhabricatorFileStorageEngineSelector', + 'search.engine-selector' => 'PhabricatorSearchEngineSelector', + 'differential.field-selector' => 'DifferentialFieldSelector', + 'maniphest.custom-task-extensions-class' => 'ManiphestTaskExtensions', + 'aphront.default-application-configuration-class' => + 'AphrontApplicationConfiguration', + 'controller.oauth-registration' => + 'PhabricatorOAuthRegistrationController', + 'mysql.implementation' => 'AphrontMySQLDatabaseConnectionBase', + 'differential.attach-task-class' => 'DifferentialTasksAttacher', + 'mysql.configuration-provider' => 'DatabaseConfigurationProvider', + ); + } + + + /** + * @task internal + */ + public static function envConfigExists($key) { + return array_key_exists($key, self::$env); + } + + + /** + * @task internal + */ + public static function getAllConfigKeys() { + return self::$env; + } + + + /** + * @task internal + */ + public static function overrideEnvConfig($stack_key, $key, $value) { + self::$stack[$stack_key][$key] = $value; + } + } diff --git a/src/infrastructure/env/PhabricatorScopedEnv.php b/src/infrastructure/env/PhabricatorScopedEnv.php new file mode 100644 index 0000000000..4e92ac2bbe --- /dev/null +++ b/src/infrastructure/env/PhabricatorScopedEnv.php @@ -0,0 +1,72 @@ +key, + $key, + $value); + return $this; + } + + +/* -( Internals )---------------------------------------------------------- */ + + + /** + * + * @task internal + */ + public function __construct($stack_key) { + $this->key = $stack_key; + } + + + /** + * Release the scoped environment. + * + * @return void + * @task internal + */ + public function __destruct() { + PhabricatorEnv::popEnvironment($this->key); + } + +} diff --git a/src/infrastructure/env/__init__.php b/src/infrastructure/env/__init__.php index cbda9af309..aa808e0b8d 100644 --- a/src/infrastructure/env/__init__.php +++ b/src/infrastructure/env/__init__.php @@ -11,3 +11,4 @@ phutil_require_module('phutil', 'utils'); phutil_require_source('PhabricatorEnv.php'); +phutil_require_source('PhabricatorScopedEnv.php'); diff --git a/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php index 9d2b367332..a5b1814285 100644 --- a/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php +++ b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php @@ -53,4 +53,56 @@ final class PhabricatorEnvTestCase extends PhabricatorTestCase { "Valid remote resource: {$uri}"); } } + + public function testOverrides() { + $outer = PhabricatorEnv::beginScopedEnv(); + $outer->overrideEnvConfig('test.value', 1); + $this->assertEqual(1, PhabricatorEnv::getEnvConfig('test.value')); + + $inner = PhabricatorEnv::beginScopedEnv(); + $inner->overrideEnvConfig('test.value', 2); + $this->assertEqual(2, PhabricatorEnv::getEnvConfig('test.value')); + unset($inner); + + $this->assertEqual(1, PhabricatorEnv::getEnvConfig('test.value')); + unset($outer); + } + + public function testOverrideOrder() { + $outer = PhabricatorEnv::beginScopedEnv(); + $middle = PhabricatorEnv::beginScopedEnv(); + $inner = PhabricatorEnv::beginScopedEnv(); + + $caught = null; + try { + unset($middle); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->assertEqual( + true, + $caught instanceof Exception, + "Destroying a scoped environment which is not on the top of the stack ". + "should throw."); + + $caught = null; + try { + unset($inner); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->assertEqual( + true, + $caught instanceof Exception, + "Destroying a scoped environment which is not on the top of the stack ". + "should throw."); + + // Although we popped the other two out-of-order, we still expect to end + // up in the right state after handling the exceptions, so this should + // execute without issues. + unset($outer); + } + } diff --git a/src/infrastructure/testing/testcase/PhabricatorTestCase.php b/src/infrastructure/testing/testcase/PhabricatorTestCase.php index 6b4d274195..b5e2e1e26f 100644 --- a/src/infrastructure/testing/testcase/PhabricatorTestCase.php +++ b/src/infrastructure/testing/testcase/PhabricatorTestCase.php @@ -1,7 +1,7 @@ getPhabricatorTestCaseConfiguration() + array( - self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK => true, + self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK => true, ); } @@ -51,6 +52,8 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase { if ($config[self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK]) { LiskDAO::beginIsolateAllLiskEffectsToCurrentProcess(); } + + $this->env = PhabricatorEnv::beginScopedEnv(); } protected function didRunTests() { @@ -59,6 +62,14 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase { if ($config[self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK]) { LiskDAO::endIsolateAllLiskEffectsToCurrentProcess(); } + + try { + unset($this->env); + } catch (Exception $ex) { + throw new Exception( + "Some test called PhabricatorEnv::beginScopedEnv(), but is still ". + "holding a reference to the scoped environment!"); + } } } diff --git a/src/infrastructure/testing/testcase/__init__.php b/src/infrastructure/testing/testcase/__init__.php index 37b9e3cd2a..79fa8d94f5 100644 --- a/src/infrastructure/testing/testcase/__init__.php +++ b/src/infrastructure/testing/testcase/__init__.php @@ -8,6 +8,7 @@ phutil_require_module('arcanist', 'unit/engine/phutil/testcase'); +phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'storage/lisk/dao'); phutil_require_module('phutil', 'moduleutils');