From 3ab9a9656c2c8fd163dbcbe34fb017e0db4af20b Mon Sep 17 00:00:00 2001 From: addshore Date: Tue, 15 Mar 2016 00:12:06 +0000 Subject: [PATCH 1/1] Reset WatchedItemStore default instance after tests Prior to this change in tests the overridden store would remain in the instance static and thus could be used in other places. This patch introduces the used of ScopedCallbacks in the override methods in WatchedItemStore. This means that any instance of WatchedItemStore should return to a regular state after each test. This is better than requiring the tests to reset the value back to the origional as this would likely be forgotten and result in long hunts for failing tests. This was found while writing more tests... Change-Id: I9aa71425642174ae9ea2c6d4f85dcd07d724af11 --- includes/WatchedItemStore.php | 26 ++++++++++++++-- .../includes/WatchedItemStoreUnitTest.php | 30 +++++++++++++++---- .../phpunit/includes/WatchedItemUnitTest.php | 16 +++++++--- 3 files changed, 61 insertions(+), 11 deletions(-) diff --git a/includes/WatchedItemStore.php b/includes/WatchedItemStore.php index 1a9868e154..11eb60ce24 100644 --- a/includes/WatchedItemStore.php +++ b/includes/WatchedItemStore.php @@ -64,8 +64,10 @@ class WatchedItemStore { * This is intended for use while testing and will fail if MW_PHPUNIT_TEST is not defined. * * @param callable $callback + * * @see DeferredUpdates::addCallableUpdate for callback signiture * + * @return ScopedCallback to reset the overridden value * @throws MWException */ public function overrideDeferredUpdatesAddCallableUpdateCallback( $callback ) { @@ -75,7 +77,12 @@ class WatchedItemStore { ); } Assert::parameterType( 'callable', $callback, '$callback' ); + + $previousValue = $this->deferredUpdatesAddCallableUpdateCallback; $this->deferredUpdatesAddCallableUpdateCallback = $callback; + return new ScopedCallback( function() use ( $previousValue ) { + $this->deferredUpdatesAddCallableUpdateCallback = $previousValue; + } ); } /** @@ -85,6 +92,7 @@ class WatchedItemStore { * @param callable $callback * @see Revision::getTimestampFromId for callback signiture * + * @return ScopedCallback to reset the overridden value * @throws MWException */ public function overrideRevisionGetTimestampFromIdCallback( $callback ) { @@ -94,24 +102,38 @@ class WatchedItemStore { ); } Assert::parameterType( 'callable', $callback, '$callback' ); + + $previousValue = $this->revisionGetTimestampFromIdCallback; $this->revisionGetTimestampFromIdCallback = $callback; + return new ScopedCallback( function() use ( $previousValue ) { + $this->revisionGetTimestampFromIdCallback = $previousValue; + } ); } /** * Overrides the default instance of this class * This is intended for use while testing and will fail if MW_PHPUNIT_TEST is not defined. * - * @param WatchedItemStore $store + * If this method is used it MUST also be called with null after a test to ensure a new + * default instance is created next time getDefaultInstance is called. * + * @param WatchedItemStore|null $store + * + * @return ScopedCallback to reset the overridden value * @throws MWException */ - public static function overrideDefaultInstance( WatchedItemStore $store ) { + public static function overrideDefaultInstance( WatchedItemStore $store = null ) { if ( !defined( 'MW_PHPUNIT_TEST' ) ) { throw new MWException( 'Cannot override ' . __CLASS__ . 'default instance in operation.' ); } + + $previousValue = self::$instance; self::$instance = $store; + return new ScopedCallback( function() use ( $previousValue ) { + self::$instance = $previousValue; + } ); } /** diff --git a/tests/phpunit/includes/WatchedItemStoreUnitTest.php b/tests/phpunit/includes/WatchedItemStoreUnitTest.php index 1dd819bf2a..e483580352 100644 --- a/tests/phpunit/includes/WatchedItemStoreUnitTest.php +++ b/tests/phpunit/includes/WatchedItemStoreUnitTest.php @@ -81,6 +81,17 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { $this->assertSame( $instanceOne, $instanceTwo ); } + public function testOverrideDefaultInstance() { + $instance = WatchedItemStore::getDefaultInstance(); + $scopedOverride = $instance->overrideDefaultInstance( null ); + + $this->assertNotSame( $instance, WatchedItemStore::getDefaultInstance() ); + + unset( $scopedOverride ); + + $this->assertSame( $instance, WatchedItemStore::getDefaultInstance() ); + } + public function testCountWatchers() { $titleValue = new TitleValue( 0, 'SomeDbKey' ); @@ -1299,7 +1310,7 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { $callableCallCounter++; $this->assertInternalType( 'callable', $callable ); }; - $store->overrideDeferredUpdatesAddCallableUpdateCallback( $mockCallback ); + $scopedOverride = $store->overrideDeferredUpdatesAddCallableUpdateCallback( $mockCallback ); $this->assertTrue( $store->resetNotificationTimestamp( @@ -1308,6 +1319,8 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { ) ); $this->assertEquals( 1, $callableCallCounter ); + + ScopedCallback::consume( $scopedOverride ); } public function testResetNotificationTimestamp_noItemForced() { @@ -1337,7 +1350,7 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { $callableCallCounter++; $this->assertInternalType( 'callable', $callable ); }; - $store->overrideDeferredUpdatesAddCallableUpdateCallback( $mockCallback ); + $scopedOverride = $store->overrideDeferredUpdatesAddCallableUpdateCallback( $mockCallback ); $this->assertTrue( $store->resetNotificationTimestamp( @@ -1347,6 +1360,8 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { ) ); $this->assertEquals( 1, $callableCallCounter ); + + ScopedCallback::consume( $scopedOverride ); } /** @@ -1397,7 +1412,7 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { // Note: This does not actually assert the job is correct $callableCallCounter = 0; - $store->overrideDeferredUpdatesAddCallableUpdateCallback( + $scopedOverride = $store->overrideDeferredUpdatesAddCallableUpdateCallback( function( $callable ) use ( &$callableCallCounter ) { $callableCallCounter++; $this->assertInternalType( 'callable', $callable ); @@ -1413,6 +1428,8 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { ) ); $this->assertEquals( 1, $callableCallCounter ); + + ScopedCallback::consume( $scopedOverride ); } public function testResetNotificationTimestamp_oldidSpecifiedNotLatestRevisionForced() { @@ -1455,7 +1472,7 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { // Note: This does not actually assert the job is correct $addUpdateCallCounter = 0; - $store->overrideDeferredUpdatesAddCallableUpdateCallback( + $scopedOverrideDeferred = $store->overrideDeferredUpdatesAddCallableUpdateCallback( function( $callable ) use ( &$addUpdateCallCounter ) { $addUpdateCallCounter++; $this->assertInternalType( 'callable', $callable ); @@ -1463,7 +1480,7 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { ); $getTimestampCallCounter = 0; - $store->overrideRevisionGetTimestampFromIdCallback( + $scopedOverrideRevision = $store->overrideRevisionGetTimestampFromIdCallback( function( $titleParam, $oldidParam ) use ( &$getTimestampCallCounter, $title, $oldid ) { $getTimestampCallCounter++; $this->assertEquals( $title, $titleParam ); @@ -1481,6 +1498,9 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { ); $this->assertEquals( 1, $addUpdateCallCounter ); $this->assertEquals( 1, $getTimestampCallCounter ); + + ScopedCallback::consume( $scopedOverrideDeferred ); + ScopedCallback::consume( $scopedOverrideRevision ); } public function testUpdateNotificationTimestamp_watchersExist() { diff --git a/tests/phpunit/includes/WatchedItemUnitTest.php b/tests/phpunit/includes/WatchedItemUnitTest.php index bc3731173a..58984cf389 100644 --- a/tests/phpunit/includes/WatchedItemUnitTest.php +++ b/tests/phpunit/includes/WatchedItemUnitTest.php @@ -51,13 +51,15 @@ class WatchedItemUnitTest extends PHPUnit_Framework_TestCase { ->method( 'loadWatchedItem' ) ->with( $user, $linkTarget ) ->will( $this->returnValue( new WatchedItem( $user, $linkTarget, $timestamp ) ) ); - WatchedItemStore::overrideDefaultInstance( $store ); + $scopedOverride = WatchedItemStore::overrideDefaultInstance( $store ); $item = WatchedItem::fromUserTitle( $user, $linkTarget, User::IGNORE_USER_RIGHTS ); $this->assertEquals( $user, $item->getUser() ); $this->assertEquals( $linkTarget, $item->getLinkTarget() ); $this->assertEquals( $timestamp, $item->getNotificationTimestamp() ); + + ScopedCallback::consume( $scopedOverride ); } /** @@ -83,10 +85,12 @@ class WatchedItemUnitTest extends PHPUnit_Framework_TestCase { return true; } ) ); - WatchedItemStore::overrideDefaultInstance( $store ); + $scopedOverride = WatchedItemStore::overrideDefaultInstance( $store ); $item = new WatchedItem( $user, $linkTarget, $timestamp ); $item->resetNotificationTimestamp( $force, $oldid ); + + ScopedCallback::consume( $scopedOverride ); } public function testAddWatch() { @@ -153,9 +157,11 @@ class WatchedItemUnitTest extends PHPUnit_Framework_TestCase { $store->expects( $this->once() ) ->method( 'duplicateAllAssociatedEntries' ) ->with( $oldTitle, $newTitle ); - WatchedItemStore::overrideDefaultInstance( $store ); + $scopedOverride = WatchedItemStore::overrideDefaultInstance( $store ); WatchedItem::duplicateEntries( $oldTitle, $newTitle ); + + ScopedCallback::consume( $scopedOverride ); } public function testBatchAddWatch() { @@ -175,9 +181,11 @@ class WatchedItemUnitTest extends PHPUnit_Framework_TestCase { $store->expects( $this->once() ) ->method( 'addWatchBatch' ) ->with( $userTargetCombinations ); - WatchedItemStore::overrideDefaultInstance( $store ); + $scopedOverride = WatchedItemStore::overrideDefaultInstance( $store ); WatchedItem::batchAddWatch( $items ); + + ScopedCallback::consume( $scopedOverride ); } } -- 2.20.1