Merge "FileJournal tests"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Tue, 3 Sep 2019 11:22:00 +0000 (11:22 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Tue, 3 Sep 2019 11:22:00 +0000 (11:22 +0000)
includes/filebackend/filejournal/DBFileJournal.php
includes/libs/filebackend/filejournal/FileJournal.php
tests/phpunit/MediaWikiIntegrationTestCase.php
tests/phpunit/includes/filebackend/FileBackendTest.php
tests/phpunit/includes/filebackend/filejournal/DBFileJournalIntegrationTest.php [new file with mode: 0644]
tests/phpunit/unit/includes/libs/filebackend/filejournal/FileJournalTest.php [new file with mode: 0644]
tests/phpunit/unit/includes/libs/filebackend/filejournal/NullFileJournalTest.php [new file with mode: 0644]
tests/phpunit/unit/includes/libs/filebackend/filejournal/TestFileJournal.php [new file with mode: 0644]

index 17cf8f0..7ce2edd 100644 (file)
@@ -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 ) ],
index e512423..c256d72 100644 (file)
@@ -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
index 82d359f..41c65b2 100644 (file)
@@ -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
@@ -670,6 +671,9 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
                        $this->fail( $message );
                }
 
+               // If anything faked the time, reset it
+               ConvertibleTimestamp::setFakeTime( false );
+
                parent::tearDown();
        }
 
index 8548fde..bc3696d 100644 (file)
@@ -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 (file)
index 0000000..9a0ba1c
--- /dev/null
@@ -0,0 +1,237 @@
+<?php
+
+use MediaWiki\MediaWikiServices;
+use Wikimedia\Timestamp\ConvertibleTimestamp;
+
+/**
+ * @coversDefaultClass DBFileJournal
+ * @covers ::__construct
+ * @covers ::getMasterDB
+ * @group Database
+ */
+class DBFileJournalIntegrationTest extends MediaWikiIntegrationTestCase {
+       public function addDBDataOnce() {
+               global $IP;
+               $db = MediaWikiServices::getInstance()->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 (file)
index 0000000..10eef7e
--- /dev/null
@@ -0,0 +1,153 @@
+<?php
+
+require_once __DIR__ . '/TestFileJournal.php';
+
+use Wikimedia\Timestamp\ConvertibleTimestamp;
+
+/**
+ * @coversDefaultClass FileJournal
+ */
+class FileJournalTest extends MediaWikiUnitTestCase {
+       private function newObj( $options = [], $backend = '' ) {
+               return FileJournal::factory(
+                       $options + [ 'class' => 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 (file)
index 0000000..c0b782b
--- /dev/null
@@ -0,0 +1,48 @@
+<?php
+
+/**
+ * @coversDefaultClass NullFileJournal
+ */
+class NullFileJournalTest extends MediaWikiUnitTestCase {
+       public function newObj() : NullFileJournal {
+               return FileJournal::factory( [ 'class' => 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 (file)
index 0000000..c115925
--- /dev/null
@@ -0,0 +1,42 @@
+<?php
+
+class TestFileJournal extends NullFileJournal {
+       /** @var bool */
+       private $purged = false;
+
+       public function getTtlDays() {
+               return $this->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;
+       }
+}