From 6419eb1a026474d312abfb2efd8d0f4d9a7a4261 Mon Sep 17 00:00:00 2001
From: Patrick Radtke <patrick@cirrusidentity.com>
Date: Mon, 24 Apr 2017 11:02:39 -0700
Subject: [PATCH] Reduce state spill over between tests
A PHPUnit listener unsets SSP environmental variables and attempts
to restore globals.
---
lib/SimpleSAML/Configuration.php | 15 +++-
lib/SimpleSAML/Utils/ClearableState.php | 17 ++++
tests/Utils/ClearStateTestListener.php | 78 +++++++++++++++++++
tests/config/no-options/config.php | 8 ++
tests/lib/SimpleSAML/ConfigurationTest.php | 10 +++
.../SimpleSAML/Locale/LocalizationTest.php | 5 ++
.../consent/lib/Auth/Process/ConsentTest.php | 3 +
.../lib/Auth/Source/Auth_Source_SP_Test.php | 1 +
tools/phpunit/phpunit.xml | 4 +
9 files changed, 139 insertions(+), 2 deletions(-)
create mode 100644 lib/SimpleSAML/Utils/ClearableState.php
create mode 100644 tests/Utils/ClearStateTestListener.php
create mode 100644 tests/config/no-options/config.php
diff --git a/lib/SimpleSAML/Configuration.php b/lib/SimpleSAML/Configuration.php
index 4370473b6..e2a9fdbc1 100644
--- a/lib/SimpleSAML/Configuration.php
+++ b/lib/SimpleSAML/Configuration.php
@@ -7,7 +7,7 @@
* @author Andreas Aakre Solberg, UNINETT AS. <andreas.solberg@uninett.no>
* @package SimpleSAMLphp
*/
-class SimpleSAML_Configuration
+class SimpleSAML_Configuration implements \SimpleSAML\Utils\ClearableState
{
/**
@@ -309,7 +309,6 @@ class SimpleSAML_Configuration
} catch (SimpleSAML\Error\ConfigurationError $e) {
throw \SimpleSAML\Error\CriticalConfigurationError::fromException($e);
}
-
}
throw new \SimpleSAML\Error\CriticalConfigurationError(
@@ -1366,4 +1365,16 @@ class SimpleSAML_Configuration
return null;
}
}
+
+ /**
+ * Clear any configuration information cached.
+ * Allows for configuration files to be changed and reloaded during a given request. Most useful
+ * when running phpunit tests and needing to alter config.php between test cases
+ */
+ public static function clearInternalState()
+ {
+ self::$configDirs = array();
+ self::$instance = array();
+ self::$loadedConfigs = array();
+ }
}
diff --git a/lib/SimpleSAML/Utils/ClearableState.php b/lib/SimpleSAML/Utils/ClearableState.php
new file mode 100644
index 000000000..b28554c6c
--- /dev/null
+++ b/lib/SimpleSAML/Utils/ClearableState.php
@@ -0,0 +1,17 @@
+<?php
+
+namespace SimpleSAML\Utils;
+
+/**
+ * Indicates an implementation caches state internally and may be cleared.
+ *
+ * Primarily designed to allow SSP state to be cleared between unit tests.
+ * @package SimpleSAML\Utils
+ */
+interface ClearableState
+{
+ /**
+ * Clear any cached internal state.
+ */
+ public static function clearInternalState();
+}
diff --git a/tests/Utils/ClearStateTestListener.php b/tests/Utils/ClearStateTestListener.php
new file mode 100644
index 000000000..327af5a9e
--- /dev/null
+++ b/tests/Utils/ClearStateTestListener.php
@@ -0,0 +1,78 @@
+<?php
+namespace SimpleSAML\Test\Utils;
+
+use PHPUnit_Framework_Test;
+use PHPUnit_Framework_TestSuite;
+
+/**
+ * A PHPUnit test listener that attempts to clear global state and cached SSP configuration between test run
+ */
+class ClearStateTestListener extends \PHPUnit_Framework_BaseTestListener
+{
+
+ /**
+ * Global state to restore between test runs
+ * @var array
+ */
+ private static $backups = array();
+
+ /**
+ * Class that implement \SimpleSAML\Utils\ClearableState and should have clearInternalState called between tests
+ * @var array
+ */
+ private static $clearableState = array('SimpleSAML_Configuration');
+
+ /**
+ * Variables
+ * @var array
+ */
+ private static $vars_to_unset = array('SIMPLESAMLPHP_CONFIG_DIR');
+
+ public function __construct()
+ {
+ // Backup any state that is needed as part of processing, so we can restore it later.
+ // TODO: phpunit's backupGlobals = false, yet we are trying to do a similar thing here. Is that an issue?
+ if (!self::$backups) {
+ self::$backups['$_COOKIE'] = $_COOKIE;
+ self::$backups['$_ENV'] = $_ENV;
+ self::$backups['$_FILES'] = $_FILES;
+ self::$backups['$_GET'] = $_GET;
+ self::$backups['$_POST'] = $_POST;
+ self::$backups['$_SERVER'] = $_SERVER;
+ self::$backups['$_SESSION'] = isset($_SESSION) ? $_SESSION : [];
+ self::$backups['$_REQUEST'] = $_REQUEST;
+ }
+ }
+
+ /**
+ * Clear any state needed prior to a test file running
+ */
+ public function startTestSuite(PHPUnit_Framework_TestSuite $suite)
+ {
+ // TODO: decide how to handle tests that want to set suite level settings with setUpBeforeClass()
+ }
+
+ /**
+ * Clear any state needed prior to a test case
+ * @param PHPUnit_Framework_Test $test
+ */
+ public function startTest(PHPUnit_Framework_Test $test)
+ {
+ $_COOKIE = self::$backups['$_COOKIE'];
+ $_ENV = self::$backups['$_ENV'];
+ $_FILES = self::$backups['$_FILES'];
+ $_GET = self::$backups['$_GET'];
+ $_POST = self::$backups['$_POST'];
+ $_SERVER = self::$backups['$_SERVER'];
+ $_SESSION = self::$backups['$_SESSION'];
+ $_REQUEST = self::$backups['$_REQUEST'];
+
+ foreach (self::$clearableState as $var) {
+ $var::clearInternalState();
+ }
+
+ foreach (self::$vars_to_unset as $var) {
+ putenv($var);
+ }
+ }
+}
diff --git a/tests/config/no-options/config.php b/tests/config/no-options/config.php
new file mode 100644
index 000000000..dddd7364c
--- /dev/null
+++ b/tests/config/no-options/config.php
@@ -0,0 +1,8 @@
+<?php
+/* A configuration desigend for tests that need no configuration options set yet the SSP requires a config.php to have
+been loaded.
+*/
+$config = array(
+ // We need to set at least one key=value pair to avoid validation issues loading the file
+ 'some_example_option' => 'a',
+);
\ No newline at end of file
diff --git a/tests/lib/SimpleSAML/ConfigurationTest.php b/tests/lib/SimpleSAML/ConfigurationTest.php
index 129f658cf..e93e96e2a 100644
--- a/tests/lib/SimpleSAML/ConfigurationTest.php
+++ b/tests/lib/SimpleSAML/ConfigurationTest.php
@@ -31,6 +31,13 @@ class Test_SimpleSAML_Configuration extends PHPUnit_Framework_TestCase
*/
public function testCriticalConfigurationError()
{
+ try {
+ SimpleSAML_Configuration::getInstance();
+ $this->fail('Exception expected');
+ } catch (\SimpleSAML\Error\CriticalConfigurationError $var) {
+ // This exception is expected.
+ }
+ //TODO: not sure this is correct. First call to getInstance() throw exception and the 2nd works?...
$c = SimpleSAML_Configuration::getInstance();
$this->assertNotEmpty($c->toArray());
}
@@ -97,6 +104,9 @@ class Test_SimpleSAML_Configuration extends PHPUnit_Framework_TestCase
* Test SimpleSAML_Configuration::getBaseURL()
*/
public function testGetBaseURL() {
+
+ // Need to set a configuration file because the SSP Logger attempts to load it.
+ putenv('SIMPLESAMLPHP_CONFIG_DIR=tests/config/no-options');
$c = SimpleSAML_Configuration::loadFromArray(array());
$this->assertEquals($c->getBaseURL(), 'simplesaml/');
diff --git a/tests/lib/SimpleSAML/Locale/LocalizationTest.php b/tests/lib/SimpleSAML/Locale/LocalizationTest.php
index f94d1c01b..c852d0fce 100644
--- a/tests/lib/SimpleSAML/Locale/LocalizationTest.php
+++ b/tests/lib/SimpleSAML/Locale/LocalizationTest.php
@@ -6,6 +6,11 @@ use SimpleSAML\Locale\Localization;
class LocalizationTest extends \PHPUnit_Framework_TestCase
{
+ protected function setUp()
+ {
+ // Localization/Language code attempts to load a cookie, and looks to config.php for a name of the cookie
+ putenv('SIMPLESAMLPHP_CONFIG_DIR=tests/config/no-options');
+ }
/**
diff --git a/tests/modules/consent/lib/Auth/Process/ConsentTest.php b/tests/modules/consent/lib/Auth/Process/ConsentTest.php
index e83659d22..cf63fe675 100644
--- a/tests/modules/consent/lib/Auth/Process/ConsentTest.php
+++ b/tests/modules/consent/lib/Auth/Process/ConsentTest.php
@@ -37,6 +37,9 @@ class ConsentTest extends \PHPUnit_Framework_TestCase
*/
public function testCheckDisable()
{
+ // Consent code path attempts to load metadata, which tries to find a config.php
+ putenv('SIMPLESAMLPHP_CONFIG_DIR=tests/config/no-options');
+
// test consent disable regex with match
$config = array();
diff --git a/tests/modules/saml/lib/Auth/Source/Auth_Source_SP_Test.php b/tests/modules/saml/lib/Auth/Source/Auth_Source_SP_Test.php
index bb3d065db..b9b07c3c9 100644
--- a/tests/modules/saml/lib/Auth/Source/Auth_Source_SP_Test.php
+++ b/tests/modules/saml/lib/Auth/Source/Auth_Source_SP_Test.php
@@ -125,6 +125,7 @@ class SP_Test extends \PHPUnit_Framework_TestCase
);
$this->config = Configuration::loadFromArray(array(), '[ARRAY]', 'simplesaml');
+
}
diff --git a/tools/phpunit/phpunit.xml b/tools/phpunit/phpunit.xml
index d0233b23a..4d455eb10 100644
--- a/tools/phpunit/phpunit.xml
+++ b/tools/phpunit/phpunit.xml
@@ -31,4 +31,8 @@
<log type="coverage-html" target="build/coverage" title="PHP Coveralls" charset="UTF-8" yui="true" highlight="true" lowUpperBound="35" highLowerBound="70"/>
<log type="coverage-clover" target="build/logs/clover.xml"/>
</logging>
+ <listeners>
+ <listener class="\SimpleSAML\Test\Utils\ClearStateTestListener" file="./tests/Utils/ClearStateTestListener.php">
+ </listener>
+ </listeners>
</phpunit>
--
GitLab