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

diff --git a/tests/phpunit/MediaWikiTestCase.php b/tests/phpunit/MediaWikiTestCase.php
index 2b46fc3..e49c391 100644 (file)
--- 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 2bfabe4..331fb3b 100644 (file)
--- 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 7e51339..d4d9651 100644 (file)
--- 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 7fdefb6..b4b1bf3 100644 (file)
--- 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 9558cc7..b40d2d2 100644 (file)
--- 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 713c32d..3ce3e1f 100644 (file)
--- 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 f23b264..63ad8c0 100644 (file)
--- 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 ),