Merge "Clean up EditStashCache and test"

includes/EditStashCache.php

@@ -7,8 +7,8 @@ use IBufferingStatsdDataFactory;
 use InvalidArgumentException;
 use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder;
 use MediaWiki\Extension\AbuseFilter\Variables\VariablesManager;
+use MediaWiki\Linker\LinkTarget;
 use Psr\Log\LoggerInterface;
-use Title;
 
 /**
  * Wrapper around cache for storing and retrieving data from edit stash
@@ -27,8 +27,8 @@ class EditStashCache {
 	/** @var LoggerInterface */
 	private $logger;
 
-	/** @var Title */
-	private $title;
+	/** @var LinkTarget */
+	private $target;
 
 	/** @var string */
 	private $group;
@@ -38,7 +38,7 @@ class EditStashCache {
 	 * @param IBufferingStatsdDataFactory $statsdDataFactory
 	 * @param VariablesManager $variablesManager
 	 * @param LoggerInterface $logger
-	 * @param Title $title
+	 * @param LinkTarget $target
 	 * @param string $group
 	 */
 	public function __construct(
@@ -46,14 +46,14 @@ class EditStashCache {
 		IBufferingStatsdDataFactory $statsdDataFactory,
 		VariablesManager $variablesManager,
 		LoggerInterface $logger,
-		Title $title,
+		LinkTarget $target,
 		string $group
 	) {
 		$this->cache = $cache;
 		$this->statsdDataFactory = $statsdDataFactory;
 		$this->variablesManager = $variablesManager;
 		$this->logger = $logger;
-		$this->title = $title;
+		$this->target = $target;
 		$this->group = $group;
 	}
 
@@ -92,7 +92,10 @@ class EditStashCache {
 		if ( !in_array( $type, [ 'store', 'hit', 'miss' ] ) ) {
 			throw new InvalidArgumentException( '$type must be either "store", "hit" or "miss"' );
 		}
-		$this->logger->debug( __METHOD__ . ": cache $type for '{$this->title}' (key $key)." );
+		$this->logger->debug(
+			__METHOD__ . ": cache {logtype} for '{target}' (key {key}).",
+			[ 'logtype' => $type, 'target' => $this->target, 'key' => $key ]
+		);
 		$this->statsdDataFactory->increment( "abusefilter.check-stash.$type" );
 	}
 

tests/phpunit/AbuseFilterConsequencesTest.php

@@ -7,6 +7,7 @@ use MediaWiki\Extension\AbuseFilter\Parser\AFPData;
 use MediaWiki\MediaWikiServices;
 use MediaWiki\Storage\PageEditStash;
 use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
 
 /**
  * Complete tests where filters are saved, actions are executed and the right
@@ -970,7 +971,7 @@ class AbuseFilterConsequencesTest extends MediaWikiTestCase {
 		foreach ( $results as $result ) {
 			if ( !$result->isGood() ) {
 				$this->fail( 'Only the last actions should have triggered a filter; the other ones ' .
-				'should have been allowed.' );
+					'should have been allowed.' );
 			}
 		}
 
@@ -1308,33 +1309,23 @@ class AbuseFilterConsequencesTest extends MediaWikiTestCase {
 		$actionParams['action'] = 'stashedit';
 		$actionParams['stashType'] = $type;
 
-		$loggerMock = new TestLogger();
-		$loggerMock->setCollect( true );
+		$loggerMock = $this->createMock( LoggerInterface::class );
+		$loggerCalls = [];
+		$loggerMock->method( 'debug' )
+			->willReturnCallback( function ( $msg, $args ) use ( &$loggerCalls ) {
+				if ( isset( $args['logtype'] ) ) {
+					$loggerCalls[] = $args['logtype'];
+				}
+			} );
 		$this->setLogger( 'StashEdit', $loggerMock );
 
 		$this->createFilters( $createIds );
 		$result = $this->doAction( $actionParams );
 
 		// Check that we stored the edit and then hit/missed the cache
-		$foundStore = false;
-		$foundHitOrMiss = false;
-		// The conversion back and forth is needed because if the wiki language is not english
-		// the given namespace has been localized and thus wouldn't match.
-		$pageName = Title::newFromText( $actionParams['target'] )->getPrefixedText();
-		foreach ( $loggerMock->getBuffer() as $entry ) {
-			if ( preg_match( "/.+::logCache: cache $type for '$pageName'/", $entry[1] ) ) {
-				$foundHitOrMiss = true;
-			}
-			if ( preg_match( "/.+::logCache: cache store for '$pageName'/", $entry[1] ) ) {
-				$foundStore = true;
-			}
-			if ( $foundStore && $foundHitOrMiss ) {
-				break;
-			}
-		}
-		if ( !$foundStore ) {
+		if ( !in_array( 'store', $loggerCalls, true ) ) {
 			$this->fail( 'Did not store the edit in cache as expected for a stashed edit.' );
-		} elseif ( !$foundHitOrMiss ) {
+		} elseif ( !in_array( $type, $loggerCalls, true ) ) {
 			$this->fail( "Did not $type the cache as expected for a stashed edit." );
 		}
 

tests/phpunit/unit/EditStashCacheTest.php

@@ -10,21 +10,15 @@ use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder;
 use MediaWiki\Extension\AbuseFilter\Variables\VariablesManager;
 use MediaWikiUnitTestCase;
 use NullStatsdDataFactory;
+use Psr\Log\LoggerInterface;
 use Psr\Log\NullLogger;
-use TestLogger;
-use Title;
+use TitleValue;
 
 /**
  * @coversDefaultClass \MediaWiki\Extension\AbuseFilter\EditStashCache
  */
 class EditStashCacheTest extends MediaWikiUnitTestCase {
 
-	private function getTitle() : Title {
-		$title = $this->createMock( Title::class );
-		$title->method( '__toString' )->willReturn( 'Prefixed:Text' );
-		return $title;
-	}
-
 	private function getVariablesManager() : VariablesManager {
 		return new VariablesManager(
 			$this->createMock( KeywordsManager::class ),
@@ -39,35 +33,30 @@ class EditStashCacheTest extends MediaWikiUnitTestCase {
 	 * @covers ::getStashKey
 	 */
 	public function testStore() {
+		$title = new TitleValue( NS_MAIN, 'Some title' );
 		$cache = $this->getMockBuilder( HashBagOStuff::class )
 			->onlyMethods( [ 'set' ] )
 			->getMock();
 		$cache->expects( $this->once() )->method( 'set' );
-		$logger = new TestLogger( true );
+		$logger = $this->createMock( LoggerInterface::class );
+		$logger->expects( $this->once() )
+			->method( 'debug' )
+			->willReturnCallback( function ( $msg, $args ) use ( $title ) {
+				unset( $args['key'] );
+				$this->assertSame( [ 'logtype' => 'store', 'target' => $title ], $args );
+			} );
 		$stash = new EditStashCache(
 			$cache,
 			new NullStatsdDataFactory(),
 			$this->getVariablesManager(),
 			$logger,
-			$this->getTitle(),
+			$title,
 			'default'
 		);
 		$vars = VariableHolder::newFromArray( [ 'page_title' => 'Title' ] );
 		$data = [ 'foo' => 'bar' ];
 		$stash->store( $vars, $data );
-
-		$found = false;
-		foreach ( $logger->getBuffer() as list( , $entry ) ) {
-			$check = preg_match(
-				"/^.+: cache store for 'Prefixed:Text' \(key .+\)\.$/",
-				$entry
-			);
-			if ( $check ) {
-				$found = true;
-				break;
-			}
-		}
-		$this->assertTrue( $found, "Test that store operation is logged." );
+		$this->addToAssertionCount( 1 );
 	}
 
 	public function provideRoundTrip() {
@@ -95,14 +84,27 @@ class EditStashCacheTest extends MediaWikiUnitTestCase {
 	 * @dataProvider provideRoundTrip
 	 */
 	public function testRoundTrip( array $storeVars, array $seekVars ) {
+		$title = new TitleValue( NS_MAIN, 'Some title' );
 		$cache = new HashBagOStuff();
-		$logger = new TestLogger( true );
+		$logger = $this->createMock( LoggerInterface::class );
+		$storeLogged = false;
+		$logger->expects( $this->exactly( 2 ) )
+			->method( 'debug' )
+			->willReturnCallback( function ( $msg, $args ) use ( $title, &$storeLogged ) {
+				unset( $args['key'] );
+				if ( !$storeLogged ) {
+					$this->assertSame( [ 'logtype' => 'store', 'target' => $title ], $args );
+					$storeLogged = true;
+				} else {
+					$this->assertSame( [ 'logtype' => 'hit', 'target' => $title ], $args );
+				}
+			} );
 		$stash = new EditStashCache(
 			$cache,
 			new NullStatsdDataFactory(),
 			$this->getVariablesManager(),
 			$logger,
-			$this->getTitle(),
+			$title,
 			'default'
 		);
 		$storeHolder = VariableHolder::newFromArray( $storeVars );
@@ -112,19 +114,6 @@ class EditStashCacheTest extends MediaWikiUnitTestCase {
 		$seekHolder = VariableHolder::newFromArray( $seekVars );
 		$value = $stash->seek( $seekHolder );
 		$this->assertArrayEquals( $data, $value );
-
-		$found = false;
-		foreach ( $logger->getBuffer() as list( , $entry ) ) {
-			$check = preg_match(
-				"/^.+: cache hit for 'Prefixed:Text' \(key .+\)\.$/",
-				$entry
-			);
-			if ( $check ) {
-				$found = true;
-				break;
-			}
-		}
-		$this->assertTrue( $found, "Test that cache hit is logged." );
 	}
 
 	/**
@@ -133,32 +122,26 @@ class EditStashCacheTest extends MediaWikiUnitTestCase {
 	 * @covers ::getStashKey
 	 */
 	public function testSeek_miss() {
+		$title = new TitleValue( NS_MAIN, 'Some title' );
 		$cache = new HashBagOStuff();
-		$logger = new TestLogger( true );
+		$logger = $this->createMock( LoggerInterface::class );
+		$logger->expects( $this->once() )
+			->method( 'debug' )
+			->willReturnCallback( function ( $msg, $args ) use ( $title ) {
+				unset( $args['key'] );
+				$this->assertSame( [ 'logtype' => 'miss', 'target' => $title ], $args );
+			} );
 		$stash = new EditStashCache(
 			$cache,
 			new NullStatsdDataFactory(),
 			$this->getVariablesManager(),
 			$logger,
-			$this->getTitle(),
+			$title,
 			'default'
 		);
 		$vars = VariableHolder::newFromArray( [ 'page_title' => 'Title' ] );
 		$value = $stash->seek( $vars );
 		$this->assertFalse( $value );
-
-		$found = false;
-		foreach ( $logger->getBuffer() as list( , $entry ) ) {
-			$check = preg_match(
-				"/^.+: cache miss for 'Prefixed:Text' \(key .+\)\.$/",
-				$entry
-			);
-			if ( $check ) {
-				$found = true;
-				break;
-			}
-		}
-		$this->assertTrue( $found, "Test that cache miss is logged." );
 	}
 
 }