From 0aa67025f25d30162309d59b4cbe133b6180b2d7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 16 May 2013 12:25:26 -0700 Subject: [PATCH] In unit test environments, install all applications Summary: Normalize the unit test environment by installing all applications. The immediate issue this fixes is that `testDropUnknownSenderMail` depends on Maniphest being installed. Some possible fixes are: # Don't rely on the Maniphest mail receiver for the test (e.g., write a stub/dummy/mock receiver). # Explicitly make sure Maniphest is installed before running the test. # Normalize the test environment to install all applications. I don't like (1) much because it turns a pretty good 10 line test into a bunch of stub classes or mock junk. I'll do it if we have more uses after a few more diffs, but so far running these tests against real code hasn't created a dependency mess and we get more coverage. I don't like (2) much because I think requiring tests to do this will do more harm than good. The number of issues we'll hypothetically uncover by exposing unrealized application interdependencies is probably very small or maybe zero, and they're probably all trivial. But tests with an undeclared but implicit dependency on an application (e.g., Differential tests depend on Differential) are common. So here's (3), which I think is reasonable. I also simplified some of this code a little bit, and moved the Application object cache one level down (this was sort of a bug -- installation status is variant across requests). Test Plan: Added unit test. Reviewers: wez, btrahan Reviewed By: wez CC: aran Differential Revision: https://secure.phabricator.com/D5938 --- .../base/PhabricatorApplication.php | 57 ++++++++----------- .../PhabricatorInfrastructureTestCase.php | 21 +++++-- .../testing/PhabricatorTestCase.php | 9 +++ 3 files changed, 49 insertions(+), 38 deletions(-) diff --git a/src/applications/base/PhabricatorApplication.php b/src/applications/base/PhabricatorApplication.php index f8e1ff8408..6dbcb37662 100644 --- a/src/applications/base/PhabricatorApplication.php +++ b/src/applications/base/PhabricatorApplication.php @@ -255,47 +255,40 @@ abstract class PhabricatorApplication { } public static function getAllApplications() { - $classes = id(new PhutilSymbolLoader()) - ->setAncestorClass(__CLASS__) - ->setConcreteOnly(true) - ->selectAndLoadSymbols(); - - $apps = array(); - - foreach ($classes as $class) { - $app = newv($class['name'], array()); - $apps[] = $app; - } - - // Reorder the applications into "application order". Notably, this ensures - // their event handlers register in application order. - $apps = msort($apps, 'getApplicationOrder'); - $apps = mgroup($apps, 'getApplicationGroup'); - $apps = array_select_keys($apps, self::getApplicationGroups()) + $apps; - $apps = array_mergev($apps); - - return $apps; - } - - public static function getAllInstalledApplications() { static $applications; - if (empty($applications)) { - $all_applications = self::getAllApplications(); - $apps = array(); - foreach ($all_applications as $app) { - if (!$app->isInstalled()) { - continue; - } + if ($applications === null) { + $apps = id(new PhutilSymbolLoader()) + ->setAncestorClass(__CLASS__) + ->loadObjects(); - $apps[] = $app; - } + // Reorder the applications into "application order". Notably, this + // ensures their event handlers register in application order. + $apps = msort($apps, 'getApplicationOrder'); + $apps = mgroup($apps, 'getApplicationGroup'); + $apps = array_select_keys($apps, self::getApplicationGroups()) + $apps; + $apps = array_mergev($apps); $applications = $apps; } + return $applications; } + public static function getAllInstalledApplications() { + $all_applications = self::getAllApplications(); + $apps = array(); + foreach ($all_applications as $app) { + if (!$app->isInstalled()) { + continue; + } + + $apps[] = $app; + } + + return $apps; + } + } diff --git a/src/infrastructure/__tests__/PhabricatorInfrastructureTestCase.php b/src/infrastructure/__tests__/PhabricatorInfrastructureTestCase.php index 60c4695798..70ba05bb4b 100644 --- a/src/infrastructure/__tests__/PhabricatorInfrastructureTestCase.php +++ b/src/infrastructure/__tests__/PhabricatorInfrastructureTestCase.php @@ -5,14 +5,23 @@ final class PhabricatorInfrastructureTestCase /** * This is more of an acceptance test case instead of a unittest. It verifies - * that all symbols can be loaded correctly. It can catch problem like missing - * methods in descendants of abstract base classes. + * that all symbols can be loaded correctly. It can catch problems like + * missing methods in descendants of abstract base classes. */ public function testEverythingImplemented() { - // Note that we don't have a try catch block around the following because, - // when it fails, it will cause a HPHP or PHP fatal which won't be caught - // by try catch. - $every_class = id(new PhutilSymbolLoader())->selectAndLoadSymbols(); + id(new PhutilSymbolLoader())->selectAndLoadSymbols(); } + + public function testApplicationsInstalled() { + $all = PhabricatorApplication::getAllApplications(); + $installed = PhabricatorApplication::getAllInstalledApplications(); + + $this->assertEqual( + count($all), + count($installed), + 'In test cases, all applications should default to installed.'); + } + + } diff --git a/src/infrastructure/testing/PhabricatorTestCase.php b/src/infrastructure/testing/PhabricatorTestCase.php index 12f18567bc..c2c6d72f08 100644 --- a/src/infrastructure/testing/PhabricatorTestCase.php +++ b/src/infrastructure/testing/PhabricatorTestCase.php @@ -87,6 +87,15 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase { } $this->env = PhabricatorEnv::beginScopedEnv(); + + // NOTE: While running unit tests, we act as though all applications are + // installed, regardless of the install's configuration. Tests which need + // to uninstall applications are responsible for adjusting state themselves + // (such tests are exceedingly rare). + + $this->env->overrideEnvConfig( + 'phabricator.uninstalled-applications', + array()); } protected function didRunTests() {