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

diff --git a/includes/WatchedItemStore.php b/includes/WatchedItemStore.php
index 1a9868e..11eb60c 100644 (file)
--- 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 1dd819b..e483580 100644 (file)
--- 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 bc37311..58984cf 100644 (file)
--- 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 );
        }
 
 }