From: Aryeh Gregor Date: Thu, 22 Aug 2019 06:47:54 +0000 (+0300) Subject: FileJournal tests X-Git-Tag: 1.34.0-rc.0~422^2 X-Git-Url: http://git.cyclocoop.org/?a=commitdiff_plain;h=5bbcaef2317fb88b1639cc8be3d6531033d709c3;p=lhc%2Fweb%2Fwiklou.git FileJournal tests 100% unit test coverage for FileJournal and NullFileJournal. 100% integration test coverage for DBFileJournal. Unit tests for DBFileJournal once it supports injection. I removed FileJournal and NullFileJournal from the list of classes that FileBackendTest tests. It doesn't actually test them, it just happens to run code from them without checking its correctness at all. Depends-On: Ic22075bb5e81b7c2c4c1b8647547aa55306a10a7 Change-Id: I46d10ab7b87c23937aa04d7ec1922abfcf3bd611 --- diff --git a/includes/filebackend/filejournal/DBFileJournal.php b/includes/filebackend/filejournal/DBFileJournal.php index 17cf8f0ca7..7ce2eddacc 100644 --- a/includes/filebackend/filejournal/DBFileJournal.php +++ b/includes/filebackend/filejournal/DBFileJournal.php @@ -24,6 +24,7 @@ use MediaWiki\MediaWikiServices; use Wikimedia\Rdbms\IDatabase; use Wikimedia\Rdbms\DBError; +use Wikimedia\Timestamp\ConvertibleTimestamp; /** * Version of FileJournal that logs to a DB table @@ -36,12 +37,12 @@ class DBFileJournal extends FileJournal { protected $domain; /** - * Construct a new instance from configuration. + * Construct a new instance from configuration. Do not call directly, use FileJournal::factory. * * @param array $config Includes: * domain: database domain ID of the wiki */ - protected function __construct( array $config ) { + public function __construct( array $config ) { parent::__construct( $config ); $this->domain = $config['domain'] ?? $config['wiki']; // b/c @@ -64,7 +65,7 @@ class DBFileJournal extends FileJournal { return $status; } - $now = wfTimestamp( TS_UNIX ); + $now = ConvertibleTimestamp::time(); $data = []; foreach ( $entries as $entry ) { @@ -80,8 +81,11 @@ class DBFileJournal extends FileJournal { try { $dbw->insert( 'filejournal', $data, __METHOD__ ); + // XXX Should we do this deterministically so it's testable? Maybe look at the last two + // digits of a hash of a bunch of the data? if ( mt_rand( 0, 99 ) == 0 ) { - $this->purgeOldLogs(); // occasionally delete old logs + // occasionally delete old logs + $this->purgeOldLogs(); // @codeCoverageIgnore } } catch ( DBError $e ) { $status->fatal( 'filejournal-fail-dbquery', $this->backend ); @@ -164,7 +168,7 @@ class DBFileJournal extends FileJournal { } $dbw = $this->getMasterDB(); - $dbCutoff = $dbw->timestamp( time() - 86400 * $this->ttlDays ); + $dbCutoff = $dbw->timestamp( ConvertibleTimestamp::time() - 86400 * $this->ttlDays ); $dbw->delete( 'filejournal', [ 'fj_timestamp < ' . $dbw->addQuotes( $dbCutoff ) ], diff --git a/includes/libs/filebackend/filejournal/FileJournal.php b/includes/libs/filebackend/filejournal/FileJournal.php index e51242375a..c256d72b1d 100644 --- a/includes/libs/filebackend/filejournal/FileJournal.php +++ b/includes/libs/filebackend/filejournal/FileJournal.php @@ -40,7 +40,7 @@ use Wikimedia\Timestamp\ConvertibleTimestamp; abstract class FileJournal { /** @var string */ protected $backend; - /** @var int */ + /** @var int|false */ protected $ttlDays; /** @@ -153,7 +153,7 @@ abstract class FileJournal { * A starting change ID and/or limit can be specified. * * @param int|null $start Starting change ID or null - * @param int $limit Maximum number of items to return + * @param int $limit Maximum number of items to return (0 = unlimited) * @param string|null &$next Updated to the ID of the next entry. * @return array List of associative arrays, each having: * id : unique, monotonic, ID for this change diff --git a/tests/phpunit/MediaWikiIntegrationTestCase.php b/tests/phpunit/MediaWikiIntegrationTestCase.php index b1eb9effa7..78983f9ca3 100644 --- a/tests/phpunit/MediaWikiIntegrationTestCase.php +++ b/tests/phpunit/MediaWikiIntegrationTestCase.php @@ -10,6 +10,7 @@ use Wikimedia\Rdbms\IDatabase; use Wikimedia\Rdbms\IMaintainableDatabase; use Wikimedia\Rdbms\Database; use Wikimedia\TestingAccessWrapper; +use Wikimedia\Timestamp\ConvertibleTimestamp; /** * @since 1.18 @@ -657,6 +658,9 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase { $this->fail( $message ); } + // If anything faked the time, reset it + ConvertibleTimestamp::setFakeTime( false ); + parent::tearDown(); } diff --git a/tests/phpunit/includes/filebackend/FileBackendTest.php b/tests/phpunit/includes/filebackend/FileBackendTest.php index 8548fdeb5b..bc3696dc0b 100644 --- a/tests/phpunit/includes/filebackend/FileBackendTest.php +++ b/tests/phpunit/includes/filebackend/FileBackendTest.php @@ -29,7 +29,6 @@ use Wikimedia\TestingAccessWrapper; * @covers FileBackendStoreShardDirIterator * @covers FileBackendStoreShardFileIterator * @covers FileBackendStoreShardListIterator - * @covers FileJournal * @covers FileOp * @covers FileOpBatch * @covers HTTPFileStreamer @@ -37,7 +36,6 @@ use Wikimedia\TestingAccessWrapper; * @covers MemoryFileBackend * @covers MoveFileOp * @covers MySqlLockManager - * @covers NullFileJournal * @covers NullFileOp * @covers StoreFileOp * @covers TempFSFile diff --git a/tests/phpunit/includes/filebackend/filejournal/DBFileJournalIntegrationTest.php b/tests/phpunit/includes/filebackend/filejournal/DBFileJournalIntegrationTest.php new file mode 100644 index 0000000000..9a0ba1c574 --- /dev/null +++ b/tests/phpunit/includes/filebackend/filejournal/DBFileJournalIntegrationTest.php @@ -0,0 +1,237 @@ +getDBLoadBalancer()->getConnection( DB_MASTER ); + if ( $db->getType() !== 'mysql' ) { + return; + } + if ( !$db->tableExists( 'filejournal' ) ) { + $db->sourceFile( "$IP/maintenance/archives/patch-filejournal.sql" ); + } + } + + protected function setUp() { + parent::setUp(); + + $db = MediaWikiServices::getInstance()->getDBLoadBalancer()->getConnection( DB_MASTER ); + if ( $db->getType() !== 'mysql' ) { + $this->markTestSkipped( 'No filejournal schema available for this database type' ); + } + + $this->tablesUsed[] = 'filejournal'; + } + + private function getJournal( $options = [] ) { + return FileJournal::factory( + $options + [ 'class' => DBFileJournal::class, 'domain' => wfWikiID() ], + 'local-backend' ); + } + + /** + * @covers ::doLogChangeBatch + */ + public function testDoLogChangeBatch_exceptionDbConnect() { + $journal = $this->getJournal( [ 'domain' => 'no-such-domain' ] ); + + $this->assertEquals( + StatusValue::newFatal( 'filejournal-fail-dbconnect', 'local-backend' ), + $journal->logChangeBatch( [ [] ], 'batch' ) ); + } + + /** + * @covers ::doLogChangeBatch + */ + public function testDoLogChangeBatch_exceptionDbQuery() { + MediaWikiServices::getInstance()->getConfiguredReadOnlyMode()->setReason( 'testing' ); + + $journal = $this->getJournal(); + + $this->assertEquals( + StatusValue::newFatal( 'filejournal-fail-dbquery', 'local-backend' ), + $journal->logChangeBatch( + [ [ 'op' => null, 'path' => '', 'newSha1' => false ] ], 'batch' ) ); + } + + /** + * @covers ::doLogChangeBatch + * @covers ::doGetCurrentPosition + */ + public function testDoGetCurrentPosition() { + $journal = $this->getJournal(); + + $this->assertNull( $journal->getCurrentPosition() ); + + $journal->logChangeBatch( + [ [ 'op' => 'create', 'path' => '/path', 'newSha1' => false ] ], 'batch1' ); + + $this->assertSame( '1', $journal->getCurrentPosition() ); + + $journal->logChangeBatch( + [ [ 'op' => 'create', 'path' => '/path', 'newSha1' => false ] ], 'batch2' ); + + $this->assertSame( '2', $journal->getCurrentPosition() ); + } + + /** + * @covers ::doLogChangeBatch + * @covers ::doGetPositionAtTime + */ + public function testDoGetPositionAtTime() { + $journal = $this->getJournal(); + + $now = time(); + + $this->assertFalse( $journal->getPositionAtTime( $now ) ); + + ConvertibleTimestamp::setFakeTime( $now - 86400 ); + + $journal->logChangeBatch( + [ [ 'op' => 'create', 'path' => '/path', 'newSha1' => false ] ], 'batch1' ); + + ConvertibleTimestamp::setFakeTime( $now - 3600 ); + + $journal->logChangeBatch( + [ [ 'op' => 'create', 'path' => '/path', 'newSha1' => false ] ], 'batch2' ); + + $this->assertFalse( $journal->getPositionAtTime( $now - 86401 ) ); + $this->assertSame( '1', $journal->getPositionAtTime( $now - 86400 ) ); + $this->assertSame( '1', $journal->getPositionAtTime( $now - 3601 ) ); + $this->assertSame( '2', $journal->getPositionAtTime( $now - 3600 ) ); + } + + /** + * @param int $expectedStart First index expected to be returned (0-based) + * @param int|null $expectedCount Number of entries expected to be returned (null for all) + * @param string|null|false $expectedNext Expected value of $next, or false not to pass + * @param array $args If any third argument is present, $next will also be tested + * @dataProvider provideDoGetChangeEntries + * @covers ::doLogChangeBatch + * @covers ::doGetChangeEntries + */ + public function testDoGetChangeEntries( + $expectedStart, $expectedCount, $expectedNext, array $args + ) { + $journal = $this->getJournal(); + + $i = 0; + $makeExpectedEntry = function ( $op, $path, $newSha1, $batch, $time ) use ( &$i ) { + $i++; + return [ + 'id' => (string)$i, + 'batch_uuid' => $batch, + 'backend' => 'local-backend', + 'path' => $path, + 'op' => $op ?? '', + 'new_sha1' => $newSha1 !== false ? $newSha1 : '0', + 'timestamp' => ConvertibleTimestamp::convert( TS_MW, $time ), + ]; + }; + + $expectedEntries = []; + + $now = time(); + + ConvertibleTimestamp::setFakeTime( $now - 3600 ); + $changes = [ + [ 'op' => 'create', 'path' => '/path1', + 'newSha1' => base_convert( sha1( 'a' ), 16, 36 ) ], + [ 'op' => 'delete', 'path' => '/path2', 'newSha1' => false ], + [ 'op' => 'null', 'path' => '', 'newSha1' => false ], + ]; + $this->assertEquals( StatusValue::newGood(), + $journal->logChangeBatch( $changes, 'batch1' ) ); + foreach ( $changes as $change ) { + $expectedEntries[] = $makeExpectedEntry( + ...array_merge( array_values( $change ), [ 'batch1', $now - 3600 ] ) ); + } + + ConvertibleTimestamp::setFakeTime( $now - 60 ); + $change = [ 'op' => 'update', 'path' => '/path1', + 'newSha1' => base_convert( sha1( 'b' ), 16, 36 ) ]; + $this->assertEquals( + StatusValue::newGood(), $journal->logChangeBatch( [ $change ], 'batch2' ) ); + $expectedEntries[] = $makeExpectedEntry( + ...array_merge( array_values( $change ), [ 'batch2', $now - 60 ] ) ); + + if ( $expectedNext === false ) { + $this->assertSame( + array_slice( $expectedEntries, $expectedStart, $expectedCount ), + $journal->getChangeEntries( ...$args ) + ); + } else { + $next = false; + $this->assertSame( + array_slice( $expectedEntries, $expectedStart, $expectedCount ), + $journal->getChangeEntries( $args[0], $args[1], $next ) + ); + $this->assertSame( $expectedNext, $next ); + } + } + + public static function provideDoGetChangeEntries() { + return [ + 'No args' => [ 0, 4, false, [] ], + 'null' => [ 0, 4, false, [ null ] ], + '1' => [ 0, 4, false, [ 1 ] ], + '2' => [ 1, 3, false, [ 2 ] ], + '4' => [ 3, 1, false, [ 4 ] ], + '5' => [ 0, 0, false, [ 5 ] ], + 'null, 0' => [ 0, 4, null, [ null, 0 ] ], + '1, 0' => [ 0, 4, null, [ 1, 0 ] ], + '2, 0' => [ 1, 3, null, [ 2, 0 ] ], + '4, 0' => [ 3, 1, null, [ 4, 0 ] ], + '5, 0' => [ 0, 0, null, [ 5, 0 ] ], + '1, 1' => [ 0, 1, '2', [ 1, 1 ] ], + '1, 2' => [ 0, 2, '3', [ 1, 2 ] ], + '1, 4' => [ 0, 4, null, [ 1, 4 ] ], + '1, 5' => [ 0, 4, null, [ 1, 5 ] ], + '2, 2' => [ 1, 2, '4', [ 2, 2 ] ], + '1, 2 with no $next' => [ 0, 2, false, [ 1, 2 ] ], + ]; + } + + /** + * @covers ::doPurgeOldLogs + */ + public function testDoPurgeOldLogs_noop() { + // If we tried to access the database, it would throw, because the domain doesn't exist + $journal = $this->getJournal( [ 'domain' => 'no-such-domain' ] ); + $this->assertEquals( StatusValue::newGood(), $journal->purgeOldLogs() ); + } + + /** + * @covers ::doPurgeOldLogs + * @covers ::doLogChangeBatch + * @covers ::doGetChangeEntries + */ + public function testDoPurgeOldLogs() { + $journal = $this->getJournal( [ 'ttlDays' => 1 ] ); + $now = time(); + + // One day and one second ago + ConvertibleTimestamp::setFakeTime( $now - 86401 ); + $this->assertEquals( StatusValue::newGood(), $journal->logChangeBatch( + [ [ 'op' => 'null', 'path' => '', 'newSha1' => false ] ], 'batch1' ) ); + + // One day ago exactly, won't get purged + ConvertibleTimestamp::setFakeTime( $now - 86400 ); + $this->assertEquals( StatusValue::newGood(), $journal->logChangeBatch( + [ [ 'op' => 'null', 'path' => '', 'newSha1' => false ] ], 'batch2' ) ); + + ConvertibleTimestamp::setFakeTime( $now ); + $this->assertCount( 2, $journal->getChangeEntries() ); + $journal->purgeOldLogs(); + $this->assertCount( 1, $journal->getChangeEntries() ); + } +} diff --git a/tests/phpunit/unit/includes/libs/filebackend/filejournal/FileJournalTest.php b/tests/phpunit/unit/includes/libs/filebackend/filejournal/FileJournalTest.php new file mode 100644 index 0000000000..10eef7e179 --- /dev/null +++ b/tests/phpunit/unit/includes/libs/filebackend/filejournal/FileJournalTest.php @@ -0,0 +1,153 @@ + TestFileJournal::class ], + $backend + ); + } + + /** + * @covers ::factory + */ + public function testConstructor_backend() { + $this->assertSame( 'some_backend', $this->newObj( [], 'some_backend' )->getBackend() ); + } + + /** + * @covers ::__construct + * @covers ::factory + */ + public function testConstructor_ttlDays() { + $this->assertSame( 42, $this->newObj( [ 'ttlDays' => 42 ] )->getTtlDays() ); + } + + /** + * @covers ::__construct + * @covers ::factory + */ + public function testConstructor_noTtlDays() { + $this->assertSame( false, $this->newObj()->getTtlDays() ); + } + + /** + * @covers ::__construct + * @covers ::factory + */ + public function testConstructor_nullTtlDays() { + $this->assertSame( false, $this->newObj( [ 'ttlDays' => null ] )->getTtlDays() ); + } + + /** + * @covers ::factory + */ + public function testFactory_invalidClass() { + $this->setExpectedException( UnexpectedValueException::class, + 'Expected instance of FileJournal, got stdClass' ); + + FileJournal::factory( [ 'class' => 'stdclass' ], '' ); + } + + /** + * @covers ::getTimestampedUUID + */ + public function testGetTimestampedUUID() { + $obj = FileJournal::factory( [ 'class' => 'NullFileJournal' ], '' ); + $uuids = []; + for ( $i = 0; $i < 10; $i++ ) { + $time1 = time(); + $uuid = $obj->getTimestampedUUID(); + $time2 = time(); + $this->assertRegexp( '/^[0-9a-z]{31}$/', $uuid ); + $this->assertArrayNotHasKey( $uuid, $uuids ); + $uuids[$uuid] = true; + + // Now test that the timestamp portion is as expected. + $time = ConvertibleTimestamp::convert( TS_UNIX, Wikimedia\base_convert( + substr( $uuid, 0, 9 ), 36, 10 ) ); + + $this->assertGreaterThanOrEqual( $time1, $time ); + $this->assertLessThanOrEqual( $time2, $time ); + } + } + + /** + * @covers ::logChangeBatch + */ + public function testLogChangeBatch() { + $this->assertEquals( + StatusValue::newGood( 'Logged' ), $this->newObj()->logChangeBatch( [ 1 ], '' ) ); + } + + /** + * @covers ::logChangeBatch + */ + public function testLogChangeBatch_empty() { + $this->assertEquals( StatusValue::newGood(), $this->newObj()->logChangeBatch( [], '' ) ); + } + + /** + * @covers ::getCurrentPosition + */ + public function testGetCurrentPosition() { + $this->assertEquals( 613, $this->newObj()->getCurrentPosition() ); + } + + /** + * @covers ::getPositionAtTime + */ + public function testGetPositionAtTime() { + $this->assertEquals( 248, $this->newObj()->getPositionAtTime( 0 ) ); + } + + /** + * @dataProvider provideGetChangeEntries + * @covers ::getChangeEntries + * @param int|null $start + * @param int $limit + * @param string|null $expectedNext + * @param string[] $expectedReturn Expected id's of returned values + */ + public function testGetChangeEntries( $start, $limit, $expectedNext, array $expectedReturn ) { + $expectedReturn = array_map( + function ( $val ) { + return [ 'id' => $val ]; + }, $expectedReturn + ); + $next = "Different from $expectedNext"; + $ret = $this->newObj()->getChangeEntries( $start, $limit, $next ); + $this->assertSame( $expectedNext, $next ); + $this->assertSame( $expectedReturn, $ret ); + } + + public static function provideGetChangeEntries() { + return [ + [ null, 0, null, [ 1, 2, 3 ] ], + [ null, 1, 2, [ 1 ] ], + [ null, 2, 3, [ 1, 2 ] ], + [ null, 3, null, [ 1, 2, 3 ] ], + [ 1, 0, null, [ 1, 2, 3 ] ], + [ 1, 2, 3, [ 1, 2 ] ], + [ 1, 1, 2, [ 1 ] ], + [ 2, 2, null, [ 2, 3 ] ], + ]; + } + + /** + * @covers ::purgeOldLogs + */ + public function testPurgeOldLogs() { + $obj = $this->newObj(); + $this->assertFalse( $obj->getPurged() ); + $obj->purgeOldLogs(); + $this->assertTrue( $obj->getPurged() ); + } +} diff --git a/tests/phpunit/unit/includes/libs/filebackend/filejournal/NullFileJournalTest.php b/tests/phpunit/unit/includes/libs/filebackend/filejournal/NullFileJournalTest.php new file mode 100644 index 0000000000..c0b782b142 --- /dev/null +++ b/tests/phpunit/unit/includes/libs/filebackend/filejournal/NullFileJournalTest.php @@ -0,0 +1,48 @@ + NullFileJournal::class ], '' ); + } + + /** + * @covers ::doLogChangeBatch + */ + public function testLogChangeBatch() { + $this->assertEquals( StatusValue::newGood(), $this->newObj()->logChangeBatch( [ 1 ], '' ) ); + } + + /** + * @covers ::doGetCurrentPosition + */ + public function testGetCurrentPosition() { + $this->assertFalse( $this->newObj()->getCurrentPosition() ); + } + + /** + * @covers ::doGetPositionAtTime + */ + public function testGetPositionAtTime() { + $this->assertFalse( $this->newObj()->getPositionAtTime( 2 ) ); + } + + /** + * @covers ::doGetChangeEntries + */ + public function testGetChangeEntries() { + $next = 1; + $entries = $this->newObj()->getChangeEntries( null, 0, $next ); + $this->assertSame( [], $entries ); + $this->assertNull( $next ); + } + + /** + * @covers ::doPurgeOldLogs + */ + public function testPurgeOldLogs() { + $this->assertEquals( StatusValue::newGood(), $this->newObj()->purgeOldLogs() ); + } +} diff --git a/tests/phpunit/unit/includes/libs/filebackend/filejournal/TestFileJournal.php b/tests/phpunit/unit/includes/libs/filebackend/filejournal/TestFileJournal.php new file mode 100644 index 0000000000..c115925e5e --- /dev/null +++ b/tests/phpunit/unit/includes/libs/filebackend/filejournal/TestFileJournal.php @@ -0,0 +1,42 @@ +ttlDays; + } + + public function getBackend() { + return $this->backend; + } + + protected function doLogChangeBatch( array $entries, $batchId ) { + return StatusValue::newGood( 'Logged' ); + } + + protected function doGetCurrentPosition() { + return 613; + } + + protected function doGetPositionAtTime( $time ) { + return 248; + } + + protected function doGetChangeEntries( $start, $limit ) { + return array_slice( [ + [ 'id' => 1 ], + [ 'id' => 2 ], + [ 'id' => 3 ], + ], $start === null ? 0 : $start - 1, $limit ? $limit : null ); + } + + protected function doPurgeOldLogs() { + $this->purged = true; + } + + public function getPurged() { + return $this->purged; + } +}