r110180 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r110179‎ | r110180 | r110181 >
Date:22:46, 27 January 2012
Author:aaron
Status:ok (Comments)
Tags:filebackend 
Comment:
Fixed a bunch of dir leakage
Modified paths:
  • /trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/filerepo/StoreBatchTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/filerepo/StoreBatchTest.php
——@@ -9,7 +9,7 @@
1010 parent::setUp();
1111
1212 # Forge a FSRepo object to not have to rely on local wiki settings
13 - $this->tmpDir = wfTempDir() . '/store-batch-test-' . time() . '-' . mt_rand();
 13+ $tmpPrefix = wfTempDir() . '/storebatch-test-' . time() . '-' . mt_rand();
1414 if ( $this->getCliArg( 'use-filebackend=' ) ) {
1515 $name = $this->getCliArg( 'use-filebackend=' );
1616 $useConfig = array();
——@@ -19,16 +19,17 @@
2020 }
2121 }
2222 $useConfig['name'] = 'localtesting'; // swap name
23 - $backend = new $conf['class']( $useConfig );
 23+ $class = $conf['class'];
 24+ self::$backendToUse = new $class( $useConfig );
2425 } else {
2526 $backend = new FSFileBackend( array(
2627 'name' => 'local-backend',
2728 'lockManager' => 'nullLockManager',
2829 'containerPaths' => array(
29 - 'unittests-public' => $this->tmpDir . "/public",
30 - 'unittests-thumb' => $this->tmpDir . "/thumb",
31 - 'unittests-temp' => $this->tmpDir . "/temp",
32 - 'unittests-deleted' => $this->tmpDir . "/deleted",
 30+ 'unittests-public' => "{$tmpPrefix}-public",
 31+ 'unittests-thumb' => "{$tmpPrefix}-thumb",
 32+ 'unittests-temp' => "{$tmpPrefix}-temp",
 33+ 'unittests-deleted' => "{$tmpPrefix}-deleted",
3334 )
3435 ) );
3536 }
Index: trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php
——@@ -5,13 +5,14 @@
66 */
77 class FileBackendTest extends MediaWikiTestCase {
88 private $backend, $multiBackend;
9 - private $filesToPrune;
 9+ private $filesToPrune = array();
 10+ private $dirsToPrune = array();
1011 private static $backendToUse;
1112
1213 function setUp() {
1314 global $wgFileBackends;
1415 parent::setUp();
15 - $tmpDir = wfTempDir() . '/file-backend-test-' . time() . '-' . mt_rand();
 16+ $tmpPrefix = wfTempDir() . '/filebackend-unittest-' . time() . '-' . mt_rand();
1617 if ( $this->getCliArg( 'use-filebackend=' ) ) {
1718 if ( self::$backendToUse ) {
1819 $this->singleBackend = self::$backendToUse;
——@@ -24,7 +25,8 @@
2526 }
2627 }
2728 $useConfig['name'] = 'localtesting'; // swap name
28 - self::$backendToUse = new $conf['class']( $useConfig );
 29+ $class = $conf['class'];
 30+ self::$backendToUse = new $class( $useConfig );
2931 $this->singleBackend = self::$backendToUse;
3032 }
3133 } else {
——@@ -32,8 +34,8 @@
3335 'name' => 'localtesting',
3436 'lockManager' => 'fsLockManager',
3537 'containerPaths' => array(
36 - 'unittest-cont1' => "$tmpDir/localtesting/unittest-cont1",
37 - 'unittest-cont2' => "$tmpDir/localtesting/unittest-cont2" )
 38+ 'unittest-cont1' => "{$tmpPrefix}-localtesting-cont1",
 39+ 'unittest-cont2' => "{$tmpPrefix}-localtesting-cont2" )
3840 ) );
3941 }
4042 $this->multiBackend = new FileBackendMultiWrite( array(
——@@ -45,8 +47,8 @@
4648 'class' => 'FSFileBackend',
4749 'lockManager' => 'nullLockManager',
4850 'containerPaths' => array(
49 - 'unittest-cont1' => "$tmpDir/localtestingmulti1/cont1",
50 - 'unittest-cont2' => "$tmpDir/localtestingmulti1/unittest-cont2" ),
 51+ 'unittest-cont1' => "{$tmpPrefix}-localtestingmulti1-cont1",
 52+ 'unittest-cont2' => "{$tmpPrefix}-localtestingmulti1-cont2" ),
5153 'isMultiMaster' => false
5254 ),
5355 array(
——@@ -54,8 +56,8 @@
5557 'class' => 'FSFileBackend',
5658 'lockManager' => 'nullLockManager',
5759 'containerPaths' => array(
58 - 'unittest-cont1' => "$tmpDir/localtestingmulti2/cont1",
59 - 'unittest-cont2' => "$tmpDir/localtestingmulti2/unittest-cont2" ),
 60+ 'unittest-cont1' => "{$tmpPrefix}-localtestingmulti2-cont1",
 61+ 'unittest-cont2' => "{$tmpPrefix}-localtestingmulti2-cont2" ),
