From 48d715148bad09faf56b5f854fde0029ac7f5dac Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Wed, 11 Feb 2015 01:42:33 +0000 Subject: [PATCH] Use MediaWikiTestCase methods for tempdir in unit tests * Use MediaWikiTestCase::getNewTempFile and getNewTempDirectory instead of wfTempDir(). The upload api tests wrote a tempnam() file directly (where wfTempDir() is typically shared with other systems and concurrent runs). Use MediaWikiTestCase::getNewTempFile and getNewTempDirectory instead. This also ensures its removal by the teardown handler without needing manual unlink() calls. And it doesn't rely on the test passing. (Many unlink calls where at the bottom of tests, which wouldn't be reached in case of failure). * For the upload test, the presistent storing of 'Oberaargletscher_from_Oberaar.jpg' (downloaded from Commons) was removed. Note that this didn't work for Jenkins builds anyway as Jenkins builds set $wgTmpDirectory to a unique directory in tmpfs associated with an individual build. * For filebackend tests, moved directory creation from the dataProvider to the main test. Implemented addTmpFiles() to allow subclasses to register additional files (created by other means) to be cleaned up also. Removed unused $tmpName and $toPath parameters in data provider for FileBackendTest::testStore. And fixed weird double $op2 variable name to be called $op3. * Skipped parserTest.inc, MockFileBackend.php, and UploadFromUrlTestSuite.php as those don't use MediaWikiTestCase. Change-Id: Ic7feb06ef0c1006eb99485470a1a59419f972545 --- tests/phpunit/MediaWikiTestCase.php | 5 +- .../includes/GlobalFunctions/GlobalTest.php | 48 +++++--------- .../includes/api/ApiTestCaseUpload.php | 19 +----- tests/phpunit/includes/api/ApiUploadTest.php | 28 +++----- .../includes/filebackend/FileBackendTest.php | 66 +++++-------------- .../phpunit/includes/parser/NewParserTest.php | 4 +- .../includes/upload/UploadBaseTest.php | 3 +- 7 files changed, 53 insertions(+), 120 deletions(-) diff --git a/tests/phpunit/MediaWikiTestCase.php b/tests/phpunit/MediaWikiTestCase.php index 2b46fc396b..e49c391a49 100644 --- a/tests/phpunit/MediaWikiTestCase.php +++ b/tests/phpunit/MediaWikiTestCase.php @@ -214,8 +214,11 @@ abstract class MediaWikiTestCase extends PHPUnit_Framework_TestCase { } - protected function tearDown() { + protected function addTmpFiles( $files ) { + $this->tmpFiles = array_merge( $this->tmpFiles, (array)$files ); + } + protected function tearDown() { $this->called['tearDown'] = true; // Cleaning up temporary files foreach ( $this->tmpFiles as $fileName ) { diff --git a/tests/phpunit/includes/GlobalFunctions/GlobalTest.php b/tests/phpunit/includes/GlobalFunctions/GlobalTest.php index 2bfabe4df7..331fb3ba4f 100644 --- a/tests/phpunit/includes/GlobalFunctions/GlobalTest.php +++ b/tests/phpunit/includes/GlobalFunctions/GlobalTest.php @@ -7,7 +7,7 @@ class GlobalTest extends MediaWikiTestCase { protected function setUp() { parent::setUp(); - $readOnlyFile = tempnam( wfTempDir(), "mwtest_readonly" ); + $readOnlyFile = $this->getNewTempFile(); unlink( $readOnlyFile ); $this->setMwGlobals( array( @@ -22,16 +22,6 @@ class GlobalTest extends MediaWikiTestCase { ) ); } - protected function tearDown() { - global $wgReadOnlyFile; - - if ( file_exists( $wgReadOnlyFile ) ) { - unlink( $wgReadOnlyFile ); - } - - parent::tearDown(); - } - /** * @dataProvider provideForWfArrayDiff2 * @covers ::wfArrayDiff2 @@ -312,46 +302,42 @@ class GlobalTest extends MediaWikiTestCase { * @covers ::wfDebugMem */ public function testDebugFunctionTest() { + $debugLogFile = $this->getNewTempFile(); - global $wgDebugLogFile, $wgDebugTimestamps; - - $old_log_file = $wgDebugLogFile; - $wgDebugLogFile = tempnam( wfTempDir(), 'mw-' ); - # @todo FIXME: $wgDebugTimestamps should be tested - $old_wgDebugTimestamps = $wgDebugTimestamps; - $wgDebugTimestamps = false; + $this->setMwGlobals( array( + 'wgDebugLogFile' => $debugLogFile, + # @todo FIXME: $wgDebugTimestamps should be tested + 'wgDebugTimestamps' => false + ) ); wfDebug( "This is a normal string" ); - $this->assertEquals( "This is a normal string\n", file_get_contents( $wgDebugLogFile ) ); - unlink( $wgDebugLogFile ); + $this->assertEquals( "This is a normal string\n", file_get_contents( $debugLogFile ) ); + unlink( $debugLogFile ); wfDebug( "This is nöt an ASCII string" ); - $this->assertEquals( "This is nöt an ASCII string\n", file_get_contents( $wgDebugLogFile ) ); - unlink( $wgDebugLogFile ); + $this->assertEquals( "This is nöt an ASCII string\n", file_get_contents( $debugLogFile ) ); + unlink( $debugLogFile ); wfDebug( "\00305This has böth UTF and control chars\003" ); $this->assertEquals( " 05This has böth UTF and control chars \n", - file_get_contents( $wgDebugLogFile ) + file_get_contents( $debugLogFile ) ); - unlink( $wgDebugLogFile ); + unlink( $debugLogFile ); wfDebugMem(); $this->assertGreaterThan( 1000, - preg_replace( '/\D/', '', file_get_contents( $wgDebugLogFile ) ) + preg_replace( '/\D/', '', file_get_contents( $debugLogFile ) ) ); - unlink( $wgDebugLogFile ); + unlink( $debugLogFile ); wfDebugMem( true ); $this->assertGreaterThan( 1000000, - preg_replace( '/\D/', '', file_get_contents( $wgDebugLogFile ) ) + preg_replace( '/\D/', '', file_get_contents( $debugLogFile ) ) ); - unlink( $wgDebugLogFile ); - - $wgDebugLogFile = $old_log_file; - $wgDebugTimestamps = $old_wgDebugTimestamps; + unlink( $debugLogFile ); } /** diff --git a/tests/phpunit/includes/api/ApiTestCaseUpload.php b/tests/phpunit/includes/api/ApiTestCaseUpload.php index 7e513394e4..d4d96512cd 100644 --- a/tests/phpunit/includes/api/ApiTestCaseUpload.php +++ b/tests/phpunit/includes/api/ApiTestCaseUpload.php @@ -21,12 +21,6 @@ abstract class ApiTestCaseUpload extends ApiTestCase { $this->clearFakeUploads(); } - protected function tearDown() { - $this->clearTempUpload(); - - parent::tearDown(); - } - /** * Helper function -- remove files and associated articles by Title * @@ -105,7 +99,7 @@ abstract class ApiTestCaseUpload extends ApiTestCase { * @return bool */ function fakeUploadFile( $fieldName, $fileName, $type, $filePath ) { - $tmpName = tempnam( wfTempDir(), "" ); + $tmpName = $this->getNewTempFile(); if ( !file_exists( $filePath ) ) { throw new Exception( "$filePath doesn't exist!" ); } @@ -132,7 +126,7 @@ abstract class ApiTestCaseUpload extends ApiTestCase { } function fakeUploadChunk( $fieldName, $fileName, $type, & $chunkData ) { - $tmpName = tempnam( wfTempDir(), "" ); + $tmpName = $this->getNewTempFile(); // copy the chunk data to temp location: if ( !file_put_contents( $tmpName, $chunkData ) ) { throw new Exception( "couldn't copy chunk data to $tmpName" ); @@ -153,15 +147,6 @@ abstract class ApiTestCaseUpload extends ApiTestCase { ); } - function clearTempUpload() { - if ( isset( $_FILES['file']['tmp_name'] ) ) { - $tmp = $_FILES['file']['tmp_name']; - if ( file_exists( $tmp ) ) { - unlink( $tmp ); - } - } - } - /** * Remove traces of previous fake uploads */ diff --git a/tests/phpunit/includes/api/ApiUploadTest.php b/tests/phpunit/includes/api/ApiUploadTest.php index 7fdefb6cad..b4b1bf3f6f 100644 --- a/tests/phpunit/includes/api/ApiUploadTest.php +++ b/tests/phpunit/includes/api/ApiUploadTest.php @@ -103,7 +103,7 @@ class ApiUploadTest extends ApiTestCaseUpload { try { $randomImageGenerator = new RandomImageGenerator(); - $filePaths = $randomImageGenerator->writeImages( 1, $extension, wfTempDir() ); + $filePaths = $randomImageGenerator->writeImages( 1, $extension, $this->getNewTempDirectory() ); } catch ( Exception $e ) { $this->markTestIncomplete( $e->getMessage() ); } @@ -143,7 +143,6 @@ class ApiUploadTest extends ApiTestCaseUpload { // clean up $this->deleteFileByFilename( $fileName ); - unlink( $filePath ); } /** @@ -152,7 +151,7 @@ class ApiUploadTest extends ApiTestCaseUpload { public function testUploadZeroLength( $session ) { $mimeType = 'image/png'; - $filePath = tempnam( wfTempDir(), "" ); + $filePath = $this->getNewTempFile(); $fileName = "apiTestUploadZeroLength.png"; $this->deleteFileByFileName( $fileName ); @@ -180,7 +179,6 @@ class ApiUploadTest extends ApiTestCaseUpload { // clean up $this->deleteFileByFilename( $fileName ); - unlink( $filePath ); } /** @@ -192,7 +190,7 @@ class ApiUploadTest extends ApiTestCaseUpload { try { $randomImageGenerator = new RandomImageGenerator(); - $filePaths = $randomImageGenerator->writeImages( 2, $extension, wfTempDir() ); + $filePaths = $randomImageGenerator->writeImages( 2, $extension, $this->getNewTempDirectory() ); } catch ( Exception $e ) { $this->markTestIncomplete( $e->getMessage() ); } @@ -251,8 +249,6 @@ class ApiUploadTest extends ApiTestCaseUpload { // clean up $this->deleteFileByFilename( $fileName ); - unlink( $filePaths[0] ); - unlink( $filePaths[1] ); } /** @@ -264,7 +260,7 @@ class ApiUploadTest extends ApiTestCaseUpload { try { $randomImageGenerator = new RandomImageGenerator(); - $filePaths = $randomImageGenerator->writeImages( 1, $extension, wfTempDir() ); + $filePaths = $randomImageGenerator->writeImages( 1, $extension, $this->getNewTempDirectory() ); } catch ( Exception $e ) { $this->markTestIncomplete( $e->getMessage() ); } @@ -333,7 +329,6 @@ class ApiUploadTest extends ApiTestCaseUpload { // clean up $this->deleteFileByFilename( $fileNames[0] ); $this->deleteFileByFilename( $fileNames[1] ); - unlink( $filePaths[0] ); } /** @@ -349,7 +344,7 @@ class ApiUploadTest extends ApiTestCaseUpload { try { $randomImageGenerator = new RandomImageGenerator(); - $filePaths = $randomImageGenerator->writeImages( 1, $extension, wfTempDir() ); + $filePaths = $randomImageGenerator->writeImages( 1, $extension, $this->getNewTempDirectory() ); } catch ( Exception $e ) { $this->markTestIncomplete( $e->getMessage() ); } @@ -417,7 +412,6 @@ class ApiUploadTest extends ApiTestCaseUpload { // clean up $this->deleteFileByFilename( $fileName ); - unlink( $filePath ); } /** @@ -431,16 +425,14 @@ class ApiUploadTest extends ApiTestCaseUpload { $chunkSize = 1048576; // Download a large image file - // ( using RandomImageGenerator for large files is not stable ) + // (using RandomImageGenerator for large files is not stable) + // @todo Don't download files from wikimedia.org $mimeType = 'image/jpeg'; $url = 'http://upload.wikimedia.org/wikipedia/commons/' . 'e/ed/Oberaargletscher_from_Oberaar%2C_2010_07.JPG'; - $filePath = wfTempDir() . '/Oberaargletscher_from_Oberaar.jpg'; + $filePath = $this->getNewTempDirectory() . '/Oberaargletscher_from_Oberaar.jpg'; try { - // Only download if the file is not avaliable in the temp location: - if ( !is_file( $filePath ) ) { - copy( $url, $filePath ); - } + copy( $url, $filePath ); } catch ( Exception $e ) { $this->markTestIncomplete( $e->getMessage() ); } @@ -564,7 +556,5 @@ class ApiUploadTest extends ApiTestCaseUpload { // clean up $this->deleteFileByFilename( $fileName ); - // don't remove downloaded temporary file for fast subquent tests. - //unlink( $filePath ); } } diff --git a/tests/phpunit/includes/filebackend/FileBackendTest.php b/tests/phpunit/includes/filebackend/FileBackendTest.php index 9558cc7db9..b40d2d213e 100644 --- a/tests/phpunit/includes/filebackend/FileBackendTest.php +++ b/tests/phpunit/includes/filebackend/FileBackendTest.php @@ -13,14 +13,13 @@ class FileBackendTest extends MediaWikiTestCase { private $multiBackend; /** @var FSFileBackend */ public $singleBackend; - private $filesToPrune = array(); private static $backendToUse; protected function setUp() { global $wgFileBackends; parent::setUp(); $uniqueId = time() . '-' . mt_rand(); - $tmpPrefix = wfTempDir() . '/filebackend-unittest-' . $uniqueId; + $tmpDir = $this->getNewTempDirectory(); if ( $this->getCliArg( 'use-filebackend' ) ) { if ( self::$backendToUse ) { $this->singleBackend = self::$backendToUse; @@ -51,8 +50,8 @@ class FileBackendTest extends MediaWikiTestCase { 'lockManager' => LockManagerGroup::singleton()->get( 'fsLockManager' ), 'wikiId' => wfWikiID(), 'containerPaths' => array( - 'unittest-cont1' => "{$tmpPrefix}-localtesting-cont1", - 'unittest-cont2' => "{$tmpPrefix}-localtesting-cont2" ) + 'unittest-cont1' => "{$tmpDir}/localtesting-cont1", + 'unittest-cont2' => "{$tmpDir}/localtesting-cont2" ) ) ); } $this->multiBackend = new FileBackendMultiWrite( array( @@ -65,21 +64,20 @@ class FileBackendTest extends MediaWikiTestCase { 'name' => 'localmultitesting1', 'class' => 'FSFileBackend', 'containerPaths' => array( - 'unittest-cont1' => "{$tmpPrefix}-localtestingmulti1-cont1", - 'unittest-cont2' => "{$tmpPrefix}-localtestingmulti1-cont2" ), + 'unittest-cont1' => "{$tmpDir}/localtestingmulti1-cont1", + 'unittest-cont2' => "{$tmpDir}/localtestingmulti1-cont2" ), 'isMultiMaster' => false ), array( 'name' => 'localmultitesting2', 'class' => 'FSFileBackend', 'containerPaths' => array( - 'unittest-cont1' => "{$tmpPrefix}-localtestingmulti2-cont1", - 'unittest-cont2' => "{$tmpPrefix}-localtestingmulti2-cont2" ), + 'unittest-cont1' => "{$tmpDir}/localtestingmulti2-cont1", + 'unittest-cont2' => "{$tmpDir}/localtestingmulti2-cont2" ), 'isMultiMaster' => true ) ) ) ); - $this->filesToPrune = array(); } private static function baseStorePath() { @@ -214,7 +212,7 @@ class FileBackendTest extends MediaWikiTestCase { * @dataProvider provider_testStore */ public function testStore( $op ) { - $this->filesToPrune[] = $op['src']; + $this->addTmpFiles( $op['src'] ); $this->backend = $this->singleBackend; $this->tearDownFiles(); @@ -224,7 +222,6 @@ class FileBackendTest extends MediaWikiTestCase { $this->backend = $this->multiBackend; $this->tearDownFiles(); $this->doTestStore( $op ); - $this->filesToPrune[] = $op['src']; # avoid file leaking $this->tearDownFiles(); } @@ -275,27 +272,15 @@ class FileBackendTest extends MediaWikiTestCase { $tmpName = TempFSFile::factory( "unittests_", 'txt' )->getPath(); $toPath = self::baseStorePath() . '/unittest-cont1/e/fun/obj1.txt'; $op = array( 'op' => 'store', 'src' => $tmpName, 'dst' => $toPath ); - $cases[] = array( - $op, // operation - $tmpName, // source - $toPath, // dest - ); + $cases[] = array( $op ); $op2 = $op; $op2['overwrite'] = true; - $cases[] = array( - $op2, // operation - $tmpName, // source - $toPath, // dest - ); + $cases[] = array( $op2 ); - $op2 = $op; - $op2['overwriteSame'] = true; - $cases[] = array( - $op2, // operation - $tmpName, // source - $toPath, // dest - ); + $op3 = $op; + $op3['overwriteSame'] = true; + $cases[] = array( $op3 ); return $cases; } @@ -948,18 +933,14 @@ class FileBackendTest extends MediaWikiTestCase { * @dataProvider provider_testConcatenate */ public function testConcatenate( $op, $srcs, $srcsContent, $alreadyExists, $okStatus ) { - $this->filesToPrune[] = $op['dst']; - $this->backend = $this->singleBackend; $this->tearDownFiles(); $this->doTestConcatenate( $op, $srcs, $srcsContent, $alreadyExists, $okStatus ); - $this->filesToPrune[] = $op['dst']; # avoid file leaking $this->tearDownFiles(); $this->backend = $this->multiBackend; $this->tearDownFiles(); $this->doTestConcatenate( $op, $srcs, $srcsContent, $alreadyExists, $okStatus ); - $this->filesToPrune[] = $op['dst']; # avoid file leaking $this->tearDownFiles(); } @@ -983,7 +964,7 @@ class FileBackendTest extends MediaWikiTestCase { $this->assertGoodStatus( $status, "Creation of source files succeeded ($backendName)." ); - $dest = $params['dst']; + $dest = $params['dst'] = $this->getNewTempFile(); if ( $alreadyExists ) { $ok = file_put_contents( $dest, 'blah...blah...waahwaah' ) !== false; $this->assertEquals( true, $ok, @@ -1029,8 +1010,6 @@ class FileBackendTest extends MediaWikiTestCase { public static function provider_testConcatenate() { $cases = array(); - $rand = mt_rand( 0, 2000000000 ) . time(); - $dest = wfTempDir() . "/randomfile!$rand.txt"; $srcs = array( self::baseStorePath() . '/unittest-cont1/e/file1.txt', self::baseStorePath() . '/unittest-cont1/e/file2.txt', @@ -1055,7 +1034,7 @@ class FileBackendTest extends MediaWikiTestCase { 'lkaem;a', 'legma' ); - $params = array( 'srcs' => $srcs, 'dst' => $dest ); + $params = array( 'srcs' => $srcs ); $cases[] = array( $params, // operation @@ -1761,16 +1740,13 @@ class FileBackendTest extends MediaWikiTestCase { $fileCContents = 'eigna[ogmewt 3qt g3qg flew[ag'; $tmpNameA = TempFSFile::factory( "unittests_", 'txt' )->getPath(); - file_put_contents( $tmpNameA, $fileAContents ); $tmpNameB = TempFSFile::factory( "unittests_", 'txt' )->getPath(); - file_put_contents( $tmpNameB, $fileBContents ); $tmpNameC = TempFSFile::factory( "unittests_", 'txt' )->getPath(); + $this->addTmpFiles( array( $tmpNameA, $tmpNameB, $tmpNameC ) ); + file_put_contents( $tmpNameA, $fileAContents ); + file_put_contents( $tmpNameB, $fileBContents ); file_put_contents( $tmpNameC, $fileCContents ); - $this->filesToPrune[] = $tmpNameA; # avoid file leaking - $this->filesToPrune[] = $tmpNameB; # avoid file leaking - $this->filesToPrune[] = $tmpNameC; # avoid file leaking - $fileA = "$base/unittest-cont1/e/a/b/fileA.txt"; $fileB = "$base/unittest-cont1/e/a/b/fileB.txt"; $fileC = "$base/unittest-cont1/e/a/b/fileC.txt"; @@ -2434,16 +2410,10 @@ class FileBackendTest extends MediaWikiTestCase { } function tearDownFiles() { - foreach ( $this->filesToPrune as $file ) { - if ( is_file( $file ) ) { - unlink( $file ); - } - } $containers = array( 'unittest-cont1', 'unittest-cont2', 'unittest-cont-bad' ); foreach ( $containers as $container ) { $this->deleteFiles( $container ); } - $this->filesToPrune = array(); } private function deleteFiles( $container ) { diff --git a/tests/phpunit/includes/parser/NewParserTest.php b/tests/phpunit/includes/parser/NewParserTest.php index 713c32d5e7..3ce3e1ff00 100644 --- a/tests/phpunit/includes/parser/NewParserTest.php +++ b/tests/phpunit/includes/parser/NewParserTest.php @@ -480,16 +480,16 @@ class NewParserTest extends MediaWikiTestCase { */ protected function getUploadDir() { if ( $this->keepUploads ) { + // Don't use getNewTempDirectory() as this is meant to persist $dir = wfTempDir() . '/mwParser-images'; if ( is_dir( $dir ) ) { return $dir; } } else { - $dir = wfTempDir() . "/mwParser-" . mt_rand() . "-images"; + $dir = $this->getNewTempDirectory(); } - // wfDebug( "Creating upload directory $dir\n" ); if ( file_exists( $dir ) ) { wfDebug( "Already exists!\n" ); diff --git a/tests/phpunit/includes/upload/UploadBaseTest.php b/tests/phpunit/includes/upload/UploadBaseTest.php index f23b264f24..63ad8c0555 100644 --- a/tests/phpunit/includes/upload/UploadBaseTest.php +++ b/tests/phpunit/includes/upload/UploadBaseTest.php @@ -93,7 +93,7 @@ class UploadBaseTest extends MediaWikiTestCase { // Helper used to create an empty file of size $size. private function createFileOfSize( $size ) { - $filename = tempnam( wfTempDir(), "mwuploadtest" ); + $filename = $this->getNewTempFile(); $fh = fopen( $filename, 'w' ); ftruncate( $fh, $size ); @@ -118,7 +118,6 @@ class UploadBaseTest extends MediaWikiTestCase { $filename = $this->createFileOfSize( 100 ); $this->upload->initializePathInfo( basename( $filename ) . '.txt', $filename, 100 ); $result = $this->upload->verifyUpload(); - unlink( $filename ); $this->assertEquals( array( 'status' => UploadBase::OK ), -- 2.20.1