From 45660456cf6cce97f924b589d59317d19b31ace2 Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Tue, 24 Apr 2018 00:46:11 +0100
Subject: [PATCH] phpunit: Clear job queues in MediaWikiTestCase::setUp()

This should fix the flaky unit test 'SiteStatsTest::testJobsCountGetCached',
which fails locally as follows, when run in isolation.

> 1) SiteStatsTest::testJobsCountGetCached
> A single job enqueued bumps jobscount stat to 1
> Failed asserting that 2 matches expected 1.
>
> /var/www/mediawiki/tests/phpunit/includes/SiteStatsTest.php:22
> /var/www/mediawiki/tests/phpunit/MediaWikiTestCase.php:421
> /var/www/mediawiki/maintenance/doMaintenance.php:94

Instrumenting JobQueueMemory::doBatchPush reveals the following
jobs to have been queued.

- MediaWikiTestCase->run/->addCoreDBData/::getTestSysop/..
  ../User->addGroup/UserGroupMembership->insert/..
  > UserGroupExpiryJob (2)
- MediaWikiTestCase->run/->addCoreDBData/WikiPage->doEditContent/..
  ../WikiPage->{closure}/WikiPage->doEditUpdates/JobQueueGroup->lazyPush/..
  > CategoryMembershipChangeJob
  > HTMLCacheUpdateJob (2)

Fix this by adding clearing of job queues to doLightweightServiceReset()
in MediaWikiTestCase.

Also:
- Move the call to doLightweightServiceReset() from run() to setUp(),
  where it is easier to understand in context. It still runs at the same
  logical point because PHPUnit calls setUp() right before run().

- Remove redundant reset for WANObjectCache->clearProcessCache that
  was both in setUp() and in doLightweightServiceReset().

- Simplify SiteStatsTest by removing the hardcoded delete() calls.
  An alternative fix for the flaky unit test would've been to add
  a delete() call to categoryMembershipChange, but rather than hardcoding
  all possible jobs that TestCase or another test could make, it's easier
  to just reset/delete them all between tests.

- Simplify SiteStatsTest by using the $cache reference directly instead
  of roundtripping through MediaWikiServices. If for some reason
  setService() didn't work, the test will fail either way because it must
  match the one used by JobQueueGroup (TODO: Use injection!), and besides
  the setService() method already has its own unit test.

Change-Id: Ia4b7871221c76c65eacf31915b515705a36940d5
---
 tests/phpunit/MediaWikiTestCase.php      | 16 ++++++++++------
 tests/phpunit/includes/SiteStatsTest.php | 11 ++---------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/tests/phpunit/MediaWikiTestCase.php b/tests/phpunit/MediaWikiTestCase.php
index 87ca918138..a27e9f9cda 100644
--- a/tests/phpunit/MediaWikiTestCase.php
+++ b/tests/phpunit/MediaWikiTestCase.php
@@ -360,15 +360,20 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
 	 * @see resetGlobalServices()
 	 */
 	private function doLightweightServiceReset() {
-		global $wgRequest;
+		global $wgRequest, $wgJobClasses;
 
+		foreach ( $wgJobClasses as $type => $class ) {
+			JobQueueGroup::singleton()->get( $type )->delete();
+		}
 		JobQueueGroup::destroySingletons();
+
 		ObjectCache::clear();
 		$services = MediaWikiServices::getInstance();
 		$services->resetServiceForTesting( 'MainObjectStash' );
 		$services->resetServiceForTesting( 'LocalServerObjectCache' );
 		$services->getMainWANObjectCache()->clearProcessCache();
 		FileBackendGroup::destroySingleton();
+		DeferredUpdates::clearPendingUpdates();
 
 		// TODO: move global state into MediaWikiServices
 		RequestContext::resetMain();
@@ -382,9 +387,6 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
 	}
 
 	public function run( PHPUnit_Framework_TestResult $result = null ) {
-		// Reset all caches between tests.
-		$this->doLightweightServiceReset();
-
 		$needsResetDB = false;
 
 		if ( !self::$dbSetup || $this->needsDB() ) {
@@ -515,8 +517,8 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
 			}
 		}
 
-		DeferredUpdates::clearPendingUpdates();
-		ObjectCache::getMainWANInstance()->clearProcessCache();
+		// Reset all caches between tests.
+		$this->doLightweightServiceReset();
 
 		// XXX: reset maintenance triggers
 		// Hook into period lag checks which often happen in long-running scripts
@@ -1147,6 +1149,8 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase {
 	 * setupTestDB() was called. Useful if we need to perform database operations
 	 * after the test run has finished (such as saving logs or profiling info).
 	 *
+	 * This is called by phpunit/bootstrap.php after the last test.
+	 *
 	 * @since 1.21
 	 */
 	public static function teardownTestDB() {
diff --git a/tests/phpunit/includes/SiteStatsTest.php b/tests/phpunit/includes/SiteStatsTest.php
index 56bde5da08..a6a92c68a6 100644
--- a/tests/phpunit/includes/SiteStatsTest.php
+++ b/tests/phpunit/includes/SiteStatsTest.php
@@ -6,17 +6,10 @@ class SiteStatsTest extends MediaWikiTestCase {
 	 * @covers SiteStats::jobs
 	 */
 	function testJobsCountGetCached() {
-		$this->setService( 'MainWANObjectCache',
-			new WANObjectCache( [ 'cache' => new HashBagOStuff() ] ) );
-		$cache = \MediaWiki\MediaWikiServices::getInstance()->getMainWANObjectCache();
+		$cache = new WANObjectCache( [ 'cache' => new HashBagOStuff() ] );
+		$this->setService( 'MainWANObjectCache', $cache );
 		$jobq = JobQueueGroup::singleton();
 
-		// Delete jobs that might have been left behind by other tests
-		$jobq->get( 'htmlCacheUpdate' )->delete();
-		$jobq->get( 'recentChangesUpdate' )->delete();
-		$jobq->get( 'userGroupExpiry' )->delete();
-		$cache->delete( $cache->makeKey( 'SiteStats', 'jobscount' ) );
-
 		$jobq->push( new NullJob( Title::newMainPage(), [] ) );
 		$this->assertEquals( 1, SiteStats::jobs(),
 			 'A single job enqueued bumps jobscount stat to 1' );
-- 
2.20.1