6062 'isMultiMaster' => true
6163 )
6264 )
——@@ -74,24 +76,26 @@
7577 /**
7678 * @dataProvider provider_testStore
7779 */
78 - public function testStore( $op, $source, $dest ) {
79 - $this->filesToPrune[] = $source;
 80+ public function testStore( $op ) {
 81+ $this->filesToPrune[] = $op['src'];
8082
8183 $this->backend = $this->singleBackend;
8284 $this->tearDownFiles();
83 - $this->doTestStore( $op, $source, $dest );
 85+ $this->doTestStore( $op );
8486 $this->tearDownFiles();
8587
8688 $this->backend = $this->multiBackend;
8789 $this->tearDownFiles();
88 - $this->doTestStore( $op, $source, $dest );
 90+ $this->doTestStore( $op );
8991 $this->tearDownFiles();
9092 }
9193
92 - function doTestStore( $op, $source, $dest ) {
 94+ function doTestStore( $op ) {
9395 $backendName = $this->backendClass();
9496
95 - $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
 97+ $source = $op['src'];
 98+ $dest = $op['dst'];
 99+ $this->prepare( array( 'dir' => dirname( $dest ) ) );
96100
97101 file_put_contents( $source, "Unit test file" );
98102
——@@ -156,23 +160,25 @@
157161 /**
158162 * @dataProvider provider_testCopy
159163 */
160 - public function testCopy( $op, $source, $dest ) {
 164+ public function testCopy( $op ) {
161165 $this->backend = $this->singleBackend;
162166 $this->tearDownFiles();
163 - $this->doTestCopy( $op, $source, $dest );
 167+ $this->doTestCopy( $op );
164168 $this->tearDownFiles();
165169
166170 $this->backend = $this->multiBackend;
167171 $this->tearDownFiles();
168 - $this->doTestCopy( $op, $source, $dest );
 172+ $this->doTestCopy( $op );
169173 $this->tearDownFiles();
170174 }
171175
172 - function doTestCopy( $op, $source, $dest ) {
 176+ function doTestCopy( $op ) {
173177 $backendName = $this->backendClass();
174178
175 - $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
176 - $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
 179+ $source = $op['src'];
 180+ $dest = $op['dst'];
 181+ $this->prepare( array( 'dir' => dirname( $source ) ) );
 182+ $this->prepare( array( 'dir' => dirname( $dest ) ) );
177183
178184 $status = $this->backend->doOperation(
179185 array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) );
——@@ -242,23 +248,25 @@
243249 /**
244250 * @dataProvider provider_testMove
245251 */
246 - public function testMove( $op, $source, $dest ) {
 252+ public function testMove( $op ) {
247253 $this->backend = $this->singleBackend;
248254 $this->tearDownFiles();
249 - $this->doTestMove( $op, $source, $dest );
 255+ $this->doTestMove( $op );
250256 $this->tearDownFiles();
251257
252258 $this->backend = $this->multiBackend;
253259 $this->tearDownFiles();
254 - $this->doTestMove( $op, $source, $dest );
 260+ $this->doTestMove( $op );
255261 $this->tearDownFiles();
256262 }
257263
258 - private function doTestMove( $op, $source, $dest ) {
 264+ private function doTestMove( $op ) {
259265 $backendName = $this->backendClass();
260266
261 - $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
262 - $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
 267+ $source = $op['src'];
 268+ $dest = $op['dst'];
 269+ $this->prepare( array( 'dir' => dirname( $source ) ) );
 270+ $this->prepare( array( 'dir' => dirname( $dest ) ) );
263271
264272 $status = $this->backend->doOperation(
265273 array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) );
——@@ -329,22 +337,23 @@
330338 /**
331339 * @dataProvider provider_testDelete
332340 */
333 - public function testDelete( $op, $source, $withSource, $okStatus ) {
 341+ public function testDelete( $op, $withSource, $okStatus ) {
334342 $this->backend = $this->singleBackend;
335343 $this->tearDownFiles();
336 - $this->doTestDelete( $op, $source, $withSource, $okStatus );
 344+ $this->doTestDelete( $op, $withSource, $okStatus );
337345 $this->tearDownFiles();
338346
339347 $this->backend = $this->multiBackend;
340348 $this->tearDownFiles();
341 - $this->doTestDelete( $op, $source, $withSource, $okStatus );
 349+ $this->doTestDelete( $op, $withSource, $okStatus );
342350 $this->tearDownFiles();
343351 }
344352
345 - private function doTestDelete( $op, $source, $withSource, $okStatus ) {
 353+ private function doTestDelete( $op, $withSource, $okStatus ) {
346354 $backendName = $this->backendClass();
347355
348 - $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
 356+ $source = $op['src'];
 357+ $this->prepare( array( 'dir' => dirname( $source ) ) );
349358
350359 if ( $withSource ) {
351360 $status = $this->backend->doOperation(
——@@ -386,14 +395,12 @@
387396 $op = array( 'op' => 'delete', 'src' => $source );
388397 $cases[] = array(
389398 $op, // operation
390 - $source, // source
391399 true, // with source
392400 true // succeeds
393401 );
394402
395403 $cases[] = array(
396404 $op, // operation
397 - $source, // source
398405 false, // without source
399406 false // fails
400407 );
——@@ -401,7 +408,6 @@
402409 $op['ignoreMissingSource'] = true;
403410 $cases[] = array(
404411 $op, // operation
405 - $source, // source
406412 false, // without source
407413 true // succeeds
408414 );
——@@ -428,7 +434,7 @@
429435 $backendName = $this->backendClass();
430436
431437 $dest = $op['dst'];
432 - $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
 438+ $this->prepare( array( 'dir' => dirname( $dest ) ) );
433439
434440 $oldText = 'blah...blah...waahwaah';
435441 if ( $alreadyExists ) {
——@@ -553,7 +559,7 @@
554560 // Create sources
555561 $ops = array();
556562 foreach ( $srcs as $i => $source ) {
557 - $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
 563+ $this->prepare( array( 'dir' => dirname( $source ) ) );
558564 $ops[] = array(
559565 'op' => 'create', // operation
560566 'dst' => $source, // source
——@@ -662,15 +668,15 @@
663669 /**
664670 * @dataProvider provider_testGetFileContents
665671 */
666 - public function testGetFileContents( $src, $content ) {
 672+ public function testGetFileContents( $source, $content ) {
667673 $this->backend = $this->singleBackend;
668674 $this->tearDownFiles();
669 - $this->doTestGetFileContents( $src, $content );
 675+ $this->doTestGetFileContents( $source, $content );
670676 $this->tearDownFiles();
671677
672678 $this->backend = $this->multiBackend;
673679 $this->tearDownFiles();
674 - $this->doTestGetFileContents( $src, $content );
 680+ $this->doTestGetFileContents( $source, $content );
675681 $this->tearDownFiles();
676682 }
677683
——@@ -680,7 +686,7 @@
681687 public function doTestGetFileContents( $source, $content ) {
682688 $backendName = $this->backendClass();
683689
684 - $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
 690+ $this->prepare( array( 'dir' => dirname( $source ) ) );
685691
686692 $status = $this->backend->doOperation(
687693 array( 'op' => 'create', 'content' => $content, 'dst' => $source ) );
——@@ -710,22 +716,22 @@
711717 /**
712718 * @dataProvider provider_testGetLocalCopy
713719 */
714 - public function testGetLocalCopy( $src, $content ) {
 720+ public function testGetLocalCopy( $source, $content ) {
715721 $this->backend = $this->singleBackend;
716722 $this->tearDownFiles();
717 - $this->doTestGetLocalCopy( $src, $content );
 723+ $this->doTestGetLocalCopy( $source, $content );
718724 $this->tearDownFiles();
719725
720726 $this->backend = $this->multiBackend;
721727 $this->tearDownFiles();
722 - $this->doTestGetLocalCopy( $src, $content );
 728+ $this->doTestGetLocalCopy( $source, $content );
723729 $this->tearDownFiles();
724730 }
725731
726732 public function doTestGetLocalCopy( $source, $content ) {
727733 $backendName = $this->backendClass();
728734
729 - $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
 735+ $this->prepare( array( 'dir' => dirname( $source ) ) );
730736
731737 $status = $this->backend->doOperation(
732738 array( 'op' => 'create', 'content' => $content, 'dst' => $source ) );
——@@ -753,22 +759,22 @@
754760 /**
755761 * @dataProvider provider_testGetLocalReference
756762 */
757 - public function testGetLocalReference( $src, $content ) {
 763+ public function testGetLocalReference( $source, $content ) {
758764 $this->backend = $this->singleBackend;
759765 $this->tearDownFiles();
760 - $this->doTestGetLocalReference( $src, $content );
 766+ $this->doTestGetLocalReference( $source, $content );
761767 $this->tearDownFiles();
762768
763769 $this->backend = $this->multiBackend;
764770 $this->tearDownFiles();
765 - $this->doTestGetLocalReference( $src, $content );
 771+ $this->doTestGetLocalReference( $source, $content );
766772 $this->tearDownFiles();
767773 }
768774
769775 private function doTestGetLocalReference( $source, $content ) {
770776 $backendName = $this->backendClass();
771777
772 - $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
 778+ $this->prepare( array( 'dir' => dirname( $source ) ) );
773779
774780 $status = $this->backend->doOperation(
775781 array( 'op' => 'create', 'content' => $content, 'dst' => $source ) );
——@@ -799,9 +805,11 @@
800806 public function testPrepareAndClean( $path, $isOK ) {
801807 $this->backend = $this->singleBackend;
802808 $this->doTestPrepareAndClean( $path, $isOK );
 809+ $this->tearDownFiles();
803810
804811 $this->backend = $this->multiBackend;
805812 $this->doTestPrepareAndClean( $path, $isOK );
 813+ $this->tearDownFiles();
806814 }
807815
808816 function provider_testPrepareAndClean() {
——@@ -817,7 +825,7 @@
818826 function doTestPrepareAndClean( $path, $isOK ) {
819827 $backendName = $this->backendClass();
820828
821 - $status = $this->backend->prepare( array( 'dir' => $path ) );
 829+ $status = $this->prepare( array( 'dir' => dirname( $path ) ) );
822830 if ( $isOK ) {
823831 $this->assertEquals( array(), $status->errors,
824832 "Preparing dir $path succeeded without warnings ($backendName)." );
——@@ -828,7 +836,7 @@
829837 "Preparing dir $path failed ($backendName)." );
830838 }
831839
832 - $status = $this->backend->clean( array( 'dir' => $path ) );
 840+ $status = $this->backend->clean( array( 'dir' => dirname( $path ) ) );
833841 if ( $isOK ) {
834842 $this->assertEquals( array(), $status->errors,
835843 "Cleaning dir $path succeeded without warnings ($backendName)." );
——@@ -844,10 +852,16 @@
845853
846854 public function testDoOperations() {
847855 $this->backend = $this->singleBackend;
 856+ $this->tearDownFiles();
848857 $this->doTestDoOperations();
 858+ $this->tearDownFiles();
849859
850860 $this->backend = $this->multiBackend;
 861+ $this->tearDownFiles();
851862 $this->doTestDoOperations();
 863+ $this->tearDownFiles();
 864+
 865+ // @TODO: test some cases where the ops should fail
852866 }
853867
854868 function doTestDoOperations() {
——@@ -861,11 +875,11 @@
862876 $fileCContents = 'eigna[ogmewt 3qt g3qg flew[ag';
863877 $fileD = "$base/unittest-cont1/a/b/fileD.txt";
864878
865 - $this->backend->prepare( array( 'dir' => dirname( $fileA ) ) );
 879+ $this->prepare( array( 'dir' => dirname( $fileA ) ) );
866880 $this->backend->create( array( 'dst' => $fileA, 'content' => $fileAContents ) );
867 - $this->backend->prepare( array( 'dir' => dirname( $fileB ) ) );
 881+ $this->prepare( array( 'dir' => dirname( $fileB ) ) );
868882 $this->backend->create( array( 'dst' => $fileB, 'content' => $fileBContents ) );
869 - $this->backend->prepare( array( 'dir' => dirname( $fileC ) ) );
 883+ $this->prepare( array( 'dir' => dirname( $fileC ) ) );
870884 $this->backend->create( array( 'dst' => $fileC, 'content' => $fileCContents ) );
871885
872886 $status = $this->backend->doOperations( array(
——@@ -920,8 +934,6 @@
921935 $this->assertEquals( wfBaseConvert( sha1( $fileBContents ), 16, 36, 31 ),
922936 $this->backend->getFileSha1Base36( array( 'src' => $fileC ) ),
923937 "Correct file SHA-1 of $fileC" );
924 -
925 - // @TODO: test some cases where the ops should fail
926938 }
927939
928940 public function testGetFileList() {
——@@ -961,7 +973,7 @@
962974 $ops = array();
963975 foreach ( $files as $file ) {
964976 $ops[] = array( 'op' => 'create', 'content' => 'xxy', 'dst' => $file );
965 - $this->backend->prepare( array( 'dir' => dirname( $file ) ) );
 977+ $this->prepare( array( 'dir' => dirname( $file ) ) );
966978 }
967979 $status = $this->backend->doOperations( $ops );
968980 $this->assertEquals( array(), $status->errors,
——@@ -1040,40 +1052,54 @@
10411053
10421054 $this->assertEquals( $expected, $list, "Correct file listing ($backendName)." );
10431055
1044 - foreach ( $files as $file ) {
1045 - $this->backend->doOperation( array( 'op' => 'delete', 'src' => "$base/$file" ) );
 1056+ foreach ( $files as $file ) { // clean up
 1057+ $this->backend->doOperation( array( 'op' => 'delete', 'src' => $file ) );
10461058 }
 1059+ foreach ( $files as $file ) { // clean up
 1060+ $this->recursiveClean( FileBackend::parentStoragePath( $file ) );
 1061+ }
10471062
10481063 $iter = $this->backend->getFileList( array( 'dir' => "$base/unittest-cont1/not/exists" ) );
10491064 foreach ( $iter as $iter ) {} // no errors
10501065 }
10511066
 1067+ private function prepare( array $params ) {
 1068+ $this->dirsToPrune[] = $params['dir'];
 1069+ return $this->backend->prepare( $params );
 1070+ }
 1071+
10521072 function tearDownFiles() {
10531073 foreach ( $this->filesToPrune as $file ) {
10541074 @unlink( $file );
10551075 }
10561076 $containers = array( 'unittest-cont1', 'unittest-cont2', 'unittest-cont3' );
10571077 foreach ( $containers as $container ) {
1058 - $this->deleteFiles( $this->backend, $container );
 1078+ $this->deleteFiles( $container );
10591079 }
 1080+ foreach ( $this->dirsToPrune as $dir ) {
 1081+ $this->recursiveClean( $dir );
 1082+ }
 1083+ $this->filesToPrune = $this->dirsToPrune = array();
10601084 }
10611085
1062 - private function deleteFiles( $backend, $container ) {
 1086+ private function deleteFiles( $container ) {
10631087 $base = $this->baseStorePath();
1064 - $iter = $backend->getFileList( array( 'dir' => "$base/$container" ) );
 1088+ $iter = $this->backend->getFileList( array( 'dir' => "$base/$container" ) );
10651089 if ( $iter ) {
1066 - foreach ( $iter as $file ) { // delete files
1067 - $backend->delete( array( 'src' => "$base/$container/$file" ), array( 'force' => 1 ) );
 1090+ foreach ( $iter as $file ) {
 1091+ $this->backend->delete( array( 'src' => "$base/$container/$file" ), array( 'force' => 1 ) );
10681092 }
1069 - foreach ( $iter as $file ) { // delete dirs
1070 - $tmp = $file;
1071 - while ( $tmp = FileBackend::parentStoragePath( $tmp ) ) {
1072 - $backend->clean( array( 'dir' => $tmp ) );
1073 - }
1074 - }
10751093 }
10761094 }
10771095
 1096+ private function recursiveClean( $dir ) {
 1097+ do {
 1098+ if ( !$this->backend->clean( array( 'dir' => $dir ) )->isOK() ) {
 1099+ break;
 1100+ }
 1101+ } while ( $dir = FileBackend::parentStoragePath( $dir ) );
 1102+ }
 1103+
10781104 function tearDown() {
10791105 parent::tearDown();
10801106 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r110198r110180: fixed copy-paste cruft for custom backend code. Also made backend na...aaron00:43, 28 January 2012

Comments

#Comment by Tim Starling (talk | contribs)   23:49, 1 February 2012

There seem to be a lot of changes here that weren't in the commit message. Were they intentional? If so, can you describe them?

#Comment by Aaron Schulz (talk | contribs)   23:52, 1 February 2012

Hmm, looks like I also removed redundant parameters to test that were already in $op. For example $dest is just $op['dest'] and so on. The rest is part of fixing the dir leakage.

Status & tagging log