From 355a6cd4c30452027653162ce7782f79df4ada8b Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Sat, 9 May 2020 02:15:03 +0100
Subject: [PATCH] phpunit: Add error_reporting/AtEase check to
 MediaWikiUnitTestCase

This existed on MediaWikiIntegrationTestCase, but not on
MediaWikiUnitTestCase. As a result of that, I spent about four
days tracking down a dangling AtEase::suppressWarnings with
missing AtEase::restoreWarnings (as part of Ib6758d724c).

Move it to the common MediaWikiTestCaseTrait instead so that we
get it on unit/ as well.

Example:

> There was 1 failure:
>
> 1) Pbkdf2PasswordTest::testCryptThrows
> PHP error_reporting setting found dirty.
> Did you forget AtEase::restoreWarnings?

Change-Id: I7dc3fe90385c8066b89a5e06c55f5455edfbb4ca
---
 .phpcs.xml                                    | 22 +++++++++++++++++
 .../phpunit/MediaWikiIntegrationTestCase.php  | 22 -----------------
 tests/phpunit/MediaWikiTestCaseTrait.php      | 24 +++++++++++++++++++
 .../includes/password/Pbkdf2PasswordTest.php  |  6 +----
 4 files changed, 47 insertions(+), 27 deletions(-)

diff --git a/.phpcs.xml b/.phpcs.xml
index 3da9f75ee39..0e3e1ad4f25 100644
--- a/.phpcs.xml
+++ b/.phpcs.xml
@@ -185,7 +185,29 @@
 		<exclude-pattern>*/tests/phpunit/maintenance/*\.php</exclude-pattern>
 		<exclude-pattern>*/tests/phpunit/integration/includes/GlobalFunctions/*\.php</exclude-pattern>
 	</rule>
+	<rule ref="Generic.PHP.NoSilencedErrors.Discouraged">
+		<!--
+		Our normal policy of using Wikimedia\AtEase does not always make sense tests.
 
+		AtEase cannot be cleanly used in tests that also use expectException() as
+		the restoreWarnings() call would never be reached:
+
+			$this->expectException( PasswordError::class );
+			AtEase::suppressWarnings();
+			$password->crypt( 'whatever' );
+			AtEase::restoreWarnings();
+
+		The above will stop at crypt(), as expected, and leave AtEase in a dirty
+		state for unrelated tests.
+
+		TODO: Stop using PHPUnit TestCase directly. Require with a structure test
+		or with a high-level check in our run() hook, that all test cases use either
+		MediaWikiUnitTestCase or MediaWikiIntegrationTestCase. Otherwise the check
+		in MediaWikiTestCaseTrait can still be bypassed and cause a random failures.
+
+		-->
+		<exclude-pattern>*/tests/*\.php</exclude-pattern>
+	</rule>
 	<rule ref="Generic.Files.OneObjectStructurePerFile.MultipleFound">
 		<!--
 			Whitelist existing violations, but enable the sniff to prevent
diff --git a/tests/phpunit/MediaWikiIntegrationTestCase.php b/tests/phpunit/MediaWikiIntegrationTestCase.php
index ed404c9a279..6ce01339722 100644
--- a/tests/phpunit/MediaWikiIntegrationTestCase.php
+++ b/tests/phpunit/MediaWikiIntegrationTestCase.php
@@ -74,13 +74,6 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
 	private static $dbSetup = false;
 	private static $oldTablePrefix = '';
 
-	/**
-	 * Original value of PHP's error_reporting setting.
-	 *
-	 * @var int
-	 */
-	private $phpErrorLevel;
-
 	/**
 	 * Holds the paths of temporary files/directories created through getNewTempFile,
 	 * and getNewTempDirectory
@@ -520,8 +513,6 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
 	 * @before
 	 */
 	protected function mediaWikiSetUp() {
-		$this->phpErrorLevel = intval( ini_get( 'error_reporting' ) );
-
 		$reflection = new ReflectionClass( $this );
 		// TODO: Eventually we should assert for test presence in /integration/
 		if ( strpos( $reflection->getFilename(), '/unit/' ) !== false ) {
@@ -647,19 +638,6 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase {
 		MediaWiki\Session\SessionManager::resetCache();
 		MediaWiki\Auth\AuthManager::resetCache();
 
-		$phpErrorLevel = intval( ini_get( 'error_reporting' ) );
-
-		if ( $phpErrorLevel !== $this->phpErrorLevel ) {
-			ini_set( 'error_reporting', $this->phpErrorLevel );
-
-			$oldVal = self::formatErrorLevel( $this->phpErrorLevel );
-			$newVal = self::formatErrorLevel( $phpErrorLevel );
-			$message = "PHP error_reporting setting was left dirty: "
-				. "was $oldVal before test, $newVal after test!";
-
-			$this->fail( $message );
-		}
-
 		// If anything faked the time, reset it
 		ConvertibleTimestamp::setFakeTime( false );
 	}
diff --git a/tests/phpunit/MediaWikiTestCaseTrait.php b/tests/phpunit/MediaWikiTestCaseTrait.php
index b1025b347b4..784ebf1a5b7 100644
--- a/tests/phpunit/MediaWikiTestCaseTrait.php
+++ b/tests/phpunit/MediaWikiTestCaseTrait.php
@@ -7,6 +7,9 @@ use PHPUnit\Framework\MockObject\MockObject;
  * For code common to both MediaWikiUnitTestCase and MediaWikiIntegrationTestCase.
  */
 trait MediaWikiTestCaseTrait {
+	/** @var int|null */
+	private $originalPhpErrorFilter;
+
 	/**
 	 * Returns a PHPUnit constraint that matches anything other than a fixed set of values. This can
 	 * be used to whitelist values, e.g.
@@ -149,4 +152,25 @@ trait MediaWikiTestCaseTrait {
 			}
 		);
 	}
+
+	/**
+	 * @before
+	 */
+	protected function phpErrorFilterSetUp() {
+		$this->originalPhpErrorFilter = intval( ini_get( 'error_reporting' ) );
+	}
+
+	/**
+	 * @after
+	 */
+	protected function phpErrorFilterTearDown() {
+		$phpErrorFilter = intval( ini_get( 'error_reporting' ) );
+
+		if ( $phpErrorFilter !== $this->originalPhpErrorFilter ) {
+			ini_set( 'error_reporting', $this->originalPhpErrorFilter );
+			$message = "PHP error_reporting setting found dirty."
+				. " Did you forget AtEase::restoreWarnings?";
+			$this->fail( $message );
+		}
+	}
 }
diff --git a/tests/phpunit/unit/includes/password/Pbkdf2PasswordTest.php b/tests/phpunit/unit/includes/password/Pbkdf2PasswordTest.php
index e4225366768..b03fe99bdac 100644
--- a/tests/phpunit/unit/includes/password/Pbkdf2PasswordTest.php
+++ b/tests/phpunit/unit/includes/password/Pbkdf2PasswordTest.php
@@ -1,7 +1,5 @@
 <?php
 
-use Wikimedia\AtEase\AtEase;
-
 /**
  * @group large
  * @covers Pbkdf2Password
@@ -41,8 +39,6 @@ class Pbkdf2PasswordTest extends PasswordTestCase {
 		);
 		$this->expectException( PasswordError::class );
 		$this->expectExceptionMessage( 'Error when hashing password.' );
-		AtEase::suppressWarnings();
-		$password->crypt( 'whatever' );
-		AtEase::restoreWarnings();
+		@$password->crypt( 'whatever' );
 	}
 }