From 4938038175bf2cb6168291c766ff15337209370b Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Sat, 4 May 2019 21:41:10 +0100 Subject: [PATCH] tests: Remove use of wfRandomString() for test fixtures This reduces confidence in the test. There is no guruantee that it won't return the same value twice during the duration of a full PHPUnit run of all test suites, whether twice in a row or 20 minutes apart. For a test that needs a string of any kind, use an explicit, consinstent and cheap literal value. For a test that specifically needs some kind of uniqueness compared to something else within the same test case, do so explicitly. Tests that require something globally unique (for some undefined/vague definition of "global") were not found, and should not exist anyway. Also, in libs/objectcache tests, fix order of parameters in some assertions (expected first, then actual), and use assertFalse/assertSame instead of assertEqual for cases where false is expected to remove tolerance of other loosely equal values. Change-Id: Ifc60e88178da471330b94bfbf12e2731d2efc77d --- .../includes/GlobalFunctions/GlobalTest.php | 8 ++----- tests/phpunit/includes/TestUserRegistry.php | 4 ++-- .../includes/config/GlobalVarConfigTest.php | 7 +++--- .../includes/filebackend/FileBackendTest.php | 8 +++---- .../includes/jobqueue/JobQueueTest.php | 3 +-- .../objectcache/MultiWriteBagOStuffTest.php | 20 ++++++++--------- .../objectcache/ReplicatedBagOStuffTest.php | 22 +++++++++---------- 7 files changed, 32 insertions(+), 40 deletions(-) diff --git a/tests/phpunit/includes/GlobalFunctions/GlobalTest.php b/tests/phpunit/includes/GlobalFunctions/GlobalTest.php index 9443b19e06..1210a507db 100644 --- a/tests/phpunit/includes/GlobalFunctions/GlobalTest.php +++ b/tests/phpunit/includes/GlobalFunctions/GlobalTest.php @@ -74,12 +74,8 @@ class GlobalTest extends MediaWikiTestCase { $this->assertFalse( wfRandomString() == wfRandomString() ); - $this->assertEquals( - strlen( wfRandomString( 10 ) ), 10 - ); - $this->assertTrue( - preg_match( '/^[0-9a-f]+$/i', wfRandomString() ) === 1 - ); + $this->assertSame( 10, strlen( wfRandomString( 10 ) ), 'length' ); + $this->assertSame( 1, preg_match( '/^[0-9a-f]+$/i', wfRandomString() ), 'pattern' ); } /** diff --git a/tests/phpunit/includes/TestUserRegistry.php b/tests/phpunit/includes/TestUserRegistry.php index 3064a3d316..40a5dc5c31 100644 --- a/tests/phpunit/includes/TestUserRegistry.php +++ b/tests/phpunit/includes/TestUserRegistry.php @@ -33,7 +33,7 @@ class TestUserRegistry { */ public static function getMutableTestUser( $testName, $groups = [] ) { $id = self::getNextId(); - $password = wfRandomString( 20 ); + $password = "password_for_test_user_id_{$id}"; $testUser = new TestUser( "TestUser $testName $id", // username "Name $id", // real name @@ -75,7 +75,7 @@ class TestUserRegistry { $password = 'UTSysopPassword'; } else { $username = "TestUser $id"; - $password = wfRandomString( 20 ); + $password = "password_for_test_user_id_{$id}"; } self::$testUsers[$key] = $testUser = new TestUser( $username, // username diff --git a/tests/phpunit/includes/config/GlobalVarConfigTest.php b/tests/phpunit/includes/config/GlobalVarConfigTest.php index ec443e7a8a..591f27dd66 100644 --- a/tests/phpunit/includes/config/GlobalVarConfigTest.php +++ b/tests/phpunit/includes/config/GlobalVarConfigTest.php @@ -19,11 +19,10 @@ class GlobalVarConfigTest extends MediaWikiTestCase { */ public function testConstructor( $prefix ) { $var = $prefix . 'GlobalVarConfigTest'; - $rand = wfRandomString(); - $this->setMwGlobals( $var, $rand ); + $this->setMwGlobals( $var, 'testvalue' ); $config = new GlobalVarConfig( $prefix ); $this->assertInstanceOf( GlobalVarConfig::class, $config ); - $this->assertEquals( $rand, $config->get( 'GlobalVarConfigTest' ) ); + $this->assertEquals( 'testvalue', $config->get( 'GlobalVarConfigTest' ) ); } public static function provideConstructor() { @@ -41,7 +40,7 @@ class GlobalVarConfigTest extends MediaWikiTestCase { * @covers GlobalVarConfig::hasWithPrefix */ public function testHas() { - $this->setMwGlobals( 'wgGlobalVarConfigTestHas', wfRandomString() ); + $this->setMwGlobals( 'wgGlobalVarConfigTestHas', 'testvalue' ); $config = new GlobalVarConfig(); $this->assertTrue( $config->has( 'GlobalVarConfigTestHas' ) ); $this->assertFalse( $config->has( 'GlobalVarConfigTestNotHas' ) ); diff --git a/tests/phpunit/includes/filebackend/FileBackendTest.php b/tests/phpunit/includes/filebackend/FileBackendTest.php index 50696af8e9..8548fdeb5b 100644 --- a/tests/phpunit/includes/filebackend/FileBackendTest.php +++ b/tests/phpunit/includes/filebackend/FileBackendTest.php @@ -98,7 +98,7 @@ class FileBackendTest extends MediaWikiTestCase { 'name' => 'localtesting', 'lockManager' => LockManagerGroup::singleton()->get( 'fsLockManager' ), 'parallelize' => 'implicit', - 'wikiId' => wfWikiID() . wfRandomString(), + 'wikiId' => 'testdb', 'backends' => [ [ 'name' => 'localmultitesting1', @@ -2569,11 +2569,9 @@ class FileBackendTest extends MediaWikiTestCase { 'wikiId' => wfWikiID() ] ) ); - $name = wfRandomString( 300 ); - $input = [ 'headers' => [ - 'content-Disposition' => FileBackend::makeContentDisposition( 'inline', $name ), + 'content-Disposition' => FileBackend::makeContentDisposition( 'inline', 'name' ), 'Content-dUration' => 25.6, 'X-LONG-VALUE' => str_pad( '0', 300 ), 'CONTENT-LENGTH' => 855055, @@ -2581,7 +2579,7 @@ class FileBackendTest extends MediaWikiTestCase { ]; $expected = [ 'headers' => [ - 'content-disposition' => FileBackend::makeContentDisposition( 'inline', $name ), + 'content-disposition' => FileBackend::makeContentDisposition( 'inline', 'name' ), 'content-duration' => 25.6, 'content-length' => 855055 ] diff --git a/tests/phpunit/includes/jobqueue/JobQueueTest.php b/tests/phpunit/includes/jobqueue/JobQueueTest.php index 1baaa5489e..ce07f78bab 100644 --- a/tests/phpunit/includes/jobqueue/JobQueueTest.php +++ b/tests/phpunit/includes/jobqueue/JobQueueTest.php @@ -259,8 +259,7 @@ class JobQueueTest extends MediaWikiTestCase { $this->assertEquals( 0, $queue->getSize(), "Queue is empty ($desc)" ); $this->assertEquals( 0, $queue->getAcquiredCount(), "Queue is empty ($desc)" ); - $id = wfRandomString( 32 ); - $root1 = Job::newRootJobParams( "nulljobspam:$id" ); // task ID/timestamp + $root1 = Job::newRootJobParams( "nulljobspam:testId" ); // task ID/timestamp for ( $i = 0; $i < 5; ++$i ) { $this->assertNull( $queue->push( $this->newJob( 0, $root1 ) ), "Push worked ($desc)" ); } diff --git a/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php index 0376803f41..9f88474e7b 100644 --- a/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php +++ b/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php @@ -28,8 +28,8 @@ class MultiWriteBagOStuffTest extends MediaWikiTestCase { * @covers MultiWriteBagOStuff::doWrite */ public function testSetImmediate() { - $key = wfRandomString(); - $value = wfRandomString(); + $key = 'key'; + $value = 'value'; $this->cache->set( $key, $value ); // Set in tier 1 @@ -42,8 +42,8 @@ class MultiWriteBagOStuffTest extends MediaWikiTestCase { * @covers MultiWriteBagOStuff */ public function testSyncMerge() { - $key = wfRandomString(); - $value = wfRandomString(); + $key = 'keyA'; + $value = 'value'; $func = function () use ( $value ) { return $value; }; @@ -56,14 +56,14 @@ class MultiWriteBagOStuffTest extends MediaWikiTestCase { // Set in tier 1 $this->assertEquals( $value, $this->cache1->get( $key ), 'Written to tier 1' ); // Not yet set in tier 2 - $this->assertEquals( false, $this->cache2->get( $key ), 'Not written to tier 2' ); + $this->assertFalse( $this->cache2->get( $key ), 'Not written to tier 2' ); $dbw->commit(); // Set in tier 2 $this->assertEquals( $value, $this->cache2->get( $key ), 'Written to tier 2' ); - $key = wfRandomString(); + $key = 'keyB'; $dbw->begin(); $this->cache->merge( $key, $func, 0, 1, BagOStuff::WRITE_SYNC ); @@ -80,8 +80,8 @@ class MultiWriteBagOStuffTest extends MediaWikiTestCase { * @covers MultiWriteBagOStuff::set */ public function testSetDelayed() { - $key = wfRandomString(); - $value = (object)[ 'v' => wfRandomString() ]; + $key = 'key'; + $value = (object)[ 'v' => 'saved value' ]; $expectValue = clone $value; // XXX: DeferredUpdates bound to transactions in CLI mode @@ -90,12 +90,12 @@ class MultiWriteBagOStuffTest extends MediaWikiTestCase { $this->cache->set( $key, $value ); // Test that later changes to $value don't affect the saved value (e.g. T168040) - $value->v = 'bogus'; + $value->v = 'other value'; // Set in tier 1 $this->assertEquals( $expectValue, $this->cache1->get( $key ), 'Written to tier 1' ); // Not yet set in tier 2 - $this->assertEquals( false, $this->cache2->get( $key ), 'Not written to tier 2' ); + $this->assertFalse( $this->cache2->get( $key ), 'Not written to tier 2' ); $dbw->commit(); diff --git a/tests/phpunit/includes/libs/objectcache/ReplicatedBagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/ReplicatedBagOStuffTest.php index b7f22ece94..550ec0bd09 100644 --- a/tests/phpunit/includes/libs/objectcache/ReplicatedBagOStuffTest.php +++ b/tests/phpunit/includes/libs/objectcache/ReplicatedBagOStuffTest.php @@ -23,40 +23,40 @@ class ReplicatedBagOStuffTest extends MediaWikiTestCase { * @covers ReplicatedBagOStuff::set */ public function testSet() { - $key = wfRandomString(); - $value = wfRandomString(); + $key = 'a key'; + $value = 'a value'; $this->cache->set( $key, $value ); // Write to master. - $this->assertEquals( $this->writeCache->get( $key ), $value ); + $this->assertEquals( $value, $this->writeCache->get( $key ) ); // Don't write to replica. Replication is deferred to backend. - $this->assertEquals( $this->readCache->get( $key ), false ); + $this->assertFalse( $this->readCache->get( $key ) ); } /** * @covers ReplicatedBagOStuff::get */ public function testGet() { - $key = wfRandomString(); + $key = 'a key'; - $write = wfRandomString(); + $write = 'one value'; $this->writeCache->set( $key, $write ); - $read = wfRandomString(); + $read = 'another value'; $this->readCache->set( $key, $read ); // Read from replica. - $this->assertEquals( $this->cache->get( $key ), $read ); + $this->assertEquals( $read, $this->cache->get( $key ) ); } /** * @covers ReplicatedBagOStuff::get */ public function testGetAbsent() { - $key = wfRandomString(); - $value = wfRandomString(); + $key = 'a key'; + $value = 'a value'; $this->writeCache->set( $key, $value ); // Don't read from master. No failover if value is absent. - $this->assertEquals( $this->cache->get( $key ), false ); + $this->assertFalse( $this->cache->get( $key ) ); } } -- 2.20.1