From bd2a43950251e6eb67135b8f4535630981c12f9c Mon Sep 17 00:00:00 2001 From: Aryeh Gregor Date: Wed, 21 Aug 2019 16:16:27 +0300 Subject: [PATCH] Integration tests for FileBackendGroup 100% coverage except for one bit of the code that I didn't understand. Unit tests to come, together with rewrite as a service. Change-Id: Ib01758d994a9e5587a4fcb5edc3d80010ef05615 --- includes/filebackend/FileBackendGroup.php | 1 + .../filebackend/filejournal/FileJournal.php | 14 +- tests/common/TestsAutoLoader.php | 3 + .../FileBackendGroupIntegrationTest.php | 57 +++ .../filebackend/FileBackendGroupTestTrait.php | 459 ++++++++++++++++++ 5 files changed, 527 insertions(+), 7 deletions(-) create mode 100644 tests/phpunit/includes/filebackend/FileBackendGroupIntegrationTest.php create mode 100644 tests/phpunit/unit/includes/filebackend/FileBackendGroupTestTrait.php diff --git a/includes/filebackend/FileBackendGroup.php b/includes/filebackend/FileBackendGroup.php index 8b31f05c83..9e04d09632 100644 --- a/includes/filebackend/FileBackendGroup.php +++ b/includes/filebackend/FileBackendGroup.php @@ -150,6 +150,7 @@ class FileBackendGroup { $class = $config['class']; if ( $class === FileBackendMultiWrite::class ) { + // @todo How can we test this? What's the intended use-case? foreach ( $config['backends'] as $index => $beConfig ) { if ( isset( $beConfig['template'] ) ) { // Config is just a modified version of a registered backend's. diff --git a/includes/libs/filebackend/filejournal/FileJournal.php b/includes/libs/filebackend/filejournal/FileJournal.php index dc007a0cec..e51242375a 100644 --- a/includes/libs/filebackend/filejournal/FileJournal.php +++ b/includes/libs/filebackend/filejournal/FileJournal.php @@ -26,6 +26,7 @@ * @ingroup FileJournal */ +use Wikimedia\ObjectFactory; use Wikimedia\Timestamp\ConvertibleTimestamp; /** @@ -43,12 +44,12 @@ abstract class FileJournal { protected $ttlDays; /** - * Construct a new instance from configuration. + * Construct a new instance from configuration. Do not call this directly, use factory(). * * @param array $config Includes: * 'ttlDays' : days to keep log entries around (false means "forever") */ - protected function __construct( array $config ) { + public function __construct( array $config ) { $this->ttlDays = $config['ttlDays'] ?? false; } @@ -61,11 +62,10 @@ abstract class FileJournal { * @return FileJournal */ final public static function factory( array $config, $backend ) { - $class = $config['class']; - $jrn = new $class( $config ); - if ( !$jrn instanceof self ) { - throw new InvalidArgumentException( "$class is not an instance of " . __CLASS__ ); - } + $jrn = ObjectFactory::getObjectFromSpec( + $config, + [ 'specIsArg' => true, 'assertClass' => __CLASS__ ] + ); $jrn->backend = $backend; return $jrn; diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php index 7c8df1a6f5..9f1d67be16 100644 --- a/tests/common/TestsAutoLoader.php +++ b/tests/common/TestsAutoLoader.php @@ -221,6 +221,9 @@ $wgAutoloadClasses += [ # tests/phpunit/unit/includes 'BadFileLookupTest' => "$testDir/phpunit/unit/includes/BadFileLookupTest.php", + # tests/phpunit/unit/includes/filebackend + 'FileBackendGroupTestTrait' => "$testDir/phpunit/unit/includes/filebackend/FileBackendGroupTestTrait.php", + # tests/phpunit/unit/includes/libs/filebackend/fsfile 'TempFSFileTestTrait' => "$testDir/phpunit/unit/includes/libs/filebackend/fsfile/TempFSFileTestTrait.php", diff --git a/tests/phpunit/includes/filebackend/FileBackendGroupIntegrationTest.php b/tests/phpunit/includes/filebackend/FileBackendGroupIntegrationTest.php new file mode 100644 index 0000000000..ee3262c4e8 --- /dev/null +++ b/tests/phpunit/includes/filebackend/FileBackendGroupIntegrationTest.php @@ -0,0 +1,57 @@ +getLockManagerGroupFactory(); + } + + private function newObj( array $options = [] ) : FileBackendGroup { + $globals = [ 'DirectoryMode', 'FileBackends', 'ForeignFileRepos', 'LocalFileRepo' ]; + foreach ( $globals as $global ) { + $this->setMwGlobals( + "wg$global", $options[$global] ?? self::getDefaultOptions()[$global] ); + } + + $serviceMembers = [ + 'configuredROMode' => 'ConfiguredReadOnlyMode', + 'srvCache' => 'LocalServerObjectCache', + 'wanCache' => 'MainWANObjectCache', + 'mimeAnalyzer' => 'MimeAnalyzer', + 'lmgFactory' => 'LockManagerGroupFactory', + 'tmpFileFactory' => 'TempFSFileFactory', + ]; + + foreach ( $serviceMembers as $key => $name ) { + if ( isset( $options[$key] ) ) { + $this->setService( $name, $options[$key] ); + } + } + + $this->assertEmpty( + array_diff( array_keys( $options ), $globals, array_keys( $serviceMembers ) ) ); + + $this->resetServices(); + FileBackendGroup::destroySingleton(); + + $services = MediaWikiServices::getInstance(); + + foreach ( $serviceMembers as $key => $name ) { + $this->$key = $services->getService( $name ); + } + + return FileBackendGroup::singleton(); + } +} diff --git a/tests/phpunit/unit/includes/filebackend/FileBackendGroupTestTrait.php b/tests/phpunit/unit/includes/filebackend/FileBackendGroupTestTrait.php new file mode 100644 index 0000000000..d23f645eeb --- /dev/null +++ b/tests/phpunit/unit/includes/filebackend/FileBackendGroupTestTrait.php @@ -0,0 +1,459 @@ + LocalRepo::class, + 'name' => 'local', + 'directory' => 'upload-dir', + 'thumbDir' => 'thumb/', + 'transcodedDir' => 'transcoded/', + 'fileMode' => 0664, + 'scriptDirUrl' => 'script-path/', + 'url' => 'upload-path/', + 'hashLevels' => 2, + 'thumbScriptUrl' => false, + 'transformVia404' => false, + 'deletedDir' => 'deleted/', + 'deletedHashLevels' => 3, + 'backend' => 'local-backend', + ]; + } + + private static function getDefaultOptions() { + return [ + 'DirectoryMode' => 0775, + 'FileBackends' => [], + 'ForeignFileRepos' => [], + 'LocalFileRepo' => self::getDefaultLocalFileRepo(), + 'wikiId' => self::getWikiID(), + ]; + } + + /** + * @covers ::__construct + */ + public function testConstructor_overrideImplicitBackend() { + $obj = $this->newObj( [ 'FileBackends' => + [ [ 'name' => 'local-backend', 'class' => '', 'lockManager' => 'fsLockManager' ] ] + ] ); + $this->assertSame( '', $obj->config( 'local-backend' )['class'] ); + } + + /** + * @covers ::__construct + */ + public function testConstructor_backendObject() { + // 'backend' being an object makes that repo from configuration ignored + // XXX This is not documented in DefaultSettings.php, does it do anything useful? + $obj = $this->newObj( [ 'ForeignFileRepos' => [ [ 'backend' => new stdclass ] ] ] ); + $this->assertSame( FSFileBackend::class, $obj->config( 'local-backend' )['class'] ); + } + + /** + * @dataProvider provideRegister_domainId + * @param string $key Key to check in return value of config() + * @param string|callable $expected Expected value of config()[$key], or callable returning it + * @param array $extraBackendsOptions To add to the FileBackends entry passed to newObj() + * @param array $otherExtraOptions To add to the array passed to newObj() (e.g., services) + * @covers ::register + */ + public function testRegister( + $key, $expected, array $extraBackendsOptions = [], array $otherExtraOptions = [] + ) { + if ( $expected instanceof Closure ) { + // Lame hack to get around providers being called too early + $expected = $expected(); + } + if ( $key === 'domainId' ) { + // This will change the expected LMG name too + $otherExtraOptions['lmgFactory'] = $this->getLockManagerGroupFactory( $expected ); + } + $obj = $this->newObj( $otherExtraOptions + [ + 'FileBackends' => [ + $extraBackendsOptions + [ + 'name' => 'myname', 'class' => '', 'lockManager' => 'fsLockManager' + ] + ], + ] ); + $this->assertSame( $expected, $obj->config( 'myname' )[$key] ); + } + + public static function provideRegister_domainId() { + return [ + 'domainId with neither wikiId nor domainId set' => [ + 'domainId', + function () { + return self::getWikiID(); + }, + ], + 'domainId with wikiId set but no domainId' => + [ 'domainId', 'id0', [ 'wikiId' => 'id0' ] ], + 'domainId with wikiId and domainId set' => + [ 'domainId', 'dom1', [ 'wikiId' => 'id0', 'domainId' => 'dom1' ] ], + 'readOnly without readOnly set' => [ 'readOnly', false ], + 'readOnly with readOnly set to string' => + [ 'readOnly', 'cuz', [ 'readOnly' => 'cuz' ] ], + 'readOnly without readOnly set but with string in passed object' => [ + 'readOnly', + 'cuz', + [], + [ 'configuredROMode' => new ConfiguredReadOnlyMode( 'cuz' ) ], + ], + 'readOnly with readOnly set to false but string in passed object' => [ + 'readOnly', + false, + [ 'readOnly' => false ], + [ 'configuredROMode' => new ConfiguredReadOnlyMode( 'cuz' ) ], + ], + ]; + } + + /** + * @dataProvider provideRegister_exception + * @param array $fileBackends Value of FileBackends to pass to constructor + * @param string $class Expected exception class + * @param string $msg Expected exception message + * @covers ::__construct + * @covers ::register + */ + public function testRegister_exception( $fileBackends, $class, $msg ) { + $this->setExpectedException( $class, $msg ); + $this->newObj( [ 'FileBackends' => $fileBackends ] ); + } + + public static function provideRegister_exception() { + return [ + 'Nameless' => [ + [ [] ], InvalidArgumentException::class, "Cannot register a backend with no name." + ], + 'Duplicate' => [ + [ [ 'name' => 'dupe', 'class' => '' ], [ 'name' => 'dupe' ] ], + LogicException::class, + "Backend with name 'dupe' already registered.", + ], + 'Classless' => [ + [ [ 'name' => 'classless' ] ], + InvalidArgumentException::class, + "Backend with name 'classless' has no class.", + ], + ]; + } + + /** + * @covers ::__construct + * @covers ::config + * @covers ::get + */ + public function testGet() { + $backend = $this->newObj()->get( 'local-backend' ); + $this->assertTrue( $backend instanceof FSFileBackend ); + } + + /** + * @covers ::get + */ + public function testGetUnrecognized() { + $this->setExpectedException( InvalidArgumentException::class, + "No backend defined with the name 'unrecognized'." ); + $this->newObj()->get( 'unrecognized' ); + } + + /** + * @covers ::__construct + * @covers ::config + */ + public function testConfig() { + $obj = $this->newObj(); + $config = $obj->config( 'local-backend' ); + + // XXX How to actually test that a profiler is loaded? + $this->assertNull( $config['profiler']( 'x' ) ); + // Equality comparison doesn't work for closures, so just set to null + $config['profiler'] = null; + + $this->assertEquals( [ + 'mimeCallback' => [ $obj, 'guessMimeInternal' ], + 'obResetFunc' => 'wfResetOutputBuffers', + 'streamMimeFunc' => [ StreamFile::class, 'contentTypeFromPath' ], + 'tmpFileFactory' => $this->tmpFileFactory, + 'statusWrapper' => [ Status::class, 'wrap' ], + 'wanCache' => $this->wanCache, + 'srvCache' => $this->srvCache, + 'logger' => LoggerFactory::getInstance( 'FileOperation' ), + // This was set to null above in $config, it's not really null + 'profiler' => null, + 'name' => 'local-backend', + 'containerPaths' => [ + 'local-public' => 'upload-dir', + 'local-thumb' => 'thumb/', + 'local-transcoded' => 'transcoded/', + 'local-deleted' => 'deleted/', + 'local-temp' => 'upload-dir/temp', + ], + 'fileMode' => 0664, + 'directoryMode' => 0775, + 'domainId' => self::getWikiID(), + 'readOnly' => false, + 'class' => FSFileBackend::class, + 'lockManager' => + $this->lmgFactory->getLockManagerGroup( self::getWikiID() )->get( 'fsLockManager' ), + 'fileJournal' => + FileJournal::factory( [ 'class' => NullFileJournal::class ], 'local-backend' ), + ], $config ); + + // For config values that are objects, check object identity. + $this->assertSame( [ $obj, 'guessMimeInternal' ], $config['mimeCallback'] ); + $this->assertSame( $this->tmpFileFactory, $config['tmpFileFactory'] ); + $this->assertSame( $this->wanCache, $config['wanCache'] ); + $this->assertSame( $this->srvCache, $config['srvCache'] ); + } + + /** + * @dataProvider provideConfig_default + * @param string $expected Expected default value + * @param string $inputName Name to set to null in LocalFileRepo setting + * @param string|array $key Key to check in array returned by config(), or array [ 'key1', + * 'key2' ] for nested key + * @covers ::__construct + * @covers ::config + */ + public function testConfig_defaultNull( $expected, $inputName, $key ) { + $config = self::getDefaultLocalFileRepo(); + $config[$inputName] = null; + + $result = $this->newObj( [ 'LocalFileRepo' => $config ] )->config( 'local-backend' ); + + $actual = is_string( $key ) ? $result[$key] : $result[$key[0]][$key[1]]; + + $this->assertSame( $expected, $actual ); + } + + /** + * @dataProvider provideConfig_default + * @param string $expected Expected default value + * @param string $inputName Name to unset in LocalFileRepo setting + * @param string|array $key Key to check in array returned by config(), or array [ 'key1', + * 'key2' ] for nested key + * @covers ::__construct + * @covers ::config + */ + public function testConfig_defaultUnset( $expected, $inputName, $key ) { + $config = self::getDefaultLocalFileRepo(); + unset( $config[$inputName] ); + + $result = $this->newObj( [ 'LocalFileRepo' => $config ] )->config( 'local-backend' ); + + $actual = is_string( $key ) ? $result[$key] : $result[$key[0]][$key[1]]; + + $this->assertSame( $expected, $actual ); + } + + public static function provideConfig_default() { + return [ + 'deletedDir' => [ false, 'deletedDir', [ 'containerPaths', 'local-deleted' ] ], + 'thumbDir' => [ 'upload-dir/thumb', 'thumbDir', [ 'containerPaths', 'local-thumb' ] ], + 'transcodedDir' => [ + 'upload-dir/transcoded', 'transcodedDir', [ 'containerPaths', 'local-transcoded' ] + ], + 'fileMode' => [ 0644, 'fileMode', 'fileMode' ], + ]; + } + + /** + * @covers ::config + */ + public function testConfig_fileJournal() { + $mockJournal = $this->createMock( FileJournal::class ); + $mockJournal->expects( $this->never() )->method( $this->anything() ); + + $obj = $this->newObj( [ 'FileBackends' => [ [ + 'name' => 'name', + 'class' => '', + 'lockManager' => 'fsLockManager', + 'fileJournal' => [ 'factory' => + function () use ( $mockJournal ) { + return $mockJournal; + } + ], + ] ] ] ); + + $this->assertSame( $mockJournal, $obj->config( 'name' )['fileJournal'] ); + } + + /** + * @covers ::config + */ + public function testConfigUnrecognized() { + $this->setExpectedException( InvalidArgumentException::class, + "No backend defined with the name 'unrecognized'." ); + $this->newObj()->config( 'unrecognized' ); + } + + /** + * @dataProvider provideBackendFromPath + * @covers ::backendFromPath + * @param string|null $expected Name of backend that will be returned from 'get', or null + * @param string $storagePath + */ + public function testBackendFromPath( $expected = null, $storagePath ) { + $obj = $this->newObj( [ 'FileBackends' => [ + [ 'name' => '', 'class' => stdclass::class, 'lockManager' => 'fsLockManager' ], + [ 'name' => 'a', 'class' => stdclass::class, 'lockManager' => 'fsLockManager' ], + [ 'name' => 'b', 'class' => stdclass::class, 'lockManager' => 'fsLockManager' ], + ] ] ); + $this->assertSame( + $expected === null ? null : $obj->get( $expected ), + $obj->backendFromPath( $storagePath ) + ); + } + + public static function provideBackendFromPath() { + return [ + 'Empty string' => [ null, '' ], + 'mwstore://' => [ null, 'mwstore://' ], + 'mwstore://a' => [ null, 'mwstore://a' ], + 'mwstore:///' => [ null, 'mwstore:///' ], + 'mwstore://a/' => [ null, 'mwstore://a/' ], + 'mwstore://a//' => [ null, 'mwstore://a//' ], + 'mwstore://a/b' => [ 'a', 'mwstore://a/b' ], + 'mwstore://a/b/' => [ 'a', 'mwstore://a/b/' ], + 'mwstore://a/b////' => [ 'a', 'mwstore://a/b////' ], + 'mwstore://a/b/c' => [ 'a', 'mwstore://a/b/c' ], + 'mwstore://a/b/c/d' => [ 'a', 'mwstore://a/b/c/d' ], + 'mwstore://b/b' => [ 'b', 'mwstore://b/b' ], + 'mwstore://c/b' => [ null, 'mwstore://c/b' ], + ]; + } + + /** + * @dataProvider provideGuessMimeInternal + * @covers ::guessMimeInternal + * @param string $storagePath + * @param string|null $content + * @param string|null $fsPath + * @param string|null $expectedExtensionType Expected return of + * MimeAnalyzer::guessTypesForExtension + * @param string|null $expectedGuessedMimeType Expected return value of + * MimeAnalyzer::guessMimeType (null if expected not to be called) + */ + public function testGuessMimeInternal( + $storagePath, + $content, + $fsPath, + $expectedExtensionType, + $expectedGuessedMimeType + ) { + $mimeAnalyzer = $this->createMock( MimeAnalyzer::class ); + $mimeAnalyzer->expects( $this->once() )->method( 'guessTypesForExtension' ) + ->willReturn( $expectedExtensionType ); + $tmpFileFactory = $this->createMock( TempFSFileFactory::class ); + + if ( !$expectedExtensionType && $fsPath ) { + $tmpFileFactory->expects( $this->never() )->method( 'newTempFSFile' ); + $mimeAnalyzer->expects( $this->once() )->method( 'guessMimeType' ) + ->with( $fsPath, false )->willReturn( $expectedGuessedMimeType ); + } elseif ( !$expectedExtensionType && strlen( $content ) ) { + // XXX What should we do about the file creation here? Really we should mock + // file_put_contents() somehow. It's not very nice to ignore the value of + // $wgTmpDirectory. + $tmpFile = ( new TempFSFileFactory() )->newTempFSFile( 'mime_', '' ); + + $tmpFileFactory->expects( $this->once() )->method( 'newTempFSFile' ) + ->with( 'mime_', '' )->willReturn( $tmpFile ); + $mimeAnalyzer->expects( $this->once() )->method( 'guessMimeType' ) + ->with( $tmpFile->getPath(), false )->willReturn( $expectedGuessedMimeType ); + } else { + $tmpFileFactory->expects( $this->never() )->method( 'newTempFSFile' ); + $mimeAnalyzer->expects( $this->never() )->method( 'guessMimeType' ); + } + + $mimeAnalyzer->expects( $this->never() ) + ->method( $this->anythingBut( 'guessTypesForExtension', 'guessMimeType' ) ); + $tmpFileFactory->expects( $this->never() ) + ->method( $this->anythingBut( 'newTempFSFile' ) ); + + $obj = $this->newObj( [ + 'mimeAnalyzer' => $mimeAnalyzer, + 'tmpFileFactory' => $tmpFileFactory, + ] ); + + $this->assertSame( $expectedExtensionType ?? $expectedGuessedMimeType ?? 'unknown/unknown', + $obj->guessMimeInternal( $storagePath, $content, $fsPath ) ); + } + + public static function provideGuessMimeInternal() { + return [ + 'With extension' => + [ 'foo.txt', null, null, 'text/plain', null ], + 'No extension' => + [ 'foo', null, null, null, null ], + 'Empty content, with extension' => + [ 'foo.txt', '', null, 'text/plain', null ], + 'Empty content, no extension' => + [ 'foo', '', null, null, null ], + 'Non-empty content, with extension' => + [ 'foo.txt', 'foo', null, 'text/plain', null ], + 'Non-empty content, no extension' => + [ 'foo', 'foo', null, null, 'text/html' ], + 'Empty path, with extension' => + [ 'foo.txt', null, '', 'text/plain', null ], + 'Empty path, no extension' => + [ 'foo', null, '', null, null ], + 'Non-empty path, with extension' => + [ 'foo.txt', null, '/bogus/path', 'text/plain', null ], + 'Non-empty path, no extension' => + [ 'foo', null, '/bogus/path', null, 'text/html' ], + 'Empty path and content, with extension' => + [ 'foo.txt', '', '', 'text/plain', null ], + 'Empty path and content, no extension' => + [ 'foo', '', '', null, null ], + 'Non-empty path and content, with extension' => + [ 'foo.txt', 'foo', '/bogus/path', 'text/plain', null ], + 'Non-empty path and content, no extension' => + [ 'foo', 'foo', '/bogus/path', null, 'image/jpeg' ], + ]; + } +} -- 2.20.1