From 29e0183a56119956ac04fe6ffe2436265fa940d2 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Wed, 7 Aug 2019 14:40:55 +0100 Subject: [PATCH] phpunit: Repair GLOBALS reset in MediaWikiUnitTestCase MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This code didn't work because the $GLOBALS array is exposed by reference. Once this reference was broken by unset(), the rest just manipulated a local array that happens to be called "GLOBALS". It must not be unset or re-assigned. It can only be changed in-place. Before this, the execution of a MediaWikiUnitTestCase test stored a copy of GLOBALS in unitGlobals, then lost the GLOBALS pointer and created a new variable called "GLOBALS". As such, the tearDown() function didn't do what it meant to do, either – which then results in odd failures like T230023 Rewrite it as follows: * In setup, store the current GLOBALS keys and values, then reduce GLOBALS to only the whitelisted keys and values. * In teardown, restore the original state. * As optimisation, do this from setUpBeforeClass as well, so that there are relatively few globals to reset between tests. (Thanks @Simetrical!) The following tests were previously passing by accident under MediaWikiUnitTestCase but actually did depend on global config. * MainSlotRoleHandlerTest (…, ContentHandler, $wgContentHandlers) * SlotRecordTest (…, ContentHandler, $wgContentHandlers) * WikiReferenceTest (wfParseUrl, $wgUrlProtocols) * DifferenceEngineSlotDiffRendererTest (DifferenceEngine, wfDebug, …) * SlotDiffRendererTest (…, ContentHandler, $wgContentHandlers) * FileBackendDBRepoWrapperTest (wfWikiID, "Backend domain ID not provided") * JpegMetadataExtractorTest (…, wfDebug, …, LoggerFactory, …) * ParserFactoryTest (…, wfDebug, …, LoggerFactory, InvalidArgumentException) * MediaWikiPageNameNormalizerTest (…, wfDebug, …, LoggerFactory, …) * SiteExporterTest (SiteImporter, wfLogWarning, …) * SiteImporterTest (Site::newForType, $wgSiteTypes) * ZipDirectoryReaderTest (…, wfDebug, …, LoggerFactory, …) Bug: T230023 Change-Id: Ic22075bb5e81b7c2c4c1b8647547aa55306a10a7 --- tests/common/TestSetup.php | 17 ++++ tests/phpunit/MediaWikiUnitTestCase.php | 85 ++++++++++++++++--- tests/phpunit/bootstrap.php | 14 ++- .../Revision/MainSlotRoleHandlerTest.php | 3 +- .../includes/Revision/SlotRecordTest.php | 3 +- .../{unit => }/includes/WikiReferenceTest.php | 2 +- .../DifferenceEngineSlotDiffRendererTest.php | 2 +- .../includes/diff/SlotDiffRendererTest.php | 2 +- .../filerepo/FileBackendDBRepoWrapperTest.php | 2 +- .../media/JpegMetadataExtractorTest.php | 4 +- .../includes/parser/ParserFactoryTest.php | 2 +- .../site/MediaWikiPageNameNormalizerTest.php | 2 +- .../includes/site/SiteExporterTest.php | 4 +- .../includes/site/SiteImporterTest.php | 2 +- .../includes/site/SiteImporterTest.xml | 0 .../includes/utils/ZipDirectoryReaderTest.php | 5 +- tests/phpunit/phpunit.php | 7 ++ 17 files changed, 116 insertions(+), 40 deletions(-) rename tests/phpunit/{unit => }/includes/Revision/MainSlotRoleHandlerTest.php (96%) rename tests/phpunit/{unit => }/includes/Revision/SlotRecordTest.php (99%) rename tests/phpunit/{unit => }/includes/WikiReferenceTest.php (98%) rename tests/phpunit/{unit => }/includes/diff/DifferenceEngineSlotDiffRendererTest.php (94%) rename tests/phpunit/{unit => }/includes/diff/SlotDiffRendererTest.php (97%) rename tests/phpunit/{unit => }/includes/filerepo/FileBackendDBRepoWrapperTest.php (98%) rename tests/phpunit/{unit => }/includes/media/JpegMetadataExtractorTest.php (97%) rename tests/phpunit/{unit => }/includes/parser/ParserFactoryTest.php (91%) rename tests/phpunit/{unit => }/includes/site/MediaWikiPageNameNormalizerTest.php (97%) rename tests/phpunit/{unit => }/includes/site/SiteExporterTest.php (97%) rename tests/phpunit/{unit => }/includes/site/SiteImporterTest.php (98%) rename tests/phpunit/{unit => }/includes/site/SiteImporterTest.xml (100%) rename tests/phpunit/{unit => }/includes/utils/ZipDirectoryReaderTest.php (92%) diff --git a/tests/common/TestSetup.php b/tests/common/TestSetup.php index d4df8ae5ee..141e307fbe 100644 --- a/tests/common/TestSetup.php +++ b/tests/common/TestSetup.php @@ -4,6 +4,23 @@ * Common code for test environment initialisation and teardown */ class TestSetup { + public static $bootstrapGlobals; + + /** + * For use in MediaWikiUnitTestCase. + * + * This should be called before DefaultSettings.php or Setup.php loads. + */ + public static function snapshotGlobals() { + self::$bootstrapGlobals = []; + foreach ( $GLOBALS as $key => $_ ) { + // Support: HHVM (avoid self-ref) + if ( $key !== 'GLOBALS' ) { + self::$bootstrapGlobals[ $key ] =& $GLOBALS[$key]; + } + } + } + /** * This should be called before Setup.php, e.g. from the finalSetup() method * of a Maintenance subclass diff --git a/tests/phpunit/MediaWikiUnitTestCase.php b/tests/phpunit/MediaWikiUnitTestCase.php index bb018aaf14..3f876ae4aa 100644 --- a/tests/phpunit/MediaWikiUnitTestCase.php +++ b/tests/phpunit/MediaWikiUnitTestCase.php @@ -34,30 +34,89 @@ abstract class MediaWikiUnitTestCase extends TestCase { use MediaWikiCoversValidator; use MediaWikiTestCaseTrait; - private $unitGlobals = []; + private static $originalGlobals; + private static $unitGlobals; - protected function setUp() { - parent::setUp(); - $reflection = new ReflectionClass( $this ); + public static function setUpBeforeClass() { + parent::setUpBeforeClass(); + + $reflection = new ReflectionClass( static::class ); $dirSeparator = DIRECTORY_SEPARATOR; if ( stripos( $reflection->getFilename(), "${dirSeparator}unit${dirSeparator}" ) === false ) { - $this->fail( 'This unit test needs to be in "tests/phpunit/unit"!' ); + self::fail( 'This unit test needs to be in "tests/phpunit/unit"!' ); + } + + if ( defined( 'HHVM_VERSION' ) ) { + // There are a number of issues we encountered in trying to make this + // work on HHVM. Specifically, once an MediaWikiIntegrationTestCase executes + // before us, the original globals go missing. This might have to do with + // one of the non-unit tests passing GLOBALS somewhere and causing HHVM + // to get confused somehow. + return; + } + + self::$unitGlobals =& TestSetup::$bootstrapGlobals; + // The autoloader may change between bootstrap and the first test, + // so (lazily) capture these here instead. + self::$unitGlobals['wgAutoloadClasses'] =& $GLOBALS['wgAutoloadClasses']; + self::$unitGlobals['wgAutoloadLocalClasses'] =& $GLOBALS['wgAutoloadLocalClasses']; + // This value should always be true. + self::$unitGlobals['wgAutoloadAttemptLowercase'] = true; + + // Would be nice if we coud simply replace $GLOBALS as a whole, + // but unsetting or re-assigning that breaks the reference of this magic + // variable. Thus we have to modify it in place. + self::$originalGlobals = []; + foreach ( $GLOBALS as $key => $_ ) { + // Stash current values + self::$originalGlobals[$key] =& $GLOBALS[$key]; + + // Remove globals not part of the snapshot (see bootstrap.php, phpunit.php). + // Support: HHVM (avoid self-ref) + if ( $key !== 'GLOBALS' && !array_key_exists( $key, self::$unitGlobals ) ) { + unset( $GLOBALS[$key] ); + } } - $this->unitGlobals = $GLOBALS; - unset( $GLOBALS ); - $GLOBALS = []; - // Add back the minimal set of globals needed for unit tests to run for core + - // extensions/skins. - foreach ( $this->unitGlobals['wgPhpUnitBootstrapGlobals'] ?? [] as $key => $value ) { - $GLOBALS[ $key ] = $this->unitGlobals[ $key ]; + // Restore values from the early snapshot + // Not by ref because tests must not be able to modify the snapshot. + foreach ( self::$unitGlobals as $key => $value ) { + $GLOBALS[ $key ] = $value; } } protected function tearDown() { - $GLOBALS = $this->unitGlobals; + if ( !defined( 'HHVM_VERSION' ) ) { + // Quick reset between tests + foreach ( $GLOBALS as $key => $_ ) { + if ( $key !== 'GLOBALS' && !array_key_exists( $key, self::$unitGlobals ) ) { + unset( $GLOBALS[$key] ); + } + } + foreach ( self::$unitGlobals as $key => $value ) { + $GLOBALS[ $key ] = $value; + } + } + parent::tearDown(); } + public static function tearDownAfterClass() { + if ( !defined( 'HHVM_VERSION' ) ) { + // Remove globals created by the test + foreach ( $GLOBALS as $key => $_ ) { + if ( $key !== 'GLOBALS' && !array_key_exists( $key, self::$originalGlobals ) ) { + unset( $GLOBALS[$key] ); + } + } + // Restore values (including reference!) + foreach ( self::$originalGlobals as $key => &$value ) { + $GLOBALS[ $key ] =& $value; + } + } + + parent::tearDownAfterClass(); + } + /** * Create a temporary hook handler which will be reset by tearDown. * This replaces other handlers for the same hook. diff --git a/tests/phpunit/bootstrap.php b/tests/phpunit/bootstrap.php index 9e79496ac5..477dbd298e 100644 --- a/tests/phpunit/bootstrap.php +++ b/tests/phpunit/bootstrap.php @@ -55,19 +55,15 @@ define( 'MW_CONFIG_FILE', "$IP/LocalSettings.php" ); // these variables must be defined before setup runs $GLOBALS['IP'] = $IP; -// Set bootstrap globals to reuse in MediaWikiUnitTestCase -$bootstrapGlobals = []; -foreach ( $GLOBALS as $key => $value ) { - $bootstrapGlobals[ $key ] = $value; -} -$GLOBALS['wgPhpUnitBootstrapGlobals'] = $bootstrapGlobals; -// Faking for Setup.php + +require_once "$IP/tests/common/TestSetup.php"; +TestSetup::snapshotGlobals(); + +// Faking in lieu of Setup.php $GLOBALS['wgScopeTest'] = 'MediaWiki Setup.php scope test'; $GLOBALS['wgCommandLineMode'] = true; $GLOBALS['wgAutoloadClasses'] = []; -require_once "$IP/tests/common/TestSetup.php"; - wfRequireOnceInGlobalScope( "$IP/includes/AutoLoader.php" ); wfRequireOnceInGlobalScope( "$IP/tests/common/TestsAutoLoader.php" ); wfRequireOnceInGlobalScope( "$IP/includes/Defines.php" ); diff --git a/tests/phpunit/unit/includes/Revision/MainSlotRoleHandlerTest.php b/tests/phpunit/includes/Revision/MainSlotRoleHandlerTest.php similarity index 96% rename from tests/phpunit/unit/includes/Revision/MainSlotRoleHandlerTest.php rename to tests/phpunit/includes/Revision/MainSlotRoleHandlerTest.php index 9dff2cc500..60d456d1a3 100644 --- a/tests/phpunit/unit/includes/Revision/MainSlotRoleHandlerTest.php +++ b/tests/phpunit/includes/Revision/MainSlotRoleHandlerTest.php @@ -3,14 +3,13 @@ namespace MediaWiki\Tests\Revision; use MediaWiki\Revision\MainSlotRoleHandler; -use MediaWikiUnitTestCase; use PHPUnit\Framework\MockObject\MockObject; use Title; /** * @covers \MediaWiki\Revision\MainSlotRoleHandler */ -class MainSlotRoleHandlerTest extends MediaWikiUnitTestCase { +class MainSlotRoleHandlerTest extends \MediaWikiIntegrationTestCase { private function makeTitleObject( $ns ) { /** @var Title|MockObject $title */ diff --git a/tests/phpunit/unit/includes/Revision/SlotRecordTest.php b/tests/phpunit/includes/Revision/SlotRecordTest.php similarity index 99% rename from tests/phpunit/unit/includes/Revision/SlotRecordTest.php rename to tests/phpunit/includes/Revision/SlotRecordTest.php index aab430aaa6..7ffe0048e3 100644 --- a/tests/phpunit/unit/includes/Revision/SlotRecordTest.php +++ b/tests/phpunit/includes/Revision/SlotRecordTest.php @@ -7,13 +7,12 @@ use LogicException; use MediaWiki\Revision\IncompleteRevisionException; use MediaWiki\Revision\SlotRecord; use MediaWiki\Revision\SuppressedDataException; -use MediaWikiUnitTestCase; use WikitextContent; /** * @covers \MediaWiki\Revision\SlotRecord */ -class SlotRecordTest extends MediaWikiUnitTestCase { +class SlotRecordTest extends \MediaWikiIntegrationTestCase { private function makeRow( $data = [] ) { $data = $data + [ diff --git a/tests/phpunit/unit/includes/WikiReferenceTest.php b/tests/phpunit/includes/WikiReferenceTest.php similarity index 98% rename from tests/phpunit/unit/includes/WikiReferenceTest.php rename to tests/phpunit/includes/WikiReferenceTest.php index a4aae86c0f..702d3d7d89 100644 --- a/tests/phpunit/unit/includes/WikiReferenceTest.php +++ b/tests/phpunit/includes/WikiReferenceTest.php @@ -3,7 +3,7 @@ /** * @covers WikiReference */ -class WikiReferenceTest extends MediaWikiUnitTestCase { +class WikiReferenceTest extends MediaWikiIntegrationTestCase { public function provideGetDisplayName() { return [ diff --git a/tests/phpunit/unit/includes/diff/DifferenceEngineSlotDiffRendererTest.php b/tests/phpunit/includes/diff/DifferenceEngineSlotDiffRendererTest.php similarity index 94% rename from tests/phpunit/unit/includes/diff/DifferenceEngineSlotDiffRendererTest.php rename to tests/phpunit/includes/diff/DifferenceEngineSlotDiffRendererTest.php index 1a8b58590a..e5c23d172b 100644 --- a/tests/phpunit/unit/includes/diff/DifferenceEngineSlotDiffRendererTest.php +++ b/tests/phpunit/includes/diff/DifferenceEngineSlotDiffRendererTest.php @@ -3,7 +3,7 @@ /** * @covers DifferenceEngineSlotDiffRenderer */ -class DifferenceEngineSlotDiffRendererTest extends \MediaWikiUnitTestCase { +class DifferenceEngineSlotDiffRendererTest extends MediaWikiIntegrationTestCase { public function testGetDiff() { $differenceEngine = new CustomDifferenceEngine(); diff --git a/tests/phpunit/unit/includes/diff/SlotDiffRendererTest.php b/tests/phpunit/includes/diff/SlotDiffRendererTest.php similarity index 97% rename from tests/phpunit/unit/includes/diff/SlotDiffRendererTest.php rename to tests/phpunit/includes/diff/SlotDiffRendererTest.php index f778115780..9f15517c99 100644 --- a/tests/phpunit/unit/includes/diff/SlotDiffRendererTest.php +++ b/tests/phpunit/includes/diff/SlotDiffRendererTest.php @@ -6,7 +6,7 @@ use Wikimedia\TestingAccessWrapper; /** * @covers SlotDiffRenderer */ -class SlotDiffRendererTest extends \MediaWikiUnitTestCase { +class SlotDiffRendererTest extends \MediaWikiIntegrationTestCase { /** * @dataProvider provideNormalizeContents diff --git a/tests/phpunit/unit/includes/filerepo/FileBackendDBRepoWrapperTest.php b/tests/phpunit/includes/filerepo/FileBackendDBRepoWrapperTest.php similarity index 98% rename from tests/phpunit/unit/includes/filerepo/FileBackendDBRepoWrapperTest.php rename to tests/phpunit/includes/filerepo/FileBackendDBRepoWrapperTest.php index 6084601b05..69fc3679b3 100644 --- a/tests/phpunit/unit/includes/filerepo/FileBackendDBRepoWrapperTest.php +++ b/tests/phpunit/includes/filerepo/FileBackendDBRepoWrapperTest.php @@ -1,6 +1,6 @@ filePath = __DIR__ . '/../../../data/media/'; + $this->filePath = __DIR__ . '/../../data/media/'; } /** diff --git a/tests/phpunit/unit/includes/parser/ParserFactoryTest.php b/tests/phpunit/includes/parser/ParserFactoryTest.php similarity index 91% rename from tests/phpunit/unit/includes/parser/ParserFactoryTest.php rename to tests/phpunit/includes/parser/ParserFactoryTest.php index f1e48c7388..e6e9db41eb 100644 --- a/tests/phpunit/unit/includes/parser/ParserFactoryTest.php +++ b/tests/phpunit/includes/parser/ParserFactoryTest.php @@ -3,7 +3,7 @@ /** * @covers ParserFactory */ -class ParserFactoryTest extends MediaWikiUnitTestCase { +class ParserFactoryTest extends MediaWikiIntegrationTestCase { use FactoryArgTestTrait; protected static function getFactoryClass() { diff --git a/tests/phpunit/unit/includes/site/MediaWikiPageNameNormalizerTest.php b/tests/phpunit/includes/site/MediaWikiPageNameNormalizerTest.php similarity index 97% rename from tests/phpunit/unit/includes/site/MediaWikiPageNameNormalizerTest.php rename to tests/phpunit/includes/site/MediaWikiPageNameNormalizerTest.php index d426306e3f..7a6647b48b 100644 --- a/tests/phpunit/unit/includes/site/MediaWikiPageNameNormalizerTest.php +++ b/tests/phpunit/includes/site/MediaWikiPageNameNormalizerTest.php @@ -27,7 +27,7 @@ use MediaWiki\Site\MediaWikiPageNameNormalizer; * * @author Marius Hoch */ -class MediaWikiPageNameNormalizerTest extends MediaWikiUnitTestCase { +class MediaWikiPageNameNormalizerTest extends MediaWikiIntegrationTestCase { /** * @dataProvider normalizePageTitleProvider diff --git a/tests/phpunit/unit/includes/site/SiteExporterTest.php b/tests/phpunit/includes/site/SiteExporterTest.php similarity index 97% rename from tests/phpunit/unit/includes/site/SiteExporterTest.php rename to tests/phpunit/includes/site/SiteExporterTest.php index dcf51ac541..158be69cd6 100644 --- a/tests/phpunit/unit/includes/site/SiteExporterTest.php +++ b/tests/phpunit/includes/site/SiteExporterTest.php @@ -27,7 +27,7 @@ * * @author Daniel Kinzler */ -class SiteExporterTest extends MediaWikiUnitTestCase { +class SiteExporterTest extends MediaWikiIntegrationTestCase { public function testConstructor_InvalidArgument() { $this->setExpectedException( InvalidArgumentException::class ); @@ -64,7 +64,7 @@ class SiteExporterTest extends MediaWikiUnitTestCase { $this->assertContains( '', $xml ); // NOTE: HHVM (at least on wmf Jenkins) doesn't like file URLs. - $xsdFile = __DIR__ . '/../../../../../docs/sitelist-1.0.xsd'; + $xsdFile = __DIR__ . '/../../../../docs/sitelist-1.0.xsd'; $xsdData = file_get_contents( $xsdFile ); $document = new DOMDocument(); diff --git a/tests/phpunit/unit/includes/site/SiteImporterTest.php b/tests/phpunit/includes/site/SiteImporterTest.php similarity index 98% rename from tests/phpunit/unit/includes/site/SiteImporterTest.php rename to tests/phpunit/includes/site/SiteImporterTest.php index d4e4103c39..c614dd43a0 100644 --- a/tests/phpunit/unit/includes/site/SiteImporterTest.php +++ b/tests/phpunit/includes/site/SiteImporterTest.php @@ -27,7 +27,7 @@ * * @author Daniel Kinzler */ -class SiteImporterTest extends MediaWikiUnitTestCase { +class SiteImporterTest extends MediaWikiIntegrationTestCase { private function newSiteImporter( array $expectedSites, $errorCount ) { $store = $this->getMockBuilder( SiteStore::class )->getMock(); diff --git a/tests/phpunit/unit/includes/site/SiteImporterTest.xml b/tests/phpunit/includes/site/SiteImporterTest.xml similarity index 100% rename from tests/phpunit/unit/includes/site/SiteImporterTest.xml rename to tests/phpunit/includes/site/SiteImporterTest.xml diff --git a/tests/phpunit/unit/includes/utils/ZipDirectoryReaderTest.php b/tests/phpunit/includes/utils/ZipDirectoryReaderTest.php similarity index 92% rename from tests/phpunit/unit/includes/utils/ZipDirectoryReaderTest.php rename to tests/phpunit/includes/utils/ZipDirectoryReaderTest.php index be7e224e03..492b2509ce 100644 --- a/tests/phpunit/unit/includes/utils/ZipDirectoryReaderTest.php +++ b/tests/phpunit/includes/utils/ZipDirectoryReaderTest.php @@ -2,16 +2,15 @@ /** * @covers ZipDirectoryReader - * NOTE: this test is more like an integration test than a unit test */ -class ZipDirectoryReaderTest extends MediaWikiUnitTestCase { +class ZipDirectoryReaderTest extends MediaWikiIntegrationTestCase { protected $zipDir; protected $entries; protected function setUp() { parent::setUp(); - $this->zipDir = __DIR__ . '/../../../data/zip'; + $this->zipDir = __DIR__ . '/../../data/zip'; } function zipCallback( $entry ) { diff --git a/tests/phpunit/phpunit.php b/tests/phpunit/phpunit.php index 65445447f5..6e236cdbb6 100755 --- a/tests/phpunit/phpunit.php +++ b/tests/phpunit/phpunit.php @@ -34,6 +34,13 @@ class PHPUnitMaintClass extends Maintenance { ); } + public function setup() { + parent::setup(); + + require_once __DIR__ . '/../common/TestSetup.php'; + TestSetup::snapshotGlobals(); + } + public function finalSetup() { parent::finalSetup(); -- 2.20.1